feat(ai): customize ai llm #1347
Conversation
2 new issues
|
Code reviewFound 1 issue:
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
| import type { Clients, DispatchBody } from './provider-dispatcher'; | ||
| import type { Messages, RemoteToolsApiKeys } from './remote-tools'; | ||
|
|
||
| import { AIUnprocessableError, ProviderDispatcher } from './index'; |
There was a problem hiding this comment.
🟡 MEDIUM - Circular dependency - router imports from index
Category: quality
Description:
router.ts imports from './index', and index.ts exports from './router', creating a circular dependency
Suggestion:
Import directly from './errors' and './provider-dispatcher' instead of from './index' to break the circular dependency
Confidence: 100%
Rule: arch_circular_dependency
packages/ai-proxy/src/router.ts
Outdated
|
|
||
| if (args.route === 'ai-query') { | ||
| return await new ProviderDispatcher(this.aiClients, remoteTools).dispatch( | ||
| args.query.provider, |
There was a problem hiding this comment.
🟠 HIGH - Potential null/undefined access on args.query
Category: bug
Description:
Accessing args.query.provider without checking if args.query is defined. Query parameter is optional (query?: Query) and will cause TypeError if undefined.
Suggestion:
Add optional chaining: args.query?.provider or add null check before accessing
Confidence: 95%
Rule: bug_null_pointer
|
|
||
| if (args.route === 'invoke-remote-tool') { | ||
| return await remoteTools.invokeTool( | ||
| args.query['tool-name'], |
There was a problem hiding this comment.
🟠 HIGH - Potential null/undefined access on args.query
Category: bug
Description:
Accessing args.query['tool-name'] without checking if args.query is defined. Query parameter is optional and will cause TypeError if undefined.
Suggestion:
Add optional chaining: args.query?.['tool-name'] or add null check before accessing
Confidence: 95%
Rule: bug_null_pointer
| this.sourceId = options.sourceId; | ||
| this.sourceType = options.sourceType; |
There was a problem hiding this comment.
🟠 HIGH - Assigning optional parameters to non-optional properties
Category: bug
Description:
Constructor accepts optional sourceId and sourceType but assigns them to non-optional class properties without validation. This will result in undefined values being stored.
Suggestion:
Either make properties optional (sourceId?: string) or provide default values in constructor: this.sourceId = options.sourceId ?? 'unknown'
Confidence: 90%
Rule: bug_missing_null_check
|
|
||
| await mcpClient.loadTools(); | ||
|
|
||
| expect(mcpClient.tools.length).toEqual(0); |
There was a problem hiding this comment.
🔵 LOW - Using toEqual for primitives instead of toBe
Category: quality
Description:
toEqual does deep comparison; use toBe for primitives
Suggestion:
Change expect(mcpClient.tools.length).toEqual(0) to expect(mcpClient.tools.length).toBe(0)
Confidence: 60%
Rule: test_js_expect_toequal_primitives
|
|
||
| await mcpClient.loadTools(); | ||
|
|
||
| expect(mcpClient.tools.length).toEqual(2); |
There was a problem hiding this comment.
🔵 LOW - Using toEqual for primitives instead of toBe
Category: quality
Description:
toEqual does deep comparison; use toBe for primitives
Suggestion:
Change expect(mcpClient.tools.length).toEqual(2) to expect(mcpClient.tools.length).toBe(2)
Confidence: 60%
Rule: test_js_expect_toequal_primitives
| `Failed to load tools from ${errors.length}/${Object.keys(this.mcpClients).length} ` + | ||
| `MCP server(s): ${errorMessage}`, |
There was a problem hiding this comment.
🔵 LOW - String concatenation instead of template literal
Category: quality
Description:
Using + operator for string concatenation instead of template literals reduces readability
Suggestion:
Replace string concatenation with template literal: Failed to load tools from ${errors.length}/${Object.keys(this.mcpClients).length} MCP server(s): ${errorMessage}
Confidence: 85%
Rule: ts_use_template_literals_instead_of_string_
| `Failed to close ${failures.length}/${results.length} MCP connections. ` + | ||
| `This may result in resource leaks.`, |
There was a problem hiding this comment.
🔵 LOW - String concatenation instead of template literal
Category: quality
Description:
Using + operator for string concatenation instead of template literals reduces readability
Suggestion:
Replace string concatenation with template literal: Failed to close ${failures.length}/${results.length} MCP connections. This may result in resource leaks.
Confidence: 85%
Rule: ts_use_template_literals_instead_of_string_
packages/ai-proxy/src/router.ts
Outdated
| // TODO: remove this route because it is no longer needed | ||
| // Do this after the front is removed the refresh button |
There was a problem hiding this comment.
🔵 LOW - TODO comment without ticket reference
Category: quality
Description:
TODO comment lacks ticket reference for tracking
Suggestion:
Add a ticket reference like TODO(#123) or TODO(JIRA-456) to track this work item
Confidence: 65%
Rule: general_todo_without_ticket
|
|
Coverage Impact This PR will not change total coverage. Modified Files with Diff Coverage (7)
🤖 Increase coverage with AI coding...🚦 See full report on Qlty Cloud » 🛟 Help
|
Scra3
force-pushed
the
feature/customize-ai-llm
branch
2 times, most recently
from
f6e938e to
902146f
Compare
Add a new `customizeAiLlm` method to the Agent class that enables AI-powered features through the `/forest/ai-proxy/*` endpoints. This includes: - New `@forestadmin/ai-proxy` package with OpenAI integration and MCP support - `AiLlmConfiguration` type derived from `Clients` type - `AiProvider` type exported for type-safe provider names - Schema metadata update with `aiLlm` field to advertise AI capabilities - New `AiProxyRoute` for handling AI proxy requests with JWT authentication 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…Admin server Add a new service to fetch MCP server configurations from the Forest Admin server endpoint `/api/mcp-server-configs-with-details`. The configurations are then converted and passed to the AI proxy router for each request. - Add `McpServerConfig` type and `getMcpServerConfigs` to `ForestAdminServerInterface` - Create `McpServerConfigService` to fetch configurations - Expose `mcpServerConfigService` in `ForestAdminClient` - Update `AiProxyRoute` to fetch and convert MCP configs per request 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…tom logger Replace the custom Logger type with the Logger from @forestadmin/datasource-toolkit which has the standard signature (level, message, error?). This ensures consistency across all Forest Admin packages. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add customizeAiLlm example usage in agent.ts - Fix import ordering (alphabetical/style) - Update @forestadmin/agent version in example package 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Zod v4 types are incompatible with TypeScript 4.9. Force resolution to zod 3.23.8 to avoid build errors from langchain dependencies pulling in newer zod versions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Upgrade TypeScript from 4.9.4 to 5.6.3 - Upgrade @langchain/core to 1.1.4 - Upgrade @langchain/mcp-adapters to 1.0.3 - Add @langchain/langgraph 1.0.4 as required peer dependency - Remove zod version constraint to allow latest versions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Match the snake_case format used by liana_features and liana_version. Also fix TypeScript 5 strict type error in update-relation.ts. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
The refresh-remote-tools route is no longer needed as MCP tools are loaded fresh on each request. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Simplified API from:
```
.customizeAiLlm({
aiClients: {
openai: {
clientOptions: { apiKey: '...' },
chatConfiguration: { model: 'gpt-4' }
}
}
})
```
To:
```
.customizeAiLlm({
openai: {
apiKey: '...',
model: 'gpt-4'
}
})
```
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Rename customizeAiLlm() to customizeAi() - Rename AiLlmConfiguration to AiConfiguration - Rename aiLlmConfig to aiConfig 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Use OpenAI's ClientOptions type directly instead of a custom type.
This gives users access to all OpenAI SDK options (baseURL, timeout,
organization, etc.) without us having to maintain our own type.
```typescript
.customizeAi({
openai: {
apiKey: process.env.OPENAI_API_KEY,
model: 'gpt-4',
// All OpenAI ClientOptions available
baseURL: 'https://custom.endpoint.com',
timeout: 30000
}
})
```
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Allow configuring multiple AI clients with a name identifier.
Each configuration includes provider, name, and provider-specific options.
```typescript
agent.customizeAi([
{
name: 'gpt4-default',
provider: 'openai',
apiKey: process.env.OPENAI_API_KEY,
model: 'gpt-4'
},
{
name: 'gpt4-fast',
provider: 'openai',
apiKey: process.env.OPENAI_API_KEY,
model: 'gpt-4-turbo',
timeout: 5000
}
]);
```
API route now uses client-name query parameter:
- /ai-query?client-name=gpt4-default
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Remove 'name' field from AiConfiguration (single AI supported for now) - Change customizeAi() to accept single configuration instead of array - Type 'model' field from ChatCompletionCreateParamsNonStreaming - Add AINotConfiguredError for better error handling 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Rename customizeAi() to addAi() for consistency with addDataSource/addChart - Add tests for addAi() method (single call restriction, schema meta) - Update AINotConfiguredError message - Remove fake Mistral configuration - Add mcpServerConfigService to test factory 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Prepare for future multi-LLM support by using an array of objects in schema meta.
- Rename ai_llm to ai_llms (Array<{ provider: string }> | null)
- When AI is configured, returns [{ provider: 'openai' }]
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Changed langchain versions: - @langchain/core: 1.1.4 -> 0.3.79 - @langchain/langgraph: 1.0.4 -> 0.4.9 - @langchain/community: 0.3.57 -> 0.3.58 - @langchain/mcp-adapters: 1.0.3 -> 0.6.0 These versions use zod@^3.25.32 instead of [email protected] which requires TypeScript 5. Since TypeScript 5 is already in use due to zod 3.25+ shipping with v4/ folder. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Reverted langchain packages to latest versions: - @langchain/core: 1.1.4 - @langchain/langgraph: 1.0.4 - @langchain/community: 0.3.57 - @langchain/mcp-adapters: 1.0.3 TypeScript 5 upgrade is required due to zod 4.x dependency. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Replace fishery Factory with simple build() function for ForestAdminServerInterface - Add mcpServerConfig factory - Add ai_llms field to schema test data - Update schema hash expectations - Add getMcpServerConfigs to server interface mock 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Move the AI proxy endpoint from /forest/ai-proxy/* to /forest/_internal/ai-proxy/* to avoid potential conflicts with user-defined routes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Scra3
force-pushed
the
feature/customize-ai-llm
branch
from
b894b11 to
541085a
Compare
- Add tests for AiProxyRoute (constructor, type, setupRoutes, handleAiProxy) - Add tests for McpServerConfigFromApiService.getConfiguration - Add test for ForestHttpApi.getMcpServerConfigs 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add eslint-disable comments for @modelcontextprotocol/sdk imports that require .js extensions, and fix import order in test files. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…ig test 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Cover the case when aiConfiguration is provided to makeRoutes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Definition of Done
General
Security