Repository navigation
refactor(types): mcp module types refactor - #172
RajuSudhar wants to merge 2 commits into
Conversation
- Consolidate all provider-specific types from providerSpecific.ts to providers.ts - Convert 20+ CLI interfaces to type aliases using modern syntax - Remove unused type imports from Amazon Bedrock and Google AI Studio providers - Delete redundant mcp.d.ts declaration file with unused function declarations - Update import paths to use centralized provider types - Maintain backward compatibility while improving type organization - Achieve zero linting errors with clean separation of concerns
• Extract 25+ interfaces from local MCP files to src/lib/types/mcpTypes.ts (763 lines) • Move ToolImplementation, ToolExecutionOptions to src/lib/types/tools.ts • Enhance MCPServerCategory with deployment + application domain categories • Update imports across 20 files: toolRegistry, factory, circuitBreaker, servers • Convert mcpContract.ts to re-export file for backward compatibility • Resolve type signature compatibility issues • Zero functional changes - pure type organization refactor
WalkthroughThis PR centralizes and consolidates TypeScript types across MCP, tools, providers, and CLI modules. It removes a local Claude config, updates .gitignore, migrates many local interfaces to shared type modules, re-exports types from index, refactors MCP registry into a class, updates import paths, and revises provider and CLI type surfaces. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Registry as MCPRegistry
participant Server as DiscoveredMcp
participant Tool as Tool.execute()
rect rgb(235, 245, 255)
note right of Registry: Registration flow
Client->>Registry: registerServer(serverId, config)
Registry->>Registry: create DiscoveredMcp entry
Registry-->>Client: Promise<void>
end
rect rgb(240, 255, 240)
note right of Client: Execution flow
Client->>Registry: executeTool(toolName, args, context)
Registry->>Server: resolve tool
Server->>Tool: execute(params, context)
Tool-->>Server: ToolResult
Server-->>Registry: ToolResult
Registry-->>Client: ToolResult
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
RajuSudhar
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (13)
src/lib/mcp/flexibleToolValidator.ts (1)
52-60: Disallow CR/LF or sanitize before logging to prevent log injectionCurrently CR/LF are allowed and logged verbatim, enabling multi‑line log injection. Either block CR/LF in names or neutralize before logging.
Apply one of these diffs (preferred: both):
- // eslint-disable-next-line no-control-regex - const hasControlCharacters = /[\x00-\x08\x0B\x0C\x0E-\x1F\x7F]/.test( + // eslint-disable-next-line no-control-regex + // Also block CR (\x0D) and LF (\x0A) to avoid log/control-flow injection + const hasControlCharacters = /[\x00-\x08\x0A\x0B\x0C\x0D\x0E-\x1F\x7F]/.test( toolId, );- registryLogger.debug( - `✅ FlexibleToolValidator: Tool '${toolId}' passed universal safety checks`, - ); + const safeToolId = toolId.replace(/[\r\n\t]/g, (c) => ({'\r':'\\r','\n':'\\n','\t':'\\t'}[c])); + registryLogger.debug( + `✅ FlexibleToolValidator: Tool '${safeToolId}' passed universal safety checks`, + );Also applies to: 101-103
todos/refactor/04-cli-module.md (1)
159-160: Align OutputFormat choices and remove redundant d.ts
- File: todos/refactor/04-cli-module.md (lines 159-160) — OutputFormat includes "csv" but the builder choices omit it. Apply one of:
- choices: ["json", "yaml", "text", "table"] as const, + choices: ["json", "yaml", "text", "table", "csv"] as const,—or—
-export type OutputFormat = "json" | "yaml" | "text" | "table" | "csv"; +export type OutputFormat = "json" | "yaml" | "text" | "table";
- Redundant declaration file still present: src/cli/commands/mcp.d.ts (verification returned "Found src/cli/commands/mcp.d.ts") — delete this file.
src/cli/factories/sagemakerCommandFactory.ts (2)
511-518: Critical: Secret credentials printed to console. Mask or remove immediately.
secretAccessKeyandsessionTokenmust never be logged. This is a direct credential leak.Apply masking and avoid printing secrets:
- logger.always(` Access Key: ${aws.accessKeyId}`); - logger.always(` Secret Key: ${aws.secretAccessKey}`); - logger.always(` Session Token: ${aws.sessionToken}`); + const mask = (v?: string) => + typeof v === "string" && v.length > 8 ? `${v.slice(0,4)}••••${v.slice(-4)}` : ""; + logger.always(` Access Key: ${mask(aws.accessKeyId as string)}`); + logger.always(` Secret Key: ${aws.secretAccessKey ? "•••• (hidden)" : ""}`); + logger.always(` Session Token: ${aws.sessionToken ? "•••• (hidden)" : ""}`);Optionally move
maskto a small util for reuse.
296-301: Also mask Access Key in status output.Even access key IDs should be partially masked in user-facing logs.
- logger.always(` Access Key: ${aws.accessKeyId}`); + const mask = (v?: string) => + typeof v === "string" && v.length > 8 ? `${v.slice(0,4)}••••${v.slice(-4)}` : ""; + logger.always(` Access Key: ${mask(aws.accessKeyId as string)}`);src/lib/mcp/mcpClientFactory.ts (1)
581-618: Fix config validation: don’t require command for SSE/WebSocket and don’t require args.Current logic rejects valid non-stdio configs and forces
argsfor stdio. Make checks transport-specific and optional forargs.Apply this diff:
- if (!config.command) { - errors.push("Command is required"); - } + if (config.transport === "stdio" && !config.command) { + errors.push("Command is required for stdio transport"); + } ... - if (config.transport === "stdio") { - if (!Array.isArray(config.args)) { - errors.push("Args array is required for stdio transport"); - } - } + if (config.transport === "stdio" && config.args && !Array.isArray(config.args)) { + errors.push("Args must be an array when provided for stdio transport"); + }src/lib/mcp/toolDiscoveryService.ts (2)
592-635: Handle JSON Schema unions and integers in parameter validation.
schema.typecan be an array (e.g.,["string","null"]) andintegeris common. Current logic may wrongly reject valid inputs.Apply this diff:
- if (!schema.type) { + if (!schema.type) { return; // No type constraint } - - const expectedType = schema.type as string; - const actualType = typeof value; + const typeSpec = schema.type as string | string[]; + const expectedTypes = Array.isArray(typeSpec) ? typeSpec : [typeSpec]; + const actualType = + value === null ? "null" : Array.isArray(value) ? "array" : typeof value; + // If actual type is in the union, accept early + if (expectedTypes.includes(actualType)) return; + // Support "integer" as number subtype + if (expectedTypes.includes("integer") && actualType === "number" && Number.isInteger(value as number)) return; + // Fall through for precise error messages using the first expected type + const expectedType = expectedTypes[0]; switch (expectedType) { case "string": if (actualType !== "string") { throw new Error( `Parameter '${name}' must be a string, got ${actualType}`, ); } break; case "number": if (actualType !== "number") { throw new Error( `Parameter '${name}' must be a number, got ${actualType}`, ); } break; + case "integer": + if (actualType !== "number" || !Number.isInteger(value as number)) { + throw new Error(`Parameter '${name}' must be an integer`); + } + break; case "boolean": if (actualType !== "boolean") { throw new Error( `Parameter '${name}' must be a boolean, got ${actualType}`, ); } break; case "array": if (!Array.isArray(value)) { throw new Error( `Parameter '${name}' must be an array, got ${actualType}`, ); } break; case "object": if (actualType !== "object" || value === null || Array.isArray(value)) { throw new Error( `Parameter '${name}' must be an object, got ${actualType}`, ); } break; + case "null": + if (value !== null) { + throw new Error(`Parameter '${name}' must be null`); + } + break; }
655-663: Do not log full tool responses (PII/perf risk) — replace with a truncated preview and gate behind debug.Logging entire tool responses can leak sensitive data and bloat logs. Replace the logged
fullResponsewith a safe, truncated preview and consider gating the preview behind a debug flag or environment variable.Location: src/lib/mcp/toolDiscoveryService.ts — lines ~656–663 (fullResponse logged at line 661).
- mcpLogger.debug("[ToolDiscoveryService] Tool response received", { - type: typeof result, - isArray: Array.isArray(result), - isObject: isObject(result), - hasKeys: isObject(result) ? Object.keys(result as object).length : 0, - fullResponse: result, // Log the complete response, not a truncated sample - }); + const preview = + typeof result === "string" + ? result.slice(0, 512) + : JSON.stringify( + result, + (_k, v) => + typeof v === "string" && v.length > 256 + ? v.slice(0, 256) + "…(truncated)" + : v, + 0, + ).slice(0, 1024); + mcpLogger.debug("[ToolDiscoveryService] Tool response received", { + type: typeof result, + isArray: Array.isArray(result), + isObject: isObject(result), + hasKeys: isObject(result) ? Object.keys(result as object).length : 0, + preview, + });src/lib/mcp/toolRegistry.ts (2)
280-287: Fix return type: executeTool always returns ToolResult, but signature is Promise.The JSDoc promises ToolResult; current generic casts mask errors and can mislead callers.
Apply:
- async executeTool<T = unknown>( + async executeTool( toolName: string, args?: unknown, context?: ExecutionContext, - ): Promise<T> { + ): Promise<ToolResult> { @@ - return result as T; + return result; @@ - } as T; + } as ToolResult; - return errorResult; + return errorResult;Also applies to: 386-405
773-775: Unify ToolInfo re-export source to centralized types.Re-exporting from contracts risks drift and circularity now that tools are centralized.
Apply:
-export type { ToolInfo } from "./contracts/mcpContract.js"; +export type { ToolInfo } from "../types/tools.js";src/lib/types/index.ts (1)
33-47: Duplicate/conflicting ToolResult export sources (tools vs mcpTypes).ToolResult is imported from tools in this block, while other modules import it from mcpTypes. Single source of truth needed to prevent type conflicts.
Apply:
export type { ToolArgs, ToolContext, - ToolResult, ToolDefinition, SimpleTool, AvailableTool, ToolInfo, ToolExecution, ToolExecutionResult, ValidationResult, ExecutionContext, CacheOptions, FallbackOptions, } from "./tools.js";src/lib/types/providers.ts (2)
690-698: DEFAULT_PROVIDER_CONFIGS references the 3.7 Sonnet entry.Remove it from defaults until a valid public modelId is confirmed to avoid runtime failures.
Apply:
{ provider: AIProviderName.BEDROCK, - models: [BedrockModels.CLAUDE_3_7_SONNET, BedrockModels.CLAUDE_3_5_SONNET], + models: [BedrockModels.CLAUDE_3_5_SONNET], },
47-51: Replace account‑scoped Bedrock ARN with official public modelIdFile: src/lib/types/providers.ts (lines 47–51)
Use the public Bedrock modelId anthropic.claude-3-7-sonnet-20250219-v1:0 instead of the account-scoped ARN — Bedrock model IDs are public API identifiers but availability may be region/account-gated.
export enum BedrockModels { CLAUDE_3_SONNET = "anthropic.claude-3-sonnet-20240229-v1:0", CLAUDE_3_HAIKU = "anthropic.claude-3-haiku-20240307-v1:0", CLAUDE_3_5_SONNET = "anthropic.claude-3-5-sonnet-20240620-v1:0", - CLAUDE_3_7_SONNET = "arn:aws:bedrock:us-east-2:225681119357:inference-profile/us.anthropic.claude-3-7-sonnet-20250219-v1:0", + CLAUDE_3_7_SONNET = "anthropic.claude-3-7-sonnet-20250219-v1:0", }src/lib/types/cli.ts (1)
334-356: Rename CLI GenerateResult → CLIGenerateResult to avoid export collisionBoth src/lib/types/cli.ts (export at line 334) and src/lib/types/generateTypes.ts (export at line 76) export GenerateResult. Rename the CLI variant, update its type guard, and update src/lib/types/index.ts to re-export CLIGenerateResult (and adjust any references).
Apply:
-export type GenerateResult = CommandResult & { +export type CLIGenerateResult = CommandResult & { @@ -export function isGenerateResult(value: unknown): value is GenerateResult { +export function isGenerateResult(value: unknown): value is CLIGenerateResult { @@ - typeof (value as GenerateResult).content === "string" + typeof (value as CLIGenerateResult).content === "string"
🧹 Nitpick comments (32)
src/lib/mcp/servers/aiProviders/aiAnalysisTools.ts (1)
154-155: Harden JSON parsing of provider outputWrap JSON.parse with a targeted error to aid diagnostics and avoid leaking raw parse errors.
- const parsedData = JSON.parse(result.content); + let parsedData: unknown; + try { + parsedData = JSON.parse(result.content); + } catch { + throw new Error("Provider returned non-JSON when JSON was requested for analysis."); + }- const parsedAnalysis = JSON.parse(analysisResult.content); + let parsedAnalysis: unknown; + try { + parsedAnalysis = JSON.parse(analysisResult.content); + } catch { + throw new Error("Provider returned non-JSON when JSON was requested for optimization analysis."); + }Also applies to: 376-377
src/lib/mcp/servers/utilities/utilityServer.ts (1)
278-286: “scientific” mentioned in description but not supported in schemaEither remove “scientific” from description or add support via Intl NumberFormat notation.
- .enum(["decimal", "currency", "percent"]) + .enum(["decimal", "currency", "percent", "scientific"])- const options: Intl.NumberFormatOptions = { - style, - }; + const options: Intl.NumberFormatOptions = { style }; + if (style === "scientific") { + options.style = "decimal"; + options.notation = "scientific"; + }- if (style === "currency") { + if (style === "currency") { options.currency = currency; }- displayString: `${number} formatted as ${formatted}`, + displayString: `${number} formatted as ${formatted}`,Also applies to: 338-345, 354-356, 363-364
src/lib/mcp/servers/aiProviders/aiWorkflowTools.ts (2)
231-234: Harden JSON parsing of AI responses.Providers often return fenced JSON or trailing text. A strict
JSON.parserisks frequent failures.Apply a tolerant extractor before parsing (and reuse across tools):
- const aiResponse = JSON.parse(result.content); + const jsonText = (result.content || "") + .trim() + .replace(/^```(?:json)?\s*/i, "") + .replace(/\s*```$/, ""); + const aiResponse = JSON.parse(jsonText);Replicate in the other parse sites noted in this comment.
Also applies to: 358-360, 489-491, 637-640
239-245: Avoid nondeterministic coverage estimates in tool output.
Math.random()makes tests and analytics flaky. Prefer deterministic calculation or omit the estimate.- coverageEstimate: Math.min(coverageTarget, 80 + Math.random() * 15), + // Derive from returned cases; clamp to target + coverageEstimate: Math.min( + coverageTarget, + 60 + Math.min(testCases.length * 8, 35) + ),src/cli/factories/sagemakerCommandFactory.ts (2)
196-201: Use stronger entropy for session IDs.
Math.random().substris weak andsubstris deprecated. Prefercrypto.randomUUID().- const sessionId = `sagemaker_${Date.now()}_${Math.random().toString(36).substr(2, 9)}`; + const sessionId = `sagemaker_${Date.now()}_${(await import("node:crypto")).randomUUID()}`;
221-259: Validation creates a client but doesn’t actually validate credentials.Consider a fast STS GetCallerIdentity with a short timeout for real verification.
Would you like a small helper that calls STS with a 3–5s timeout and maps common errors?
src/lib/providers/openaiCompatible.ts (2)
309-317: Guard against variant model list shapes.Some OpenAI‑compatible gateways return
{models:[...]}or extended objects. Current code assumesdata[].- const data: ModelsResponse = await response.json(); - if (!data.data || !Array.isArray(data.data)) { + const payload = (await response.json()) as unknown; + const list = Array.isArray((payload as any)?.data) + ? (payload as any).data + : Array.isArray((payload as any)?.models) + ? (payload as any).models + : []; + if (list.length === 0) { logger.warn("Invalid models response format"); return this.getFallbackModels(); } - const models = data.data.map((model) => model.id).filter(Boolean); + const models = list.map((m: any) => m?.id ?? m?.name).filter(Boolean);
341-349: Fallback list mixes non‑OpenAI‑compatible model families.Endpoints may not host Gemini/Claude; consider restricting to OpenAI‑compatible defaults unless endpoint is known.
- return [ - "gpt-4o", - "gpt-4o-mini", - "gpt-4-turbo", - FALLBACK_OPENAI_COMPATIBLE_MODEL, - "claude-3-5-sonnet", - "claude-3-haiku", - "gemini-pro", - ]; + return ["gpt-4o", "gpt-4o-mini", "gpt-4-turbo", FALLBACK_OPENAI_COMPATIBLE_MODEL];src/lib/providers/googleAiStudio.ts (1)
249-291: Live session lifecycle and backpressure.Consider exposing a way to close the session and apply backpressure if consumer stops reading, to avoid buffering.
Provide an AbortSignal in options and call
session.close?.()on abort/finish.src/lib/mcp/mcpCircuitBreaker.ts (1)
257-281: Stats consistency: window vs. all‑time counts.
failureRateis windowed butsuccessfulCalls/failedCallsare all‑time, which can confuse consumers.Either return windowed counts alongside all‑time, or make both windowed:
- totalCalls: this.callHistory.length, - successfulCalls: this.callHistory.filter((call) => call.success).length, - failedCalls: this.callHistory.filter((call) => !call.success).length, + totalCalls: windowCalls.length, + successfulCalls: windowCalls.filter((call) => call.success).length, + failedCalls: windowCalls.filter((call) => !call.success).length,If all‑time metrics are needed, add separate fields (e.g.,
lifetimeSuccessfulCalls).src/lib/mcp/mcpClientFactory.ts (1)
207-211: Remove duplicate command checks.
Command is required for stdio transportis validated twice; keep a single check.Apply either removal from the early or late block to avoid redundancy.
Also applies to: 274-277
src/lib/mcp/factory.ts (2)
23-46: Keep Zod enum in sync withMCPServerCategory.Hard‑coding category literals risks drift from the centralized type. Consider deriving from a single source (e.g., a shared
const categories = [...] as const) and usingz.enum(categories).Confirm
MCPServerCategoryunion exactly matches these literals to avoid validation mismatches at runtime.
207-207: Remove stale comment.“Types are already exported above via export interface declarations” is no longer true after centralization.
-// Types are already exported above via export interface declarations +// (removed)src/lib/mcp/registry.ts (1)
68-93: Type guard forserverConfigto avoid non-plain objects.
typeof serverConfig === "object"will accept arrays/functions. Use a dedicated guard (e.g.,isPlainObject) before reading fields.I can add a small utility and wire it here if you want.
src/lib/mcp/toolDiscoveryService.ts (1)
439-439: Confirm intention: new options type includescontextbut it’s unused.
ExternalToolExecutionOptionscan carrycontext; current execution path ignores it. If intentional, add a brief comment; otherwise, plumb it through.src/lib/mcp/toolRegistry.ts (3)
379-386: Stats should key by resolved toolId (fully-qualified), not by input name.Mixing unqualified names will conflate stats across servers with same tool name.
Apply:
- this.updateStats(toolName, duration); + this.updateStats(toolId, duration);And rename param and usages:
- private updateStats(toolName: string, executionTime: number): void { - const stats = this.toolExecutionStats.get(toolName) || { + private updateStats(toolKey: string, executionTime: number): void { + const stats = this.toolExecutionStats.get(toolKey) || { count: 0, totalTime: 0, }; - this.toolExecutionStats.set(toolName, stats); + this.toolExecutionStats.set(toolKey, stats); }Also applies to: 316-325, 291-303, 535-545, 559-565
455-464: Filter/context discriminator is brittle.Checking for sessionId/userId at top-level misclassifies filters that happen to include those keys.
Use an explicit discriminator:
- if ("sessionId" in filterOrContext || "userId" in filterOrContext) { + if ( + !( + "category" in filterOrContext || + "serverId" in filterOrContext || + "serverCategory" in filterOrContext || + "permissions" in filterOrContext || + "context" in filterOrContext + ) + ) { // It's an ExecutionContext, treat as no filter filter = undefined;
765-767: Stale TODO — remove or update it.FlexibleToolValidator is already implemented at src/lib/mcp/flexibleToolValidator.ts; remove or update the TODO in src/lib/mcp/toolRegistry.ts (lines 765–767) to reflect that.
todos/refactor/05-mcp-module.md (2)
95-108: “Type signature fixes applied” note: specify executeTool return type update or remove claim.executeTool currently returns Promise in code; if the intent is ToolResult, reflect that here after code change.
181-185: FlexibleToolValidator timeline note conflicts with current usage.Docs say it’s a “next task,” but code already imports/uses it. Update the doc to “Done” or remove the note.
Also applies to: 249-256
src/lib/types/cli.ts (1)
5-9: Avoid importing EvaluationData via index to reduce cycles.Import directly from ./evaluation.js for cleaner type deps.
Apply:
-import type { EvaluationData } from "../index.js"; +import type { EvaluationData } from "./evaluation.js";src/lib/types/mcpTypes.ts (6)
80-88: Tighten MCPServerInfo.tools types (schemas/context)Use JsonObject for schemas and ExecutionContext for context to improve type-safety.
tools: Array<{ name: string; description: string; - inputSchema?: object; - execute?: ( - params: unknown, - context?: unknown, - ) => Promise<unknown> | unknown; + inputSchema?: JsonObject; + outputSchema?: JsonObject; + execute?: (params: unknown, context?: ExecutionContext) => Promise<unknown> | unknown; }>;
324-353: Use ExecutionContext in UnifiedMCPRegistry.executeTool signatureCurrent context: JsonObject loses structure and duplicates ExecutionContext elsewhere.
executeTool( toolName: string, params: JsonObject, - context: JsonObject, + context?: ExecutionContext, ): Promise<unknown>;
242-295: Avoid re-specifying common fields; extend ExecutionContextNeuroLinkExecutionContext repeats sessionId/userId/etc. Prefer extending ExecutionContext and only adding extra fields.
-export type NeuroLinkExecutionContext = { +export type NeuroLinkExecutionContext = ExecutionContext & { - // Core identifiers - sessionId?: string; - userId?: string; // AI context aiProvider?: string; modelId?: string; temperature?: number; maxTokens?: number; ... };
180-189: Define MCPToolInfo atop ToolInfo to reduce duplicationLeverage ToolInfo and add MCP-specific fields.
-export type MCPToolInfo = { - name: string; - description: string; - serverId: string; - isExternal: boolean; - isImplemented?: boolean; - inputSchema?: JsonObject; - outputSchema?: JsonObject; - metadata?: MCPToolMetadata; -}; +export type MCPToolInfo = ToolInfo & { + serverId: string; + isExternal: boolean; + isImplemented?: boolean; + inputSchema?: JsonObject; + outputSchema?: JsonObject; + metadata?: MCPToolMetadata; +};
166-175: Date fields in public types — prefer ISO string or epoch for portabilityIf these cross process/JSON boundaries, Date loses fidelity. Consider string (ISO) or number (ms epoch).
49-65: Category proliferation — consider consolidating or namespacingLarge unions increase churn. Consider “domain:” namespacing or an enum map to keep set stable.
src/lib/types/tools.ts (5)
188-193: Avoid duplicating Result fields in ToolResultResult already declares success/data/error. Keep metadata only to prevent divergence.
-export type ToolResult<T = JsonValue> = Result<T, ErrorInfo> & { - success: boolean; - data?: T; - error?: ErrorInfo; - metadata?: ToolResultMetadata; -}; +export type ToolResult<T = JsonValue> = Result<T, ErrorInfo> & { + metadata?: ToolResultMetadata; +};
6-16: Use type-only imports from zod (verbatimModuleSyntax-friendly, no runtime)You use only Zod types. Switch to type imports to avoid pulling zod at runtime.
-import { z } from "zod"; +import type { ZodSchema, ZodObject, ZodRawShape, ZodString } from "zod"; ... -export type ZodAnySchema = z.ZodSchema<unknown>; -export type ZodObjectSchema = z.ZodObject<z.ZodRawShape>; -export type ZodStringSchema = z.ZodString; +export type ZodAnySchema = ZodSchema<unknown>; +export type ZodObjectSchema = ZodObject<ZodRawShape>; +export type ZodStringSchema = ZodString;
115-124: timeout vs timeoutMs duplicationTwo timeout fields with ambiguous units invites bugs. Keep one (prefer timeoutMs to match ExecutionContext).
export type ToolExecutionOptions = { - timeout?: number; retries?: number; context?: unknown; preferredSource?: string; fallbackEnabled?: boolean; validateBeforeExecution?: boolean; timeoutMs?: number; };
256-262: AvailableTool: name vs toolNameHaving both can desync. Either document the distinction or drop toolName and use name consistently.
40-59: Make NeuroLinkExecutionContext extend ExecutionContextExecutionContext (src/lib/types/tools.ts) and NeuroLinkExecutionContext (src/lib/types/mcpTypes.ts) duplicate core fields — change NeuroLinkExecutionContext to extend the base type so MCP only adds its specifics, e.g.
export type NeuroLinkExecutionContext = ExecutionContext & { /* mcp-specific fields */ }.
Files: src/lib/types/tools.ts, src/lib/types/mcpTypes.ts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
.claude/settings.local.json(0 hunks).gitignore(1 hunks)src/cli/factories/sagemakerCommandFactory.ts(1 hunks)src/lib/mcp/contracts/mcpContract.ts(1 hunks)src/lib/mcp/factory.ts(1 hunks)src/lib/mcp/flexibleToolValidator.ts(1 hunks)src/lib/mcp/mcpCircuitBreaker.ts(1 hunks)src/lib/mcp/mcpClientFactory.ts(1 hunks)src/lib/mcp/registry.ts(1 hunks)src/lib/mcp/servers/agent/directToolsServer.ts(1 hunks)src/lib/mcp/servers/aiProviders/aiAnalysisTools.ts(1 hunks)src/lib/mcp/servers/aiProviders/aiCoreServer.ts(1 hunks)src/lib/mcp/servers/aiProviders/aiWorkflowTools.ts(1 hunks)src/lib/mcp/servers/utilities/utilityServer.ts(1 hunks)src/lib/mcp/toolDiscoveryService.ts(2 hunks)src/lib/mcp/toolRegistry.ts(1 hunks)src/lib/providers/amazonBedrock.ts(1 hunks)src/lib/providers/googleAiStudio.ts(2 hunks)src/lib/providers/index.ts(1 hunks)src/lib/providers/openaiCompatible.ts(2 hunks)src/lib/types/cli.ts(14 hunks)src/lib/types/index.ts(2 hunks)src/lib/types/mcpTypes.ts(9 hunks)src/lib/types/providers.ts(1 hunks)src/lib/types/tools.ts(2 hunks)src/lib/utils/parameterValidation.ts(1 hunks)todos/refactor/03-providers-module.md(7 hunks)todos/refactor/04-cli-module.md(1 hunks)todos/refactor/05-mcp-module.md(1 hunks)
💤 Files with no reviewable changes (1)
- .claude/settings.local.json
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-19T06:38:07.850Z
Learnt from: sinha-sahil
PR: juspay/neurolink#81
File: todos/refactor/01-global-imports.md:10-22
Timestamp: 2025-08-19T06:38:07.850Z
Learning: The team plans to update TypeScript's moduleResolution settings to support extensionless imports across the codebase, addressing potential NodeNext ESM runtime issues during the refactor process.
Applied to files:
src/lib/mcp/servers/agent/directToolsServer.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/googleAiStudio.tssrc/lib/providers/openaiCompatible.tssrc/lib/types/providers.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/providers/openaiCompatible.tssrc/lib/types/cli.ts
🧬 Code graph analysis (5)
src/lib/providers/index.ts (1)
src/lib/types/providers.ts (1)
ProviderName(139-139)
src/lib/types/mcpTypes.ts (3)
src/lib/types/tools.ts (3)
ToolResult(188-193)ExecutionContext(44-58)ToolInfo(85-93)src/lib/types/streamTypes.ts (1)
ToolResult(71-85)src/lib/types/typeAliases.ts (1)
StandardRecord(40-40)
src/lib/types/cli.ts (2)
src/lib/types/generateTypes.ts (1)
GenerateResult(76-132)src/lib/types/common.ts (1)
UnknownRecord(13-13)
src/lib/mcp/toolDiscoveryService.ts (1)
src/lib/types/mcpTypes.ts (1)
ExternalToolExecutionOptions(511-523)
src/lib/types/tools.ts (3)
src/lib/types/typeAliases.ts (6)
JsonValue(489-489)StandardRecord(40-40)ToolExecutionResult(430-433)ToolContext(250-254)Result(275-277)ZodUnknownSchema(18-18)src/lib/types/common.ts (3)
JsonValue(23-29)Result(50-54)ErrorInfo(40-45)src/lib/sdk/toolRegistration.ts (2)
ToolContext(101-126)SimpleTool(132-160)
🪛 GitHub Check: 🛡️ Code Quality & Security Gate
src/lib/types/mcpTypes.ts
[failure] 7-7:
'ToolInfo' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
[failure] 7-7:
'ExecutionContext' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
🪛 GitHub Actions: CI
src/lib/types/mcpTypes.ts
[error] 7-7: TS1484: 'ExecutionContext' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled. (Command: npx tsc --noEmit --strict --project tsconfig.ci.json)
🔇 Additional comments (27)
.gitignore (1)
115-117: Ignore Claude Code workspace — looks goodScoped, non-invasive addition.
src/lib/mcp/flexibleToolValidator.ts (1)
14-14: Type import relocation — OKAligned with centralized MCP types.
src/lib/mcp/servers/aiProviders/aiCoreServer.ts (1)
10-13: Type import centralization — OKImports now come from types/mcpTypes; consistent with the refactor.
src/lib/mcp/servers/aiProviders/aiAnalysisTools.ts (1)
12-12: Type import relocation — OKNo runtime impact.
src/lib/mcp/servers/utilities/utilityServer.ts (1)
8-11: Type import centralization — OKConsistent with MCP types consolidation.
src/lib/utils/parameterValidation.ts (1)
15-17: MCP type import centralization — verifiedNo type-only imports remain referencing ../mcp/factory(.js|.ts); src/lib/types/mcpTypes.ts exports NeuroLinkMCPTool, NeuroLinkExecutionContext, and ToolResult. Only a runtime import of dist/lib/mcp/factory.js exists in examples/quick-start (expected).
todos/refactor/03-providers-module.md (1)
40-50: Provider classification: anthropicBaseProvider.ts is a concrete provider — rename or clarifysrc/lib/providers/anthropicBaseProvider.ts exports AnthropicProviderV2 (extends BaseProvider), so it is a concrete provider implementation (not just a base); rename the file or mark it clearly in the list and update the "14 providers" count accordingly.
Likely an incorrect or invalid review comment.
src/lib/mcp/servers/aiProviders/aiWorkflowTools.ts (1)
12-12: Import source switch to centralized types looks good.Path and ESM extension are correct; no runtime impact expected.
Also applies to: 14-14
src/lib/providers/amazonBedrock.ts (2)
29-33: Consolidated Bedrock types import is correct.No API changes; narrows duplication across providers.
Also applies to: 39-39
820-835: ReadableStream availability in Node.Code assumes global Web Streams. On Node <18.5 or custom runtimes this can throw.
If supporting older Node, import from node:stream/web:
+import { ReadableStream } from "node:stream/web";Otherwise, confirm CI/runtime is on Node >= 18.5.
src/lib/mcp/servers/agent/directToolsServer.ts (1)
9-12: Type import path switch LGTM.Keeps MCP types centralized; no runtime impact.
src/lib/providers/openaiCompatible.ts (1)
7-7: ModelsResponse type moved to shared module—good.Improves consistency across providers.
Also applies to: 53-53
src/lib/providers/index.ts (1)
60-63: Helpers are useful; keep them aligned with unified ProviderName.After unifying the type, these utilities remain valid.
Confirm that every
AVAILABLE_PROVIDERSentry is assignable to the centralizedProviderName.Also applies to: 67-69, 74-78
src/lib/providers/googleAiStudio.ts (1)
12-16: Centralizing Google AI Live types—good cleanup.Removes large local type blocks; keeps runtime unchanged.
Also applies to: 31-31
src/lib/mcp/mcpCircuitBreaker.ts (1)
9-15: Type import consolidation LGTM.Removes duplicate in-file type aliases; API unchanged.
src/cli/factories/sagemakerCommandFactory.ts (1)
1-188: Sweep for secret logs — ensure no credentials or full config dumps are ever loggedRedact/omit accessKeyId, secretAccessKey, sessionToken, AWS_; search and remove any logger. / console.* calls, JSON.stringify or util.inspect of config/credentials, and any outputs from getConfigurationSummary, formatDiagnosticReport, runQuickDiagnostics; inspect src/cli/factories/sagemakerCommandFactory.ts (secureCredentialStore).
src/lib/mcp/mcpClientFactory.ts (2)
20-20: Type centralization import looks good.Importing
MCPClientResultfrom shared types aligns with the PR’s goals.Ensure there’s no circular import between
mcpClientFactoryandmcpTypes.jsvia downstream type aliases.
196-293: Critical: don't spawn the stdio process twice — let the transport own the child processcreateStdioTransport currently spawns a ChildProcess and then constructs StdioClientTransport with { command, args } — this duplicates process creation and will leak the manually-spawned process and break cleanup. Remove the manual spawn and let the transport manage the process (or have the transport expose the ChildProcess) so lifecycle/cleanup are consistent.
Location: src/lib/mcp/mcpClientFactory.ts (createStdioTransport)
Apply this diff:
- private static async createStdioTransport( - config: MCPServerInfo, - ): Promise<TransportWithProcessResult> { + private static async createStdioTransport( + config: MCPServerInfo, + ): Promise<TransportResult> { mcpLogger.debug( `[MCPClientFactory] Creating stdio transport for ${config.id}`, { command: config.command, args: config.args, }, ); // Validate command is present if (!config.command) { throw new Error(`Command is required for stdio transport`); } - // Spawn the process - const childProcess = spawn(config.command, config.args || [], { - stdio: ["pipe", "pipe", "pipe"], - env: Object.fromEntries( - Object.entries({ - ...process.env, - ...config.env, - }) - .filter(([, value]) => value !== undefined) - .map(([k, v]) => [k, String(v)]), - ) as Record<string, string>, - cwd: config.cwd, - }); - - // Handle process errors - const processErrorPromise = new Promise<never>((_, reject) => { - childProcess.on("error", (error: Error) => { - reject(new Error(`Process spawn error: ${error.message}`)); - }); - childProcess.on( - "exit", - (code: number | null, signal: NodeJS.Signals | null) => { - if (code !== 0) { - reject( - new Error(`Process exited with code ${code}, signal ${signal}`), - ); - } - }, - ); - }); - - // Wait for process to be ready or fail using AbortController for better async patterns - const processStartupController = new AbortController(); - const processStartupTimeout = setTimeout(() => { - processStartupController.abort(); - }, 1000); - try { - await Promise.race([ - new Promise<void>((resolve) => { - const checkReady = () => { - if (processStartupController.signal.aborted) { - resolve(); - } else { - setTimeout(checkReady, 100); - } - }; - checkReady(); - }), - processErrorPromise, - ]); - } finally { - clearTimeout(processStartupTimeout); - } - - // Check if process is still running - if (childProcess.killed || childProcess.exitCode !== null) { - throw new Error("Process failed to start or exited immediately"); - } - - // Create transport - if (!config.command) { - throw new Error(`Command is required for stdio transport`); - } - const transport = new StdioClientTransport({ command: config.command, args: config.args || [], env: Object.fromEntries( Object.entries({ ...process.env, ...config.env, }) .filter(([, value]) => value !== undefined) .map(([key, value]) => [key, String(value)]), ), cwd: config.cwd, }); - return { transport, process: childProcess }; + // transport.close() will handle child lifecycle + return { transport }; }I inspected src/lib/mcp/mcpClientFactory.ts and src/lib/types/typeAliases.ts (TransportResult.process is optional) but could not find the SDK implementation of StdioClientTransport in this repo — verify whether the SDK spawns a process when given {command, args} or expects stdin/stdout streams. If the SDK does not expose the ChildProcess handle, make the process optional in MCPClientResult or update the transport to expose it.
src/lib/mcp/factory.ts (1)
8-13: Type centralization import: OK.Imports from
../types/mcpTypes.jsare consistent with the consolidation aim.src/lib/mcp/registry.ts (2)
5-6: Imports aligned to shared types. LGTM.
178-201: Ensure interface completeness.
McpRegistryImplcurrently proxies a subset of methods. Verify it satisfies the externalMcpRegistryinterface in CI.Run a type check build to confirm no missing members or mismatched signatures.
src/lib/mcp/toolDiscoveryService.ts (1)
17-22: Type imports updated to centralized module: OK.src/lib/mcp/contracts/mcpContract.ts (1)
5-19: Fix incorrect re-exports and remove commented scaffolding.
- Verified: src/lib/types/index.ts re-exports ToolExecutionResult, ExecutionContext, CacheOptions, FallbackOptions, ToolInfo, and ValidationResult (export block around lines 40–46).
- Action required: src/lib/mcp/contracts/mcpContract.ts was not found in the repo; cannot verify its exports. If that file exists, apply the suggested diff to explicitly export DiscoveredMcp, McpMetadata, ProviderStatus from ../../types/mcpTypes.js and the remaining types from ../../types/index.js.
todos/refactor/05-mcp-module.md (1)
51-66: Resolved — index.ts already re-exports ToolResult from mcpTypes.js
index.ts re-exports ToolResult from './mcpTypes.js' (src/lib/types/index.ts:101).src/lib/types/cli.ts (1)
371-377: ```shell
#!/bin/bash
set -euo pipefailecho "PWD: $(pwd)"
echo "Top-level files:"
ls -la | sed -n '1,200p'echo
echo "Check for the target file src/lib/types/cli.ts"
if [ -f src/lib/types/cli.ts ]; then
echo "=== FOUND src/lib/types/cli.ts (first 260 lines) ==="
sed -n '1,260p' src/lib/types/cli.ts || true
else
echo "=== src/lib/types/cli.ts not found ==="
fiecho
echo "Search for 'OutputOptions' symbol"
rg -n --hidden -S '\bOutputOptions\b' || trueecho
echo "Search for occurrences of 'format' fields/usages (context lines)"
rg -n --hidden -S '\bformat\b' -C2 || trueecho
echo "Search for yaml/yml mentions across repo"
rg -n --hidden -S '\byaml\b|\byml\b' -C2 || trueecho
echo "Check package.json for yaml-related deps"
if [ -f package.json ]; then
jq '(.dependencies // {}) + (.devDependencies // {})' package.json || true
else
echo "package.json not found"
fiecho
echo "Search for printer/formatter/output handling (keywords)"
rg -n --hidden -S '\b(formatter|printer|print|output|writeOutput|write|render|toYAML|toYaml)\b' -C2 || true</blockquote></details> <details> <summary>src/lib/types/index.ts (1)</summary><blockquote> `69-101`: **Do not export ToolResult from mcpTypes — consolidate conflicting definitions first** Search shows multiple ToolResult definitions in the repo: src/lib/types/mcpTypes.ts, src/lib/types/tools.ts, src/lib/types/streamTypes.ts (and a doc). Re-exporting ToolResult from mcpTypes would be ambiguous and may break consumers. Either consolidate to a single canonical ToolResult (and update callers) or rename the variant types (e.g., ToolResultStream / ToolResultClient) before adding a central export. Also note duplicate GenerateResult definitions in src/lib/types/cli.ts and src/lib/types/generateTypes.ts; treat similarly. </blockquote></details> <details> <summary>src/lib/types/mcpTypes.ts (1)</summary><blockquote> `487-506`: **Partial verification done — externalMcp exports OK; confirm SDK package exports** - Found exports: src/lib/types/externalMcp.ts exports ExternalMCPToolInfo (line 137) and ExternalMCPToolContext (line 250). - Files importing @modelcontextprotocol/sdk paths: src/lib/types/mcpTypes.ts (lines 722, 725, 737), src/lib/types/externalMcp.ts (lines 9–10), src/lib/mcp/mcpClientFactory.ts (lines 7–15), src/lib/mcp/toolDiscoveryService.ts (lines 8–9), src/lib/mcp/externalServerManager.ts (lines 12–13). - Installed package exports not verifiable in this environment — verify @modelcontextprotocol/sdk is installed and that it exports client/index.js, shared/transport.js, and types.js (e.g. run: npm ls @modelcontextprotocol/sdk && ls -la node_modules/@modelcontextprotocol/sdk). </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| export type ProviderName = keyof typeof PROVIDERS; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Type collision: ProviderName is defined elsewhere with a different meaning.
There’s already a ProviderName in src/lib/types/providers.ts (keys of AIProviderName). Redefining it here (keys of PROVIDERS) will cause drift and confusing type errors.
Unify on the centralized type and avoid duplicate export:
-export type ProviderName = keyof typeof PROVIDERS;
+import type { ProviderName } from "../types/providers.js";Then ensure PROVIDERS keys conform to that union, or rename this local alias to ProviderKey to avoid collision.
🏁 Script executed:
#!/bin/bash
# Show all ProviderName type declarations to confirm duplication
rg -nP 'export\s+type\s+ProviderName\b' -C2Length of output: 501
Unify ProviderName — remove the duplicate and use the centralized type
Duplicate definitions found at src/lib/types/providers.ts:139 and src/lib/providers/index.ts:43 — keep a single source of truth to avoid drift and confusing type errors.
-export type ProviderName = keyof typeof PROVIDERS;
+import type { ProviderName } from "../types/providers.js";Either import the central ProviderName and ensure PROVIDERS' keys conform to that union, or rename the local alias (e.g., ProviderKey) if it intentionally differs.
📝 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.
| export type ProviderName = keyof typeof PROVIDERS; | |
| import type { ProviderName } from "../types/providers.js"; |
🤖 Prompt for AI Agents
In src/lib/providers/index.ts around lines 43 to 44, there's a duplicate
ProviderName type also defined in src/lib/types/providers.ts:139; remove the
local alias and instead import the centralized ProviderName from
src/lib/types/providers.ts (or if this file intends a different meaning, rename
the local type to ProviderKey and update usages) so there is a single source of
truth and ensure the PROVIDERS object keys conform to that imported type.
| */ | ||
|
|
||
| import type { JsonValue, JsonObject } from "./common.js"; | ||
| import { ExecutionContext, ToolInfo } from "./tools.js"; |
There was a problem hiding this comment.
Fix CI: use type-only import under verbatimModuleSyntax
TS1484 error. Import these as types.
-import { ExecutionContext, ToolInfo } from "./tools.js";
+import type { ExecutionContext, ToolInfo, ToolResult } from "./tools.js";📝 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.
| import { ExecutionContext, ToolInfo } from "./tools.js"; | |
| import type { ExecutionContext, ToolInfo, ToolResult } from "./tools.js"; |
🧰 Tools
🪛 GitHub Check: 🛡️ Code Quality & Security Gate
[failure] 7-7:
'ToolInfo' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
[failure] 7-7:
'ExecutionContext' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
🪛 GitHub Actions: CI
[error] 7-7: TS1484: 'ExecutionContext' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled. (Command: npx tsc --noEmit --strict --project tsconfig.ci.json)
🤖 Prompt for AI Agents
In src/lib/types/mcpTypes.ts around line 7, the import from "./tools.js" causes
TS1484 under verbatimModuleSyntax; change it to a type-only import (import type
{ ExecutionContext, ToolInfo } from "./tools.js") and ensure those symbols are
used only as types (no runtime references) so the compiler emits no runtime
import.
| export type ToolResult = { | ||
| success: boolean; | ||
| data?: unknown; | ||
| error?: string | Error; | ||
| usage?: { | ||
| tokens?: number; | ||
| cost?: number; | ||
| provider?: string; | ||
| model?: string; | ||
| executionTime?: number; | ||
| }; | ||
| metadata?: { | ||
| toolName?: string; | ||
| serverId?: string; | ||
| serverTitle?: string; | ||
| sessionId?: string; | ||
| timestamp?: number; | ||
| executionTime?: number; | ||
| executionId?: string; | ||
| [key: string]: unknown; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Duplicate/Conflicting ToolResult type — unify with tools.ts
This redefines ToolResult differently from src/lib/types/tools.ts and from streamTypes.ts, causing confusion and accidental mis-imports. Prefer a single canonical ToolResult (from tools.ts) and delete this block.
-/**
- * Tool execution result - Standardized result format
- */
-export type ToolResult = {
- success: boolean;
- data?: unknown;
- error?: string | Error;
- usage?: {
- tokens?: number;
- cost?: number;
- provider?: string;
- model?: string;
- executionTime?: number;
- };
- metadata?: {
- toolName?: string;
- serverId?: string;
- serverTitle?: string;
- sessionId?: string;
- timestamp?: number;
- executionTime?: number;
- executionId?: string;
- [key: string]: unknown;
- };
-};Also ensure the import at Line 7 includes ToolResult (see prior diff).
📝 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.
| export type ToolResult = { | |
| success: boolean; | |
| data?: unknown; | |
| error?: string | Error; | |
| usage?: { | |
| tokens?: number; | |
| cost?: number; | |
| provider?: string; | |
| model?: string; | |
| executionTime?: number; | |
| }; | |
| metadata?: { | |
| toolName?: string; | |
| serverId?: string; | |
| serverTitle?: string; | |
| sessionId?: string; | |
| timestamp?: number; | |
| executionTime?: number; | |
| executionId?: string; | |
| [key: string]: unknown; | |
| }; | |
| }; |
🤖 Prompt for AI Agents
In src/lib/types/mcpTypes.ts around lines 300 to 321, there's a duplicate
ToolResult type that conflicts with the canonical definition in
src/lib/types/tools.ts; remove this local ToolResult block, update the file to
import ToolResult from src/lib/types/tools.ts (ensure the import statement at
line 7 includes ToolResult), and replace any local references to the removed
type with the imported ToolResult; after changes, run TypeScript build/TS server
to surface and fix any remaining mismatched fields or imports.
1 participant
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
Refactor
Chores
Documentation