Repository navigation
refactor(types): Centralize type system and extract enums to constants - #224
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughCentralized many type/enums into new constants/types modules, converted numerous interfaces to type aliases, relocated and added CLI/provider/model types, adjusted imports across providers and core, introduced in-memory MCP server APIs in Neurolink, and replaced MCP/memory implementations with simplified stubs and initializers. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant TypesOld as types/index.js
participant Enums as constants/enums.js
Note over CLI,TypesOld: Before
CLI->>TypesOld: import type AIProviderName
Note over CLI,Enums: After
CLI->>Enums: import AIProviderName (value & type)
Provider->>Enums: import AIProviderName (value & type)
sequenceDiagram
participant Client as Consumer
participant Neurolink as Neurolink
participant MemStore as In-Memory Store
participant MCP as MCP API
Client->>Neurolink: addInMemoryMCPServer(id, info)
Neurolink->>MemStore: store(serverId -> MCPServerInfo)
Client->>Neurolink: getInMemoryServers()
Neurolink->>MemStore: return Map
Client->>Neurolink: listMCPs()
Neurolink->>MCP: call stubbed listMCPs() (returns [])
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/lib/mcp/index.ts (1)
13-16: Fix “initialized” always false in getMCPStats.initializeMCPEcosystem never flips a flag, so getMCPStats.initialized remains false after init. Track module state.
+let _initialized = false; + export async function initializeMCPEcosystem(): Promise<void> { - // Simplified initialization - no complex ecosystem needed - return Promise.resolve(); + // Simplified initialization + _initialized = true; } export async function getMCPStats(): Promise<{ initialized: boolean; pluginsDiscovered: number; pluginsBySource: Record<string, number>; availablePlugins: string[]; }> { return { - initialized: false, + initialized: _initialized, pluginsDiscovered: 0, pluginsBySource: {}, availablePlugins: [], }; }Also applies to: 43-55
src/lib/factories/providerRegistry.ts (1)
30-35: Close the race on “registered” with concurrent callers.Two concurrent calls can pass the check before registered flips to true. Flip early and revert on failure.
static async registerAllProviders(): Promise<void> { - if (this.registered) { - return; - } + if (this.registered) return; + // optimistic guard to prevent concurrent double-registration + this.registered = true; try { @@ - logger.debug("All providers registered successfully"); - this.registered = true; + logger.debug("All providers registered successfully"); } catch (error) { logger.error("Failed to register providers:", error); + // rollback guard on failure + this.registered = false; throw error; } }Also applies to: 254-259
src/lib/types/universalProviderOptions.ts (1)
36-39: GenericProviderOptions Omit is a no-op.UniversalProviderOptions has no providerType, so Omit does nothing. Use a union that preserves provider-specific extras without providerType.
-export type GenericProviderOptions = Omit< - UniversalProviderOptions, - "providerType" ->; +export type GenericProviderOptions = + | UniversalProviderOptions + | Omit<ProviderSpecificOptions, "providerType">;src/lib/neurolink.ts (3)
198-209: Duplicate event names with incompatible payloads.Emitting "tool:end" and "tool:start" twice (object and positional) can double-trigger listeners and break handlers expecting a consistent payload.
- // ADD: Bedrock-compatible tool:end event (positional parameters) - this.emitter.emit("tool:end", toolName, success ? result : error); + // Bedrock-compatible: use a distinct event to avoid double dispatch + this.emitter.emit("tool:end:compat", toolName, success ? result : error);- // ADD: Bedrock-compatible tool:start event (positional parameters) - this.emitter.emit("tool:start", toolName, params); + // Bedrock-compatible: use a distinct event to avoid double dispatch + this.emitter.emit("tool:start:compat", toolName, params);Also applies to: 3811-3819
2815-2821: Async iterator detection bug blocks audio streams.Using Symbol.asyncIterator as a string always fails. Check the symbol directly.
- const hasAudio = !!( - options?.input?.audio && - options.input.audio.frames && - typeof (options.input.audio.frames as unknown as Record<string, unknown>)[ - Symbol.asyncIterator as unknown as string - ] !== "undefined" - ); + const hasAudio = !!( + options?.input?.audio && + options.input.audio.frames && + typeof (options.input.audio.frames as any)?.[Symbol.asyncIterator] === "function" + );
3423-3493: Missing await on toolRegistry.registerServer() in registerTool() — confirmed and requires API change.The issue is genuine:
registerServer()is async (returnsPromise<void), yet at line 3481 it's called withoutawait. This creates multiple problems:
- The "tools-register:end" success event (line 3484) fires immediately, before the async operation completes
- Unhandled rejections if
registerServer()fails asynchronously- Inconsistent with other calls (lines 1066, 3682) which correctly await
Making
registerTool()andregisterTools()async is a breaking change to the public API. However, this change is necessary for correctness. The methods must be updated toasyncwithPromise<void>return type, and all callers inregisterTools()(lines 3544, 3549) must await the calls. External code will need to update; check if this aligns with your versioning strategy.- registerTool(name: string, tool: MCPExecutableTool): void { + async registerTool(name: string, tool: MCPExecutableTool): Promise<void> {- this.toolRegistry.registerServer(mcpServerInfo); + await this.toolRegistry.registerServer(mcpServerInfo);- registerTools( + async registerTools( tools: | Record<string, MCPExecutableTool> | Array<{ name: string; tool: MCPExecutableTool }>, - ): void { + ): Promise<void> {- this.registerTool(name, tool); + await this.registerTool(name, tool);
🧹 Nitpick comments (23)
src/lib/providers/litellm.ts (1)
4-4: Use a type-only import for AIProviderName hereThis file only uses AIProviderName in type positions. Importing it as a value is unnecessary and can bloat bundles or introduce circular init risk.
-import { AIProviderName } from "../constants/enums.js"; +import type { AIProviderName } from "../constants/enums.js";src/lib/types/analytics.ts (1)
60-72: Add arrayBuffers to memoryDelta for Node parityNodeJS.MemoryUsage includes arrayBuffers. Adding it improves completeness of deltas.
export type PerformanceMetrics = { startTime: number; endTime?: number; duration?: number; memoryStart: NodeJS.MemoryUsage; memoryEnd?: NodeJS.MemoryUsage; memoryDelta?: { rss: number; heapTotal: number; heapUsed: number; external: number; + arrayBuffers?: number; }; };src/lib/types/cli.ts (1)
651-668: Align with the “type aliases over interfaces” refactor (namespaced setup types)For consistency with this PR’s pattern, consider converting these interface declarations to type aliases.
Example for one namespace:
export namespace OpenAISetup { - export interface SetupOptions { + export type SetupOptions = { checkOnly?: boolean; interactive?: boolean; - } + }; - export interface SetupArgv { + export type SetupArgv = { check?: boolean; nonInteractive?: boolean; - } + }; - export interface Config { + export type Config = { apiKey?: string; organization?: string; model?: string; isReconfiguring?: boolean; - } + }; }Also applies to: 673-689, 694-710, 715-734, 739-763, 768-785, 790-796, 801-807
src/cli/loop/optionsSchema.ts (1)
10-30: I need to get better visibility into the actual type definitions to verify the review comment. Let me extract the full definitions:Based on my research into TypeScript's
satisfiesoperator andRecordtype behavior, I can now provide the rewritten review comment.The web search confirms that using a union of keys in
Record<>is restrictive and TypeScript requires all keys from the union to be present in the object literal—missing any key results in a compilation error. Additionally, usingsatisfieswithPartial<Record<>>allows you to verify that a value satisfies a type while preserving its narrow inferred type.Consider using
satisfieswithPartial<Record<...>>to allow optional key coverageThe current
Record<keyof Omit<...>, OptionSchema>annotation requires an entry for every remainingTextGenerationOptionskey. If new keys are added to the type, this breaks compilation. Usingsatisfiesmeans "this value satisfies this type" while keeping it as narrow as possible, making it resilient to new type additions.-export const textGenerationOptionsSchema: Record< - keyof Omit< - TextGenerationOptions, - | "prompt" - | "input" - | "schema" - | "tools" - | "context" - | "conversationHistory" - | "conversationMessages" - | "conversationMemoryConfig" - | "originalPrompt" - | "middleware" - | "expectedOutcome" - | "evaluationCriteria" - | "region" - | "csvOptions" - >, - OptionSchema -> = { +export const textGenerationOptionsSchema = { /* ... */ -}; +} satisfies Partial< + Record< + keyof Omit< + TextGenerationOptions, + | "prompt" + | "input" + | "schema" + | "tools" + | "context" + | "conversationHistory" + | "conversationMessages" + | "conversationMemoryConfig" + | "originalPrompt" + | "middleware" + | "expectedOutcome" + | "evaluationCriteria" + | "region" + | "csvOptions" + >, + OptionSchema + > +>;src/lib/types/mcpTypes.ts (1)
232-247: LGTM! Well-structured MCP status type.The new MCPStatus type effectively consolidates MCP initialization state and metrics. The index signature
[key: string]: unknownprovides flexibility for future extensions, though it reduces type safety. Consider whether all dynamic properties can be made explicit optional fields for better type checking.If all potential fields are known, consider replacing the index signature with explicit optional properties:
export type MCPStatus = { mcpInitialized: boolean; totalServers: number; // ... other fields ... error?: string; - [key: string]: unknown; // Add index signature for flexible object access + // Add specific optional fields as needed instead of index signature };This would provide better autocomplete and type checking while maintaining flexibility.
src/lib/mcp/index.ts (1)
28-38: Tighten executeMCP signature; mark stubs deprecated.The function always throws; advertise that in the type and deprecate the stubbed API to guide callers.
-export async function executeMCP<T = unknown>( +/** + * @deprecated MCP ecosystem was removed. This function always throws. + */ +export async function executeMCP( _name: string, _config: unknown, _args: unknown, _context?: { sessionId?: string; userId?: string; }, -): Promise<T> { +): Promise<never> { throw new Error("MCP execution not available - ecosystem removed"); }src/lib/types/hitlTypes.ts (2)
74-76: Avoid Node-only type for timeout handles.Use ReturnType for cross-platform compatibility (Node, browsers, Deno).
- /** Timeout handle for cleanup */ - timeoutHandle: NodeJS.Timeout; + /** Timeout handle for cleanup */ + timeoutHandle: ReturnType<typeof setTimeout>;
61-83: Consider renaming ‘arguments’ fields to avoid shadowing and improve clarity.The property name ‘arguments’ (in multiple HITL payloads/logs) can be confused with the function-scoped ‘arguments’ object and some linters flag it. Prefer args, params, or toolArgs. Optional, but improves DX.
Also applies to: 106-145, 151-178, 202-237
src/lib/factories/providerRegistry.ts (1)
128-134: Use centralized enums for default model names.Stay consistent with constants/enums.ts. Replace the string literal with the OpenAIModels enum already imported.
- "gpt-4o-mini", + OpenAIModels.GPT_4O_MINI,Also consider swapping other hard-coded model strings (e.g., Anthropic, Vertex) to their enums for consistency and typo safety.
src/lib/types/universalProviderOptions.ts (2)
112-116: Preserve literal type for providerType.Return the precise discriminant type, not string.
- ): string | null { - return "providerType" in options ? options.providerType : null; + ): ProviderSpecificOptions["providerType"] | null { + return "providerType" in options ? (options as ProviderSpecificOptions).providerType : null;
151-156: Defensive spreads to avoid nullish sources.Some toolchains/polyfills still choke on spreading undefined. Use nullish coalescing for safety.
- context: { ...defaults.context, ...options.context } as + context: { ...(defaults.context ?? {}), ...(options.context ?? {}) } as | BaseContext | undefined, - contextConfig: { ...defaults.contextConfig, ...options.contextConfig }, - metadata: { ...defaults.metadata, ...options.metadata }, + contextConfig: { + ...(defaults.contextConfig ?? {}), + ...(options.contextConfig ?? {}), + }, + metadata: { ...(defaults.metadata ?? {}), ...(options.metadata ?? {}) },src/lib/models/modelRegistry.ts (4)
7-7: Move DEFAULT_MODEL_ALIASES out of types and into constants.Importing a runtime value from ../types/providers.js breaks the “types-only” contract and risks cycles. Prefer ../constants (e.g., ../constants/modelAliases.js). Also confirm this is a value export, not type-only.
397-413: Avoid silent alias collisions and freeze the alias map.Currently later inserts overwrite earlier ones. Guard against collisions and expose an immutable map.
-export const MODEL_ALIASES: Record<string, string> = {}; - -// Build aliases from model data -Object.values(MODEL_REGISTRY).forEach((model) => { - model.aliases.forEach((alias) => { - MODEL_ALIASES[alias.toLowerCase()] = model.id; - }); -}); - -// Pull canonical alias recommendations from core/types -Object.entries(DEFAULT_MODEL_ALIASES).forEach(([k, v]) => { - MODEL_ALIASES[k.toLowerCase().replace(/_/g, "-")] = v; -}); - -MODEL_ALIASES.local = "llama3.2:latest"; +const _ALIASES: Record<string, string> = Object.create(null); + +// Build aliases from model data +for (const model of Object.values(MODEL_REGISTRY)) { + for (const alias of model.aliases) { + const key = alias.toLowerCase(); + if (!(key in _ALIASES)) _ALIASES[key] = model.id; // keep first, avoid silent overwrite + } +} + +// Pull canonical alias recommendations +for (const [k, v] of Object.entries(DEFAULT_MODEL_ALIASES)) { + const key = k.toLowerCase().replace(/_/g, "-"); + if (!(key in _ALIASES)) _ALIASES[key] = v; +} + +// Sensible 'local' default (prefer actual local model if present) +_ALIASES.local = MODEL_REGISTRY["llama3.2:latest"] + ? "llama3.2:latest" + : Object.keys(MODEL_REGISTRY).find((id) => MODEL_REGISTRY[id].isLocal) ?? "llama3.2:latest"; + +export const MODEL_ALIASES: Readonly<Record<string, string>> = Object.freeze(_ALIASES);
495-501: Stable output order for providers (optional).For CLI determinism, consider returning a sorted list.
- return Array.from(providers); + return Array.from(providers).sort();
506-514: Guard cost inputs (optional).Clamp negatives/NaN to 0 to avoid surprising totals.
export function calculateCost( model: ModelInfo, input: number, output: number, ): number { - const inputCost = (input / 1000) * model.pricing.inputCostPer1K; - const outputCost = (output / 1000) * model.pricing.outputCostPer1K; + const safeIn = Number.isFinite(input) && input > 0 ? input : 0; + const safeOut = Number.isFinite(output) && output > 0 ? output : 0; + const inputCost = (safeIn / 1000) * model.pricing.inputCostPer1K; + const outputCost = (safeOut / 1000) * model.pricing.outputCostPer1K; return inputCost + outputCost; }src/lib/neurolink.ts (3)
334-337: Parse env duration defensively.Handle NaN and non-integers.
- this.toolCacheDuration = cacheDurationEnv - ? parseInt(cacheDurationEnv, 10) - : 20000; + const parsed = Number.parseInt(cacheDurationEnv ?? "", 10); + this.toolCacheDuration = Number.isFinite(parsed) ? parsed : 20000;
4344-4356: Provider list should source from a single enum/util.Hardcoding names risks drift (e.g., "googleVertex", "litellm"). Derive from AIProviderName or getAvailableProviders().
- const providers = [ - "openai","bedrock","vertex","googleVertex","anthropic","azure", - "google-ai","huggingface","ollama","mistral","litellm", - ] as const; + const providers = await this.getAvailableProviders();
5219-5286: Guard Redis-only storeToolExecutions.Method assumes RedisConversationMemoryManager. Add a type guard to avoid runtime errors when memory backend differs.
- const redisMemory = this - .conversationMemory as RedisConversationMemoryManager; + const redisMemory = this + .conversationMemory as RedisConversationMemoryManager; + if ( + !redisMemory || + redisMemory.constructor?.name !== "RedisConversationMemoryManager" + ) { + return; // Not using Redis-backed memory; skip + }src/lib/types/modelTypes.ts (5)
8-8: Use a type-only import for AIProviderName.Avoid pulling runtime enum into the bundle when only used as a type.
-import { AIProviderName } from "../constants/enums.js"; +import type { AIProviderName } from "../constants/enums.js";
44-73: ProviderConfiguration.provider should use AIProviderName.Stronger typing prevents drift and invalid values.
-export type ProviderConfiguration = { +export type ProviderConfiguration = { /** Provider name */ - provider: string; + provider: AIProviderName; @@ };
78-90: Schema/name mismatch: displayName vs name.ModelConfig uses name, but ModelConfigSchema uses displayName. Align or support both to avoid divergent configs.
Option A (align to name):
-export const ModelConfigSchema = z.object({ - id: z.string(), - displayName: z.string(), +export const ModelConfigSchema = z.object({ + id: z.string(), + name: z.string(),Option B (back-compat: accept either, normalize later):
export const ModelConfigSchema = z.object({ id: z.string(), name: z.string().optional(), displayName: z.string().optional(), // ... }); // At load time, normalize: name = name ?? displayNameAlso applies to: 23-39
235-243: Tighten ModelComparison.performance typing.Use keys of ModelPerformance for safer access.
- performance: Record<string, ModelInfo[]>; + performance: Record<keyof ModelPerformance, ModelInfo[]>;
128-132: Currency type (optional).If always USD, tighten to "USD" to prevent typos.
- currency: string; // Always USD for now + currency: "USD";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (58)
src/cli/commands/mcp.ts(1 hunks)src/cli/commands/models.ts(2 hunks)src/cli/commands/setup-google-ai.ts(1 hunks)src/cli/loop/optionsSchema.ts(1 hunks)src/cli/loop/session.ts(1 hunks)src/cli/utils/interactiveSetup.ts(1 hunks)src/lib/constants/enums.ts(1 hunks)src/lib/core/baseProvider.ts(1 hunks)src/lib/core/factory.ts(1 hunks)src/lib/factories/providerFactory.ts(1 hunks)src/lib/factories/providerRegistry.ts(1 hunks)src/lib/hitl/hitlManager.ts(1 hunks)src/lib/hitl/index.ts(0 hunks)src/lib/index.ts(2 hunks)src/lib/mcp/index.ts(1 hunks)src/lib/memory/mem0Initializer.ts(1 hunks)src/lib/middleware/builtin/guardrails.ts(0 hunks)src/lib/middleware/utils/guardrailsUtils.ts(1 hunks)src/lib/models/modelRegistry.ts(1 hunks)src/lib/models/modelResolver.ts(1 hunks)src/lib/neurolink.ts(6 hunks)src/lib/providers/amazonBedrock.ts(1 hunks)src/lib/providers/amazonSagemaker.ts(1 hunks)src/lib/providers/anthropic.ts(1 hunks)src/lib/providers/anthropicBaseProvider.ts(1 hunks)src/lib/providers/azureOpenai.ts(1 hunks)src/lib/providers/googleAiStudio.ts(1 hunks)src/lib/providers/googleVertex.ts(1 hunks)src/lib/providers/huggingFace.ts(1 hunks)src/lib/providers/index.ts(0 hunks)src/lib/providers/litellm.ts(1 hunks)src/lib/providers/mistral.ts(1 hunks)src/lib/providers/ollama.ts(1 hunks)src/lib/providers/openAI.ts(1 hunks)src/lib/providers/openaiCompatible.ts(1 hunks)src/lib/proxy/proxyFetch.ts(2 hunks)src/lib/types/analytics.ts(1 hunks)src/lib/types/cli.ts(2 hunks)src/lib/types/configTypes.ts(2 hunks)src/lib/types/fileTypes.ts(3 hunks)src/lib/types/generateTypes.ts(1 hunks)src/lib/types/guardrails.ts(1 hunks)src/lib/types/hitlTypes.ts(10 hunks)src/lib/types/index.ts(1 hunks)src/lib/types/mcpTypes.ts(1 hunks)src/lib/types/modelTypes.ts(2 hunks)src/lib/types/observability.ts(3 hunks)src/lib/types/providers.ts(4 hunks)src/lib/types/sdkTypes.ts(0 hunks)src/lib/types/streamTypes.ts(1 hunks)src/lib/types/taskClassificationTypes.ts(2 hunks)src/lib/types/universalProviderOptions.ts(4 hunks)src/lib/types/utilities.ts(1 hunks)src/lib/utils/errorHandling.ts(2 hunks)src/lib/utils/modelRouter.ts(2 hunks)src/lib/utils/providerHealth.ts(1 hunks)src/lib/utils/providerSetupMessages.ts(1 hunks)src/lib/utils/providerUtils.ts(1 hunks)
💤 Files with no reviewable changes (4)
- src/lib/middleware/builtin/guardrails.ts
- src/lib/providers/index.ts
- src/lib/hitl/index.ts
- src/lib/types/sdkTypes.ts
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: RajuSudhar
PR: juspay/neurolink#173
File: src/lib/types/index.ts:58-62
Timestamp: 2025-09-17T18:14:34.960Z
Learning: RajuSudhar explained that in the Neurolink codebase, there are multiple ProviderConfig types causing inconsistency. One existing ProviderConfig type better suited the "ProviderConfig" name, so they renamed the less-suitable one to AIModelProviderConfig to free up the name. Adding backward compatibility aliases would worsen naming inconsistency rather than help. The remaining duplicates will be systematically deduplicated in the 07-Types-Module.md TODO as part of their phased refactor approach.
Learnt from: RajuSudhar
PR: juspay/neurolink#174
File: src/lib/mcp/contracts/mcpContract.ts:0-0
Timestamp: 2025-09-28T21:00:08.243Z
Learning: The src/lib/mcp/contracts/mcpContract.ts file was completely removed during the MCP types refactor in PR #174, with its types moved to centralized modules like src/lib/types/mcpTypes.ts and src/lib/types/index.ts.
📚 Learning: 2025-09-17T17:55:15.261Z
Learnt from: RajuSudhar
PR: juspay/neurolink#173
File: src/lib/index.ts:16-16
Timestamp: 2025-09-17T17:55:15.261Z
Learning: In src/lib/types/providers.ts, ProviderConfig was renamed to AIModelProviderConfig to deduplicate type names, as there was an existing ProviderConfig type that better suited the "ProviderConfig" name. This was an intentional breaking change for better type organization.
Applied to files:
src/lib/utils/providerUtils.tssrc/lib/providers/anthropicBaseProvider.tssrc/cli/commands/setup-google-ai.tssrc/cli/utils/interactiveSetup.tssrc/lib/providers/azureOpenai.tssrc/lib/types/providers.tssrc/lib/utils/modelRouter.tssrc/lib/providers/amazonBedrock.tssrc/lib/providers/litellm.tssrc/lib/providers/mistral.tssrc/lib/types/streamTypes.tssrc/lib/core/factory.tssrc/lib/constants/enums.tssrc/lib/providers/anthropic.tssrc/lib/providers/amazonSagemaker.tssrc/lib/utils/providerHealth.tssrc/lib/types/generateTypes.tssrc/lib/factories/providerFactory.tssrc/lib/providers/ollama.tssrc/lib/core/baseProvider.tssrc/lib/providers/googleVertex.tssrc/lib/types/universalProviderOptions.tssrc/lib/providers/openAI.tssrc/lib/utils/providerSetupMessages.tssrc/lib/factories/providerRegistry.tssrc/lib/types/fileTypes.tssrc/lib/providers/huggingFace.tssrc/lib/neurolink.tssrc/lib/types/modelTypes.tssrc/lib/providers/googleAiStudio.tssrc/lib/providers/openaiCompatible.tssrc/lib/index.tssrc/lib/models/modelRegistry.tssrc/lib/models/modelResolver.tssrc/cli/commands/models.ts
📚 Learning: 2025-09-28T21:00:08.243Z
Learnt from: RajuSudhar
PR: juspay/neurolink#174
File: src/lib/mcp/contracts/mcpContract.ts:0-0
Timestamp: 2025-09-28T21:00:08.243Z
Learning: The src/lib/mcp/contracts/mcpContract.ts file was completely removed during the MCP types refactor in PR #174, with its types moved to centralized modules like src/lib/types/mcpTypes.ts and src/lib/types/index.ts.
Applied to files:
src/lib/types/mcpTypes.tssrc/lib/types/index.tssrc/lib/mcp/index.tssrc/cli/commands/mcp.ts
📚 Learning: 2025-09-02T13:50:42.770Z
Learnt from: YasmeenOgo
PR: juspay/neurolink#145
File: src/lib/core/types.ts:0-0
Timestamp: 2025-09-02T13:50:42.770Z
Learning: The APIVersions enum in src/lib/core/types.ts now contains comprehensive API version constants for all major AI providers: Azure OpenAI (latest, stable, legacy), OpenAI (current, beta), Google AI (current, beta), and Anthropic (current). This centralization helps avoid API version drift across the codebase.
Applied to files:
src/lib/providers/anthropicBaseProvider.tssrc/lib/providers/azureOpenai.tssrc/lib/types/providers.tssrc/lib/core/factory.tssrc/lib/constants/enums.tssrc/lib/providers/anthropic.tssrc/lib/types/generateTypes.tssrc/lib/core/baseProvider.tssrc/lib/providers/openAI.tssrc/lib/types/modelTypes.tssrc/lib/providers/openaiCompatible.ts
📚 Learning: 2025-09-17T18:14:34.960Z
Learnt from: RajuSudhar
PR: juspay/neurolink#173
File: src/lib/types/index.ts:58-62
Timestamp: 2025-09-17T18:14:34.960Z
Learning: RajuSudhar explained that in the Neurolink codebase, there are multiple ProviderConfig types causing inconsistency. One existing ProviderConfig type better suited the "ProviderConfig" name, so they renamed the less-suitable one to AIModelProviderConfig to free up the name. Adding backward compatibility aliases would worsen naming inconsistency rather than help. The remaining duplicates will be systematically deduplicated in the 07-Types-Module.md TODO as part of their phased refactor approach.
Applied to files:
src/lib/types/providers.tssrc/lib/core/factory.tssrc/lib/types/modelTypes.tssrc/lib/providers/openaiCompatible.tssrc/lib/index.ts
📚 Learning: 2025-09-01T22:58:39.149Z
Learnt from: sudharsan-juspay
PR: juspay/neurolink#140
File: src/lib/core/types.ts:198-203
Timestamp: 2025-09-01T22:58:39.149Z
Learning: In src/lib/core/types.ts, StreamOptions (imported from streamTypes.js) and StreamingOptions are intentionally different types with different use cases. StreamingOptions is for unified AI requests with multiple provider configurations, while StreamOptions is for individual streaming operations.
Applied to files:
src/lib/types/streamTypes.ts
🧬 Code graph analysis (8)
src/lib/types/providers.ts (1)
src/lib/utils/providerHealth.ts (1)
ProviderHealthCheckOptions(31-37)
src/lib/types/taskClassificationTypes.ts (2)
src/lib/types/index.ts (5)
TaskClassification(82-82)TaskType(81-81)ClassificationScores(83-83)ClassificationStats(84-84)ClassificationValidation(85-85)src/lib/types/sdkTypes.ts (5)
TaskClassification(185-185)TaskType(184-184)ClassificationScores(186-186)ClassificationStats(187-187)ClassificationValidation(188-188)
src/lib/types/configTypes.ts (3)
src/lib/types/hitlTypes.ts (1)
HITLConfig(13-37)src/lib/mcp/toolRegistry.ts (1)
MCPToolRegistry(28-861)src/lib/types/observability.ts (1)
ObservabilityConfig(47-52)
src/lib/constants/enums.ts (1)
src/lib/index.ts (4)
AIProviderName(38-38)BedrockModels(39-39)OpenAIModels(40-40)VertexModels(41-41)
src/lib/types/cli.ts (1)
src/cli/utils/envManager.ts (2)
EnvBackupResult(11-14)EnvUpdateResult(16-22)
src/lib/neurolink.ts (1)
src/lib/types/utilities.ts (1)
Mem0Memory(52-66)
src/lib/types/modelTypes.ts (1)
src/lib/index.ts (1)
AIProviderName(38-38)
src/lib/types/observability.ts (1)
src/lib/index.ts (3)
LangfuseConfig(63-63)OpenTelemetryConfig(64-64)ObservabilityConfig(62-62)
🔇 Additional comments (46)
src/lib/types/fileTypes.ts (1)
77-83: Interface → type alias conversions look goodShapes are preserved; no functional impact. Consistent with the refactor goals.
Also applies to: 110-122
src/lib/types/taskClassificationTypes.ts (1)
14-21: Type alias conversions LGTMTypes are unchanged semantically; aligns with centralized type strategy.
Also applies to: 26-33, 38-47, 52-57
src/cli/loop/session.ts (1)
12-12: LGTM! Clean type import centralization.The move of
OptionSchemato the centralized CLI types module aligns well with the PR's goal of consolidating type definitions for better maintainability.src/lib/providers/anthropic.ts (1)
4-4: LGTM! Enum centralization improves maintainability.Moving
AIProviderNameandAnthropicModelsto the centralized constants module is a solid architectural improvement that will help prevent enum drift across the codebase.src/lib/providers/amazonSagemaker.ts (1)
10-10: LGTM! Consistent with enum centralization pattern.src/lib/providers/googleVertex.ts (1)
17-17: LGTM! Import path updated correctly.src/lib/providers/anthropicBaseProvider.ts (1)
4-4: LGTM! Enum centralization applied consistently.src/lib/providers/amazonBedrock.ts (1)
26-26: LGTM! Import path centralized correctly.src/cli/utils/interactiveSetup.ts (1)
8-8: LGTM! Centralized enum import maintains consistency.The CLI utilities now correctly reference the centralized enum module, improving maintainability across the codebase.
src/lib/types/streamTypes.ts (1)
10-10: LGTM! Type definitions now reference centralized enums.This change ensures that type definitions and runtime code share the same source of truth for enum values, reducing the risk of inconsistencies.
src/lib/utils/providerUtils.ts (1)
8-9: LGTM! Clean enum import centralization.The import path update correctly moves AIProviderName to the centralized constants module while preserving the ProviderError import from the types index. No runtime changes.
src/lib/utils/providerSetupMessages.ts (1)
6-11: LGTM! Provider constants properly centralized.The model enums and API versions are now correctly imported from the centralized constants module, maintaining consistency with the broader refactor.
src/lib/hitl/hitlManager.ts (1)
11-20: LGTM! HITL types successfully relocated.The HITL type imports have been correctly updated to reference the new centralized types module. The relative path adjustment (
./types.js→../types/hitlTypes.js) properly reflects the new module location.src/lib/providers/azureOpenai.ts (1)
4-4: LGTM! Azure provider enum imports updated correctly.The AIProviderName and APIVersions imports are now sourced from the centralized constants module, consistent with the refactor pattern. Based on learnings, APIVersions centralization helps avoid API version drift across the codebase.
src/lib/providers/openAI.ts (1)
4-4: LGTM! OpenAI provider enum import centralized.The AIProviderName import correctly references the centralized constants module, maintaining consistency across all provider implementations.
src/lib/factories/providerFactory.ts (1)
1-4: LGTM! Factory imports properly organized.The import separation is clean: AIProvider type from the types index, AIProviderName enum from the centralized constants. This maintains the proper distinction between type definitions and runtime values.
src/lib/core/baseProvider.ts (1)
18-18: LGTM! BaseProvider enum import centralized.The AIProviderName import correctly references the centralized constants module. This is the foundational change that enables all provider subclasses to consistently use the centralized enums.
src/lib/utils/providerHealth.ts (1)
7-13: LGTM! Clean import consolidation.The enum imports have been successfully moved to the centralized constants module. All runtime usage of these enums throughout the file (e.g.,
AIProviderName.VERTEX,BedrockModels.CLAUDE_3_SONNET) remains compatible with this change.src/lib/providers/googleAiStudio.ts (1)
4-4: LGTM! Import path updated to centralized enums module.The import change aligns with the PR's objective to consolidate enums. The runtime usage of
GoogleAIModels.GEMINI_2_5_FLASHat line 84 remains fully compatible.src/lib/providers/mistral.ts (1)
4-4: LGTM! Centralized enum import.Clean refactor moving AIProviderName to the constants module. Usage throughout the provider (lines 51, 201) is unaffected.
src/cli/commands/setup-google-ai.ts (1)
18-18: LGTM! CLI updated to use centralized enums.The import path change maintains consistency with the project-wide enum consolidation. Runtime usage at line 44 (
GoogleAIModels.GEMINI_2_5_FLASH) works correctly with this change.src/lib/types/generateTypes.ts (1)
7-7: LGTM! Import correctly changed to value import.The transition from
import typetoimportfor AIProviderName is appropriate. While this type is primarily used in type positions (lines 38, 186), importing it as a value allows it to be used for runtime validation or comparison logic elsewhere in the codebase. This aligns with the centralized enum module pattern.src/cli/commands/models.ts (2)
8-8: LGTM! Enum import centralized.AIProviderName correctly moved to the centralized enums module. Type-only import preserved since it's used exclusively in type positions.
23-28: LGTM! Model types consolidated.The consolidation of model-related types (
RecommendationContext,ModelSearchFilters,ModelCapabilities,UseCaseSuitability) into a dedicatedmodelTypes.tsmodule improves type organization. All type usages throughout the file (e.g., lines 39, 502, 586) remain fully compatible.src/lib/providers/ollama.ts (1)
1-1: LGTM! Correctly changed to value import.The change from
import typetoimportis appropriate here, as AIProviderName is used as a runtime value for type assertions (lines 347, 368:"ollama" as AIProviderName). The import path update to the centralized enums module aligns with the PR's objectives.src/lib/middleware/utils/guardrailsUtils.ts (1)
3-9: LGTM! Guardrails types centralized.The import of
ContentFilteringResult,EvaluationActionResult, andPrecallEvaluationResultfrom the dedicated guardrails types module represents good type organization. The return type at line 373 and usage throughoutapplyContentFilteringremain fully compatible with this centralized definition.src/lib/providers/huggingFace.ts (1)
10-10: LGTM! Import centralization applied correctly.The AIProviderName import has been correctly updated to use the centralized constants module, aligning with the PR's goal to consolidate enums.
src/lib/providers/openaiCompatible.ts (1)
4-4: LGTM! Consistent with enum centralization.The import update correctly references the centralized constants module.
src/lib/proxy/proxyFetch.ts (1)
10-10: LGTM! Type successfully centralized.The ParsedProxyConfig type has been correctly moved to the utilities module, and the comment at line 54 clearly documents the change.
Also applies to: 54-54
src/lib/utils/errorHandling.ts (1)
29-40: LGTM! Interface-to-type conversion applied correctly.The conversion from interface to type alias maintains all fields and is consistent with the PR's standardization goal.
src/lib/types/guardrails.ts (1)
130-142: LGTM! New type definition is well-structured.The ContentFilteringResult type provides a clear contract for content filtering operations with appropriate fields for tracking applied filters and statistics.
src/lib/types/utilities.ts (2)
35-47: LGTM! Proxy configuration type centralized correctly.The ParsedProxyConfig type has been successfully moved to the utilities module with all fields intact.
49-66: LGTM! Mem0Memory interface is well-defined.The type provides a comprehensive contract for mem0 memory operations with appropriate method signatures and return types.
src/lib/utils/modelRouter.ts (2)
14-19: LGTM! ModelRoute conversion is correct.The interface-to-type alias conversion maintains all fields and aligns with the PR's standardization effort.
21-30: LGTM! ModelRoutingOptions updated correctly.The conversion to type alias is correct, and the new
requireFastfield is properly integrated into the routing logic (used at lines 124-127) without breaking existing functionality.src/lib/types/configTypes.ts (1)
24-33: LGTM! Well-structured constructor config type.The new
NeurolinkConstructorConfigtype provides a clean, centralized configuration interface for the NeuroLink constructor with appropriate optional fields.src/lib/core/factory.ts (1)
8-9: LGTM! Proper enum centralization.The import reorganization correctly moves
AIProviderNameenum to the centralized constants module while keeping other types in their appropriate location.src/lib/memory/mem0Initializer.ts (2)
9-9: LGTM! Proper type-only import.The type import correctly uses the
typekeyword and references the centralized type definition.
26-52: LGTM! Resilient fallback implementation.The fallback memory pattern provides graceful degradation with appropriate no-op implementations for all Mem0Memory methods, improving error resilience.
src/lib/types/providers.ts (2)
6-13: LGTM! Proper enum centralization.The enum imports correctly reference the new centralized constants module, aligning with the PR's refactoring goals.
51-62: LGTM! Well-structured provider types.The new types (
ProviderStatus,ProviderRegistryOptions,ProviderHealthStatusOptions) provide clear, comprehensive interfaces for provider management functionality.Also applies to: 370-379, 1181-1192
src/lib/constants/enums.ts (1)
1-150: LGTM! Excellent enum centralization.This new constants module successfully centralizes all provider, model, API version, and error-related enums into a single source of truth, significantly improving maintainability and preventing drift across the codebase. The use of string enums provides better debugging experience.
Based on learnings: The centralized APIVersions enum helps avoid API version drift across the codebase, which was a previous concern.
src/lib/types/observability.ts (1)
10-52: LGTM! Consistent interface-to-type conversion.The conversion from
interfacetotypemaintains consistency with the PR's refactoring goals while preserving all field definitions and optionality. Type aliases are appropriate for these configuration types.src/lib/models/modelResolver.ts (1)
20-28: LGTM! Proper type centralization.The import correctly references the new centralized
modelTypes.tsmodule using type-only imports, successfully extracting inline type definitions to a shared location.src/lib/index.ts (1)
204-205: Disregard this review comment; LogLevel is correctly exported as type-only.LogLevel is defined as a type-only union (
"debug" | "info" | "warn" | "error") on line 24 of logger.ts, not a runtime enum or const. It is used exclusively for type annotations (e.g., function parameters, class properties). The type-only export in index.ts is the correct pattern and does not break consumers—no code imports or uses LogLevel as a runtime value.Likely an incorrect or invalid review comment.
src/lib/factories/providerRegistry.ts (1)
1-1: Remove this review comment; no circular dependency or import conflict exists.The codebase has no circular dependency between
providerRegistry.tsandproviderFactory.ts. The static import at line 1 is legitimately used inclearRegistrations()(line 273), while the dynamic import at line 36 is used within the asyncregisterAllProviders()method. These serve different purposes and do not create initialization order issues. There is no "cycle risk" to address.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Pull Request Overview
This PR implements a comprehensive type system refactoring that centralizes enums and improves type organization across the codebase. The refactoring establishes clear separation between type definitions and implementation code by:
- Moving all provider and model enums from
src/lib/types/providers.tsto a new centralized locationsrc/lib/constants/enums.ts - Converting interfaces to type aliases throughout the codebase for consistency
- Extracting scattered type definitions to appropriate type modules
- Migrating HITL types to a dedicated type file
Reviewed Changes
Copilot reviewed 58 out of 58 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/constants/enums.ts | New centralized location for all enums (AIProviderName, model enums, API versions, error categories) |
| src/lib/types/providers.ts | Removed enum definitions, added new type definitions (ProviderStatus, registry options, health check types) |
| src/lib/types/*Types.ts | Converted interfaces to type aliases for consistency |
| src/lib/types/utilities.ts | Added ParsedProxyConfig and Mem0Memory type definitions |
| src/lib/types/cli.ts | Added extensive CLI-related type definitions previously scattered across CLI modules |
| src/lib/types/modelTypes.ts | Restructured model-related types with new capabilities, pricing, and search types |
| src/lib/providers/*.ts | Updated imports to reference enums from constants/enums.js |
| src/lib/neurolink.ts | Moved internal type definitions to appropriate type modules |
| src/cli/**/*.ts | Updated imports to reference enums from constants/enums.js |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3ce1d11 to
01a9779
Compare
|
@coderabbitai generate |
|
@RajuSudhar I'll review the changes in this PR for you. 🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/types/providers.ts (1)
39-44: FixProviderNameto use enum values, not keys.Switching
AIProviderNameto a string enum meanskeyof typeof AIProviderNamenow resolves to the member names ("BEDROCK","OPENAI", …), whereas the rest of the code (and the enum values themselves) use the lower-case string values ("bedrock","openai", …). Any assignment likeconst provider: ProviderName = AIProviderName.BEDROCKnow fails, breaking compilation across the codebase. Update the alias to reference the enum values instead of the keys.-export type ProviderName = keyof typeof AIProviderName; +export type ProviderName = + (typeof AIProviderName)[keyof typeof AIProviderName];
🧹 Nitpick comments (1)
src/lib/types/modelTypes.ts (1)
156-164: Consider adding type-level constraints for scoring scalesThe
UseCaseSuitabilitytype defines numeric scores with a comment indicating "1-10 scale", but the type itself doesn't enforce this constraint. Similarly, line 205 usesscore: numberwithout bounds.While runtime validation might exist elsewhere, you could consider using branded types or Zod schemas to enforce these constraints at the type level for better type safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (59)
src/cli/commands/mcp.ts(1 hunks)src/cli/commands/models.ts(2 hunks)src/cli/commands/setup-google-ai.ts(1 hunks)src/cli/loop/optionsSchema.ts(1 hunks)src/cli/loop/session.ts(1 hunks)src/cli/utils/envManager.ts(1 hunks)src/cli/utils/interactiveSetup.ts(1 hunks)src/lib/constants/enums.ts(1 hunks)src/lib/core/baseProvider.ts(1 hunks)src/lib/core/factory.ts(1 hunks)src/lib/factories/providerFactory.ts(1 hunks)src/lib/factories/providerRegistry.ts(1 hunks)src/lib/hitl/hitlManager.ts(1 hunks)src/lib/hitl/index.ts(0 hunks)src/lib/index.ts(2 hunks)src/lib/mcp/index.ts(1 hunks)src/lib/memory/mem0Initializer.ts(1 hunks)src/lib/middleware/builtin/guardrails.ts(0 hunks)src/lib/middleware/utils/guardrailsUtils.ts(1 hunks)src/lib/models/modelRegistry.ts(1 hunks)src/lib/models/modelResolver.ts(1 hunks)src/lib/neurolink.ts(6 hunks)src/lib/providers/amazonBedrock.ts(1 hunks)src/lib/providers/amazonSagemaker.ts(1 hunks)src/lib/providers/anthropic.ts(1 hunks)src/lib/providers/anthropicBaseProvider.ts(1 hunks)src/lib/providers/azureOpenai.ts(1 hunks)src/lib/providers/googleAiStudio.ts(1 hunks)src/lib/providers/googleVertex.ts(1 hunks)src/lib/providers/huggingFace.ts(1 hunks)src/lib/providers/index.ts(0 hunks)src/lib/providers/litellm.ts(1 hunks)src/lib/providers/mistral.ts(1 hunks)src/lib/providers/ollama.ts(1 hunks)src/lib/providers/openAI.ts(1 hunks)src/lib/providers/openaiCompatible.ts(1 hunks)src/lib/proxy/proxyFetch.ts(2 hunks)src/lib/types/analytics.ts(1 hunks)src/lib/types/cli.ts(2 hunks)src/lib/types/configTypes.ts(2 hunks)src/lib/types/fileTypes.ts(3 hunks)src/lib/types/generateTypes.ts(1 hunks)src/lib/types/guardrails.ts(1 hunks)src/lib/types/hitlTypes.ts(10 hunks)src/lib/types/index.ts(1 hunks)src/lib/types/mcpTypes.ts(1 hunks)src/lib/types/modelTypes.ts(2 hunks)src/lib/types/observability.ts(3 hunks)src/lib/types/providers.ts(4 hunks)src/lib/types/sdkTypes.ts(0 hunks)src/lib/types/streamTypes.ts(1 hunks)src/lib/types/taskClassificationTypes.ts(2 hunks)src/lib/types/universalProviderOptions.ts(4 hunks)src/lib/types/utilities.ts(1 hunks)src/lib/utils/errorHandling.ts(2 hunks)src/lib/utils/modelRouter.ts(2 hunks)src/lib/utils/providerHealth.ts(23 hunks)src/lib/utils/providerSetupMessages.ts(1 hunks)src/lib/utils/providerUtils.ts(1 hunks)
💤 Files with no reviewable changes (4)
- src/lib/providers/index.ts
- src/lib/types/sdkTypes.ts
- src/lib/hitl/index.ts
- src/lib/middleware/builtin/guardrails.ts
✅ Files skipped from review due to trivial changes (2)
- src/lib/hitl/hitlManager.ts
- src/lib/providers/amazonBedrock.ts
🚧 Files skipped from review as they are similar to previous changes (31)
- src/lib/providers/googleAiStudio.ts
- src/lib/types/mcpTypes.ts
- src/cli/commands/setup-google-ai.ts
- src/lib/types/taskClassificationTypes.ts
- src/lib/middleware/utils/guardrailsUtils.ts
- src/lib/proxy/proxyFetch.ts
- src/lib/providers/openAI.ts
- src/lib/types/configTypes.ts
- src/lib/providers/openaiCompatible.ts
- src/lib/types/fileTypes.ts
- src/lib/types/guardrails.ts
- src/lib/types/streamTypes.ts
- src/lib/types/index.ts
- src/lib/providers/ollama.ts
- src/lib/types/universalProviderOptions.ts
- src/lib/providers/googleVertex.ts
- src/lib/types/observability.ts
- src/lib/utils/providerUtils.ts
- src/cli/loop/session.ts
- src/lib/utils/providerSetupMessages.ts
- src/lib/core/factory.ts
- src/lib/utils/modelRouter.ts
- src/lib/models/modelResolver.ts
- src/lib/providers/anthropicBaseProvider.ts
- src/cli/commands/mcp.ts
- src/cli/utils/interactiveSetup.ts
- src/lib/providers/amazonSagemaker.ts
- src/lib/types/generateTypes.ts
- src/lib/providers/huggingFace.ts
- src/lib/providers/litellm.ts
- src/lib/neurolink.ts
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: RajuSudhar
Repo: juspay/neurolink PR: 173
File: src/lib/types/index.ts:58-62
Timestamp: 2025-09-17T18:14:34.960Z
Learning: RajuSudhar explained that in the Neurolink codebase, there are multiple ProviderConfig types causing inconsistency. One existing ProviderConfig type better suited the "ProviderConfig" name, so they renamed the less-suitable one to AIModelProviderConfig to free up the name. Adding backward compatibility aliases would worsen naming inconsistency rather than help. The remaining duplicates will be systematically deduplicated in the 07-Types-Module.md TODO as part of their phased refactor approach.
Learnt from: RajuSudhar
Repo: juspay/neurolink PR: 173
File: src/lib/types/index.ts:20-33
Timestamp: 2025-09-17T17:57:36.381Z
Learning: RajuSudhar follows a phased refactor approach to avoid merge conflicts - first consolidating types in focused PRs, then addressing import path updates in dedicated refactor tasks like todos/refactor/07-types-module.md.
Learnt from: RajuSudhar
Repo: juspay/neurolink PR: 174
File: src/lib/types/tools.ts:166-171
Timestamp: 2025-09-28T21:09:49.608Z
Learning: RajuSudhar prefers to defer type naming collision fixes to follow-up refactor phases when the primary focus is type consolidation and centralization.
Learnt from: sudharsan-juspay
Repo: juspay/neurolink PR: 160
File: src/lib/providers/index.ts:43-44
Timestamp: 2025-09-10T08:22:11.910Z
Learning: In the Neurolink project, type deduplication across modules (like ProviderName definitions) should be handled as separate tasks rather than mixed with other refactoring efforts, as there are multiple such occurrences throughout the codebase that need systematic cleanup.
Learnt from: RajuSudhar
Repo: juspay/neurolink PR: 173
File: src/lib/index.ts:16-16
Timestamp: 2025-09-17T17:55:15.261Z
Learning: In src/lib/types/providers.ts, ProviderConfig was renamed to AIModelProviderConfig to deduplicate type names, as there was an existing ProviderConfig type that better suited the "ProviderConfig" name. This was an intentional breaking change for better type organization.
Learnt from: RajuSudhar
Repo: juspay/neurolink PR: 174
File: src/lib/mcp/contracts/mcpContract.ts:0-0
Timestamp: 2025-09-28T21:00:08.243Z
Learning: The src/lib/mcp/contracts/mcpContract.ts file was completely removed during the MCP types refactor in PR #174, with its types moved to centralized modules like src/lib/types/mcpTypes.ts and src/lib/types/index.ts.
📚 Learning: 2025-09-17T17:55:15.261Z
Learnt from: RajuSudhar
Repo: juspay/neurolink PR: 173
File: src/lib/index.ts:16-16
Timestamp: 2025-09-17T17:55:15.261Z
Learning: In src/lib/types/providers.ts, ProviderConfig was renamed to AIModelProviderConfig to deduplicate type names, as there was an existing ProviderConfig type that better suited the "ProviderConfig" name. This was an intentional breaking change for better type organization.
Applied to files:
src/cli/loop/optionsSchema.tssrc/lib/factories/providerFactory.tssrc/lib/constants/enums.tssrc/lib/factories/providerRegistry.tssrc/lib/core/baseProvider.tssrc/lib/types/hitlTypes.tssrc/lib/providers/mistral.tssrc/lib/providers/azureOpenai.tssrc/lib/providers/anthropic.tssrc/lib/index.tssrc/lib/models/modelRegistry.tssrc/lib/types/providers.tssrc/lib/utils/providerHealth.tssrc/cli/commands/models.tssrc/lib/types/modelTypes.tssrc/lib/types/cli.tssrc/cli/utils/envManager.tssrc/lib/utils/errorHandling.tssrc/lib/types/utilities.ts
📚 Learning: 2025-09-01T22:58:39.149Z
Learnt from: sudharsan-juspay
Repo: juspay/neurolink PR: 140
File: src/lib/core/types.ts:198-203
Timestamp: 2025-09-01T22:58:39.149Z
Learning: In src/lib/core/types.ts, StreamOptions (imported from streamTypes.js) and StreamingOptions are intentionally different types with different use cases. StreamingOptions is for unified AI requests with multiple provider configurations, while StreamOptions is for individual streaming operations.
Applied to files:
src/cli/loop/optionsSchema.tssrc/lib/factories/providerRegistry.tssrc/lib/providers/azureOpenai.tssrc/lib/index.tssrc/lib/types/providers.ts
📚 Learning: 2025-09-02T13:50:42.770Z
Learnt from: YasmeenOgo
Repo: juspay/neurolink PR: 145
File: src/lib/core/types.ts:0-0
Timestamp: 2025-09-02T13:50:42.770Z
Learning: The APIVersions enum in src/lib/core/types.ts now contains comprehensive API version constants for all major AI providers: Azure OpenAI (latest, stable, legacy), OpenAI (current, beta), Google AI (current, beta), and Anthropic (current). This centralization helps avoid API version drift across the codebase.
Applied to files:
src/cli/loop/optionsSchema.tssrc/lib/factories/providerFactory.tssrc/lib/constants/enums.tssrc/lib/factories/providerRegistry.tssrc/lib/core/baseProvider.tssrc/lib/providers/mistral.tssrc/lib/providers/azureOpenai.tssrc/lib/providers/anthropic.tssrc/lib/index.tssrc/lib/models/modelRegistry.tssrc/lib/types/providers.tssrc/lib/utils/providerHealth.tssrc/cli/commands/models.tssrc/lib/types/modelTypes.tssrc/lib/types/cli.tssrc/lib/types/utilities.ts
📚 Learning: 2025-09-17T18:14:34.960Z
Learnt from: RajuSudhar
Repo: juspay/neurolink PR: 173
File: src/lib/types/index.ts:58-62
Timestamp: 2025-09-17T18:14:34.960Z
Learning: RajuSudhar explained that in the Neurolink codebase, there are multiple ProviderConfig types causing inconsistency. One existing ProviderConfig type better suited the "ProviderConfig" name, so they renamed the less-suitable one to AIModelProviderConfig to free up the name. Adding backward compatibility aliases would worsen naming inconsistency rather than help. The remaining duplicates will be systematically deduplicated in the 07-Types-Module.md TODO as part of their phased refactor approach.
Applied to files:
src/lib/factories/providerFactory.tssrc/lib/constants/enums.tssrc/lib/factories/providerRegistry.tssrc/lib/core/baseProvider.tssrc/lib/providers/mistral.tssrc/lib/providers/azureOpenai.tssrc/lib/providers/anthropic.tssrc/lib/index.tssrc/lib/models/modelRegistry.tssrc/lib/types/providers.tssrc/lib/utils/providerHealth.tssrc/cli/commands/models.tssrc/lib/types/modelTypes.tssrc/lib/types/cli.ts
📚 Learning: 2025-09-28T21:00:08.243Z
Learnt from: RajuSudhar
Repo: juspay/neurolink PR: 174
File: src/lib/mcp/contracts/mcpContract.ts:0-0
Timestamp: 2025-09-28T21:00:08.243Z
Learning: The src/lib/mcp/contracts/mcpContract.ts file was completely removed during the MCP types refactor in PR #174, with its types moved to centralized modules like src/lib/types/mcpTypes.ts and src/lib/types/index.ts.
Applied to files:
src/lib/factories/providerRegistry.tssrc/lib/types/hitlTypes.tssrc/lib/index.tssrc/lib/mcp/index.ts
📚 Learning: 2025-09-10T08:22:11.910Z
Learnt from: sudharsan-juspay
Repo: juspay/neurolink PR: 160
File: src/lib/providers/index.ts:43-44
Timestamp: 2025-09-10T08:22:11.910Z
Learning: In the Neurolink project, type deduplication across modules (like ProviderName definitions) should be handled as separate tasks rather than mixed with other refactoring efforts, as there are multiple such occurrences throughout the codebase that need systematic cleanup.
Applied to files:
src/lib/index.tssrc/lib/types/providers.tssrc/lib/types/cli.ts
📚 Learning: 2025-09-17T17:57:36.381Z
Learnt from: RajuSudhar
Repo: juspay/neurolink PR: 173
File: src/lib/types/index.ts:20-33
Timestamp: 2025-09-17T17:57:36.381Z
Learning: RajuSudhar follows a phased refactor approach to avoid merge conflicts - first consolidating types in focused PRs, then addressing import path updates in dedicated refactor tasks like todos/refactor/07-types-module.md.
Applied to files:
src/lib/types/providers.tssrc/lib/types/cli.ts
📚 Learning: 2025-09-28T21:09:49.608Z
Learnt from: RajuSudhar
Repo: juspay/neurolink PR: 174
File: src/lib/types/tools.ts:166-171
Timestamp: 2025-09-28T21:09:49.608Z
Learning: RajuSudhar prefers to defer type naming collision fixes to follow-up refactor phases when the primary focus is type consolidation and centralization.
Applied to files:
src/lib/types/providers.tssrc/lib/types/cli.ts
📚 Learning: 2025-09-28T21:08:19.655Z
Learnt from: RajuSudhar
Repo: juspay/neurolink PR: 174
File: todos/refactor/03-providers-module.md:3-6
Timestamp: 2025-09-28T21:08:19.655Z
Learning: RajuSudhar mentioned that remaining interfaces in the SageMaker module (30+ interfaces across 5 files) will be resolved in a different PR, but no specific PR was found. The providers module refactor was marked COMPLETED prematurely while significant SageMaker interface conversion work remains pending.
Applied to files:
src/lib/types/providers.ts
🔇 Additional comments (14)
src/lib/types/hitlTypes.ts (1)
1-261: LGTM! Clean type consolidation and migration.The HITL types have been successfully migrated to this centralized location with consistent interface-to-type conversions throughout. The type definitions are well-documented, properly structured, and maintain the original contracts without functional changes. The use of discriminated unions for events,
unknownfor type-safe argument handling, and comprehensive JSDoc comments demonstrates good TypeScript practices.src/lib/types/analytics.ts (1)
60-72: LGTM! Clean type definition for performance tracking.The PerformanceMetrics type provides a well-structured approach to tracking execution time and memory usage, with optional fields appropriately marked.
src/lib/index.ts (1)
35-42: LGTM! Enum centralization aligns with refactor strategy.The import path updates move enums to the centralized
constants/enums.jsmodule, consistent with the PR's type system reorganization. Based on learnings, this phased approach consolidates types first before addressing deduplication tasks.src/lib/providers/anthropic.ts (1)
4-4: LGTM! Import path updated for centralized enums.Consistent with the repository-wide migration of enums to
constants/enums.js.src/lib/providers/azureOpenai.ts (1)
4-4: LGTM! Import path updated for centralized enums.The migration of
AIProviderNameandAPIVersionstoconstants/enums.jshelps centralize API version constants and prevent drift across providers. Based on learnings.src/lib/factories/providerFactory.ts (1)
1-4: LGTM! Import separation reflects type/enum module boundaries.The split between
AIProvider(from types) andAIProviderName(from constants/enums) properly reflects the new module organization where enums are centralized separately from type definitions.src/lib/providers/mistral.ts (1)
4-4: LGTM! Import path updated for centralized enums.Consistent with the repository-wide enum centralization to
constants/enums.js.src/lib/core/baseProvider.ts (1)
18-18: LGTM! Import path updated for centralized enums.The
AIProviderNameimport now sources from the centralizedconstants/enums.jsmodule. SinceBaseProvideris the foundation for all providers, this change ensures consistent enum usage across the provider hierarchy.src/lib/mcp/index.ts (2)
13-23: No callers found for these functions in the codebase.Verification shows
initializeMCPEcosystemandlistMCPsare defined and exported but have no actual callers anywhere in the repository. The functions only appear in their definitions (src/lib/mcp/index.ts) and re-exports (src/lib/index.ts). Since there are no callers to verify, the original concern about verifying caller handling is moot—these are unused exports with no dependencies on their return values.Likely an incorrect or invalid review comment.
28-38: No issues found — this is intentional ecosystem refactoring.The
executeMCPfunction throwing an error is not a breaking change oversight but a deliberate refactoring. All functions insrc/lib/mcp/index.tsare intentionally stubbed with comments labeled "simplified" (includinginitializeMCPEcosystem,listMCPs, andgetMCPStats). SinceexecuteMCPhas no internal callers and exists only as an exported API contract for backward compatibility, this maintains the public API surface while signaling that the ecosystem has been removed. External consumers will need to migrate, but that is the explicit purpose.Likely an incorrect or invalid review comment.
src/lib/utils/providerHealth.ts (2)
7-20: LGTM: Clean import refactoringThe import consolidation correctly references the centralized enums module and the new type definitions. The use of the
typekeyword for type-only imports and.jsextensions maintains ESM compatibility.
23-29: LGTM: Consistent type renaming throughoutThe refactoring consistently updates
ProviderHealthStatustoProviderHealthStatusOptionsacross all method signatures, return types, local variables, and cache storage. The type usage is correct and maintains the same functionality.Also applies to: 60-63, 84-105, 187-189, 246-248, 360-362, 457-459, 606-608, 629-630, 668-669, 696-697, 728-729, 748-749, 767-769, 799-801, 824-826, 851-852, 868-869, 898-899, 970-970, 1691-1691
src/lib/types/modelTypes.ts (2)
8-8: LGTM: Correct import from centralized enumsThe import correctly references
AIProviderNamefrom the centralized enums module with the appropriate relative path and ESM extension.
115-243: LGTM: Well-structured model type systemThe new model types are comprehensive and well-designed:
- Composable types that build logically on each other
- Good use of TypeScript features (keyof, literal types, unions)
- Consistent use of centralized
AIProviderNameenum- Clear JSDoc documentation
The type system provides a solid foundation for model registry, search, and recommendation features.
01a9779 to
3853334
Compare
This commit implements a comprehensive type system refactoring that establishes clear separation of concerns between type definitions and implementation code across the entire codebase. - **Centralized all enums** to `src/lib/constants/enums.ts` - AIProviderName, BedrockModels, OpenAIModels, VertexModels, GoogleAIModels - AnthropicModels, APIVersions, ErrorCategory, ErrorSeverity - **Migrated HITL types** from `src/lib/hitl/types.ts` to `src/lib/types/hitlTypes.ts` - **Extracted scattered types** to appropriate type modules: - Model registry types → `types/modelTypes.ts` - Provider health types → `types/providers.ts` - CLI types → `types/cli.ts` - Guardrails types → `types/guardrails.ts` - Utility types → `types/utilities.ts` - **Consistent relative imports** throughout (`../constants/enums.js`) - **Type-only imports** properly marked with `type` keyword - **ESM compatibility** maintained with `.js` extensions - Added `PerformanceMetrics` to analytics types - Added `ContentFilteringResult` to guardrails types - Added `ProviderRegistryOptions` and `ProviderHealthCheckOptions` - Added `MCPStatus` and `ProviderStatus` to centralized locations - Added `NeurolinkConstructorConfig` to config types - Converted interfaces to type aliases for consistency
3853334 to
5b21c52
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 59 out of 59 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Extract provider names from enum | ||
| */ | ||
| export type ProviderName = keyof typeof AIProviderName; | ||
| export type ProviderName = (typeof AIProviderName)[keyof typeof AIProviderName]; |
There was a problem hiding this comment.
The type definition for ProviderName has changed from keyof typeof AIProviderName to (typeof AIProviderName)[keyof typeof AIProviderName]. This changes the type from a union of the enum keys (e.g., 'BEDROCK' | 'OPENAI' | ...) to a union of enum values (e.g., 'bedrock' | 'openai' | ...). This is a breaking change that could affect consumers expecting enum keys rather than values.
This commit implements a comprehensive type system refactoring that establishes clear separation of concerns between type definitions and implementation code across the entire codebase.
Centralized all enums to
src/lib/constants/enums.tsMigrated HITL types from
src/lib/hitl/types.tstosrc/lib/types/hitlTypes.tsExtracted scattered types to appropriate type modules:
types/modelTypes.tstypes/providers.tstypes/cli.tstypes/guardrails.tstypes/utilities.tsConsistent relative imports throughout (
../constants/enums.js)Type-only imports properly marked with
typekeywordESM compatibility maintained with
.jsextensionsAdded
PerformanceMetricsto analytics typesAdded
ContentFilteringResultto guardrails typesAdded
ProviderRegistryOptionsandProviderHealthCheckOptionsAdded
MCPStatusandProviderStatusto centralized locationsAdded
NeurolinkConstructorConfigto config typesConverted interfaces to type aliases for consistency
Pull Request
Description
Type of Change
Related Issues
Changes Made
AI Provider Impact
Component Impact
Testing
Test Environment
Performance Impact
Breaking Changes
Screenshots/Demo
Checklist
Additional Notes
Summary by CodeRabbit
New Features
Improvements