Repository navigation
Conversation
|
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. 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 WalkthroughThis PR enforces stricter TypeScript/ESLint rules, adds explicit return types widely, and performs extensive refactors across CLI, core provider logic, MCP integration, streaming pipelines, and SageMaker error handling. It removes several report/docs and MCP tool modules, introduces new helper flows, strengthens runtime validations, and updates multiple public type signatures/exports. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI (yargs)
participant CmdFactory as CLICommandFactory
participant SDK as AI SDK
participant Out as Output
User->>CLI: neurolink generate --input ...
CLI->>CmdFactory: executeGenerate(argv)
CmdFactory->>CmdFactory: handleInputValidation / processContextAndInput
CmdFactory->>CmdFactory: dry-run? (optional)
CmdFactory->>SDK: executeGenerationWithSDK(input, options, context)
SDK-->>CmdFactory: result
CmdFactory->>CmdFactory: handleOutput(format: text/json/yaml)
CmdFactory-->>Out: printed result
sequenceDiagram
participant App as NeuroLink
participant MCP as MCP Manager
participant Prov as Provider (BaseProvider)
participant Tools as Tools (MCP/In-memory)
participant Stream as Stream
App->>App: validateStreamInput
App->>MCP: initializeMCP
App->>Prov: createStreamProvider(enableMcp=true)
Prov->>Tools: setupToolExecutor(...)
App->>Prov: stream(input, options)
Prov-->>App: StreamResult (stream, analytics)
alt MCP fails
App->>Prov: createStreamProvider(enableMcp=false)
App->>Prov: stream(...) (fallback)
end
App-->>Stream: emit stream:start/end + chunks
sequenceDiagram
participant Base as BaseProvider
participant Src as Tool Sources
participant SDK as AI SDK
participant Zod as Schema Converter
Base->>Base: setupToolsForGeneration()
Base->>Src: addCustomToolsFromExecutor
Base->>Src: addInMemoryServerTools
Base->>Src: addExternalMCPTools
Src->>Zod: convertMCPSchemaToZod / convertJsonSchemaTypeToZod
Base->>SDK: executeAIGeneration(tools, input)
SDK-->>Base: AISDKGenerateResult
Base->>Base: processGenerationResult (format calls, usage, toolsUsed)
Base-->>Caller: EnhancedGenerateResult
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120–180 minutes Possibly related PRs
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🔭 Outside diff range comments (22)
src/lib/core/dynamicModels.ts (1)
183-205: Timeout for local file path is ineffective due to synchronous I/O (blocks event loop).The Promise timeout cannot preempt a synchronous read; if the file is large/slow, the read blocks the event loop and the timeout fires only after the read completes. This defeats the stated goal “with timeout (for very large files).”
Refactor to asynchronous FS APIs so the timeout is meaningful.
Suggested fix within this block:
- (async (): Promise<void> => { + (async (): Promise<void> => { try { - const fs = await import("fs"); - const path = await import("path"); + const fs = await import("node:fs/promises"); + const path = await import("node:path"); const fullPath = path.resolve(source); // Check if file exists first - if (!fs.existsSync(fullPath)) { - throw new Error(`File not found: ${fullPath}`); - } + try { + await fs.access(fullPath); + } catch { + throw new Error(`File not found: ${fullPath}`); + } - const content = fs.readFileSync(fullPath, "utf8"); - const data = JSON.parse(content); + const content = await fs.readFile(fullPath, "utf8"); + const data = JSON.parse(content); clearTimeout(timeoutId); resolve(data); } catch (error) { clearTimeout(timeoutId); reject(error); } })();Optional: If you want to actively cancel the read, you can wire an AbortController and pass its signal to readFile (supported in modern Node), and call controller.abort() in the timeout handler.
src/lib/core/evaluation.ts (1)
292-304: Ensure required legacy fields are always present (avoid unsafe type assertion)
validatedResultis asserted toUnifiedEvaluationResult, butparseUnifiedEvaluationResultdoesn’t guaranteeisOffTopicandalertSeverity. Legacy consumers relying on these fields may seeundefinedat runtime.Apply this diff to provide safe defaults and ensure
reasoningis non-empty:- const validatedResult = { - ...parsed, + const validatedResult = { + ...parsed, + // Ensure required legacy fields are present with sensible defaults + isOffTopic: + typeof (parsed as UnknownRecord)?.isOffTopic === "boolean" + ? ((parsed as UnknownRecord).isOffTopic as boolean) + : false, + alertSeverity: ((): UnifiedEvaluationResult["alertSeverity"] => { + const v = (parsed as UnknownRecord)?.alertSeverity as unknown; + return v === "low" || v === "medium" || v === "high" || v === "none" ? v : "none"; + })(), + reasoning: + typeof parsed.reasoning === "string" && parsed.reasoning.trim() + ? parsed.reasoning + : "No detailed reasoning provided", evaluationModel: `${evaluationProvider}/${evaluationModel}`, evaluationTime: Date.now() - startTime, evaluationProvider, evaluationAttempt: 1, evaluationConfig: { mode: "standard", fallbackUsed: false, costEstimate: 0.001, // Rough estimate }, };Optional (builds on the schema refactor above): normalize numeric scores via Zod before assembling:
// Before constructing validatedResult: const normalizedScores = typeof UnifiedEvaluationSchema !== "undefined" ? UnifiedEvaluationSchema.partial().safeParse(parsed) : null; if (normalizedScores?.success) { Object.assign(parsed, normalizedScores.data); }src/lib/proxy/proxyFetch.ts (2)
44-44: HTTPS requests ignore HTTP_PROXY when HTTPS_PROXY is unset — use fallbackCommon deployments only set HTTP_PROXY for both HTTP/HTTPS. Current logic skips the proxy for HTTPS in that scenario.
Apply this diff:
- const proxyUrl = url.protocol === "https:" ? httpsProxy : httpProxy; + const proxyUrl = url.protocol === "https:" ? (httpsProxy || httpProxy) : httpProxy;
25-26: Do not log raw proxy URLs — redact credentials to avoid secret leakageEnvironment proxy URLs can contain
username:password@host. Even at debug level, logging these is a credential leak.Apply this diff to redact proxy credentials in logs:
- logger.debug(`[Proxy Fetch] HTTP_PROXY: ${httpProxy || "not set"}`); - logger.debug(`[Proxy Fetch] HTTPS_PROXY: ${httpsProxy || "not set"}`); + logger.debug(`[Proxy Fetch] HTTP_PROXY: ${redactProxyUrl(httpProxy)}`); + logger.debug(`[Proxy Fetch] HTTPS_PROXY: ${redactProxyUrl(httpsProxy)}`); @@ - logger.debug( - `[Proxy Fetch] Creating ProxyAgent for ${url.hostname} via ${proxyUrl}`, - ); + logger.debug( + `[Proxy Fetch] Creating ProxyAgent for ${url.hostname} via ${redactProxyUrl(proxyUrl)}`, + );Add this helper near the imports (outside the selected ranges):
function redactProxyUrl(proxy: string | null | undefined): string { if (!proxy) return "not set"; try { const u = new URL(proxy); if (u.username) u.username = "***"; if (u.password) u.password = "***"; return u.toString(); } catch { return "[invalid proxy url]"; } }Also applies to: 47-49
src/lib/utils/providerUtils.ts (3)
131-146: Ollama availability should respect OLLAMA_BASE_URL and OLLAMA_MODEL (avoid hardcoded localhost/model).The current check hardcodes "http://localhost:11434" and "llama3.2:latest". This can lead to false negatives when users configure OLLAMA_BASE_URL/OLLAMA_MODEL. Use env vars if present and fall back safely.
Apply this diff:
if (providerName === "ollama") { try { - const response = await fetch("http://localhost:11434/api/tags", { - method: "GET", - signal: AbortSignal.timeout(2000), - }); + const baseUrl = (process.env.OLLAMA_BASE_URL ?? "http://localhost:11434") + .replace(/\/+$/, ""); + const response = await fetch(`${baseUrl}/api/tags`, { + method: "GET", + signal: AbortSignal.timeout(2000), + }); if (response.ok) { - const { models } = await response.json(); - const defaultOllamaModel = "llama3.2:latest"; - return models.some((m: UnknownRecord) => m.name === defaultOllamaModel); + const { models } = (await response.json()) as { + models?: Array<{ name?: string }>; + }; + if (!Array.isArray(models)) return false; + const desiredModel = process.env.OLLAMA_MODEL ?? "llama3.2:latest"; + return models.some((m) => m?.name === desiredModel); } return false; } catch { return false; } }
369-466: Regexes for OpenAI, Anthropic, HuggingFace, and Mistral keys are too strict/outdated and can reject valid keys.
- OpenAI now issues multiple key formats (e.g., sk-... and project-scoped variants) that may contain hyphens/underscores and varying lengths.
- Anthropic keys (sk-ant-...) can vary in length; your 95+ requirement is unnecessarily strict.
- HuggingFace tokens are typically longer than 37 chars after hf_ and may vary.
- Mistral keys commonly start with sk-; your pattern disallows hyphens and will mark valid keys invalid.
These will cause confusing “invalidVars” in validation output.
Apply these diffs to make validation flexible but still meaningful:
function validateOpenAICredentials(result: EnvVarValidationResult): void { const apiKey = process.env.OPENAI_API_KEY; if (!apiKey) { result.missingVars.push("OPENAI_API_KEY"); - } else if (!/^sk-[A-Za-z0-9]{48,}$/.test(apiKey)) { + } else if (!/^sk-[A-Za-z0-9_-]{20,}$/.test(apiKey)) { result.invalidVars.push( - "OPENAI_API_KEY (should start with 'sk-' followed by 48+ characters)", + "OPENAI_API_KEY (should start with 'sk-' and be reasonably long)", ); } }function validateAnthropicCredentials(result: EnvVarValidationResult): void { const apiKey = process.env.ANTHROPIC_API_KEY; if (!apiKey) { result.missingVars.push("ANTHROPIC_API_KEY"); - } else if (!/^sk-ant-[A-Za-z0-9-_]{95,}$/.test(apiKey)) { + } else if (!/^sk-ant-[A-Za-z0-9_-]{30,}$/.test(apiKey)) { result.invalidVars.push( - "ANTHROPIC_API_KEY (should start with 'sk-ant-' followed by 95+ characters)", + "ANTHROPIC_API_KEY (should start with 'sk-ant-' and be reasonably long)", ); } }function validateHuggingFaceCredentials(result: EnvVarValidationResult): void { const apiKey = process.env.HUGGINGFACE_API_KEY || process.env.HF_TOKEN; if (!apiKey) { result.missingVars.push("HUGGINGFACE_API_KEY (or HF_TOKEN)"); - } else if (!/^hf_[A-Za-z0-9]{37}$/.test(apiKey)) { + } else if (!/^hf_[A-Za-z0-9]{35,}$/.test(apiKey)) { result.invalidVars.push( - "HUGGINGFACE_API_KEY (should start with 'hf_' followed by 37 characters)", + "HUGGINGFACE_API_KEY (should start with 'hf_' and be reasonably long)", ); } }function validateMistralCredentials(result: EnvVarValidationResult): void { const apiKey = process.env.MISTRAL_API_KEY; if (!apiKey) { result.missingVars.push("MISTRAL_API_KEY"); - } else if (!/^[A-Za-z0-9]{32,}$/.test(apiKey)) { + } else if (!/^sk-[A-Za-z0-9_-]{20,}$/.test(apiKey)) { result.invalidVars.push( - "MISTRAL_API_KEY (should be 32+ alphanumeric characters)", + "MISTRAL_API_KEY (should start with 'sk-' and be reasonably long)", ); } }If you want, I can cross-verify exact formats with current provider docs and tighten these while avoiding false negatives.
653-665: 'litellm' is supported by helpers but missing from getAvailableProviders(); isValidProvider will reject it.You added isLiteLlmProvider/hasLiteLlmProviderEnvVars, but getAvailableProviders() omits "litellm". This breaks isValidProvider("litellm").
Apply this diff:
export function getAvailableProviders(): string[] { return [ "bedrock", "vertex", "openai", "anthropic", "azure", "google-ai", "huggingface", "ollama", "mistral", + "litellm", ]; }Optionally, consider making isValidProvider synonym-aware via a normalization helper so inputs like "aws", "amazon", "gemini", "google" don’t get rejected while validators accept them.
Also applies to: 672-674
src/lib/agent/directTools.ts (1)
154-213: Unsafe expression evaluation in calculateMath allows code execution.
Current validation is ineffective and the function evaluates the originalexpressionvianew Function(...), enabling injection. The single-character regex checks with a global regex are also flawed due tolastIndexstatefulness.Recommend restricting to basic arithmetic and evaluating only a sanitized expression:
- try { - // Simple safe evaluation - only allow basic math operations - const sanitizedExpression = expression.replace(/[^0-9+\-*/().\s]/g, ""); - - if (sanitizedExpression !== expression) { - // Try Math functions for more complex operations - const allowedMathFunctions = [ - "Math.abs", - "Math.ceil", - "Math.floor", - "Math.round", - "Math.sqrt", - "Math.pow", - "Math.sin", - "Math.cos", - "Math.tan", - "Math.log", - "Math.exp", - "Math.PI", - "Math.E", - ]; - - let safeExpression = expression; - for (const func of allowedMathFunctions) { - safeExpression = safeExpression.replace( - new RegExp(func, "g"), - func, - ); - } - - // Remove remaining non-safe characters except Math functions - const mathSafe = - /^[0-9+\-*/().\s]|Math\.(abs|ceil|floor|round|sqrt|pow|sin|cos|tan|log|exp|PI|E)/g; - if ( - !safeExpression - .split("") - .every( - (char) => - mathSafe.test(char) || - char === "(" || - char === ")" || - char === "," || - char === " ", - ) - ) { - return { - success: false, - error: `Unsafe expression: Only basic math operations and Math functions are allowed`, - }; - } - } - - // Use Function constructor for safe evaluation - const result = new Function(`'use strict'; return (${expression})`)(); + try { + // Strict whitelist: digits, whitespace, parentheses, and + - * / + const sanitized = expression.replace(/[^\d+\-*/().\s]/g, ""); + if (sanitized !== expression) { + return { + success: false, + error: + "Unsupported characters: only numbers, spaces, parentheses, and + - * / are allowed", + }; + } + // Evaluate only the sanitized expression + const result = new Function(`'use strict'; return (${sanitized})`)(); const roundedResult = typeof result === "number" ? Number(result.toFixed(precision)) : result;If support for Math.* is required, switch to a proper math expression parser (e.g.,
expr-eval) rather thannew Function. I can provide a follow-up patch if desired.src/cli/utils/interactiveSetup.ts (1)
323-345: Don’t overwrite existing env var with default when the user declines to updateWhen the confirm prompt in
whenreturns false (user chooses not to update), the subsequent assignment at Lines 348-350 overwrites the existing value with a default if present. Preserve the current value instead.Apply this diff near Lines 348-351:
- if (value || envVar.default) { - result.credentials[envVar.key] = value || envVar.default || ""; - } + // Preserve existing value if user declines to update; otherwise use entered value or default + const finalValue = + (typeof value === "string" && value !== "") + ? value + : hasCurrentValue + ? currentValue + : envVar.default; + if (finalValue !== undefined) { + result.credentials[envVar.key] = finalValue; + }src/lib/utils/timeout.ts (1)
396-435: Streaming timeout isn’t enforced while awaiting generator.next()Current logic races the already-resolved
itemwithtimeoutPromise, so the timeout cannot interrupt a stalledfor awaiton the generator. This can hang indefinitely if the stream stalls between chunks. Racegenerator.next()with the timeout instead.Apply this diff to enforce total streaming timeout correctly:
export async function* withStreamingTimeout<T>( generator: AsyncGenerator<T>, timeout: number | string | undefined, provider: string, ): AsyncGenerator<T> { const timeoutMs = parseTimeout(timeout); if (!timeoutMs) { yield* generator; return; } - let timeoutId: NodeJS.Timeout | undefined; - const timeoutPromise = new Promise<never>((_, reject) => { - timeoutId = setTimeout(() => { - reject( - new TimeoutError( - `${provider} streaming operation timed out after ${timeoutMs}ms`, - timeoutMs, - provider, - "stream", - ), - ); - }, timeoutMs); - }); - - try { - for await (const item of generator) { - const raceResult = await Promise.race([ - Promise.resolve(item), - timeoutPromise, - ]); - yield raceResult; - } - } finally { - if (timeoutId) { - clearTimeout(timeoutId); - } - } + let timeoutId: NodeJS.Timeout | undefined; + const timeoutPromise = new Promise<never>((_, reject) => { + timeoutId = setTimeout(() => { + reject( + new TimeoutError( + `${provider} streaming operation timed out after ${timeoutMs}ms`, + timeoutMs, + provider, + "stream", + ), + ); + }, timeoutMs); + if (typeof timeoutId?.unref === "function") { + timeoutId.unref(); + } + }); + + try { + while (true) { + // Race the next() call with the timeout so a stalled stream doesn’t hang forever + const nextResult = (await Promise.race([ + generator.next(), + timeoutPromise, + ])) as IteratorResult<T>; + if (nextResult.done) break; + yield nextResult.value; + } + } finally { + if (timeoutId) { + clearTimeout(timeoutId); + } + } }src/lib/utils/optionsUtils.ts (1)
566-575: executionContext is assigned at the top-level; it should live under options.contextIn applyContextConversion, executionContext is added to the root of UnifiedGenerationOptions, but other flows (e.g., applyLegacyMigration) place it under options.context. This inconsistency will break consumers expecting executionContext in context.
Apply this diff:
- // Add converted context to unified options - Object.assign(unifiedOptions, { executionContext }); + // Add converted context under the context property for consistency + unifiedOptions.context = { + ...unifiedOptions.context, + executionContext, + };src/lib/core/modelConfiguration.ts (1)
1146-1148: Required env var check should require all, not anyCurrently, a provider is considered “available” if any one of the required env vars is set. Typically, all required env vars must be present.
Apply this diff:
- return config.requiredEnvVars.some((envVar) => - Boolean(process.env[envVar]), - ); + return config.requiredEnvVars.every((envVar) => + Boolean(process.env[envVar]), + );src/lib/providers/sagemaker/detection.ts (1)
673-708: Semaphore bug: permits can go negative due to double-decrement, throttling progressIn acquire(), queued waiters decrement count when resumed, but release() does not increment prior to invoking them. This drives count negative and distorts concurrency control.
Apply this fix to ensure waiters do not decrement:
async acquire(): Promise<void> { return new Promise((resolve) => { if (this.count > 0) { this.count--; resolve(); } else { - this.waiters.push(() => { - this.count--; - resolve(); - }); + this.waiters.push(() => { + // Grant the permit to the waiter without changing count. + // When no waiters remain, release() will increment count. + resolve(); + }); } }); }, release(): void { if (this.waiters.length > 0) { const waiter = this.waiters.shift(); if (waiter) { waiter(); } } else { this.count++; } },src/lib/providers/sagemaker/structured-parser.ts (1)
496-528: Incomplete quoted strings are treated as complete values (parsing bug)When the buffer ends inside a string, parseQuotedString returns a value with endIndex = i - 1. Upstream parseJsonValue treats this as a complete string and commits it into the partial object, corrupting state for streaming inputs.
Return null on unterminated strings so callers can defer until the next chunk:
while (i < buffer.length) { const char = buffer[i]; if (char === '"') { return { value: result, endIndex: i }; } ... } - // Unterminated string - return partial for streaming - return { value: result, endIndex: i - 1 }; + // Unterminated string - signal incomplete so upstream can wait for next chunk + return null;This change harmonizes with handleSeekingKey/handleSeekingValue which already propagate “incomplete” via shouldReturn.
src/lib/providers/huggingFace.ts (1)
160-175: Apply enhanced system prompt and remove unnecessary type assertions on tools/toolChoiceCurrently, the enhanced system prompt computed in prepareStreamOptions is never applied (we pass messages only). Also, we can avoid the forced casts if prepareStreamOptions returns properly typed fields.
Suggested change:
- Pass system: streamOptions.system to streamText so tool instructions actually affect generations.
- After tightening prepareStreamOptions’ return types (see next comment), remove the casts.
const result = await streamText({ model: this.model, messages: messages, + system: streamOptions.system, temperature: options.temperature, maxTokens: options.maxTokens || DEFAULT_MAX_TOKENS, - tools: streamOptions.tools as ToolSet, // Tools format conversion handled by prepareStreamOptions - toolChoice: streamOptions.toolChoice as ToolChoice<ToolSet>, // Tool choice handled by prepareStreamOptions + tools: streamOptions.tools, // Already typed as ToolSet + toolChoice: streamOptions.toolChoice, // Proper ToolChoice<ToolSet> type abortSignal: timeoutController?.controller.signal, });src/cli/commands/mcp.ts (1)
503-514: Bug: merged env (_env) is computed but never applied to serverInfoargv.env JSON is parsed into _env but not used when creating serverInfo, so overrides are dropped. Also, install options for args and transport aren’t applied either.
Apply this fix when building serverInfo to ensure env/args/transport from CLI are honored:
- const serverInfo = createExternalServerInfo({ - ...serverConfig, - id: serverName, - name: serverName, - }); + const serverInfo = createExternalServerInfo({ + ...serverConfig, + id: serverName, + name: serverName, + env: _env, + ...(argv.args ? { args: argv.args as string[] } : {}), + ...(argv.transport ? { transport: argv.transport as MCPTransportType } : {}), + });src/cli/commands/ollama.ts (1)
85-87: Shell injection risk via execSync with interpolated model argumentUser-supplied model is interpolated into a shell command, enabling command injection on all platforms. Use execFileSync/spawnSync with argument arrays to avoid the shell.
Apply this diff within the selected ranges:
- execSync(`ollama pull ${model}`, { stdio: "inherit" }); + execFileSync("ollama", ["pull", model], { stdio: "inherit" });- execSync(`ollama rm ${model}`, { encoding: "utf8" }); + execFileSync("ollama", ["rm", model], { stdio: "inherit" });And update the import at the top of the file:
// at line 2 import { execSync, execFileSync } from "child_process";Optionally, validate model names (basic hardening) before invocation:
const isSafeModel = /^[a-zA-Z0-9._:/+-]+$/.test(model); if (!isSafeModel) { throw new Error("Invalid model name."); }Also applies to: 120-121
src/cli/commands/sagemaker.ts (1)
520-528: Do not print AWS secret material to consoleThese fields can leak credentials in logs. Mask Access Key ID and never print Secret Access Key or Session Token.
Apply this diff:
- 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 > 4 ? `${v.slice(0, 4)}****${v.slice(-4)}` : "****"; + logger.always(` Access Key: ${mask(aws.accessKeyId as string)}`); + logger.always(` Secret Key: ${aws.secretAccessKey ? "[REDACTED]" : "Not set"}`); + logger.always(` Session Token: ${aws.sessionToken ? "[REDACTED]" : "Not set"}`);src/lib/providers/openaiCompatible.ts (1)
237-248: disableTools flag is ignored; tools are always enabledexecuteStream unconditionally passes tools and sets toolChoice to "auto", contrary to the comment. Respect disableTools and supportsTools for parity with other providers.
Apply this diff:
- const result = await streamText({ + const shouldUseTools = !options.disableTools && this.supportsTools(); + const result = await streamText({ model, prompt: options.input.text, system: options.systemPrompt, temperature: options.temperature, maxTokens: options.maxTokens || DEFAULT_MAX_TOKENS, - tools: options.tools, - toolChoice: "auto", + tools: shouldUseTools ? options.tools : undefined, + toolChoice: shouldUseTools ? "auto" : "none", abortSignal: timeoutController?.controller.signal, });Also applies to: 253-261
src/lib/providers/mistral.ts (1)
80-91: Caller-provided tools are ignored in streaming pathexecuteStream only uses getAllTools() and drops any tools passed via options.tools. This is inconsistent with generate(), which merges custom tools with base tools.
Apply this diff to merge caller-supplied tools:
- const shouldUseTools = !options.disableTools && this.supportsTools(); - const tools = shouldUseTools ? await this.getAllTools() : {}; + const shouldUseTools = !options.disableTools && this.supportsTools(); + const baseTools = shouldUseTools ? await this.getAllTools() : {}; + const tools = shouldUseTools + ? { ...baseTools, ...(options as { tools?: Record<string, unknown> }).tools } + : {};If StreamOptions does not define tools, consider extending it for parity with TextGenerationOptions.
src/lib/providers/ollama.ts (1)
1028-1034: Construct TimeoutError with full contextTimeoutError likely expects more context (timeoutMs, provider, operationType). Provide those for better diagnostics.
- return new TimeoutError( - `Ollama request timed out. The model might be loading or the request is too complex.`, - this.defaultTimeout, - ); + return new TimeoutError( + `Ollama request timed out. The model might be loading or the request is too complex.`, + this.defaultTimeout, + this.providerName, + "stream", + );src/cli/factories/commandFactory.ts (1)
385-447: Analytics token fields mismatch (no tokens printed currently)formatAnalyticsForTextMode reads analytics.tokens as { input, output, total }, but AnalyticsData provides tokenUsage with inputTokens/outputTokens/totalTokens. As a result, token counts never render.
Apply this diff to support AnalyticsData.tokenUsage and fall back gracefully:
private static formatAnalyticsForTextMode(result: GenerateResult): string { if (!result.analytics) { return ""; } const analytics = result.analytics; let analyticsText = "\n\n📊 Analytics:\n"; @@ - // Token usage - if (this.isValidTokenUsage(analytics.tokens)) { - const tokens = analytics.tokens as AnalyticsTokens; - analyticsText += ` Tokens: ${tokens.input} input + ${tokens.output} output = ${tokens.total} total\n`; - } + // Token usage (supports both AnalyticsData.tokenUsage and legacy analytics.tokens) + const tokenUsage = + (analytics as { tokenUsage?: { inputTokens?: number; outputTokens?: number; totalTokens?: number } }) + .tokenUsage || + undefined; + if (tokenUsage && + typeof tokenUsage.inputTokens === "number" && + typeof tokenUsage.outputTokens === "number" && + typeof tokenUsage.totalTokens === "number") { + analyticsText += ` Tokens: ${tokenUsage.inputTokens} input + ${tokenUsage.outputTokens} output = ${tokenUsage.totalTokens} total\n`; + } else if (this.isValidTokenUsage((analytics as unknown as { tokens?: AnalyticsTokens }).tokens)) { + const tokens = (analytics as unknown as { tokens: AnalyticsTokens }).tokens; + analyticsText += ` Tokens: ${tokens.input} input + ${tokens.output} output = ${tokens.total} total\n`; + }
♻️ Duplicate comments (5)
src/lib/core/dynamicModels.ts (1)
433-437: Same refactor applies here (use assertion to drop the cast).Leverage the ensureInitialized assertion approach to replace the local NonNullable cast and loop directly over this.modelRegistry.models.
test/array-tool-registration.test.ts (4)
114-116: Same helper applies here to keep tests DRY
199-207: Same helper applies here to keep tests DRY
343-345: Same helper applies here to keep tests DRY
365-367: Same helper applies here to keep tests DRY
There was a problem hiding this comment.
Pull Request Overview
This PR eliminates all ESLint warnings and significantly enhances code quality standards across the entire NeuroLink codebase. The changes focus on adding explicit return types to functions, improving type safety, reducing code complexity, and removing unused imports while maintaining enterprise-grade code standards.
Key Changes
- Added explicit return types to 146+ functions across CLI and core modules
- Replaced all TypeScript 'any' types with proper type definitions
- Reduced code complexity by refactoring nested functions and control structures
- Eliminated 1,158 lines of dead code and unused imports
Reviewed Changes
Copilot reviewed 79 out of 79 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/basicFunctionality.ts | Refactored JSON parsing with helper function to reduce duplication |
| test/array-tool-registration.test.ts | Replaced 'any' types with proper type assertions |
| src/test/setup-minimal.ts | Added explicit return types to test utility functions |
| src/lib/utils/*.ts | Enhanced type safety and added comprehensive return type annotations |
| src/lib/providers/*.ts | Improved error handling and added explicit function return types |
| src/lib/providers/sagemaker/*.ts | Major refactoring to reduce complexity and standardize error construction |
| src/lib/neurolink.ts | Extracted complex methods into smaller functions for better maintainability |
| src/lib/mcp/*.ts | Updated tool execution functions with proper type signatures |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| expect(() => JSON.parse(jsonString)).not.toThrow(); | ||
| const jsonResult = JSON.parse(jsonString); | ||
| const jsonResult = parseJsonFromOutput(stdout); | ||
| expect(() => JSON.parse(JSON.stringify(jsonResult))).not.toThrow(); |
There was a problem hiding this comment.
This line performs an unnecessary round-trip JSON serialization/deserialization just to test JSON parsing. Since jsonResult is already parsed JSON, this test doesn't validate anything meaningful. Consider removing this line or testing the actual JSON structure instead.
| expect(() => JSON.parse(JSON.stringify(jsonResult))).not.toThrow(); |
| */ | ||
| function checkAbortSignal(abortSignal?: AbortSignal): void { | ||
| if (abortSignal?.aborted) { | ||
| throw new SageMakerError({ |
There was a problem hiding this comment.
The function signature shows this should throw SageMakerError but the constructor pattern has changed from the old SageMakerError(message, code, statusCode) to object-based constructor. Ensure all callers are updated to use the new constructor pattern consistently.
| /** | ||
| * Check if we can pop the specified bracket type from the stack | ||
| */ | ||
| private canPopBracketType(expectedType: string): boolean { |
There was a problem hiding this comment.
Consider adding parameter validation to ensure expectedType is one of the valid bracket types ('{', '[') to prevent silent failures when invalid bracket types are passed.
| private canPopBracketType(expectedType: string): boolean { | |
| private canPopBracketType(expectedType: string): boolean { | |
| if (expectedType !== "{" && expectedType !== "[") { | |
| logger.warn(`Invalid bracket type passed to canPopBracketType: '${expectedType}'. Expected '{' or '['.`); | |
| return false; | |
| } |
| this.endpoint = config.endpoint; | ||
| this.requestId = config.requestId; | ||
| this.retryable = config.retryable ?? false; | ||
|
|
There was a problem hiding this comment.
This is a breaking change in the constructor API. The old constructor accepted positional parameters, but now requires a configuration object. Consider providing a backward-compatible constructor overload or ensuring all existing usages are migrated.
| const promise = new Promise<never>((_, reject) => { | ||
| timer = setTimeout(() => { | ||
| reject( | ||
| new TimeoutError(`Operation timeout after ${timeoutMs}ms`, timeoutMs), |
There was a problem hiding this comment.
The parameter _operationId is prefixed with underscore indicating it's unused, but the function name suggests it should be used for operation tracking. Consider either using this parameter for logging/debugging or removing it if truly unnecessary.
| new TimeoutError(`Operation timeout after ${timeoutMs}ms`, timeoutMs), | |
| operationId: string, | |
| ): { promise: Promise<never>; timer: NodeJS.Timeout } { | |
| let timer: NodeJS.Timeout | undefined; | |
| const promise = new Promise<never>((_, reject) => { | |
| timer = setTimeout(() => { | |
| reject( | |
| new TimeoutError( | |
| `Operation timeout after ${timeoutMs}ms`, | |
| timeoutMs, | |
| operationId | |
| ), |
| * @returns EventEmitter instance | ||
| */ | ||
| getEventEmitter() { | ||
| getEventEmitter(): EventEmitter { |
There was a problem hiding this comment.
Missing import for EventEmitter type. The return type annotation references EventEmitter but it's not imported at the top of the file.
38e24d7 to
7561d83
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
7561d83 to
7a4791b
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
7a4791b to
ca1620a
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
1 similar comment
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
c79a804 to
b7dace8
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
b7dace8 to
c09f3e5
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
- Achieve 100% ESLint compliance (0 errors, 0 warnings) - Fix TypeScript compilation issues introduced by ESLint cleanup - Update SageMakerError constructor calls to use options object pattern - Fix property initialization and variable declaration issues - Add missing imports and resolve type casting problems - Remove obsolete memory-bank reports - Ensure build process works end-to-end 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
c09f3e5 to
2012b26
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
…standards
Comprehensive ESLint remediation across entire codebase:
Technical enhancements:
Impact:
Files: 71 modified, 3 deleted
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
Breaking Changes