feat(core,cli): integrate models.dev registry for unified model hydration and discovery #1124
Conversation
… CLI command Complete models.dev integration featuring: - ModelsRegistry singleton with fallback loading strategy (cache → bundled JSON → API) - Schema validation and model transformation pipeline - Provider integration layer mapping llxprt providers to models.dev IDs - Profile generation for model-family-specific defaults - /models CLI command with search, filtering, and verbose output - 194 comprehensive unit tests covering all layers - Provider updates (Gemini, OpenAI, Anthropic) with registry lookup fallback Resolves models.dev integration requirements across all phases.
…teractive models dialog - Add modelsDevProviderId to all provider alias configs for models.dev filtering - Implement ModelsDialog component with search, filtering, and dynamic column sizing - Add model switching functionality on selection - Fix all 118 failing tests: - Mock loadProviderAliasEntries() globally in test-setup.ts - Allow integration tests to unmock and use real configs - Update modelsCommand test expectations to match implementation - Update modelsCommand to parse --provider and --all flags correctly - Improve DialogManager with model selection handler - Add UIState and UIActions for models dialog management
- Migrate from ProviderModelDialog to ModelsDialog - Rename command from /models to /model (singular) - Support direct model switching: /model gpt-4o - Flags + positional arg open dialog: /model gpt-4o --tools - Delete legacy ProviderModelDialog and useProviderModelDialog - Fix footer help text to show ^A instead of P for all providers - All 2591 tests passing
…ls.dev integration - Fix model property access to use standard IModel.id instead of custom modelId - Update HydratedModel type usage in DialogManager and ModelsDialog - Correct Chutes.ai baseUrl (remove trailing slash) - Add provider ID mappings for model filtering - Export HydratedModel type from core package
…gration - Delete getModelsFromRegistry() function (now unused) - Remove models.dev lookups from GeminiProvider and AnthropicProvider - Remove getModelsFromRegistry tests and exports - Create barrel exports for generated/ and prompts/ modules to fix import/no-internal-modules linting - Update all internal imports to use new barrel export paths - Providers now return base models; hydration handled at ProviderManager level per spec
…t provider When user selects a model from ModelsDialog that belongs to a different provider: - Automatically switches to that provider (like /provider command) - Sets the selected model - Shows provider switch messages in correct order: * Switched from X to Y * Base URL set to... * Active model is... * Use /key to set API key if needed Handles edge cases: - If no current provider, shows "Switched to Y" instead of "from X" - Same provider selection skips provider switch, just sets model - Gemini doesn't show /key reminder
WalkthroughAdds a models.dev integration (schema, registry, transformer, hydration, profiles, provider integration), migrates provider registration to alias-driven registration, replaces ProviderModel dialog with a new Models dialog and CLI flows, and updates providers and runtime to return hydrated models. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI/Runtime
participant Dialog as ModelsDialog
participant Provider as Provider
participant Registry as Models Registry
User->>CLI: /model --tools --vision
CLI->>Dialog: openModelsDialog(initialFilters)
Dialog->>Provider: listAvailableModels(provider?)
Provider->>Registry: initializeModelRegistry() (non-blocking)
Registry->>Registry: load cache or fetch API
Registry-->>Provider: hydrated model metadata
Provider-->>Dialog: HydratedModel[]
Dialog->>Dialog: filter & render models
User->>Dialog: select model
alt provider differs
Dialog->>CLI: switchActiveProvider()
end
Dialog->>CLI: setActiveModel()
CLI-->>User: confirmation/info messages
sequenceDiagram
participant PM as ProviderManager
participant PA as ProviderAliases
participant OAuth as OAuthManager
participant Provider as Provider (alias)
PM->>PA: load alias entries (.config)
PA-->>PM: ProviderAliasEntry[]
loop for each alias
PM->>PM: createAliasProvider(entry, config, authOnly)
PM->>Provider: instantiate provider using alias config
Provider->>OAuth: register OAuth (if applicable)
PM-->>PM: provider registered
end
PM-->>PM: all providers registered via aliases
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2026-01-13T19:28:00.766ZApplied to files:
📚 Learning: 2025-12-18T14:06:22.557ZApplied to files:
📚 Learning: 2025-11-16T22:51:26.374ZApplied to files:
🧬 Code graph analysis (1)packages/core/src/providers/ProviderManager.ts (4)
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (5)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Resolved conflicts: - packages/cli/src/ui/commands/types.ts: merged ModelsDialogData with ProfileDialogData - packages/cli/src/ui/components/ProviderModelDialog.tsx: kept deletion (replaced by ModelsDialog)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/providers/anthropic/AnthropicProvider.ts (1)
438-473: Inconsistency:getDefaultModels()still uses hardcoded'anthropic'instead ofthis.name.While the OAuth models and API-derived models were updated to use
this.name, this fallback method still uses hardcodedprovider: 'anthropic'. This creates an inconsistency where unauthenticated calls return models with a different provider value than authenticated calls ifthis.nameever differs.🔧 Proposed fix
private getDefaultModels(): IModel[] { return [ { id: 'claude-opus-4-5-20251101', name: 'Claude Opus 4.5', - provider: 'anthropic', + provider: this.name, supportedToolFormats: ['anthropic'], contextWindow: 500000, maxOutputTokens: 32000, }, { id: 'claude-opus-4-1-20250805', name: 'Claude Opus 4.1', - provider: 'anthropic', + provider: this.name, supportedToolFormats: ['anthropic'], contextWindow: 500000, maxOutputTokens: 32000, }, { id: 'claude-sonnet-4-5-20250929', name: 'Claude Sonnet 4.5', - provider: 'anthropic', + provider: this.name, supportedToolFormats: ['anthropic'], contextWindow: 400000, maxOutputTokens: 64000, }, { id: 'claude-haiku-4-5-20251001', name: 'Claude Haiku 4.5', - provider: 'anthropic', + provider: this.name, supportedToolFormats: ['anthropic'], contextWindow: 500000, maxOutputTokens: 16000, }, ]; }packages/cli/src/ui/components/ProfileCreateWizard/ModelSelectStep.tsx (1)
169-171: HTML entity in string literal will render literally.
'is an HTML entity that isn't processed inside JavaScript string literals. It will display as "provider's" rather than "provider's".Proposed fix
{focusedComponent === 'input' - ? 'Enter the model name exactly as it appears in your provider's documentation' + ? "Enter the model name exactly as it appears in your provider's documentation" : `Choose the AI model for ${providerOption?.label || state.config.provider}`}
🤖 Fix all issues with AI agents
In `@packages/cli/src/ui/AppContainer.tsx`:
- Around line 895-909: The inline state type for modelsDialogData is missing
providerOverride and showAllProviders; replace the explicit inline type with the
canonical ModelsDialogData type (importing it if necessary) when declaring
modelsDialogData and setModelsDialogData, and also update the openModelsDialog
callback parameter type to ModelsDialogData so the state and callback accept the
full shape used by the /model command.
- Around line 940-960: The openModelsDialog callback currently types its
parameter inline; change it to use the canonical ModelsDialogData type (import
ModelsDialogData from types.ts) and update the signature to openModelsDialog =
useCallback((data?: ModelsDialogData) => { ... }, []); ensure
setModelsDialogData is still called with that typed value so callers (e.g.,
modelsCommand.ts) match the same shape.
In `@packages/cli/src/ui/commands/modelsCommand.ts`:
- Around line 34-53: The loop assigns parts[++i] to result.provider without
checking bounds, so a trailing "--provider" yields undefined; modify the branch
handling '--provider' / '-p' in modelsCommand.ts to first verify i+1 <
parts.length and that parts[i+1] does not start with '-' before consuming it
(e.g., if valid then result.provider = parts[++i]), otherwise handle the missing
value explicitly (set result.provider to null/undefined and/or emit a
user-friendly error/throw or skip) to avoid assigning undefined downstream.
In `@packages/cli/src/ui/components/DialogManager.tsx`:
- Around line 125-127: The comment on the else-branch is inaccurate because when
currentProvider is null execution goes into the if-branch; update the comment
above the else that precedes the call to runtime.setActiveModel(model.id) to
accurately describe the branch (e.g., "Same provider — just set model") or, if
you intend to handle the "no current provider" case here, change the condition
to explicitly allow currentProvider === null; reference the variables
currentProvider, selectedProvider and the call runtime.setActiveModel to locate
the code to update.
In `@packages/cli/src/ui/components/ModelsDialog.tsx`:
- Around line 541-545: The cursor indicator is being rendered before the search
term; in the ModelsDialog render where it checks state.mode === 'search' and
renders the Text with '\u258C', move that conditional Text so it is rendered
after the Text showing state.searchTerm (i.e., render <Text
color={SemanticColors.text.primary}>{state.searchTerm}</Text> first, then
conditionally render the cursor Text when state.mode === 'search') so the block
cursor appears at the end of the search text; update the JSX in the ModelsDialog
component referencing state.mode and state.searchTerm accordingly.
In `@packages/core/src/models/hydration.ts`:
- Around line 89-94: The inner loop declares a local variable named `models`
that shadows the outer function parameter `models`; rename the inner variable
returned from `registry.getByProvider(providerId)` (for example to
`providerModels` or `registryModelsForProvider`) and update the spread push into
`registryModels.push(...providerModels)` so there is no shadowing and the outer
`models` parameter remains unmodified.
- Around line 62-69: The implementation of getModelsDevProviderIds currently
returns [providerName] as a fallback which contradicts the JSDoc and can
accidentally match unintended provider IDs; change the function to return null
when PROVIDER_ID_MAP[providerName] is missing or empty (i.e., remove the
fallback return of [providerName]) so callers can explicitly handle unknown
providers, and update any callers expecting a string[] to handle the null case;
references: getModelsDevProviderIds and PROVIDER_ID_MAP.
- Around line 142-160: The findPartialMatch function currently returns the first
registry entry where normalizedId.includes(normalizedKey) or vice versa, causing
false positives; change it to iterate all entries in registryMap and compute a
match score (prefer exact match on normalizedId === normalizedKey, then exact
prefix/suffix with separator characters like '-', '_', '.', or '/', then longest
common substring or token overlap length) for each LlxprtModel, track the
best-scoring model and only return it if its score passes a reasonable threshold
(otherwise return undefined); use the existing symbols
normalizedId/normalizedKey and registryMap and update findPartialMatch to
perform scoring and selection instead of an early return.
In `@packages/core/src/models/registry.ts`:
- Around line 178-217: The refresh() function can leave the timeout active if
fetch throws; move creation of the AbortController and the timeout (the
AbortController and the timeout variable referencing
setTimeout(FETCH_TIMEOUT_MS)) so they are in scope for a finally block, then add
a finally { clearTimeout(timeout); } (and optionally controller.abort() if you
want to ensure cancellation) to guarantee the timeout is cleared on all paths
(successful response, non-ok responses, schema validation failures, or
exceptions). Ensure you update references to AbortController, timeout,
FETCH_TIMEOUT_MS, fetch, and refresh() accordingly so the clearTimeout call runs
regardless of how the try/catch exits.
In `@packages/core/src/providers/IProviderManager.ts`:
- Line 8: The return types in runtimeSettings wrappers are incorrect: update the
signature of listAvailableModels() to return Promise<HydratedModel[]> (not
Promise<IModel[]>) and change the availableModels variable declaration to
HydratedModel[] (not IModel[]) so callers receive the actual HydratedModel[]
with hydration metadata; locate and update the type annotations in
packages/cli/src/runtime/runtimeSettings.ts for the listAvailableModels function
and the availableModels variable to use HydratedModel from
packages/core/src/models/hydration.js.
In `@packages/core/src/providers/ProviderManager.ts`:
- Line 867: The fallback path uses strict falsy check on
rm.capabilities?.toolCalling while the main path only excludes explicit false,
causing inconsistent model sets; change the fallback logic in ProviderManager
(the rm loop that currently does "if (!rm.capabilities?.toolCalling) continue")
to mirror the main-path filter by only skipping when toolCalling === false, and
ensure any other occurrence of the same check (the hydratedModels.filter usage)
remains the permissive "!== false" behavior so both branches treat undefined the
same way.
In `@packages/core/test/models/schema.test.ts`:
- Around line 216-220: The test description is misleading:
ModelsDevProviderSchema.safeParse(invalidProviderData) fails because the fixture
invalidProviderData is missing the required env field, not id; either update the
test description to reflect the actual missing field (e.g., change "fails when
id is missing" to "fails when env is missing") or modify the fixture in
packages/core/test/models/__fixtures__/mock-data.ts to actually remove id (and
rename the fixture appropriately) so the test and fixture align; locate the test
by the describe block 'required fields' and the it block using
ModelsDevProviderSchema and invalidProviderData to make the fix.
🧹 Nitpick comments (21)
packages/core/src/providers/gemini/GeminiProvider.ts (1)
467-520: Model lists differ between fallback and OAuth branches.The
fallbackModelsarray (lines 468-487) containsgemini-2.5-flash-expbut lacksgemini-2.5-flash-liteandgemini-3-pro-preview, while the OAuth branch (lines 494-519) has the opposite. This could lead to inconsistent model availability when users switch between auth modes or when the API is unavailable.Consider aligning these lists or documenting the intentional differences. If the experimental model (
gemini-2.5-flash-exp) is intentionally excluded from OAuth, it may also be worth excluding from the fallback to ensure consistent behavior.scripts/fetch-models.ts (1)
58-70: Consider adding schema validation before writing the fallback file.The fetched JSON is written directly without validation. While the runtime registry performs validation, adding build-time validation using
ModelsDevApiResponseSchemawould catch API schema changes early and prevent bundling malformed data.Optional: Add schema validation
+import { ModelsDevApiResponseSchema } from '../packages/core/src/models/schema.js'; + // ... in fetchModels() const data = await response.json(); + // Validate API response shape + const parsed = ModelsDevApiResponseSchema.safeParse(data); + if (!parsed.success) { + return { + success: false, + error: `API response validation failed: ${parsed.error.message}`, + }; + } + // Count providers and modelspackages/core/src/models/profiles.ts (1)
12-71: Family-specific overrides may unintentionally override reasoning model temperature.For a reasoning model in the DeepSeek family (e.g.,
deepseek-reasoner), the code first setstemperature: 0.7at line 24 for reasoning models, then the DeepSeek branch at lines 55-58 overwrites it with the same value0.7. This happens to work out, but if the reasoning temperature were different (e.g.,0.5), the family override would silently overwrite it.Consider restructuring to apply family defaults first, then let reasoning-specific logic override when needed, or add explicit checks to preserve reasoning-specific settings.
packages/core/src/models/provider-integration.ts (1)
72-130: Consider extracting shared registry lookup logic.
hasModelInRegistryandgetExtendedModelInfoshare nearly identical structure: registry initialization check, provider ID mapping, and iteration over provider IDs to find a model. The only difference is the return type (booleanvsLlxprtModel | undefined).♻️ Suggested refactor
// Private helper to find model in registry function findModelInRegistry( providerName: string, modelId: string, ): LlxprtModel | undefined { try { const registry = getModelsRegistry(); if (!registry.isInitialized()) { return undefined; } const modelsDevProviderIds = PROVIDER_ID_MAP[providerName] ?? [providerName]; for (const providerId of modelsDevProviderIds) { const fullId = `${providerId}/${modelId}`; const model = registry.getById(fullId); if (model) { return model; } } return undefined; } catch { return undefined; } } export function hasModelInRegistry(providerName: string, modelId: string): boolean { return findModelInRegistry(providerName, modelId) !== undefined; } export function getExtendedModelInfo(providerName: string, modelId: string): LlxprtModel | undefined { return findModelInRegistry(providerName, modelId); }packages/core/src/models/schema.ts (1)
108-177: Minor redundancy between IModel-compatible fields and limits object.The schema defines both
contextWindow/maxOutputTokens(lines 162-163) for IModel compatibility and alimitsobject (line 168) withcontextWindow/maxOutput. This creates potential for inconsistency if not kept in sync during transformation.Consider documenting that the top-level fields are for IModel compatibility and the
limitsobject is the authoritative source, or removing the top-level fields if IModel can be adapted to uselimitsdirectly.packages/core/src/models/transformer.ts (2)
19-34: Consider extensibility for new providers.The hardcoded
PROVIDER_TOOL_FORMATSmapping works well for known providers, but new providers added to models.dev won't automatically get tool format support. The fallback to['openai']on line 62 is a reasonable default.Consider adding a comment documenting the maintenance expectation or making this configurable in the future.
141-146: Consider explicit mapping for clarity.The
mapStatusfunction is simple but could benefit from an explicit mapping to make the transformation intent clearer and ease future modifications.♻️ Optional: Explicit status mapping
function mapStatus( status?: 'alpha' | 'beta' | 'deprecated', ): 'stable' | 'beta' | 'alpha' | 'deprecated' { - if (!status) return 'stable'; - return status; + switch (status) { + case 'alpha': + case 'beta': + case 'deprecated': + return status; + default: + return 'stable'; + } }packages/cli/src/providers/providerAliases.codex.test.ts (1)
26-59: Consider consolidating redundant assertions.The test at lines 26-36 already asserts
baseUrl,baseProvider, anddefaultModel. The subsequent individual tests (lines 38-59) duplicate these exact assertions. This redundancy doesn't affect correctness but adds maintenance overhead.packages/core/src/providers/ProviderManager.ts (2)
838-848: Consider logging the registry initialization error for debugging.The catch block silently swallows the error. While returning unhydrated models is the correct fallback behavior, logging the error (not just a debug message) would help diagnose registry issues in production.
Proposed improvement
try { await initializeModelsRegistry(); - } catch { + } catch (error) { // Registry init failed - return unhydrated logger.debug( () => - `[getAvailableModels] Registry init failed for provider: ${provider!.name}`, + `[getAvailableModels] Registry init failed for provider: ${provider!.name}, error: ${error instanceof Error ? error.message : String(error)}`, ); return baseModels.map((m) => ({ ...m, hydrated: false })); }
862-890: ManualHydratedModelconstruction may drift from the hydration module's definition.This block manually constructs
HydratedModelobjects with hardcoded defaults (e.g.,supportedToolFormats: ['openai']). If theHydratedModeltype or hydration logic evolves, this fallback path could produce inconsistent model shapes.Consider extracting a shared helper function in the hydration module that both this fallback and
hydrateModelsWithRegistrycan use.packages/core/test/models/provider-integration.test.ts (1)
244-254: Strengthen assertions to verify actual sorting behavior.These tests describe expected sorting behavior in comments but don't actually verify it. They will pass even if sorting is broken.
Suggested improvement
it('sorts by price when preferCheaper: true', () => { const result = getRecommendedModel('openai', { preferCheaper: true }); expect(result).toBeDefined(); - // Should return model with lowest input price + // Verify it returns the cheapest model by checking against known prices + // or comparing against all candidates + const allModels = /* get all openai models */; + const cheapest = allModels.reduce((min, m) => + (m.pricing?.input ?? Infinity) < (min.pricing?.input ?? Infinity) ? m : min + ); + expect(result?.modelId).toBe(cheapest.modelId); }); it('sorts by context window by default', () => { const result = getRecommendedModel('openai'); expect(result).toBeDefined(); - // Should return model with highest context window + // Verify it returns the model with highest context window + expect(result?.contextWindow).toBeGreaterThanOrEqual(128000); });packages/cli/src/ui/components/DialogManager.tsx (1)
102-106: Consider structured data over string matching for Base URL message.The string matching with
includes('Base URL')is fragile and case-sensitive. IfswitchActiveProviderchanges its message format, this will silently fail to display the Base URL info.Consider having
switchActiveProviderreturn a structured object with explicitbaseUrlfield rather than extracting it from free-form info messages.packages/core/test/models/registry.test.ts (3)
309-328: Potential flaky test due to timing-based assertion.The test relies on
setTimeout(r, 100)to wait for a background refresh. This timing-based approach can be unreliable in CI environments with varying CPU loads.Consider making the refresh deterministic by either:
- Directly calling
await registry.refresh()and asserting on its return value- Using an event listener to detect when refresh completes
♻️ Suggested alternative using explicit refresh
it('getCacheMetadata returns data after successful refresh', async () => { vi.mocked(fs.existsSync).mockReturnValue(true); vi.mocked(fs.readFileSync).mockReturnValue( JSON.stringify(mockApiResponse), ); mockFetch.mockResolvedValue({ ok: true, json: async () => mockApiResponse, } as Response); await registry.initialize(); - - // Wait a bit for background refresh - await new Promise((r) => setTimeout(r, 100)); + // Explicitly trigger refresh and await completion + await registry.refresh(); const metadata = registry.getCacheMetadata(); expect(metadata).not.toBeNull(); expect(metadata?.modelCount).toBe(7); expect(metadata?.providerCount).toBe(4); });
418-428: Consider improving test coverage for dispose.The current test only verifies that
dispose()doesn't throw. While the comment acknowledges the limitation, you could improve coverage by verifying that event callbacks are no longer invoked after dispose.♻️ Suggested improvement
describe('dispose', () => { - it('clears listeners on dispose', () => { + it('clears listeners on dispose', async () => { const callback = vi.fn(); registry.on('models:updated', callback); registry.dispose(); - // Internal state cleared - can't easily test this without exposing internals - // Just ensure dispose doesn't throw - expect(true).toBe(true); + // Verify callback is not invoked after dispose + vi.mocked(fs.existsSync).mockReturnValue(false); + mockFetch.mockResolvedValue({ + ok: true, + json: async () => mockApiResponse, + } as Response); + + // Create new registry to test (old one is disposed) + const newRegistry = new ModelsRegistry(); + newRegistry.on('models:updated', callback); + newRegistry.dispose(); + + // Attempt refresh on disposed registry should not trigger callback + // This verifies listeners are cleared + expect(callback).not.toHaveBeenCalled(); }); });
78-97: Test assertion could be strengthened.The test for concurrent initialization verifies the registry is initialized but doesn't directly assert that the initialization logic only ran once. The comment on lines 94-96 acknowledges this limitation. Consider tracking initialization via a spy on the internal loading method or counting fetch calls.
packages/cli/test/ui/commands/modelCommand.test.ts (1)
328-377: Document the surprising behavior of ignored flags.The tests document that unknown flags like
--limit 10result in10becoming the search term and triggering a direct switch attempt. While this is tested behavior, it could be confusing for users expecting--limitto be recognized.Consider whether the command should emit a warning for unrecognized flags, or if this behavior is intentional for forward compatibility.
packages/core/src/models/registry.ts (2)
421-441: Singleton instance lacks reset capability for testing.The module-level
registryInstancevariable has no way to reset it, which could cause issues in test environments where tests need isolation. Consider adding aresetModelsRegistry()function for testing purposes.♻️ Suggested addition for test support
// Singleton instance let registryInstance: ModelsRegistry | null = null; /** * Get the global ModelsRegistry instance */ export function getModelsRegistry(): ModelsRegistry { if (!registryInstance) { registryInstance = new ModelsRegistry(); } return registryInstance; } /** * Initialize the global ModelsRegistry */ export async function initializeModelsRegistry(): Promise<ModelsRegistry> { const registry = getModelsRegistry(); await registry.initialize(); return registry; } + +/** + * Reset the global ModelsRegistry (for testing only) + * `@internal` + */ +export function resetModelsRegistry(): void { + if (registryInstance) { + registryInstance.dispose(); + registryInstance = null; + } +}
300-336: Search creates multiple intermediate arrays.The search method chains multiple filter operations, each creating a new array. For the typical model count (~100s), this is likely acceptable, but could be optimized to a single pass if performance becomes a concern.
packages/cli/src/ui/components/ModelsDialog.tsx (2)
52-58: Potential division by zero or unexpected result with fractional millions.
formatContextreturns${tokens / 1_000_000}Mwithout rounding, which could produce values like1.5Mor1.048576Mfor context windows that aren't exact multiples of 1M.Consider using
Math.roundortoFixedfor cleaner display:Suggested improvement
function formatContext(tokens: number | undefined): string { if (!tokens) return '-'; - if (tokens >= 1_000_000) return `${tokens / 1_000_000}M`; + if (tokens >= 1_000_000) return `${(tokens / 1_000_000).toFixed(1).replace(/\.0$/, '')}M`; if (tokens >= 1_000) return `${Math.floor(tokens / 1_000)}K`; return String(tokens); }
129-202: Effect hasstate.providerFilterin dependency array but reads it inside, causing potential re-fetch loops.The effect depends on
state.providerFilter, butstate.providerFilteris set viasetStatewhich could trigger the effect again. ThefetchedProvidersRefmitigation helps, but there's a subtle issue: when switching from a single provider filter back tonull(all providers), previously fetched providers won't be re-fetched even if their data is stale.Additionally, the
cancelledflag check at line 166 occurs after the async operation completes, butsetStateat line 196 runs synchronously beforeloadModels()starts, potentially causing a brief flash of loading state even when no fetch is needed.Consider moving the loading state update inside loadModels
useEffect(() => { let cancelled = false; const loadModels = async () => { const providersToFetch = state.providerFilter ? [state.providerFilter] : supportedProviders; const newProviders = providersToFetch.filter( (p) => !fetchedProvidersRef.current.has(p), ); if (newProviders.length === 0) { setState((prev) => ({ ...prev, isLoading: false })); return; } + // Only set loading if we actually have providers to fetch + setState((prev) => ({ ...prev, isLoading: true })); + try { // ... rest of fetch logic } }; - setState((prev) => ({ ...prev, isLoading: true })); loadModels(); return () => { cancelled = true; }; }, [runtime, supportedProviders, state.providerFilter]);packages/cli/src/providers/providerManagerInstance.ts (1)
356-378: OAuth provider registration usesvoidto ignore promises.While using
voidto explicitly ignore the promise return value is valid, theseensureOAuthProviderRegisteredcalls run asynchronously without error handling. If any fail, errors may be silently swallowed.Consider whether these should be awaited or at least have
.catch()handlers for logging purposes.Add error handling for OAuth registration
- void ensureOAuthProviderRegistered( - 'gemini', - oauthManager, - tokenStore, - addItem, - ); + ensureOAuthProviderRegistered( + 'gemini', + oauthManager, + tokenStore, + addItem, + ).catch((err) => logger.debug('Failed to register gemini OAuth provider', err));
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (79)
package.jsonpackages/cli/src/providers/aliases/anthropic.configpackages/cli/src/providers/aliases/cerebras-code.configpackages/cli/src/providers/aliases/chutes-ai.configpackages/cli/src/providers/aliases/codex.configpackages/cli/src/providers/aliases/fireworks.configpackages/cli/src/providers/aliases/gemini.configpackages/cli/src/providers/aliases/kimi.configpackages/cli/src/providers/aliases/llama-cpp.configpackages/cli/src/providers/aliases/lm-studio.configpackages/cli/src/providers/aliases/mistral.configpackages/cli/src/providers/aliases/openai-responses.configpackages/cli/src/providers/aliases/openai-vercel.configpackages/cli/src/providers/aliases/openai.configpackages/cli/src/providers/aliases/openrouter.configpackages/cli/src/providers/aliases/qwen.configpackages/cli/src/providers/aliases/qwenvercel.configpackages/cli/src/providers/aliases/synthetic.configpackages/cli/src/providers/aliases/xai.configpackages/cli/src/providers/providerAliases.builtin-qwen.test.tspackages/cli/src/providers/providerAliases.codex.test.tspackages/cli/src/providers/providerAliases.tspackages/cli/src/providers/providerManagerInstance.oauthRegistration.test.tspackages/cli/src/providers/providerManagerInstance.tspackages/cli/src/services/BuiltinCommandLoader.test.tspackages/cli/src/services/BuiltinCommandLoader.tspackages/cli/src/ui/AppContainer.tsxpackages/cli/src/ui/commands/bugCommand.test.tspackages/cli/src/ui/commands/bugCommand.tspackages/cli/src/ui/commands/modelCommand.tspackages/cli/src/ui/commands/modelsCommand.tspackages/cli/src/ui/commands/providerCommand.test.tspackages/cli/src/ui/commands/types.tspackages/cli/src/ui/components/AboutBox.tsxpackages/cli/src/ui/components/DialogManager.tsxpackages/cli/src/ui/components/ModelsDialog.tsxpackages/cli/src/ui/components/ProfileCreateWizard/ModelSelectStep.tsxpackages/cli/src/ui/components/ProviderModelDialog.responsive.test.tsxpackages/cli/src/ui/components/ProviderModelDialog.test.tsxpackages/cli/src/ui/components/ProviderModelDialog.tsxpackages/cli/src/ui/contexts/UIActionsContext.tsxpackages/cli/src/ui/contexts/UIStateContext.tsxpackages/cli/src/ui/hooks/index.tspackages/cli/src/ui/hooks/slashCommandProcessor.test.tspackages/cli/src/ui/hooks/slashCommandProcessor.tspackages/cli/src/ui/hooks/useEditorSettings.test.tsxpackages/cli/src/ui/hooks/useProviderModelDialog.tspackages/cli/src/ui/layouts/DefaultAppLayout.tsxpackages/cli/src/ui/reducers/appReducer.test.tspackages/cli/src/ui/reducers/appReducer.tspackages/cli/test-setup.tspackages/cli/test/providers/providerAliases.test.tspackages/cli/test/ui/commands/modelCommand.test.tspackages/core/src/assets/fallback-models.jsonpackages/core/src/config/config.tspackages/core/src/index.tspackages/core/src/models/hydration.tspackages/core/src/models/index.tspackages/core/src/models/profiles.tspackages/core/src/models/provider-integration.tspackages/core/src/models/registry.tspackages/core/src/models/schema.tspackages/core/src/models/transformer.tspackages/core/src/providers/IProviderManager.tspackages/core/src/providers/ProviderManager.tspackages/core/src/providers/anthropic/AnthropicProvider.tspackages/core/src/providers/gemini/GeminiProvider.tspackages/core/src/providers/openai/OpenAIProvider.tspackages/core/src/tools/mcp-client-manager.test.tspackages/core/src/tools/mcp-client-manager.tspackages/core/src/tools/mcp-client.test.tspackages/core/src/tools/mcp-client.tspackages/core/test/models/__fixtures__/mock-data.tspackages/core/test/models/profiles.test.tspackages/core/test/models/provider-integration.test.tspackages/core/test/models/registry.test.tspackages/core/test/models/schema.test.tspackages/core/test/models/transformer.test.tsscripts/fetch-models.ts
💤 Files with no reviewable changes (10)
- packages/cli/src/ui/commands/modelCommand.ts
- packages/cli/src/ui/hooks/index.ts
- packages/cli/src/ui/hooks/useEditorSettings.test.tsx
- packages/cli/src/ui/hooks/slashCommandProcessor.test.ts
- packages/cli/src/ui/components/ProviderModelDialog.tsx
- packages/cli/src/ui/components/ProviderModelDialog.test.tsx
- packages/cli/src/ui/reducers/appReducer.test.ts
- packages/cli/src/ui/components/ProviderModelDialog.responsive.test.tsx
- packages/cli/src/ui/hooks/useProviderModelDialog.ts
- packages/cli/src/ui/reducers/appReducer.ts
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2025-12-14T03:59:56.073Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-14T03:59:56.073Z
Learning: Qwen-3-coder models (qwen3-coder-pro, qwen3-coder) support both OAuth (free, use /auth qwen enable) and API key authentication (paid, use /provider qwen with key).
Applied to files:
packages/cli/src/providers/aliases/qwenvercel.configpackages/cli/src/providers/aliases/qwen.config
📚 Learning: 2025-11-21T23:05:45.164Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-11-21T23:05:45.164Z
Learning: Cerebras in quick-reference should use GLM-4.6 (model "zai-glm-4.6") via OpenAI-compatible base-url; do not show Qwen models for Cerebras.
Applied to files:
packages/cli/src/providers/aliases/qwenvercel.configpackages/cli/src/providers/aliases/lm-studio.configpackages/cli/src/providers/aliases/mistral.configpackages/cli/src/providers/aliases/synthetic.configpackages/cli/src/providers/aliases/qwen.configpackages/cli/src/providers/aliases/cerebras-code.config
📚 Learning: 2025-12-14T03:59:56.073Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-14T03:59:56.073Z
Learning: GLM-4.6 can be accessed via Z.ai using Anthropic provider with base-url https://api.z.ai/v1 and model zai-glm-4.6, in addition to Synthetic (hf:zai-org/GLM-4.6) and Cerebras (zai-glm-4.6).
Applied to files:
packages/cli/src/providers/aliases/lm-studio.configpackages/cli/src/providers/aliases/codex.configpackages/cli/src/providers/aliases/synthetic.configpackages/cli/src/providers/aliases/anthropic.configpackages/cli/src/providers/aliases/chutes-ai.config
📚 Learning: 2025-12-16T16:21:00.552Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-16T16:21:00.552Z
Learning: The correct base URL for Kimi API is `https://api.kimi.com/coding/v1`, not `https://api.moonshot.cn/v1`.
Applied to files:
packages/cli/src/providers/aliases/kimi.config
📚 Learning: 2025-12-16T16:15:21.347Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-16T16:15:21.347Z
Learning: Kimi/Moonshot AI models use API key (PAT) authentication via `/key` or `/keyfile` commands, not OAuth. Do not suggest `/auth kimi enable` as Kimi is not an OAuth provider.
Applied to files:
packages/cli/src/providers/aliases/kimi.config
📚 Learning: 2025-12-18T14:06:22.557Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-18T14:06:22.557Z
Learning: OpenAIResponsesProvider (packages/core/src/providers/openai-responses/OpenAIResponsesProvider.ts) currently bypasses the ephemeral truncation system by using direct `JSON.stringify(toolResponseBlock.result)` and needs to be updated to support ephemeral settings like the other providers.
Applied to files:
packages/cli/src/providers/aliases/openai-responses.configpackages/core/src/providers/anthropic/AnthropicProvider.tspackages/core/src/providers/openai/OpenAIProvider.ts
📚 Learning: 2025-12-14T03:59:56.073Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-14T03:59:56.073Z
Learning: Gemini 3 Pro Preview model identifier is gemini-3-pro-preview and is available as the latest preview model from Google.
Applied to files:
packages/cli/src/providers/aliases/gemini.config
📚 Learning: 2025-12-14T16:16:39.939Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-14T16:16:39.939Z
Learning: OpenAI's current model family (as of December 2025) is GPT-5, including gpt-5, gpt-5-mini, gpt-5-nano, gpt-5-thinking, gpt-5-pro, and GPT-5.2 variants (Instant/Thinking/Pro). GPT-4.1 and GPT-4o are outdated.
Applied to files:
packages/cli/src/providers/aliases/codex.config
📚 Learning: 2026-01-13T19:28:00.766Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-13T19:28:00.766Z
Learning: In the llxprt-code codebase (`packages/core/src/providers/anthropic/AnthropicProvider.ts`), Anthropic's API returns `contentBlock.input` as an already-parsed JavaScript object, not a JSON string. The code was incorrectly calling `JSON.stringify(contentBlock.input)` before passing it to `processToolParameters()`, which was designed for OpenAI-style string parameters. This causes arrays and other complex types to be corrupted into strings (e.g., `paths` array becomes a string `"[\"**/*.toml\"]"` instead of actual array). The fix is to use `contentBlock.input` directly without stringifying for Anthropic provider.
Applied to files:
packages/cli/src/providers/aliases/anthropic.configpackages/core/src/providers/anthropic/AnthropicProvider.ts
📚 Learning: 2025-12-14T16:22:17.722Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-14T16:22:17.722Z
Learning: Anthropic's current Claude model family (as of December 2025) is Claude 4.5, including claude-haiku-4-5-20251001, claude-sonnet-4-5-20250929, and claude-opus-4-5-20251101. Older Claude 3.5 and 4.x models should not be mentioned in documentation as they are outdated.
Applied to files:
packages/cli/src/providers/aliases/anthropic.configpackages/core/src/providers/anthropic/AnthropicProvider.ts
📚 Learning: 2026-01-09T07:46:06.289Z
Learnt from: sogoiii
Repo: vybestack/llxprt-code PR: 1052
File: packages/cli/src/ui/components/ProfileCreateWizard/utils.ts:35-40
Timestamp: 2026-01-09T07:46:06.289Z
Learning: In the Profile Create Wizard (packages/cli/src/ui/components/ProfileCreateWizard), provider values are always passed as `provider.value` from provider objects that originate from PROVIDER_OPTIONS, ensuring consistent provider ID lookups without needing normalization.
Applied to files:
packages/cli/src/ui/components/ProfileCreateWizard/ModelSelectStep.tsx
📚 Learning: 2025-11-17T08:33:42.962Z
Learnt from: MinWooPark-dotcom
Repo: vybestack/llxprt-code PR: 577
File: packages/cli/src/ui/components/messages/ToolMessage.tsx:89-167
Timestamp: 2025-11-17T08:33:42.962Z
Learning: In the vybestack/llxprt-code repository, single-use helper functions in React components can be kept inline rather than extracted to separate utilities, especially when the extraction doesn't provide immediate value (no reuse, no isolated testing planned) and keeps the PR scope focused on feature delivery.
Applied to files:
packages/cli/src/ui/components/DialogManager.tsx
🧬 Code graph analysis (11)
packages/core/src/models/profiles.ts (2)
packages/core/src/models/index.ts (5)
generateDefaultProfile(79-79)ModelsDevModel(45-45)LlxprtDefaultProfile(63-63)getRecommendedThinkingBudget(80-80)mergeProfileWithDefaults(81-81)packages/core/src/models/schema.ts (2)
ModelsDevModel(99-99)LlxprtDefaultProfile(195-195)
packages/core/src/models/provider-integration.ts (4)
packages/core/src/models/index.ts (5)
LlxprtModel(57-57)hasModelInRegistry(87-87)getModelsRegistry(33-33)getExtendedModelInfo(88-88)getRecommendedModel(89-89)packages/core/src/models/schema.ts (1)
LlxprtModel(196-196)packages/core/src/providers/IProvider.ts (1)
IModel(123-123)packages/core/src/models/registry.ts (1)
getModelsRegistry(427-432)
scripts/fetch-models.ts (1)
packages/cli/src/ui/utils/updateCheck.ts (1)
FETCH_TIMEOUT_MS(12-12)
packages/cli/src/ui/hooks/slashCommandProcessor.ts (1)
packages/cli/src/ui/commands/types.ts (1)
ModelsDialogData(147-163)
packages/core/src/providers/IProviderManager.ts (1)
packages/core/src/models/index.ts (1)
HydratedModel(96-96)
packages/core/test/models/schema.test.ts (2)
packages/core/src/models/schema.ts (4)
ModelsDevModelSchema(14-82)ModelsDevProviderSchema(84-92)ModelsDevApiResponseSchema(94-97)LlxprtModelSchema(148-177)packages/core/test/models/__fixtures__/mock-data.ts (9)
fullModel(30-58)minimalModel(16-25)reasoningModel(63-77)deprecatedModel(82-97)openaiProvider(208-221)emptyProvider(286-291)invalidProviderData(307-312)mockApiResponse(271-276)invalidModelData(296-302)
packages/core/src/models/transformer.ts (2)
packages/core/src/models/schema.ts (5)
ModelsDevProvider(100-100)ModelsDevModel(99-99)LlxprtModel(196-196)LlxprtModelCapabilities(189-191)LlxprtProvider(197-197)packages/core/src/models/profiles.ts (1)
generateDefaultProfile(12-71)
packages/cli/src/ui/components/ProfileCreateWizard/ModelSelectStep.tsx (3)
packages/cli/src/ui/contexts/RuntimeContext.tsx (1)
useRuntimeApi(187-189)packages/core/src/models/hydration.ts (1)
HydratedModel(54-54)packages/core/src/models/index.ts (1)
HydratedModel(96-96)
packages/core/src/models/hydration.ts (1)
packages/core/src/models/provider-integration.ts (1)
PROVIDER_ID_MAP(22-52)
packages/core/test/models/__fixtures__/mock-data.ts (2)
packages/core/src/models/index.ts (3)
ModelsDevModel(45-45)ModelsDevProvider(46-46)ModelsDevApiResponse(47-47)packages/core/src/models/schema.ts (3)
ModelsDevModel(99-99)ModelsDevProvider(100-100)ModelsDevApiResponse(101-101)
packages/core/src/models/schema.ts (1)
packages/core/src/models/index.ts (17)
ModelsDevModelSchema(42-42)ModelsDevProviderSchema(43-43)ModelsDevApiResponseSchema(44-44)ModelsDevModel(45-45)ModelsDevProvider(46-46)ModelsDevApiResponse(47-47)LlxprtModelSchema(50-50)LlxprtProviderSchema(51-51)LlxprtModelCapabilities(59-59)LlxprtModelPricing(60-60)LlxprtModelLimits(61-61)LlxprtModelMetadata(62-62)LlxprtDefaultProfile(63-63)LlxprtModel(57-57)LlxprtProvider(58-58)ModelsCacheMetadataSchema(66-66)ModelsCacheMetadata(67-67)
| @@ -5,7 +5,7 @@ | |||
| */ | |||
|
|
|||
| import { type IProvider } from './IProvider.js'; | |||
| import { type IModel } from './IModel.js'; | |||
| import { type HydratedModel } from '../models/hydration.js'; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's look at the file structure and the interface definition
echo "=== IProviderManager.ts content ==="
cat -n packages/core/src/providers/IProviderManager.ts
echo -e "\n=== Search for getAvailableModels usages ==="
rg -n --type=ts 'getAvailableModels' -C 3Repository: vybestack/llxprt-code
Length of output: 12817
🏁 Script executed:
#!/bin/bash
# Check HydratedModel definition
echo "=== HydratedModel definition ==="
find . -name "hydration.ts" -o -name "hydration.js" | head -5
rg -n "type HydratedModel|interface HydratedModel" -A 5
echo -e "\n=== IModel definition ==="
rg -n "type IModel|interface IModel" -A 10 --type=ts | head -50
echo -e "\n=== Full runtimeSettings.ts excerpt with return type ==="
sed -n '815,830p' packages/cli/src/runtime/runtimeSettings.ts
echo -e "\n=== Full context around line 1691 in runtimeSettings.ts ==="
sed -n '1685,1705p' packages/cli/src/runtime/runtimeSettings.tsRepository: vybestack/llxprt-code
Length of output: 7556
Update type signatures to accurately reflect HydratedModel return type.
The interface change from IModel[] to HydratedModel[] is real, but HydratedModel extends IModel (defined as IModel & Partial<ModelHydrationData>), making the assignment compatible at runtime. However, wrapper functions in packages/cli/src/runtime/runtimeSettings.ts have misleading type signatures that mask the actual return type:
listAvailableModels()(line 820) declares return typePromise<IModel[]>but returnsPromise<HydratedModel[]>- Variable
availableModels(line 1691) is typed asIModel[]but receivesHydratedModel[]
Update these type annotations to Promise<HydratedModel[]> and HydratedModel[] respectively to accurately reflect the API contract and preserve type information for callers who may need access to hydration metadata.
🤖 Prompt for AI Agents
In `@packages/core/src/providers/IProviderManager.ts` at line 8, The return types
in runtimeSettings wrappers are incorrect: update the signature of
listAvailableModels() to return Promise<HydratedModel[]> (not Promise<IModel[]>)
and change the availableModels variable declaration to HydratedModel[] (not
IModel[]) so callers receive the actual HydratedModel[] with hydration metadata;
locate and update the type annotations in
packages/cli/src/runtime/runtimeSettings.ts for the listAvailableModels function
and the availableModels variable to use HydratedModel from
packages/core/src/models/hydration.js.
…tion - Fix return types to use HydratedModel[] instead of IModel[] - Fix variable shadowing in hydration loop - Remove fallback that contradicts JSDoc in getModelsDevProviderIds - Implement scoring in findPartialMatch to prevent false positives - Add finally block to ensure timeout cleanup in registry refresh - Make toolCalling check consistent between main and fallback paths - Replace hardcoded 'anthropic' with this.name for provider flexibility - Add bounds checking for --provider command argument - Use canonical ModelsDialogData type for type safety - Fix cursor position in search bar (render after search term) - Fix HTML entity rendering in profile wizard - Update comment accuracy in DialogManager - Fix misleading test description
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
packages/core/src/models/registry.ts (1)
178-217: Previous timeout cleanup issue has been addressed.The
finallyblock at lines 214-216 now ensuresclearTimeout(timeout)is called regardless of how the try block exits (success, error, or abort).packages/core/src/models/hydration.ts (3)
62-69: Previous fallback behavior issue has been addressed.The function now correctly returns
nullwhen no mapping is found, allowing callers to handle unknown providers explicitly.
89-94: Previous variable shadowing issue has been addressed.The inner variable has been renamed from
modelstoproviderModels, eliminating the shadowing of the outermodelsparameter.
158-238: Previous partial match false-positive issue has been addressed.The implementation now includes a proper scoring mechanism with:
- Exact match: 100 points
- Prefix/suffix with separator: 80 points
- Token overlap: proportional scoring (max 60)
- Threshold of 50 to filter weak matches
🧹 Nitpick comments (5)
packages/cli/src/ui/commands/modelsCommand.ts (1)
79-94: Consider usingundefinedinstead ofnullfor consistency.The
providerOverridefield uses?? undefined(line 90), but theModelsDialogDatainterface defines it asstring | null. While this works due to TypeScript's handling, usingargs.provider ?? nullwould be more explicit and consistent with the type definition.Suggested fix for consistency
- providerOverride: args.provider ?? undefined, + providerOverride: args.provider ?? null,packages/cli/src/ui/components/DialogManager.tsx (1)
109-113: Base URL extraction is fragile.The substring matching for base URL messages (
m?.includes('Base URL') || m?.includes('base URL')) could miss variations or match unintended messages. Consider a more robust approach if theswitchResultstructure can be enhanced to expose base URL explicitly.packages/cli/src/ui/components/ProfileCreateWizard/ModelSelectStep.tsx (1)
85-108: Consider adding cleanup for async operation on unmount.If the component unmounts while
loadModels()is in flight, callingsetModels()andsetIsLoading()on an unmounted component could cause a React warning. Consider using an abort flag or AbortController.♻️ Proposed fix with cleanup
useEffect(() => { + let cancelled = false; const loadModels = async () => { if (!state.config.provider) { - setIsLoading(false); + if (!cancelled) setIsLoading(false); return; } setIsLoading(true); try { const hydratedModels: HydratedModel[] = await runtime.listAvailableModels(state.config.provider); // Filter out deprecated models and extract IDs const modelIds = hydratedModels .filter((m) => m.metadata?.status !== 'deprecated') .map((m) => m.id); - setModels(modelIds); + if (!cancelled) setModels(modelIds); } catch { // If fetching fails, allow manual entry - setModels([]); + if (!cancelled) setModels([]); } - setIsLoading(false); + if (!cancelled) setIsLoading(false); }; loadModels(); + return () => { cancelled = true; }; }, [runtime, state.config.provider]);packages/core/src/models/registry.ts (1)
421-441: Consider adding a reset function for testing.The singleton pattern works well for production, but unit tests may need to reset the registry between test cases. Consider exporting a
resetModelsRegistry()function (conditionally or for test builds).♻️ Optional: Add test reset capability
// Singleton instance let registryInstance: ModelsRegistry | null = null; +/** + * Reset the global ModelsRegistry instance (for testing) + * `@internal` + */ +export function resetModelsRegistry(): void { + if (registryInstance) { + registryInstance.dispose(); + registryInstance = null; + } +} + /** * Get the global ModelsRegistry instance */ export function getModelsRegistry(): ModelsRegistry {packages/core/src/models/hydration.ts (1)
96-103: Consider documenting potential key collision behavior.The registry map indexes by
modelId,id, andname. If multiple models share a key (e.g., same display name), later entries will overwrite earlier ones. This is likely intentional for flexible matching, but a brief comment would clarify the precedence.📝 Optional: Add clarifying comment
// Build lookup map: modelId -> LlxprtModel // Index by multiple keys for flexible matching + // Note: If multiple models share a key, later entries take precedence const registryMap = new Map<string, LlxprtModel>();
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/cli/src/runtime/runtimeSettings.tspackages/cli/src/ui/AppContainer.tsxpackages/cli/src/ui/commands/modelsCommand.tspackages/cli/src/ui/commands/types.tspackages/cli/src/ui/components/DialogManager.tsxpackages/cli/src/ui/components/ModelsDialog.tsxpackages/cli/src/ui/components/ProfileCreateWizard/ModelSelectStep.tsxpackages/core/src/models/hydration.tspackages/core/src/models/registry.tspackages/core/src/providers/ProviderManager.tspackages/core/src/providers/anthropic/AnthropicProvider.tspackages/core/test/models/schema.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/providers/ProviderManager.ts
- packages/cli/src/ui/components/ModelsDialog.tsx
- packages/core/src/providers/anthropic/AnthropicProvider.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-13T19:28:00.766Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-13T19:28:00.766Z
Learning: In the llxprt-code codebase (`packages/core/src/providers/anthropic/AnthropicProvider.ts`), Anthropic's API returns `contentBlock.input` as an already-parsed JavaScript object, not a JSON string. The code was incorrectly calling `JSON.stringify(contentBlock.input)` before passing it to `processToolParameters()`, which was designed for OpenAI-style string parameters. This causes arrays and other complex types to be corrupted into strings (e.g., `paths` array becomes a string `"[\"**/*.toml\"]"` instead of actual array). The fix is to use `contentBlock.input` directly without stringifying for Anthropic provider.
Applied to files:
packages/cli/src/ui/commands/modelsCommand.ts
📚 Learning: 2025-12-18T14:06:22.557Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-18T14:06:22.557Z
Learning: OpenAIResponsesProvider (packages/core/src/providers/openai-responses/OpenAIResponsesProvider.ts) currently bypasses the ephemeral truncation system by using direct `JSON.stringify(toolResponseBlock.result)` and needs to be updated to support ephemeral settings like the other providers.
Applied to files:
packages/cli/src/ui/commands/modelsCommand.ts
📚 Learning: 2026-01-09T07:46:06.289Z
Learnt from: sogoiii
Repo: vybestack/llxprt-code PR: 1052
File: packages/cli/src/ui/components/ProfileCreateWizard/utils.ts:35-40
Timestamp: 2026-01-09T07:46:06.289Z
Learning: In the Profile Create Wizard (packages/cli/src/ui/components/ProfileCreateWizard), provider values are always passed as `provider.value` from provider objects that originate from PROVIDER_OPTIONS, ensuring consistent provider ID lookups without needing normalization.
Applied to files:
packages/cli/src/ui/components/ProfileCreateWizard/ModelSelectStep.tsx
🧬 Code graph analysis (6)
packages/cli/src/runtime/runtimeSettings.ts (1)
packages/core/src/models/hydration.ts (1)
HydratedModel(54-54)
packages/core/test/models/schema.test.ts (2)
packages/core/src/models/schema.ts (4)
ModelsDevModelSchema(14-82)ModelsDevProviderSchema(84-92)ModelsDevApiResponseSchema(94-97)LlxprtModelSchema(148-177)packages/core/test/models/__fixtures__/mock-data.ts (8)
fullModel(30-58)minimalModel(16-25)reasoningModel(63-77)deprecatedModel(82-97)openaiProvider(208-221)invalidProviderData(307-312)mockApiResponse(271-276)invalidModelData(296-302)
packages/cli/src/ui/commands/modelsCommand.ts (2)
packages/cli/src/ui/commands/types.ts (5)
ModelsDialogData(147-163)SlashCommand(283-313)CommandContext(33-99)OpenDialogActionReturn(205-221)MessageActionReturn(120-124)packages/cli/src/ui/contexts/RuntimeContext.tsx (1)
getRuntimeApi(211-213)
packages/core/src/models/registry.ts (2)
packages/core/src/models/schema.ts (5)
LlxprtModel(196-196)LlxprtProvider(197-197)ModelsDevApiResponse(101-101)ModelsDevApiResponseSchema(94-97)ModelsCacheMetadata(209-209)packages/core/src/models/transformer.ts (1)
transformApiResponse(151-170)
packages/cli/src/ui/AppContainer.tsx (1)
packages/cli/src/ui/commands/types.ts (1)
ModelsDialogData(147-163)
packages/cli/src/ui/components/ProfileCreateWizard/ModelSelectStep.tsx (2)
packages/cli/src/ui/contexts/RuntimeContext.tsx (1)
useRuntimeApi(189-191)packages/core/src/models/hydration.ts (1)
HydratedModel(54-54)
🔇 Additional comments (28)
packages/cli/src/ui/commands/modelsCommand.ts (3)
30-60: Argument parsing looks correct after bounds fix.The
--providerargument now properly checks bounds (i + 1 < parts.length) and validates the next argument isn't another flag before consuming it. This addresses the previous review concern.One minor note: unknown flags (e.g.,
--unknown) are silently ignored. This is acceptable behavior for a CLI but consider whether a warning would improve UX.
62-74:hasAnyFlagshelper is clear and correct.The function correctly checks all filter flags to determine if the dialog should open vs. direct model switch.
96-135: Command implementation is well-structured.The dual behavior (direct switch with positional arg only, dialog with flags) is clearly documented and implemented. Error handling for the direct switch case properly catches and formats errors.
Minor observation: The
_contextparameter is unused. This is intentional per theSlashCommandinterface contract, but the underscore prefix correctly signals this.packages/cli/src/ui/commands/types.ts (3)
144-163:ModelsDialogDatainterface is well-designed.The interface provides clear, optional properties with good JSDoc comments. The
providerOverridefield usingstring | nullallows explicit "no override" semantics.
187-199: Dialog type additions are consistent.The
'models'type is correctly added to bothDialogTypeunion andDialogDataMap, maintaining type safety for dialog data access.
205-221:OpenDialogActionReturn.dialogDataunion is updated correctly.The
ModelsDialogDatais properly added to the union type, and the JSDoc is updated to document the mapping.packages/cli/src/ui/components/DialogManager.tsx (3)
71-78:currentProviderderivation handles errors gracefully.The try-catch with fallback to
nullis appropriate for cases where the runtime might not be fully initialized.Note: The
useMemodependency onruntimemay cause recalculation more often than necessary ifruntimeobject identity changes. However, this is a minor optimization concern and the current implementation is safe.
84-156: Provider switching logic is comprehensive but has complexity.The handler correctly:
- Switches provider first when needed (line 94-97)
- Builds ordered messages (lines 99-131)
- Sets the model after provider switch (line 116)
- Handles same-provider case separately (lines 132-142)
- Catches errors with status context (lines 143-152)
- Always closes dialog in success paths (line 153)
The comment at line 133 ("Same provider — just set model") is now accurate, addressing the previous review concern.
One observation: If
runtime.switchActiveProvidersucceeds butruntime.setActiveModelfails (line 116), the user sees partial success messages before the error. Consider wrapping both calls or adjusting message emission timing.
469-484: ModelsDialog rendering is correctly wired.All props are properly passed from state and the handler. The
Boxwrapper withflexDirection="column"is consistent with other dialog renderings in this file.packages/cli/src/ui/AppContainer.tsx (6)
24-24: Canonical type import addresses previous review concern.The
ModelsDialogDatatype is now imported from./commands/types.js, ensuring type consistency across the codebase.
921-925: State typing is now correct.The
modelsDialogDatastate usesModelsDialogData | undefined, which includes all fields (providerOverride,showAllProviders, etc.) that the previous review flagged as missing from the inline type.
956-964: Callback typing is now correct.The
openModelsDialogcallback parameter is properly typed asModelsDialogData, addressing the previous review concern about type mismatch withmodelsCommand.ts.
1455-1456:openModelsDialogis properly wired in slash command processor actions.The action is correctly added to both the object and the dependency array for the
useMemo.Also applies to: 1478-1478
2124-2124: UI state exports Models dialog data correctly.Both
isModelsDialogOpenandmodelsDialogDataare exposed in theuiStateobject for consumption by child components.Also applies to: 2140-2140
2333-2335: UI actions properly expose Models dialog controls.The
openModelsDialogandcloseModelsDialogcallbacks are correctly added touiActionsobject and its dependency array.Also applies to: 2432-2433
packages/core/test/models/schema.test.ts (11)
1-24: LGTM!License header and imports are well-organized. All imported schemas and fixtures are utilized in the test suite.
25-49: LGTM!Good coverage of valid model scenarios including full, minimal, reasoning, and deprecated models. The conditional check before accessing
result.data.statusis a nice defensive pattern.
51-86: LGTM!Thorough testing of required field validation. The destructuring pattern with eslint-disable comments is clean and idiomatic for creating test fixtures with missing fields.
88-116: LGTM!Good coverage of optional field behavior, verifying that the schema accepts undefined values for capability booleans, cost, and modalities.
149-194: LGTM!Good validation of cost object (complete and minimal) and interleaved field variants (boolean, object form with valid/invalid field values).
197-258: LGTM! Minor redundancy noted.Provider schema tests are well-structured. Note that lines 217-220 and 222-230 both test the missing
envscenario (one usinginvalidProviderDatafixture, one with an inline fixture). This redundancy is harmless but could be consolidated if desired.
260-285: LGTM!Good coverage of API response schema including validation of nested model data within providers.
287-322: LGTM!Comprehensive test for the transformed model structure with all required fields properly specified.
324-344: LGTM!Good test ensuring all capability booleans are required in the transformed schema, validating that partial capabilities cause validation failure.
346-408: 'stable' is a valid status value for LlxprtModelSchema — the test is correct.The
LlxprtModelMetadataSchemaexplicitly defines status asz.enum(['stable', 'beta', 'alpha', 'deprecated']), which includes 'stable' alongside the other values. This differs fromModelsDevModelSchema(which uses['alpha', 'beta', 'deprecated']), but that's expected since they are separate schemas. The test correctly uses the LlxprtModelSchema with LlxprtModelMetadataSchema, so thestatus: 'stable'value is valid.
118-147: Enum validation is correctly enforced on modality values.The schema properly uses
z.enum(['text', 'audio', 'image', 'video', 'pdf'])on bothinputandoutputarrays within the modalities object, not just string arrays. The test expectation thatinvalid-modalityfails validation is correct.packages/cli/src/runtime/runtimeSettings.ts (1)
25-26: LGTM! Type alignment with core hydration layer.The
HydratedModelreturn type correctly reflects thatProviderManager.getAvailableModels()now returns enriched models from the models.dev registry. SinceHydratedModel = IModel & Partial<ModelHydrationData>, this is backward-compatible with existing consumers expectingIModel.Also applies to: 819-824
packages/cli/src/ui/components/ProfileCreateWizard/ModelSelectStep.tsx (1)
146-157: LGTM! Clean loading state implementation.The loading UI provides good feedback during model fetching, and the step number display maintains consistency with the overall wizard flow.
…enerated modules - Add generated/** to ESLint import/no-internal-modules allow list - Change prompts/index.js → prompts/prompt-registry.js or prompts/mcp-prompts.js - Change generated/index.js → generated/git-commit.js - Fixes CI build failure from gitignored barrel exports
- Remove fetch-models script from build process - Delete bundled fallback-models.json (1.6MB) - Remove loadBundledFallback() from ModelsRegistry - Models now load from cache or live API only - Update tests to mock fresh cache instead of bundled fallback
sogoiii
force-pushed
the
feat/models-dev-integration
branch
from
41e155c to
2976aed
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/core/test/models/provider-integration.test.ts`:
- Around line 1-26: Prettier flagged formatting issues in the
provider-integration.test.ts test file (imports and surrounding whitespace for
symbols like llxprtModelToIModel, hasModelInRegistry, getExtendedModelInfo,
getRecommendedModel, ModelsRegistry, getModelsRegistry, LlxprtModel,
mockApiResponse, fullModel, openaiProvider, transformModel); run the project
formatter (npm run format) or run Prettier on that file, then stage and commit
the resulting changes so the CI passes, ensuring import order/spacing and EOF
newline are preserved.
♻️ Duplicate comments (1)
packages/core/src/models/registry.ts (1)
73-81: Race condition:initializedset outside the promise chain.When concurrent callers invoke
initialize(), there's a window whereinitPromisehas resolved butinitializedis stillfalse. A third caller arriving in this window would return the resolved promise but may observe staleinitializedstate.🐛 Proposed fix to ensure atomic state update
async initialize(): Promise<void> { if (this.initialized) return; if (this.initPromise) return this.initPromise; - this.initPromise = this.loadModels(); - await this.initPromise; - this.startBackgroundRefresh(); - this.initialized = true; + this.initPromise = (async () => { + await this.loadModels(); + this.initialized = true; + this.startBackgroundRefresh(); + })(); + await this.initPromise; }
🧹 Nitpick comments (6)
packages/core/src/models/registry.ts (2)
103-132: Consider async file operations to avoid blocking the event loop.The synchronous
existsSync,statSync, andreadFileSynccalls block the thread during cache loading. While acceptable for startup, switching tofs.promisesvariants would improve concurrency, especially if the cache file grows large.
374-377: Consider error isolation inemitto prevent one listener from breaking others.If a callback throws, the
forEachiteration stops and subsequent listeners won't be notified. Wrapping each callback in a try-catch would improve resilience.♻️ Proposed fix
private emit(event: ModelsRegistryEvent): void { const callbacks = this.listeners.get(event) || []; - callbacks.forEach((cb) => cb()); + callbacks.forEach((cb) => { + try { + cb(); + } catch { + // Prevent one listener from breaking others + } + }); }packages/core/test/models/provider-integration.test.ts (1)
250-260: Strengthen sorting assertions to validate actual behavior.These tests only verify that a result is returned, but don't confirm the sorting logic works correctly. Consider asserting on the specific model returned or comparing against other candidates.
💡 Example strengthened assertion
it('sorts by price when preferCheaper: true', () => { const result = getRecommendedModel('openai', { preferCheaper: true }); const allModels = getModelsRegistry().getByProvider('openai'); const cheapestModel = allModels .filter(m => m.pricing && m.metadata?.status !== 'deprecated') .sort((a, b) => (a.pricing?.input ?? Infinity) - (b.pricing?.input ?? Infinity))[0]; expect(result?.id).toBe(cheapestModel?.id); });packages/core/test/models/registry.test.ts (3)
142-154: Timing-based wait is fragile but acceptable for this test.The 100ms
setTimeoutwait could occasionally flake if the system is slow. Consider using a polling approach or event-based synchronization for more robustness.
323-329: The 100ms wait afterawait registry.refresh()is likely unnecessary.Since
refresh()is already awaited on line 325, the metadata should be populated synchronously before line 330 executes.♻️ Proposed simplification
await registry.initialize(); // Explicitly call refresh to populate metadata await registry.refresh(); - // Wait a bit for refresh to complete - await new Promise((r) => setTimeout(r, 100)); - const metadata = registry.getCacheMetadata();
424-433: Placeholder assertion provides no verification.
expect(true).toBe(true)doesn't test anything. Consider verifying thatdispose()doesn't throw and optionally that subsequent event emissions don't invoke cleared listeners.💡 Improved dispose test
it('clears listeners on dispose', async () => { vi.mocked(fs.existsSync).mockReturnValue(false); mockFetch.mockResolvedValue({ ok: true, json: async () => mockApiResponse, } as Response); const callback = vi.fn(); registry.on('models:updated', callback); registry.dispose(); // Verify event no longer fires after dispose await registry.refresh(); expect(callback).not.toHaveBeenCalled(); });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/models/registry.tspackages/core/test/models/provider-integration.test.tspackages/core/test/models/registry.test.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/core/src/models/registry.ts (2)
packages/core/src/models/schema.ts (5)
LlxprtModel(196-196)LlxprtProvider(197-197)ModelsDevApiResponse(101-101)ModelsDevApiResponseSchema(94-97)ModelsCacheMetadata(209-209)packages/core/src/models/transformer.ts (1)
transformApiResponse(151-170)
packages/core/test/models/provider-integration.test.ts (8)
packages/core/src/models/registry.ts (1)
ModelsRegistry(53-378)scripts/tests/get-release-version.test.js (1)
mod(17-17)packages/core/src/models/schema.ts (1)
LlxprtModel(196-196)packages/core/src/models/transformer.ts (1)
transformModel(39-118)packages/core/test/models/__fixtures__/mock-data.ts (3)
openaiProvider(208-221)fullModel(30-58)mockApiResponse(271-276)packages/core/src/models/provider-integration.ts (4)
llxprtModelToIModel(58-67)hasModelInRegistry(72-98)getExtendedModelInfo(104-130)getRecommendedModel(136-196)scripts/get-release-version.js (1)
now(23-23)scripts/probe-ide-mcp.cjs (1)
JSON(33-33)
🪛 GitHub Actions: LLxprt Code CI
packages/core/test/models/provider-integration.test.ts
[error] 1-1: Formatting changes detected by Prettier during 'npm run format' (prettier --experimental-cli --write .). Run 'npm run format' or 'prettier --write .' to fix the issues.
packages/core/test/models/registry.test.ts
[error] 1-1: Formatting changes detected by Prettier during 'npm run format' (prettier --experimental-cli --write .). Run 'npm run format' or 'prettier --write .' to fix the issues.
🔇 Additional comments (14)
packages/core/src/models/registry.ts (5)
86-98: LGTM!The silent-failure pattern is appropriate here—models remain empty until the next background refresh, which is consistent with the non-blocking design.
137-176: LGTM!The
finallyblock ensures timeout cleanup on all paths—network errors, non-ok responses, or schema validation failures. Good use ofAbortControllerfor request cancellation.
181-195: LGTM!Silent failure for cache writes is appropriate—the in-memory registry remains functional even if persistence fails.
259-295: LGTM!Filter logic is clean, and the non-null assertions are safe within their respective
ifguards.
380-400: LGTM!Standard lazy singleton pattern with an async initialization wrapper.
packages/core/test/models/provider-integration.test.ts (4)
27-74: LGTM!The registry mock with
__resetRegistryhelper provides clean test isolation. Good use ofdispose()before nullifying the instance.
76-102: LGTM!Good coverage of the model transformation, including the important distinction between
modelId(short) and fullid.
104-199: LGTM!Thorough coverage of both uninitialized and initialized states, plus cross-provider ID mapping validation.
262-281: LGTM!Good edge case coverage for filter combinations, deprecated model exclusion, and unknown provider handling.
packages/core/test/models/registry.test.ts (5)
45-104: LGTM!Comprehensive initialization tests covering idempotency and concurrent call handling.
157-248: LGTM!Thorough query API coverage including edge cases for missing IDs and unknown providers.
250-298: LGTM!Provider API and count tests are well-structured with clear expectations.
337-422: LGTM!Good coverage of refresh outcomes and event system behavior including listener removal.
1-11: Fix formatting issues flagged by Prettier.Same CI pipeline error—run
npm run formatto resolve.⛔ Skipped due to learnings
Learnt from: CR Repo: vybestack/llxprt-code PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-11-25T16:56:18.980Z Learning: Before reporting a task as finished, run `npm run format` from the repository root and ensure it succeeds (exit code 0)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…race condition - Update availableModels type from IModel[] to HydratedModel[] in runtimeSettings - Remove unused IModel import - Fix initialize() race condition by setting initialized before startBackgroundRefresh
- CLI: Rename modelsCommand.ts → modelCommand.ts - CLI: Rename ModelsDialog.tsx → ModelDialog.tsx - Core: Rename ModelsRegistry → ModelRegistry - Core: Rename getModelsRegistry() → getModelRegistry() - Core: Rename initializeModelsRegistry() → initializeModelRegistry() - Core: Rename ModelsRegistryEvent → ModelRegistryEvent - Core: Rename ModelsCacheMetadata → ModelCacheMetadata - Updates all imports, exports, and test files - Kept ModelsDev* names unchanged (external API proper noun)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/core/src/models/hydration.ts`:
- Around line 96-103: hydrateModelsWithRegistry builds registryMap by indexing
registryModels with rm.modelId, rm.id and rm.name which causes name collisions
when models from different providers share the same rm.name; update the indexing
so name entries are provider-scoped (e.g., use a compound key like
`${rm.providerId}:${rm.name}`) or map names to arrays of LlxprtModel to preserve
all candidates, and adjust the fallback lookup logic that uses model.name to
consult the provider-scoped key or choose from the array; ensure you update
references to registryMap.get(...) (the primary lookup by model.id remains
unchanged) so the name-based fallback uses the new compound-key or multi-value
handling.
In `@packages/core/src/models/provider-integration.ts`:
- Around line 82-84: The code here uses PROVIDER_ID_MAP to compute
modelsDevProviderIds and falls back to [providerName], which is inconsistent
with hydration.ts's getModelsDevProviderIds that returns null when no mapping
exists; unify the behavior by extracting a shared helper (e.g.,
getModelsDevProviderIds) used by both provider-integration.ts and hydration.ts,
and make them consistently return the same fallback (either null or
[providerName]) — update hasModelInRegistry and any callers to rely on that
helper and handle the unified return value accordingly; ensure you reference and
replace the local logic that sets modelsDevProviderIds and the hydration.ts
implementation so both use the same function.
🧹 Nitpick comments (8)
packages/cli/src/ui/commands/modelCommand.ts (1)
52-54: Positional argument handling may capture unintended values.If an unknown flag like
--foois followed by a value likebar, thebarwill be captured asresult.searchon the next iteration. This matches the test expectation in "ignores unknown flags" but may surprise users.Consider whether unknown flags should emit a warning or if this behavior should be documented in the command help.
packages/cli/src/ui/components/DialogManager.tsx (1)
110-113: Base URL message extraction uses loose string matching.The
findcall searches for messages containing 'Base URL' or 'base URL'. This is case-sensitive for the first character. Consider using case-insensitive matching to be more robust:-const baseUrlMsg = (switchResult.infoMessages ?? []).find( - (m) => m?.includes('Base URL') || m?.includes('base URL'), -); +const baseUrlMsg = (switchResult.infoMessages ?? []).find( + (m) => m?.toLowerCase().includes('base url'), +);packages/cli/test/ui/commands/modelCommand.test.ts (1)
328-377: Ignored flags tests document expected behavior clearly.The tests document that ignored flags like
--limitcause their values to be treated as search terms, triggering direct switches. The comments explain the expected behavior well.Consider adding a test that verifies multiple positional-looking values only capture the first one:
it('only first positional arg is used as search', async () => { // '/model foo bar' should use 'foo' as search, not 'bar' });packages/core/src/models/registry.ts (3)
105-134: Synchronous file I/O may block the event loop.The
loadFromCachemethod uses synchronousfs.existsSync,fs.statSync, andfs.readFileSynccalls. While acceptable for CLI startup, this could cause noticeable delays if the cache file is large or on a slow filesystem.Consider using async alternatives (
fs.promises) for better non-blocking behavior, especially since this method is already async.♻️ Optional refactor to use async fs
private async loadFromCache(): Promise<ModelsDevApiResponse | null> { try { const cachePath = ModelRegistry.getCachePath(); - if (!fs.existsSync(cachePath)) { - return null; - } - - const stats = fs.statSync(cachePath); + let stats: fs.Stats; + try { + stats = await fs.promises.stat(cachePath); + } catch { + return null; // File doesn't exist + } + const age = Date.now() - stats.mtimeMs; // Check if cache is stale if (age > CACHE_MAX_AGE_MS) { return null; } - const content = fs.readFileSync(cachePath, 'utf-8'); + const content = await fs.promises.readFile(cachePath, 'utf-8'); const data = JSON.parse(content);
183-197: Synchronous file writes in saveToCache.Similar to
loadFromCache, this uses synchronousfs.existsSync,fs.mkdirSync, andfs.writeFileSync. Since this is called after a network fetch completes, blocking the event loop here is less critical but still suboptimal.
353-379: Event system is functional but basic.The event system works correctly for the current use case. One minor consideration: if a callback throws, it will prevent subsequent callbacks from executing. Consider wrapping callbacks in try-catch if robustness is important.
♻️ Optional: Add error isolation for callbacks
private emit(event: ModelRegistryEvent): void { const callbacks = this.listeners.get(event) || []; - callbacks.forEach((cb) => cb()); + callbacks.forEach((cb) => { + try { + cb(); + } catch { + // Prevent one callback from breaking others + } + }); }packages/cli/src/ui/components/ModelDialog.tsx (1)
470-494: Potential key collision in renderRow.The key
${model.provider}:${model.id}:${visibleIndex}includesvisibleIndex, which changes as the user scrolls. This could cause unnecessary re-renders. Consider using a stable key without the index:♻️ Use stable key for rows
-<Box key={`${model.provider}:${model.id}:${visibleIndex}`}> +<Box key={`${model.provider}:${model.id}`}>packages/core/src/models/provider-integration.ts (1)
185-189: Accessing nested property without null check.Line 186-187 accesses
a.limits.contextWindowdirectly. WhileLlxprtModel.limitsis likely always present based on the schema, adding a null-coalescing check forlimitswould be more defensive:♻️ Add defensive null check
candidates.sort((a, b) => { - const ctxA = a.limits.contextWindow ?? 0; - const ctxB = b.limits.contextWindow ?? 0; + const ctxA = a.limits?.contextWindow ?? 0; + const ctxB = b.limits?.contextWindow ?? 0; return ctxB - ctxA; });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/cli/src/ui/commands/modelCommand.tspackages/cli/src/ui/components/DialogManager.tsxpackages/cli/src/ui/components/ModelDialog.tsxpackages/cli/test/ui/commands/modelCommand.test.tspackages/core/src/models/hydration.tspackages/core/src/models/index.tspackages/core/src/models/provider-integration.tspackages/core/src/models/registry.tspackages/core/src/models/schema.tspackages/core/src/providers/ProviderManager.tspackages/core/test/models/provider-integration.test.tspackages/core/test/models/registry.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/models/index.ts
- packages/core/test/models/provider-integration.test.ts
- packages/core/test/models/registry.test.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2026-01-13T19:28:00.766Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-13T19:28:00.766Z
Learning: In the llxprt-code codebase (`packages/core/src/providers/anthropic/AnthropicProvider.ts`), Anthropic's API returns `contentBlock.input` as an already-parsed JavaScript object, not a JSON string. The code was incorrectly calling `JSON.stringify(contentBlock.input)` before passing it to `processToolParameters()`, which was designed for OpenAI-style string parameters. This causes arrays and other complex types to be corrupted into strings (e.g., `paths` array becomes a string `"[\"**/*.toml\"]"` instead of actual array). The fix is to use `contentBlock.input` directly without stringifying for Anthropic provider.
Applied to files:
packages/core/src/providers/ProviderManager.tspackages/cli/src/ui/commands/modelCommand.ts
📚 Learning: 2025-12-18T14:06:22.557Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-18T14:06:22.557Z
Learning: OpenAIResponsesProvider (packages/core/src/providers/openai-responses/OpenAIResponsesProvider.ts) currently bypasses the ephemeral truncation system by using direct `JSON.stringify(toolResponseBlock.result)` and needs to be updated to support ephemeral settings like the other providers.
Applied to files:
packages/core/src/providers/ProviderManager.tspackages/cli/src/ui/commands/modelCommand.ts
📚 Learning: 2025-11-16T22:51:26.374Z
Learnt from: e2720pjk
Repo: vybestack/llxprt-code PR: 583
File: packages/core/src/providers/openai/OpenAIProvider.ts:935-959
Timestamp: 2025-11-16T22:51:26.374Z
Learning: In the llxprt-code codebase (packages/core/src/providers/openai/OpenAIProvider.ts), tools like `run_shell_command` use internal streaming only for real-time UI updates during execution, but each tool execution produces exactly ONE final `ToolResponseBlock` containing the full result. The streaming chunks are never sent to the LLM and are not converted into multiple tool messages. The OpenAI Chat Completions API requires that each tool call (tool_call_id) corresponds to exactly one message with role 'tool', so duplicate tool response detection that removes subsequent tool messages with the same tool_call_id is correct and necessary for API compliance.
Applied to files:
packages/core/src/providers/ProviderManager.ts
📚 Learning: 2026-01-09T07:46:06.289Z
Learnt from: sogoiii
Repo: vybestack/llxprt-code PR: 1052
File: packages/cli/src/ui/components/ProfileCreateWizard/utils.ts:35-40
Timestamp: 2026-01-09T07:46:06.289Z
Learning: In the Profile Create Wizard (packages/cli/src/ui/components/ProfileCreateWizard), provider values are always passed as `provider.value` from provider objects that originate from PROVIDER_OPTIONS, ensuring consistent provider ID lookups without needing normalization.
Applied to files:
packages/cli/src/ui/commands/modelCommand.ts
🧬 Code graph analysis (2)
packages/cli/src/ui/components/ModelDialog.tsx (6)
packages/core/src/models/hydration.ts (1)
HydratedModel(54-54)packages/core/src/models/index.ts (1)
HydratedModel(96-96)packages/cli/src/ui/contexts/RuntimeContext.tsx (1)
useRuntimeApi(189-191)packages/cli/src/ui/hooks/useKeypress.ts (1)
useKeypress(23-41)packages/cli/test-utils/ink-stub.ts (2)
Text(23-23)Box(22-22)packages/cli/src/ui/colors.ts (1)
SemanticColors(83-153)
packages/core/src/models/provider-integration.ts (3)
packages/core/src/models/index.ts (5)
llxprtModelToIModel(86-86)LlxprtModel(57-57)hasModelInRegistry(87-87)getModelRegistry(33-33)getRecommendedModel(89-89)packages/core/src/models/schema.ts (1)
LlxprtModel(196-196)packages/core/src/models/registry.ts (1)
getModelRegistry(388-393)
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test (macos-latest, 24.x)
- GitHub Check: Test (ubuntu-latest, 24.x)
- GitHub Check: Test (windows-latest, 24.x)
🔇 Additional comments (39)
packages/core/src/models/schema.ts (5)
14-82: Well-structured external API schema with appropriate optionality.The
ModelsDevModelSchemacorrectly handles the inconsistent nature of the external API by making capability fields optional. The nestedcostandmodalitiesstructures are well-defined with proper enum constraints.One consideration: the
interleavedfield uses a union type that may be difficult to work with downstream. Consider documenting the expected usage pattern.
108-117: Internal capabilities schema uses required booleans - good for type safety.Unlike the external API schema,
LlxprtModelCapabilitiesSchemamakes all capability fields required booleans. This is a good design choice as the transformer layer should normalize undefined values tofalse, providing consistent downstream behavior.
148-177: Comprehensive internal model schema with clear IModel compatibility.The
LlxprtModelSchemacorrectly combines core identity fields (compatible withIModel), provider info, and extended models.dev data. The dualcontextWindow/maxOutputTokensfields (lines 162-163) alongsidelimits(line 168) provide flexibility for different consumption patterns.
202-209: Cache metadata schema is minimal and appropriate.The
ModelCacheMetadataSchemacaptures essential cache validation fields. Consider whether attlSecondsorexpiresAtfield would be useful for cache invalidation logic, though this may be handled at the registry level.
56-59: Makelimitfield optional to match API inconsistency handling for other fields.The
limitfield is currently required in the schema whilecost,modalities, and all capability fields are optional due to acknowledged API inconsistency. If the models.dev API is inconsistent with other optional fields, it's likely inconsistent withlimitas well. Either verify that ALL models in the API providelimit.contextandlimit.outputwithout exception, or mark it as optional (matching the defensive pattern used forcostandmodalities).packages/core/src/providers/ProviderManager.ts (5)
11-19: Clean import organization for hydration integration.The new imports are well-organized, separating hydration utilities from registry utilities. The
HydratedModeltype import ensures type safety for the updated return type.
839-848: Good defensive handling of registry initialization failure.The non-blocking try-catch around
initializeModelRegistry()ensures that provider model fetching degrades gracefully when the registry is unavailable. Returning unhydrated models withhydrated: falseis the correct fallback behavior.
862-890: Registry fallback loop constructs HydratedModel correctly.The fallback path properly constructs
HydratedModelobjects with all required fields from registry data. ThesupportedToolFormats: ['openai']default is reasonable for most providers.Minor observation: Line 872 uses
provider!.namewith non-null assertion. While safe here due to the earlier check, consider extractingprovider.nameto a variable before the loop for clarity.
867-867: ConsistenttoolCallingfilter logic - previous issue addressed.The fallback path (line 867) now uses
rm.capabilities?.toolCalling === falseand the main path (line 905) usesm.capabilities?.toolCalling !== false. Both now consistently only exclude models that explicitly disable tool calling, allowingundefinedto pass through.Also applies to: 904-905
854-857: The edge case is already handled correctly by thegetModelsDevProviderIdsfunction.The function explicitly returns
nullwhen no mapping exists or the mapping array is empty, rather than an empty array. This means the conditionbaseModels.length === 0 && modelsDevProviderIdswill never receive an empty array—only a non-emptystring[]ornull. The fallback logic is working as intended.Likely an incorrect or invalid review comment.
packages/cli/src/ui/commands/modelCommand.ts (3)
30-60: Robust argument parsing with proper bounds checking.The
parseArgsfunction correctly handles:
- Flag-value pairs with bounds checking (lines 37-40)
- Short flags (-p, -r, -t, -a)
- Positional arguments as search terms
- Ignored flags (--limit, --verbose) by omission
The previous issue about
--providerwithout a value has been properly addressed.
65-74:hasAnyFlagscorrectly identifies filter presence.The function properly checks all filter-related flags. Note that
args.allis included, which means/model somename --allwill open the dialog rather than direct-switch, which aligns with the intent of showing all providers.
109-125: Direct switch path with clear error handling.The direct switch logic is clean:
- Only triggers when positional arg exists AND no flags
- Uses runtime API for the actual switch
- Returns appropriate info/error messages
- Handles
nullpreviousModel with "unknown" fallbackpackages/cli/src/ui/components/DialogManager.tsx (4)
71-78: Memoization ofcurrentProvideris appropriate.Using
useMemowithruntimedependency ensures the provider name is only re-computed when the runtime changes. The try-catch withnullfallback handles edge cases gracefully.
85-156: Comprehensive model selection handler with provider switching.The
handleModelsDialogSelectfunction properly handles:
- Provider switch when needed (lines 92-131)
- Message ordering (provider → base URL → model → key reminder)
- Same-provider model switch (lines 132-142)
- Error handling with provider status (lines 143-152)
- Dialog close in all paths (line 153)
The previous issue about the misleading comment has been addressed (line 133 now says "Same provider — just set model").
143-152: Error handler provides useful context with provider status.Good pattern to include provider status in error messages. This helps users diagnose issues when model switching fails.
469-484: ModelsDialog rendering with comprehensive prop passing.All relevant props are passed to
ModelsDialog:
- Selection and close handlers
- Initial search and filters from dialog data
- Current provider for context
- Provider override and "show all" options
packages/cli/test/ui/commands/modelCommand.test.ts (4)
19-25: Minimal mock setup is appropriate for unit testing.The mock only provides
setActiveModel, which is the only runtime API method used bymodelCommand. This keeps tests focused and fast.
109-156: Direct switch tests cover success, failure, and edge cases.Good coverage of:
- Successful model switch with previous/next model info
- Error handling when switch fails
- Handling of
nullpreviousModel
28-53: Mock context could benefit from type checking.The
createMockContextfunction creates a partial mock. Consider usingvi.fn()return type assertions or a more complete mock to catch type mismatches ifCommandContextchanges.
1-426: Comprehensive test coverage for the new modelCommand implementation.The test suite provides excellent coverage across:
- Command metadata verification
- All argument parsing scenarios
- Direct switch vs dialog branching
- Capability flag combinations
- Error handling paths
This is a well-structured test file that will catch regressions effectively.
packages/core/src/models/registry.ts (5)
19-24: LGTM! Well-defined constants.The timeout, cache, and refresh interval constants are reasonable defaults with clear comments explaining their purpose.
73-83: LGTM! Race condition fix properly applied.The initialization logic correctly wraps the async operations in an IIFE, ensuring
initializedis set totruebeforestartBackgroundRefresh()runs. This prevents the inconsistent state identified in the previous review.
139-178: LGTM! Timeout cleanup properly handled.The
clearTimeout(timeout)is now in afinallyblock (line 176), ensuring the timeout is cleared regardless of whether the fetch succeeds, fails, or throws. This addresses the previous review concern about the timeout not being cleared on error paths.
261-297: Search method filters correctly with AND logic.The
searchmethod properly applies all filters cumulatively. The non-null assertion onquery.capability!(line 269) is safe because it's guarded by theif (query.capability)check.
382-402: Singleton pattern is clean and properly exported.The lazy singleton initialization via
getModelRegistry()and the async initializerinitializeModelRegistry()provide a clean API for consumers.packages/cli/src/ui/components/ModelDialog.tsx (5)
52-58: LGTM! Clean context formatting helper.The
formatContextfunction handles edge cases well (undefined, millions, thousands) and produces human-readable output.
129-202: Well-structured model loading with cancellation and deduplication.The effect properly handles:
- Cancellation via the
cancelledflag- Tracking fetched providers to avoid redundant API calls
- Parallel fetching with
Promise.allSettled- Deduplication when merging models
However,
state.providerFilterin the dependency array may cause unnecessary re-fetches when switching between filters if models were already loaded.
256-259: Missing dependency in useEffect.The effect resets
selectedIndexandscrollOffsetwhen filters change, butstate.providerFilteris both read in the dependency array and set viasetState. This is correct for the intended behavior but could be clearer with a comment.
291-421: Comprehensive keyboard handling.The
handleKeypresscallback handles all expected interactions: escape, tab mode switching, Ctrl+A provider toggle, enter selection, arrow navigation with scroll, and text input. The logic is correct and well-organized.
509-627: Well-structured dialog layout with responsive design.The component properly handles narrow vs wide displays, shows loading state, and provides helpful UI hints. The scroll indicator and legend are useful additions for the user experience.
packages/core/src/models/hydration.ts (5)
29-49: LGTM! Well-documented hydration data interface.The
ModelHydrationDatainterface clearly documents all optional hydration fields with appropriate comments. Thehydratedboolean flag provides a clear indicator of hydration status.
62-69: JSDoc now matches implementation.The previous review noted a mismatch between JSDoc ("returns null if no mapping found") and implementation (fallback to
[providerName]). This has been fixed - the function now returnsnullwhen no mapping is found (line 68), matching the JSDoc.
89-94: Variable shadowing fix applied.The previous review identified that
modelswas shadowed inside the loop. This has been fixed - the inner variable is now namedproviderModels(line 92).
158-207: Well-designed scoring algorithm for partial matching.The
calculateMatchScorefunction implements a sensible scoring strategy:
- Exact match: 100 points
- Prefix/suffix with separator: 80 points
- Token overlap: proportional up to 60 points
This addresses the previous review concern about false positives with a threshold-based approach.
213-238: Scoring-based matching with threshold properly implemented.The
findPartialMatchfunction correctly implements the scoring approach suggested in the previous review:
- Iterates all entries to find the best match
- Returns
undefinedif no match exceedsMATCH_SCORE_THRESHOLD(50)This prevents false positives from overly loose matching.
packages/core/src/models/provider-integration.ts (3)
22-52: Provider ID mapping is comprehensive.The
PROVIDER_ID_MAPcovers a wide range of providers and aliases. The mapping ofgeminito['google', 'google-vertex']correctly handles the multi-provider scenario.One consideration: this map will need maintenance as new providers are added or models.dev naming changes.
58-67: llxprtModelToIModel uses modelId correctly.The function properly uses
model.modelId(the short ID like "gpt-4o") rather thanmodel.id(the full "provider/model" format). The comment on line 60 documents this design decision clearly.
136-196: getRecommendedModel is well-implemented.The function:
- Handles uninitialized registry gracefully
- Filters deprecated models
- Applies capability filters correctly
- Sorts by price or context window based on preference
- Returns
undefinedwhen no candidates match
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Build lookup map: modelId -> LlxprtModel | ||
| // Index by multiple keys for flexible matching | ||
| const registryMap = new Map<string, LlxprtModel>(); | ||
| for (const rm of registryModels) { | ||
| registryMap.set(rm.modelId, rm); // Short ID (e.g., "gpt-4o") | ||
| registryMap.set(rm.id, rm); // Full ID (e.g., "openai/gpt-4o") | ||
| registryMap.set(rm.name, rm); // Display name | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the hydration.ts file to understand the context
cat -n packages/core/src/models/hydration.ts | head -120Repository: vybestack/llxprt-code
Length of output: 4437
🏁 Script executed:
# Let's examine the schema to understand LlxprtModel structure
cat -n packages/core/src/models/schema.ts | head -150Repository: vybestack/llxprt-code
Length of output: 5209
🏁 Script executed:
# Continue reading schema.ts to see the complete LlxprtModelSchema definition
cat -n packages/core/src/models/schema.ts | sed -n '148,200p'Repository: vybestack/llxprt-code
Length of output: 2171
🏁 Script executed:
# Now let's find where LlxprtModel instances are created from ModelsDevModel
# Look for the transformer/conversion logic
fd -e ts -e js | xargs grep -l "transformApiResponse\|ModelsDevModel" | head -10Repository: vybestack/llxprt-code
Length of output: 429
🏁 Script executed:
# Examine the transformer to understand how modelId, id, and name are derived
cat -n packages/core/src/models/transformer.tsRepository: vybestack/llxprt-code
Length of output: 5525
🏁 Script executed:
# Let's check the registry implementation to understand how models are stored
cat -n packages/core/src/models/registry.ts | head -100Repository: vybestack/llxprt-code
Length of output: 3592
🏁 Script executed:
# Continue examining registry.ts to find the getByProvider method
cat -n packages/core/src/models/registry.ts | sed -n '100,200p'Repository: vybestack/llxprt-code
Length of output: 3128
🏁 Script executed:
# Continue to find populateModels and getByProvider methods
cat -n packages/core/src/models/registry.ts | sed -n '200,280p'Repository: vybestack/llxprt-code
Length of output: 2556
🏁 Script executed:
# Let's understand what input models look like in hydrateModelsWithRegistry
# Find usages of hydrateModelsWithRegistry to see what kind of models are passed in
rg "hydrateModelsWithRegistry" -A 3 -B 3 | head -50Repository: vybestack/llxprt-code
Length of output: 2070
🏁 Script executed:
# Let's check what IModel looks like and what baseModels contain
cat -n packages/core/src/providers/IModel.ts | head -60Repository: vybestack/llxprt-code
Length of output: 995
🏁 Script executed:
# Let's check the test fixtures to understand the actual data structure
cat -n packages/core/test/models/__fixtures__/mock-data.ts | head -200Repository: vybestack/llxprt-code
Length of output: 5119
🏁 Script executed:
# Let's check the hydration tests to see if there are any collision tests
cat -n packages/core/test/models/hydration.test.tsRepository: vybestack/llxprt-code
Length of output: 140
🏁 Script executed:
# Let's find what test files exist for models
find packages/core/test/models -name "*.test.ts" -o -name "*.spec.ts"Repository: vybestack/llxprt-code
Length of output: 292
🏁 Script executed:
# Let's check the transformer tests to understand how models are tested
cat -n packages/core/test/models/transformer.test.ts | head -200Repository: vybestack/llxprt-code
Length of output: 6727
🏁 Script executed:
# Continue reading transformer tests to see if there are collision tests
cat -n packages/core/test/models/transformer.test.ts | sed -n '200,300p'Repository: vybestack/llxprt-code
Length of output: 3408
🏁 Script executed:
# Continue reading transformer tests to see the full scope
cat -n packages/core/test/models/transformer.test.ts | sed -n '300,400p'Repository: vybestack/llxprt-code
Length of output: 3375
Map collisions with name key when multiple providers indexed.
When hydrateModelsWithRegistry receives multiple provider IDs, the registry map will combine models from different providers. If two models from different providers share the same name (e.g., both called "GPT-4"), the second entry will overwrite the first in the map. This affects the name-based fallback lookup on line 110, potentially returning the wrong model or causing a miss.
However, the primary lookup on line 109 uses model.id (the full provider-qualified ID like "openai/gpt-4o"), which remains collision-free. The name index only matters if the primary lookup fails.
To mitigate the risk fully, consider:
- Using compound keys for
name(e.g.,providerId:name) to preserve all models, or - Document this behavior clearly as an expected limitation of the fallback strategy
🤖 Prompt for AI Agents
In `@packages/core/src/models/hydration.ts` around lines 96 - 103,
hydrateModelsWithRegistry builds registryMap by indexing registryModels with
rm.modelId, rm.id and rm.name which causes name collisions when models from
different providers share the same rm.name; update the indexing so name entries
are provider-scoped (e.g., use a compound key like
`${rm.providerId}:${rm.name}`) or map names to arrays of LlxprtModel to preserve
all candidates, and adjust the fallback lookup logic that uses model.name to
consult the provider-scoped key or choose from the array; ensure you update
references to registryMap.get(...) (the primary lookup by model.id remains
unchanged) so the name-based fallback uses the new compound-key or multi-value
handling.
There was a problem hiding this comment.
Addressed this in a comment. not a issue.
There was a problem hiding this comment.
@sogoiii, understood! Thank you for documenting this as a known limitation. The primary lookup by model.id remains collision-free, and the name-based fallback is a secondary convenience path, so the tradeoff makes sense for the use case.
- Add shared getModelsDevProviderIds() helper in provider-integration.ts - Update hasModelInRegistry, getExtendedModelInfo, getRecommendedModel to use helper - Remove duplicate function from hydration.ts, re-export from provider-integration - Change hydrateModelsWithRegistry signature from string[] | null to string[] - Add documentation note about name-based map collision limitation
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/core/src/models/provider-integration.ts`:
- Around line 94-106: The loop claims to try both full ID and short ID but only
builds and checks fullId; update the loop that iterates providerIds to also
construct the shortId (e.g., modelId alone or providerId === modelId? — use the
intended short form) and call registry.getById(shortId) before returning true,
or if shortId lookup is not desired, change the comment to remove the "short ID"
mention; modify the block around providerIds, fullId, modelId, and
registry.getById accordingly.
In `@packages/core/src/providers/ProviderManager.ts`:
- Around line 869-884: The hardcoded supportedToolFormats: ['openai'] in the
registryModels push is incorrect for non-OpenAI providers; update the code that
builds the registryModels object (the block that pushes objects with
provider!.name and rm.* fields) to derive supportedToolFormats from the provider
or the registry model metadata instead of hardcoding: first check for a
provider-level property like provider.supportedToolFormats, then fall back to
rm.metadata.supportedToolFormats, and finally fallback to ['openai'] only if
neither exists; ensure the new value is used for the supportedToolFormats field
when pushing into registryModels.
♻️ Duplicate comments (2)
packages/core/src/models/hydration.ts (1)
83-96: Name collision limitation is now documented.The comment block (lines 86-90) acknowledges the name-based indexing collision risk and explains why it's acceptable for the current use case. This addresses the past review concern about undocumented behavior.
packages/core/src/models/provider-integration.ts (1)
76-78: Fallback behavior is now consistent via re-export.The past review concern about inconsistent fallback behavior between this file and
hydration.tsis resolved -hydration.tsnow re-exportsgetModelsDevProviderIdsfrom this file (line 56), ensuring consistent behavior.
🧹 Nitpick comments (3)
packages/core/src/providers/ProviderManager.ts (1)
839-848: Consider logging the specific error for debugging.The registry initialization failure is silently caught and logged as a generic message. Capturing the error details would help with troubleshooting.
🔧 Proposed enhancement
try { await initializeModelRegistry(); - } catch { + } catch (error) { // Registry init failed - return unhydrated logger.debug( () => - `[getAvailableModels] Registry init failed for provider: ${provider!.name}`, + `[getAvailableModels] Registry init failed for provider: ${provider!.name}, error: ${error instanceof Error ? error.message : String(error)}`, ); return baseModels.map((m) => ({ ...m, hydrated: false })); }packages/core/src/models/hydration.ts (1)
50-53: Type definition could be more precise abouthydratedfield.
HydratedModelusesPartial<ModelHydrationData>, makinghydratedoptional at the type level. However, the implementation always setshydratedtotrueorfalse. Consider makinghydratedrequired.🔧 Proposed type refinement
-export type HydratedModel = IModel & Partial<ModelHydrationData>; +export type HydratedModel = IModel & + Partial<Omit<ModelHydrationData, 'hydrated'>> & + Pick<ModelHydrationData, 'hydrated'>;Or simpler:
+export type HydratedModel = IModel & { + hydrated: boolean; +} & Partial<Omit<ModelHydrationData, 'hydrated'>>;packages/core/src/models/provider-integration.ts (1)
189-195: Consider defensive access forlimits.contextWindow.Similar to capabilities,
m.limits.contextWindowis accessed without optional chaining. While the outer try/catch handles errors, defensive access would provide cleaner fallback behavior.🔧 Proposed defensive access
// Default: prefer larger context window (usually better models) candidates.sort((a, b) => { - const ctxA = a.limits.contextWindow ?? 0; - const ctxB = b.limits.contextWindow ?? 0; + const ctxA = a.limits?.contextWindow ?? 0; + const ctxB = b.limits?.contextWindow ?? 0; return ctxB - ctxA; });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/models/hydration.tspackages/core/src/models/provider-integration.tspackages/core/src/providers/ProviderManager.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-13T19:28:00.766Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-13T19:28:00.766Z
Learning: In the llxprt-code codebase (`packages/core/src/providers/anthropic/AnthropicProvider.ts`), Anthropic's API returns `contentBlock.input` as an already-parsed JavaScript object, not a JSON string. The code was incorrectly calling `JSON.stringify(contentBlock.input)` before passing it to `processToolParameters()`, which was designed for OpenAI-style string parameters. This causes arrays and other complex types to be corrupted into strings (e.g., `paths` array becomes a string `"[\"**/*.toml\"]"` instead of actual array). The fix is to use `contentBlock.input` directly without stringifying for Anthropic provider.
Applied to files:
packages/core/src/providers/ProviderManager.ts
📚 Learning: 2025-12-18T14:06:22.557Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-18T14:06:22.557Z
Learning: OpenAIResponsesProvider (packages/core/src/providers/openai-responses/OpenAIResponsesProvider.ts) currently bypasses the ephemeral truncation system by using direct `JSON.stringify(toolResponseBlock.result)` and needs to be updated to support ephemeral settings like the other providers.
Applied to files:
packages/core/src/providers/ProviderManager.ts
📚 Learning: 2025-11-16T22:51:26.374Z
Learnt from: e2720pjk
Repo: vybestack/llxprt-code PR: 583
File: packages/core/src/providers/openai/OpenAIProvider.ts:935-959
Timestamp: 2025-11-16T22:51:26.374Z
Learning: In the llxprt-code codebase (packages/core/src/providers/openai/OpenAIProvider.ts), tools like `run_shell_command` use internal streaming only for real-time UI updates during execution, but each tool execution produces exactly ONE final `ToolResponseBlock` containing the full result. The streaming chunks are never sent to the LLM and are not converted into multiple tool messages. The OpenAI Chat Completions API requires that each tool call (tool_call_id) corresponds to exactly one message with role 'tool', so duplicate tool response detection that removes subsequent tool messages with the same tool_call_id is correct and necessary for API compliance.
Applied to files:
packages/core/src/providers/ProviderManager.ts
🧬 Code graph analysis (2)
packages/core/src/models/provider-integration.ts (5)
packages/core/src/models/index.ts (7)
llxprtModelToIModel(86-86)LlxprtModel(57-57)getModelsDevProviderIds(95-95)hasModelInRegistry(87-87)getModelRegistry(33-33)getExtendedModelInfo(88-88)getRecommendedModel(89-89)packages/core/src/models/schema.ts (1)
LlxprtModel(196-196)packages/core/src/providers/IProvider.ts (1)
IModel(123-123)packages/core/src/models/hydration.ts (1)
getModelsDevProviderIds(56-56)packages/core/src/models/registry.ts (1)
getModelRegistry(388-393)
packages/core/src/providers/ProviderManager.ts (4)
packages/core/src/models/hydration.ts (3)
HydratedModel(53-53)getModelsDevProviderIds(56-56)hydrateModelsWithRegistry(65-129)packages/core/src/models/index.ts (5)
HydratedModel(96-96)initializeModelRegistry(34-34)getModelsDevProviderIds(95-95)getModelRegistry(33-33)hydrateModelsWithRegistry(94-94)packages/core/src/models/registry.ts (2)
initializeModelRegistry(398-402)getModelRegistry(388-393)packages/core/src/models/provider-integration.ts (1)
getModelsDevProviderIds(76-78)
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test (macos-latest, 24.x)
- GitHub Check: Test (windows-latest, 24.x)
- GitHub Check: Test (ubuntu-latest, 24.x)
🔇 Additional comments (9)
packages/core/src/providers/ProviderManager.ts (2)
11-19: LGTM - Clean import organization for hydration and registry modules.The imports are well-structured, separating hydration utilities from registry initialization functions.
899-906: LGTM - Hydration flow and filtering are now consistent.The hydration workflow correctly:
- Hydrates models with registry data
- Filters to only include models that don't explicitly disable tool support
The filter logic
toolCalling !== falseis now consistent with the fallback path (line 867), addressing the previous review concern.packages/core/src/models/hydration.ts (3)
65-81: LGTM - Clean function signature and initialization check.The function properly:
- Accepts non-null
modelsDevProviderIdsarray (signature updated per PR commits)- Returns early with unhydrated models when registry is unavailable
- Uses
providerModelsvariable name (addressing past review about shadowing)
151-200: Well-designed scoring algorithm addressing previous false-positive concerns.The scoring system properly prioritizes matches:
- Exact match (100)
- Prefix/suffix with separator (80)
- Token overlap (up to 60, proportional)
The threshold of 50 effectively requires high token similarity when no structural match exists, preventing the false positives flagged in the previous review.
206-231: LGTM - Improved partial matching with best-score selection.The implementation now correctly iterates all registry entries and returns the highest-scoring match above the threshold, addressing the previous review concern about first-match-wins potentially returning wrong models.
packages/core/src/models/provider-integration.ts (4)
22-52: LGTM - Comprehensive provider ID mapping.The mapping covers both internal provider names and display aliases, enabling flexible model lookup across different naming conventions.
58-67: LGTM - Clean model conversion.Using
model.modelId(short ID) forIModel.idis the correct choice as noted in the comment, maintaining consistency with how providers identify models.
113-137: LGTM - Clean implementation with proper error handling.The try/catch pattern correctly handles registry unavailability. The function follows the same lookup pattern as
hasModelInRegistry.
169-175: No action needed. Thecapabilitiesfield is guaranteed to exist onLlxprtModelinstances because the Zod schema defines it as a required field (not optional), and all models returned from the registry are validated against this schema. The type system ensures that accessingm.capabilities.toolCallingandm.capabilities.reasoningis safe.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Fix misleading comment in hasModelInRegistry (only checks full ID, not short) - Change supportedToolFormats from hardcoded ['openai'] to empty array
1 participant
Purpose
Integrate models.dev API as the authoritative source for AI model metadata. This enables intelligent model discovery with capability filtering directly from the
/modelcommand:Summary
Unified Model Fetching Flow
All model access now goes through a single path ensuring consistent data across UI surfaces:
Key Changes
Core Models Registry (packages/core/src/models/)
CLI Changes (packages/cli/src/)
/modelcommand with capability filteringBuild Scripts
Architecture
Images
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.