feat: integrate models.dev as dynamic model source#1117
feat: integrate models.dev as dynamic model source#1117tommy-ca wants to merge 7 commits intoeyaltoledano:nextfrom
Conversation
- Update AI SDK from v4.3.16 to v5.0.0-beta.34 - Add ai-sdk-provider-claude-code integration - Replace custom SDK with official ai-sdk-provider-claude-code package - Add OAuth token authentication via Claude Code CLI - Support for 'sonnet' and 'opus' models - Comprehensive integration tests and documentation - Direct integration test script for verification 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Updates @anthropic-ai/claude-code to latest version - Ensures compatibility with latest Claude Code CLI features - Maintains optional dependency status for graceful degradation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
WalkthroughThis update introduces dynamic AI model discovery in Task Master by integrating the external models.dev API, replacing the previous static model list. It adds new service and merging layers, implements caching, updates CLI commands with filtering options, and enhances Claude Code provider integration. Documentation and comprehensive tests are included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI/TaskMaster
participant ConfigManager
participant ModelsDevService
participant ModelMerger
participant StaticModels
User->>CLI/TaskMaster: Request available models (e.g., via CLI)
CLI/TaskMaster->>ConfigManager: getAllAvailableModels()
ConfigManager->>ModelsDevService: searchModels()
ModelsDevService->>ModelsDevService: Check cache (24h TTL)
alt Cache valid
ModelsDevService-->>ConfigManager: Return cached dynamic models
else Cache expired or missing
ModelsDevService->>ModelsDevService: Fetch from models.dev API
ModelsDevService->>ModelsDevService: Save to cache
ModelsDevService-->>ConfigManager: Return dynamic models
Note right of ModelsDevService: On failure, fallback to stale cache or error
end
ConfigManager->>StaticModels: getAvailableModels()
ConfigManager->>ModelMerger: mergeStaticAndDynamic(static, dynamic)
ModelMerger-->>ConfigManager: Merged models
ConfigManager-->>CLI/TaskMaster: All available models
CLI/TaskMaster-->>User: Display models (with CLI filters if specified)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
- Add ModelsDevService for API integration with 24h caching - Add ModelMerger for intelligent static/dynamic model deduplication - Update config-manager with getAllAvailableModels() async function - Maintain backward compatibility with existing CLI interface - Add comprehensive integration tests and documentation - Support graceful fallback to static models when API unavailable 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add missing haiku model definition to static sources - Ensures all three Claude Code models (opus, sonnet, haiku) are properly supported - Resolves validation warning when setting haiku as main model - SWE score: 40.0%, maxTokens: 200000, free pricing
🔧 Latest Updates - Bug Fixes & RefinementsRecent commits added important fixes:✅ Fix: Resolve empty log lines and claude-code model availability (86b9d4c)Issues Resolved:
Before: After: ✅ All Tests Passing
These refinements ensure a polished user experience with the new models.dev integration. |
tommy-ca
force-pushed
the
feature/models-dev-minimal
branch
from
956d088 to
86b9d4c
Compare
- Remove haiku model from claude-code provider (only supports opus/sonnet) - Fix empty [INFO] log lines in models command output by adding proper log levels - Update models-dev-service.js: add log level parameters to all log calls - Update model-merger.js: add log level parameters to all log calls 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
tommy-ca
force-pushed
the
feature/models-dev-minimal
branch
from
86b9d4c to
2bdc226
Compare
🧹 Cleaned Up CommitUpdated commit 2bdc226: Removed ✅ What's Included:
❌ What's Excluded:
The PR is now clean and focused on the core feature and bug fixes without including development environment artifacts. |
- Add --filter-provider option to show models from specific providers - Add --exclude-provider option to hide models from providers (supports multiple) - Add --only-free option to show only free models - Add --only-paid option to show only paid models - Filters can be combined for refined model lists - Updated help documentation with filtering examples - Maintains backward compatibility with existing functionality Examples: task-master models --filter-provider claude-code task-master models --exclude-provider bedrock --exclude-provider openrouter task-master models --only-free task-master models --filter-provider anthropic --only-paid 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
🎛️ New Feature: Provider & Cost FiltersLatest commit 682b114: Added comprehensive filtering capabilities to the ✨ New Filter Options:🔍 Provider Filters:
💰 Cost Filters:
🚀 Key Features:
📋 Usage Examples:# Show only Anthropic models
task-master models --filter-provider anthropic
# Show only Claude Code models
task-master models --filter-provider claude-code
# Exclude multiple providers
task-master models --exclude-provider bedrock --exclude-provider openrouter
# Show only free models
task-master models --only-free
# Combine filters: free Anthropic models
task-master models --filter-provider anthropic --only-freePerfect for users who want clean, focused model lists without the overwhelming full catalog! 🎯 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 37
🔭 Outside diff range comments (2)
package.json (1)
13-27: Add the full test script set per repository standardsPer coding_guidelines, include test:unit, test:integration, and test:ci scripts alongside existing ones.
"scripts": { "test": "node --experimental-vm-modules node_modules/.bin/jest", + "test:unit": "node --experimental-vm-modules node_modules/.bin/jest tests/unit", + "test:integration": "node --experimental-vm-modules node_modules/.bin/jest tests/integration", "test:watch": "node --experimental-vm-modules node_modules/.bin/jest --watch", "test:coverage": "node --experimental-vm-modules node_modules/.bin/jest --coverage", "test:e2e": "./tests/e2e/run_e2e.sh", + "test:ci": "node --experimental-vm-modules node_modules/.bin/jest --ci --coverage", "test:e2e-report": "./tests/e2e/run_e2e.sh --analyze-log",tests/unit/ai-providers/claude-code.test.js (1)
76-87: Expand assertions to validate commandName handling (optional)You can assert that getClient accepts commandName without throwing, which you do. If feasible, also assert createClaudeCode was invoked once (ensuring integration wiring).
it('should handle commandName parameter', () => { const client = provider.getClient({ commandName: 'test-command' }); expect(client).toBeDefined(); + const { createClaudeCode } = jest.mocked(await import('ai-sdk-provider-claude-code')); + expect(createClaudeCode).toHaveBeenCalledTimes(1); });Note: If adding the above, move the import to a beforeAll and reference its mocked instance here to keep the test body synchronous.
| # Claude Code Integration Guide | ||
|
|
||
| This guide covers how to use Task Master with Claude Code AI SDK integration for enhanced AI-powered development workflows. | ||
|
|
||
| ## Overview | ||
|
|
||
| Claude Code integration allows Task Master to leverage the Claude Code CLI for AI operations without requiring direct API keys. The integration uses OAuth tokens managed by the Claude Code CLI itself. | ||
|
|
||
| ## Authentication Setup | ||
|
|
||
| The Claude Code provider uses token authentication managed by the Claude Code CLI. |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix markdown spacing and code-fence language for top section
Headings and fenced blocks should be separated by blank lines, and fences should have a language.
-# Claude Code Integration Guide
-
-This guide covers how to use Task Master with Claude Code AI SDK integration for enhanced AI-powered development workflows.
-
-## Overview
+# Claude Code Integration Guide
+
+This guide covers how to use Task Master with Claude Code AI SDK integration for enhanced AI-powered development workflows.
+
+## OverviewAnd for the first fenced block:
- ```
+ ```bash
# Installation method depends on your system
# Follow Claude Code documentation for installation
```Apply similarly throughout to address MD022/MD031/MD040.
🧰 Tools
🪛 LanguageTool
[grammar] ~1-~1: Use correct spacing
Context: # Claude Code Integration Guide This guide covers how to use Task Master...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~3-~3: Use correct spacing
Context: ...ion for enhanced AI-powered development workflows. ## Overview Claude Code integration allows...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~7-~7: Use correct spacing
Context: ...h tokens managed by the Claude Code CLI itself. ## Authentication Setup The Claude Code pr...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~9-~9: Use correct spacing
Context: ...ude Code CLI itself. ## Authentication Setup The Claude Code provider uses token auth...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~11-~11: Use correct spacing
Context: ...thentication managed by the Claude Code CLI. ### Prerequisites 1. **Install Claude Code ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
🪛 markdownlint-cli2 (0.17.2)
3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
6-6: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
7-7: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
🤖 Prompt for AI Agents
In docs/claude-code-integration.md around lines 1 to 11, fix markdown formatting
by ensuring there are blank lines before and after headings and fenced code
blocks. Add the appropriate language identifier (e.g., bash) to all fenced code
blocks. Review the entire document to apply these spacing and code-fence
language fixes consistently to comply with markdown linting rules MD022, MD031,
and MD040.
| 1. **Environment Variable** (optional): `CLAUDE_CODE_OAUTH_TOKEN` | ||
| - Useful for CI/CD environments or when you want to override the default token | ||
| - Not required if you've set up the CLI token | ||
|
|
||
| 2. **Claude Code CLI Token** (recommended): Token managed by `claude setup-token` | ||
| - Automatically used when available | ||
| - Most convenient for local development |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Environment variable naming mismatch with implementation
The doc recommends CLAUDE_CODE_OAUTH_TOKEN, but the provider’s getRequiredApiKeyName() returns CLAUDE_CODE_API_KEY (and isRequiredApiKey() returns false). Align docs and code on a single env var name, or clarify precedence.
Proposed options:
- Update provider to look for CLAUDE_CODE_OAUTH_TOKEN (preferred for clarity with CLI OAuth).
- Or, update this doc to reference CLAUDE_CODE_API_KEY if that’s what code actually honors.
Would you like a patch to wire getClient() to prefer CLAUDE_CODE_OAUTH_TOKEN?
Also applies to: 67-73
🧰 Tools
🪛 LanguageTool
[grammar] ~35-~35: Use correct spacing
Context: ...his order: 1. Environment Variable (optional): CLAUDE_CODE_OAUTH_TOKEN - Useful for CI/CD environments or when you want...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~36-~36: There might be a mistake here.
Context: ...r when you want to override the default token - Not required if you've set up the CLI to...
(QB_NEW_EN_OTHER)
[grammar] ~37-~37: There might be a mistake here.
Context: ...- Not required if you've set up the CLI token 2. Claude Code CLI Token (recommended): T...
(QB_NEW_EN_OTHER)
[grammar] ~40-~40: Insert the missing word
Context: ...tup-token` - Automatically used when available - Most convenient for local developm...
(QB_NEW_EN_OTHER_ERROR_IDS_32)
[grammar] ~41-~41: There might be a mistake here.
Context: ...vailable - Most convenient for local development 3. Fallback: Error if neither is availabl...
(QB_NEW_EN_OTHER)
🤖 Prompt for AI Agents
In docs/claude-code-integration.md around lines 35 to 41 and also lines 67 to
73, the environment variable name CLAUDE_CODE_OAUTH_TOKEN mentioned in the
documentation does not match the environment variable CLAUDE_CODE_API_KEY used
in the provider code. To fix this, either update the provider code to use
CLAUDE_CODE_OAUTH_TOKEN for consistency with the CLI OAuth naming or revise the
documentation to reference CLAUDE_CODE_API_KEY to align with the current
implementation. Choose one naming convention and ensure both the code and docs
consistently use it, clarifying any precedence if needed.
| 3. **Test Task Master integration**: | ||
| ```bash | ||
| task-master models --test claude-code:sonnet | ||
| # Should successfully connect and test the model | ||
| ``` |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify CLI command existence: task-master models --test
The guide references a models --test subcommand. Confirm this exists; otherwise, replace with an existing health-check command or add the command.
🏁 Script executed:
#!/bin/bash
# Search for a --test flag on the models command
rg -n "models\s+--test" -- tests src scripts
rg -n "command\(['\"]models['\"]" -A 20Length of output: 1338
Replace the nonexistent --test flag with the interactive --setup command
The models --test option isn’t implemented in the CLI. Update the documentation (docs/claude-code-integration.md:147–151) to use the existing --setup command—or, if you intended to provide a health-check flag, implement --test in the models command.
Locations to update:
- docs/claude-code-integration.md (lines 147–151)
Suggested diff:
- 3. **Test Task Master integration**:
- ```bash
- task-master models --test claude-code:sonnet
- # Should successfully connect and test the model
- ```
+ 3. **Configure and verify Task Master integration**:
+ ```bash
+ task-master models --setup
+ # Follow the interactive prompts to configure your provider/model
+ # and confirm connectivity
+ ```🧰 Tools
🪛 LanguageTool
[grammar] ~147-~147: There might be a mistake here.
Context: ... is valid 3. **Test Task Master integration**: bash task-master models --test claude-code:sonnet # Should successfully connect and test the model ``` 4. Check logs: - Task Master logs...
(QB_NEW_EN_OTHER)
🤖 Prompt for AI Agents
In docs/claude-code-integration.md around lines 147 to 151, the documentation
incorrectly uses the nonexistent `--test` flag with the `task-master models`
command. Replace the `--test` flag usage with the interactive `--setup` command
and update the instructions to indicate following the interactive prompts to
configure and verify the provider/model connectivity.
| ## Goals | ||
| 1. **Dynamic Model Discovery**: Replace static model lists with live data from models.dev API | ||
| 2. **Enhanced Model Information**: Provide richer model metadata (pricing, capabilities, context limits) | ||
| 3. **Zero Breaking Changes**: Maintain existing CLI interface and behavior | ||
| 4. **Graceful Degradation**: Fall back to static models if models.dev is unavailable | ||
| 5. **Performance**: Cache models.dev data with appropriate TTL to avoid API rate limits | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Minor wording: avoid tautology “CLI interface”
Use “CLI” instead of “CLI interface” and keep consistent capitalization (“API”, “TTL”), plus tidy spacing around headings.
-3. **Zero Breaking Changes**: Maintain existing CLI interface and behavior
+3. **Zero Breaking Changes**: Maintain existing CLI behavior
-5. **Performance**: Cache models.dev data with appropriate TTL to avoid API rate limits
+5. **Performance**: Cache models.dev data with an appropriate TTL to avoid API rate limits📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## Goals | |
| 1. **Dynamic Model Discovery**: Replace static model lists with live data from models.dev API | |
| 2. **Enhanced Model Information**: Provide richer model metadata (pricing, capabilities, context limits) | |
| 3. **Zero Breaking Changes**: Maintain existing CLI interface and behavior | |
| 4. **Graceful Degradation**: Fall back to static models if models.dev is unavailable | |
| 5. **Performance**: Cache models.dev data with appropriate TTL to avoid API rate limits | |
| ## Goals | |
| 1. **Dynamic Model Discovery**: Replace static model lists with live data from models.dev API | |
| 2. **Enhanced Model Information**: Provide richer model metadata (pricing, capabilities, context limits) | |
| 3. **Zero Breaking Changes**: Maintain existing CLI behavior | |
| 4. **Graceful Degradation**: Fall back to static models if models.dev is unavailable | |
| 5. **Performance**: Cache models.dev data with an appropriate TTL to avoid API rate limits |
🧰 Tools
🪛 LanguageTool
[grammar] ~7-~7: Use proper capitalization
Context: ...el lists with live data from models.dev API 2. Enhanced Model Information: Provide ri...
(QB_NEW_EN_OTHER_ERROR_IDS_6)
[grammar] ~8-~8: There might be a mistake here.
Context: ...etadata (pricing, capabilities, context limits) 3. Zero Breaking Changes: Maintain existi...
(QB_NEW_EN_OTHER)
[style] ~9-~9: This phrase is redundant (‘I’ stands for ‘interface’). Use simply “CLI”.
Context: ...o Breaking Changes**: Maintain existing CLI interface and behavior 4. *Graceful Degradation...
(ACRONYM_TAUTOLOGY)
[grammar] ~9-~9: There might be a mistake here.
Context: ...**: Maintain existing CLI interface and behavior 4. Graceful Degradation: Fall back to sta...
(QB_NEW_EN_OTHER)
[grammar] ~10-~10: There might be a mistake here.
Context: ... back to static models if models.dev is unavailable 5. Performance: Cache models.dev data wit...
(QB_NEW_EN_OTHER)
[grammar] ~11-~11: There might be a mistake here.
Context: ... with appropriate TTL to avoid API rate limits ## Current State Analysis ### Existing Mod...
(QB_NEW_EN_OTHER)
🤖 Prompt for AI Agents
In docs/SPEC-models-dev-integration.md around lines 6 to 12, replace the phrase
"CLI interface" with just "CLI" to avoid tautology, ensure consistent
capitalization of terms like "API" and "TTL", and adjust spacing around headings
for better formatting and readability.
| #### Enhanced getAvailableModels() | ||
| ```typescript | ||
| async function getAvailableModels(): Promise<EnhancedModel[]> { | ||
| try { | ||
| // Load static models (existing logic) | ||
| const staticModels = loadStaticModels(); | ||
|
|
||
| // Load dynamic models from models.dev | ||
| const dynamicModels = await modelsDevService.fetchModels(); | ||
|
|
||
| // Merge and deduplicate | ||
| return modelMerger.mergeStaticAndDynamic(staticModels, dynamicModels); | ||
| } catch (error) { | ||
| // Fallback to static models | ||
| console.warn('models.dev unavailable, using static models:', error.message); | ||
| return loadStaticModels(); | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Align function naming with implementation: use getAllAvailableModels()
Spec shows enhancing getAvailableModels(), but PR adds a new async getter getAllAvailableModels() and keeps getAvailableModels() unchanged for compatibility. Update the snippet to avoid confusion.
-#### Enhanced getAvailableModels()
+#### New getAllAvailableModels() (Non-breaking enhancement)
```typescript
-async function getAvailableModels(): Promise<EnhancedModel[]> {
+async function getAllAvailableModels(): Promise<EnhancedModel[]> {
try {
// Load static models (existing logic)
const staticModels = loadStaticModels();
// Load dynamic models from models.dev
const dynamicModels = await modelsDevService.fetchModels();
// Merge and deduplicate
return modelMerger.mergeStaticAndDynamic(staticModels, dynamicModels);
} catch (error) {
// Fallback to static models
console.warn('models.dev unavailable, using static models:', error.message);
return loadStaticModels();
}
}
+// getAvailableModels() remains for backward compatibility and returns static models.Confirm doc examples elsewhere use getAllAvailableModels(). I can scan and patch remaining references.
🏁 Script executed:
#!/bin/bash
set -e
echo "Searching for 'getAllAvailableModels' in the codebase:"
rg --color=never -n "getAllAvailableModels" || true
echo
echo "Searching for 'getAvailableModels' references in docs:"
rg --color=never -n "getAvailableModels" docs || true
echo
echo "Searching for 'getAllAvailableModels' references in docs:"
rg --color=never -n "getAllAvailableModels" docs || trueLength of output: 1836
Use getAllAvailableModels() in the spec to match the implementation
The docs currently describe enhancing getAvailableModels(), but the code adds a new non-breaking async getter getAllAvailableModels(). To avoid confusion, update the SPEC and all doc references:
• docs/SPEC-models-dev-integration.md (around line 141):
- Change the section header and function name in the snippet.
- Note that
getAvailableModels()remains for backward compatibility.
• docs/SPEC-models-dev-integration.md (around line 217):
- Update the checklist to reference
getAllAvailableModels().
• All other docs references to getAvailableModels() → review and update where they describe the new dynamic-model flow.
Suggested diff for the snippet in SPEC-models-dev-integration.md:
-#### Enhanced getAvailableModels()
+#### New getAllAvailableModels() (non-breaking enhancement)
```typescript
-async function getAvailableModels(): Promise<EnhancedModel[]> {
+async function getAllAvailableModels(): Promise<EnhancedModel[]> {
try {
// Load static models (existing logic)
const staticModels = loadStaticModels();
// Load dynamic models from models.dev
const dynamicModels = await modelsDevService.fetchModels();
// Merge and deduplicate
return modelMerger.mergeStaticAndDynamic(staticModels, dynamicModels);
} catch (error) {
// Fallback to static models
console.warn('models.dev unavailable, using static models:', error.message);
return loadStaticModels();
}
}
+// Note: getAvailableModels() remains unchanged for backward compatibility and still returns only static models.🧰 Tools
🪛 LanguageTool
[grammar] ~141-~141: Use correct spacing
Context: ...## 3. Integration Points #### Enhanced getAvailableModels() typescript async function getAvailableModels(): Promise<EnhancedModel[]> { try { // Load static models (existing logic) const staticModels = loadStaticModels(); // Load dynamic models from models.dev const dynamicModels = await modelsDevService.fetchModels(); // Merge and deduplicate return modelMerger.mergeStaticAndDynamic(staticModels, dynamicModels); } catch (error) { // Fallback to static models console.warn('models.dev unavailable, using static models:', error.message); return loadStaticModels(); } } #### Backward Compatibility Strategy 1. **Int...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
🤖 Prompt for AI Agents
In docs/SPEC-models-dev-integration.md around lines 141 to 159, update the
function name in the code snippet from getAvailableModels() to
getAllAvailableModels() to match the actual implementation. Change the section
header accordingly and add a note that getAvailableModels() remains unchanged
for backward compatibility, returning only static models. Also, review and
update all other references in this file and elsewhere in the docs where
getAvailableModels() is mentioned in the context of the new dynamic model flow,
replacing them with getAllAvailableModels() as appropriate.
| // Skip tests if no internet connection or API is down | ||
| const isOnline = process.env.NODE_ENV !== 'test' || process.env.TEST_MODELS_DEV === 'true'; | ||
|
|
||
| describe('Models.dev Integration', () => { | ||
| // Only run these tests if explicitly enabled or not in CI | ||
| const runTest = isOnline ? test : test.skip; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid network by default; good gating, but prefer full mocks
Guarding with TEST_MODELS_DEV is fine. For integration in CI, favor mocking service modules to keep tests deterministic and fast.
🤖 Prompt for AI Agents
In tests/integration/models-dev-integration.test.js around lines 12 to 18, the
current setup conditionally skips tests based on environment variables but still
relies on actual network calls. To fix this, replace or supplement the
conditional test gating with full mocks of the external service modules used in
the integration tests. This will avoid real network calls, making tests
deterministic and faster, especially in CI environments.
| runTest('should handle dynamic model loading', async () => { | ||
| const enhanced = await getAllAvailableModels(); | ||
|
|
||
| expect(Array.isArray(enhanced)).toBe(true); | ||
| expect(enhanced.length).toBeGreaterThan(0); | ||
|
|
||
| // Should have models from both sources | ||
| const sources = [...new Set(enhanced.map(m => m.source))]; | ||
| expect(sources).toEqual(expect.arrayContaining(['static'])); | ||
| // May also contain 'models.dev' if API is available | ||
| }, 30000); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Respect mock-first-then-import pattern
Per repo rules, declare mocks before importing the module under test. Convert static imports of getAllAvailableModels to dynamic import after mocks to ensure isolation.
🤖 Prompt for AI Agents
In tests/integration/models-dev-integration.test.js around lines 80 to 90, the
test imports getAllAvailableModels statically before setting up mocks, violating
the mock-first-then-import pattern. To fix this, remove the static import of
getAllAvailableModels at the top, set up all necessary mocks first, then
dynamically import getAllAvailableModels within the test or before running tests
to ensure mocks are applied correctly and isolation is maintained.
| test('should fallback gracefully when API fails', async () => { | ||
| // Mock API failure | ||
| const originalFetch = global.fetch; | ||
| global.fetch = jest.fn().mockRejectedValue(new Error('Network error')); | ||
|
|
||
| try { | ||
| const enhanced = await getAllAvailableModels(); | ||
|
|
||
| // Should still return models (may be cached or static) | ||
| expect(Array.isArray(enhanced)).toBe(true); | ||
| expect(enhanced.length).toBeGreaterThan(0); | ||
|
|
||
| // All models should have a source property | ||
| expect(enhanced.every(m => m.source)).toBe(true); | ||
| } finally { | ||
| global.fetch = originalFetch; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
🛠️ Refactor suggestion
Mock the service module, not global.fetch
modelsDevService may not use global.fetch. Mock the module before imports (ESM) to force an error and validate fallback.
Use ESM pre-mock + dynamic import:
// At top of file (before any imports that pull config-manager)
import { jest } from '@jest/globals';
jest.unstable_mockModule('../../src/services/models-dev-service.js', () => ({
modelsDevService: {
fetchModels: jest.fn(async () => { throw new Error('Network error'); }),
getProviderModels: jest.fn(async () => { throw new Error('Network error'); }),
searchModels: jest.fn(async () => { throw new Error('Network error'); }),
}
}));
const { getAllAvailableModels } = await import('../../scripts/modules/config-manager.js');
test('should fallback gracefully when API fails', async () => {
const enhanced = await getAllAvailableModels();
expect(Array.isArray(enhanced)).toBe(true);
expect(enhanced.length).toBeGreaterThan(0);
expect(enhanced.every(m => m.source)).toBe(true);
});I can refactor the file to this pattern if you prefer.
🤖 Prompt for AI Agents
In tests/integration/models-dev-integration.test.js around lines 92 to 109, the
test mocks global.fetch which may not affect the actual API calls since
modelsDevService likely does not use global.fetch directly. Instead, mock the
modelsDevService module itself before importing the module under test using
Jest's unstable_mockModule for ESM support. Replace the global.fetch mock with a
pre-import mock of modelsDevService's methods to throw errors, then dynamically
import getAllAvailableModels and run the test to verify fallback behavior.
| createClaudeCode: jest.fn(() => { | ||
| const provider = (modelId, settings) => ({ | ||
| // Minimal mock language model surface | ||
| id: modelId, | ||
| settings, | ||
| doGenerate: jest.fn(async () => ({ text: 'ok', usage: {} })), | ||
| doStream: jest.fn(async () => ({ stream: true })) | ||
| }); | ||
| provider.languageModel = jest.fn((id, settings) => ({ id, settings })); | ||
| provider.chat = provider.languageModel; | ||
| return provider; | ||
| }) | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make mock methods synchronous to comply with test guidelines
Tests should avoid async ops. These mocks can be synchronous without changing expectations.
- doGenerate: jest.fn(async () => ({ text: 'ok', usage: {} })),
- doStream: jest.fn(async () => ({ stream: true }))
+ doGenerate: jest.fn(() => ({ text: 'ok', usage: {} })),
+ doStream: jest.fn(() => ({ stream: true }))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| createClaudeCode: jest.fn(() => { | |
| const provider = (modelId, settings) => ({ | |
| // Minimal mock language model surface | |
| id: modelId, | |
| settings, | |
| doGenerate: jest.fn(async () => ({ text: 'ok', usage: {} })), | |
| doStream: jest.fn(async () => ({ stream: true })) | |
| }); | |
| provider.languageModel = jest.fn((id, settings) => ({ id, settings })); | |
| provider.chat = provider.languageModel; | |
| return provider; | |
| }) | |
| })); | |
| createClaudeCode: jest.fn(() => { | |
| const provider = (modelId, settings) => ({ | |
| // Minimal mock language model surface | |
| id: modelId, | |
| settings, | |
| doGenerate: jest.fn(() => ({ text: 'ok', usage: {} })), | |
| doStream: jest.fn(() => ({ stream: true })) | |
| }); | |
| provider.languageModel = jest.fn((id, settings) => ({ id, settings })); | |
| provider.chat = provider.languageModel; | |
| return provider; | |
| }) | |
| })); |
🤖 Prompt for AI Agents
In tests/unit/ai-providers/claude-code.test.js between lines 5 and 17, the mock
methods doGenerate and doStream are asynchronous but should be synchronous to
comply with test guidelines. Change these jest.fn implementations to return
their results synchronously instead of using async functions, ensuring the mocks
still return the expected objects without using async/await.
| it('should handle client initialization errors', async () => { | ||
| // Force an error by making createClaudeCode throw | ||
| const { createClaudeCode } = await import( | ||
| '../../../src/ai-providers/custom-sdk/claude-code/index.js' | ||
| 'ai-sdk-provider-claude-code' | ||
| ); | ||
| createClaudeCode.mockImplementationOnce(() => { | ||
| throw new Error('Mock initialization error'); | ||
| }); | ||
|
|
||
| // Create a new provider instance to use the mocked createClaudeCode | ||
| const errorProvider = new ClaudeCodeProvider(); | ||
| expect(() => errorProvider.getClient({})).toThrow( | ||
| 'Mock initialization error' | ||
| ); | ||
| expect(() => errorProvider.getClient({})).toThrow('Mock initialization error'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid async in test body; import the mocked module in setup and keep the test synchronous
Per guidelines, avoid async in tests. Move the dynamic import of the mocked module to beforeAll and perform mockImplementationOnce in the test.
- it('should handle client initialization errors', async () => {
- const { createClaudeCode } = await import('ai-sdk-provider-claude-code');
- createClaudeCode.mockImplementationOnce(() => {
- throw new Error('Mock initialization error');
- });
- const errorProvider = new ClaudeCodeProvider();
- expect(() => errorProvider.getClient({})).toThrow('Mock initialization error');
- });
+ let mockedCreate;
+ beforeAll(async () => {
+ ({ createClaudeCode: mockedCreate } = await import('ai-sdk-provider-claude-code'));
+ });
+ it('should handle client initialization errors', () => {
+ mockedCreate.mockImplementationOnce(() => { throw new Error('Mock initialization error'); });
+ const errorProvider = new ClaudeCodeProvider();
+ expect(() => errorProvider.getClient({})).toThrow('Mock initialization error');
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should handle client initialization errors', async () => { | |
| // Force an error by making createClaudeCode throw | |
| const { createClaudeCode } = await import( | |
| '../../../src/ai-providers/custom-sdk/claude-code/index.js' | |
| 'ai-sdk-provider-claude-code' | |
| ); | |
| createClaudeCode.mockImplementationOnce(() => { | |
| throw new Error('Mock initialization error'); | |
| }); | |
| // Create a new provider instance to use the mocked createClaudeCode | |
| const errorProvider = new ClaudeCodeProvider(); | |
| expect(() => errorProvider.getClient({})).toThrow( | |
| 'Mock initialization error' | |
| ); | |
| expect(() => errorProvider.getClient({})).toThrow('Mock initialization error'); | |
| }); | |
| let mockedCreate; | |
| beforeAll(async () => { | |
| ({ createClaudeCode: mockedCreate } = await import('ai-sdk-provider-claude-code')); | |
| }); | |
| it('should handle client initialization errors', () => { | |
| mockedCreate.mockImplementationOnce(() => { | |
| throw new Error('Mock initialization error'); | |
| }); | |
| const errorProvider = new ClaudeCodeProvider(); | |
| expect(() => errorProvider.getClient({})).toThrow('Mock initialization error'); | |
| }); |
🤖 Prompt for AI Agents
In tests/unit/ai-providers/claude-code.test.js around lines 111 to 123, the test
uses an async function and dynamically imports the mocked module inside the test
body. To fix this, move the import of 'ai-sdk-provider-claude-code' to a
beforeAll hook at the top of the test suite, so it runs once before tests. Then,
keep the test function synchronous and only perform the mockImplementationOnce
inside the test. This avoids async in the test body and follows best practices
for mocking.
Crunchyman-ralph
left a comment
Crunchyman-ralph
left a comment
There was a problem hiding this comment.
watch out, this PR is integrating changes from this PR which are separate concerns: 1114
|
Closing as stale, but I like the base of this @claude can you make a separate fresh PR based on next that tries to do this, pelase be clean, try to write in typescript if you can. make sure it goes where it needs to (apps/cli or packages/tm-bridge ?) Please create a good PR description, title, and make sure to run npm run format before you push and create your PR |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
2 participants
Summary
Implementation Details
getAvailableModels()unchanged for compatibilityKey Features
curl https://models.dev/api.jsonas requestedFiles Changed (7 total)
src/services/models-dev-service.js- Core API integration servicesrc/services/model-merger.js- Static/dynamic model merging logicscripts/modules/config-manager.js- Added getAllAvailableModels() functionscripts/modules/task-manager/models.js- Integrated dynamic model loadingscripts/cache-models.js- Optional cache management utilitytests/integration/models-dev-integration.test.js- Comprehensive test suitedocs/SPEC-models-dev-integration.md- Technical specification🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
modelsCLI command with new filtering options (by provider, free/paid status).Bug Fixes
Documentation
Tests
Chores
.gitignoreto support new features and cache management.