feat(mcp): MCP enhancement modules with routing, caching, batching, and wire into core SDK - #828
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ 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 |
|
@coderabbitai full review |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds a large MCP (Model Context Protocol) feature set: CLI commands, public MCP API, server base, multi-server manager, routing, caching, batching, elicitation protocol/manager, tool annotations/conversion/integration, agent/workflow exposure, registry client, server capabilities, docs, and many tests. Changes are additive; no public removals. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Router as ToolRouter
participant MSM as MultiServerMgr
participant Cache as ToolCache
participant Batcher as RequestBatcher
participant Server as MCPServer
Client->>Router: route(tool, params, context)
Router->>MSM: selectServer(tool, criteria)
MSM->>MSM: evaluateHealthAndWeights()
MSM-->>Router: selectedServer
Router->>Cache: get(toolKey)
alt cache hit
Cache-->>Client: cachedResult
else cache miss
Router->>Batcher: addRequest(tool, params, serverId)
Batcher->>Batcher: accumulate / flush on conditions
Batcher->>Server: executeBatch(batch)
Server-->>Batcher: batchResults
Batcher->>Cache: set(toolKey, result)
Batcher-->>Client: results
end
sequenceDiagram
participant User as User
participant Tool as Tool
participant EMgr as ElicitationMgr
participant Handler as ElicitationHandler
participant UI as UserInterface
User->>Tool: execute(possiblyDestructive)
Tool->>EMgr: requestConfirmation(message)
EMgr->>Handler: invoke(handler, elicitation)
Handler->>UI: presentPrompt
UI->>User: prompt
User-->>UI: respond
UI-->>Handler: response
Handler-->>EMgr: elicitationResponse
EMgr-->>Tool: deliverResponse
alt confirmed
Tool-->>User: successResult
else cancelled
Tool-->>User: cancelledResponse
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
✅ Actions performedFull review triggered. |
Documentation Validation Results🚀 Documentation validation passed!
📦 Build artifact uploaded successfully. Ready for deployment preview. Commit: |
🤖 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (20)
src/lib/utils/schemaConversion.ts-154-158 (1)
154-158:⚠️ Potential issue | 🟡 Minor
typeof null === "object"— missing null guard onschema.jsonSchema.If
schema.jsonSchemaisnull, the condition on line 155 passes (typeof null === "object"istrue), andensureTypeFieldwill attempt to set properties onnull, throwing aTypeErrorat runtime.Proposed fix
- if ("jsonSchema" in schema && typeof schema.jsonSchema === "object") { + if ("jsonSchema" in schema && schema.jsonSchema !== null && typeof schema.jsonSchema === "object") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/utils/schemaConversion.ts` around lines 154 - 158, The guard for the AI SDK jsonSchema wrapper is too loose because typeof null === "object"; update the condition that checks schema.jsonSchema (used in the block that returns ensureTypeField(extracted)) to also exclude null (e.g., check schema.jsonSchema !== null) before casting to Record and calling ensureTypeField, so ensureTypeField never receives null.src/lib/mcp/serverCapabilities.ts-797-802 (1)
797-802:⚠️ Potential issue | 🟡 MinorTemplate argument keys are injected into a regex without escaping, risking ReDoS or broken substitution.
If an argument key contains regex metacharacters (e.g.,
a+b),new RegExp(\\{${key}\}`, "g")produces an invalid or dangerous pattern. UseString.prototype.replaceAll` or escape the key first:Proposed fix
generator: async (args) => { // Simple template substitution let text = template; for (const [key, value] of Object.entries(args)) { - text = text.replace(new RegExp(`\\{${key}\\}`, "g"), String(value)); + text = text.replaceAll(`{${key}}`, String(value)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/serverCapabilities.ts` around lines 797 - 802, The template substitution in the generator function uses new RegExp(`\\{${key}\\}`, "g") which can produce invalid or unsafe regexes for keys containing regex metacharacters; update the substitution to either (a) escape regex metacharacters in the key before constructing the RegExp, or (b) avoid RegExp entirely and use a safe literal replacement such as String.prototype.replaceAll (or split/join) on the template string; ensure you coerce key to string and apply the replacement for every entry in args while preserving the existing behavior in generator and the template variable.test/continuous-test-suite-mcp.ts-118-156 (1)
118-156:⚠️ Potential issue | 🟡 MinorTest framework silently passes when checking for non-existent methods, masking API regressions.
testToolRouterchecks foraddRoute()andresolve()methods that don't exist (actual methods:registerServer(),unregisterServer(),route()). Similarly,testToolCachechecks for publicisExpired()which is private. Since the tests record PASS even when methods aren't available, API changes go undetected.Recommend asserting that the classes have at least one expected method to catch breaking changes. For example, after instantiation, verify
router.route !== undefinedor similar to ensure the actual API exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/continuous-test-suite-mcp.ts` around lines 118 - 156, The tests silently pass when expected methods are missing; update testToolRouter to assert the actual ToolRouter API methods (check for registerServer, unregisterServer, and route on the instantiated router) and fail the test if those methods are undefined instead of recording a PASS; replace or augment the current checks for addRoute and resolve with explicit existence checks for registerServer/unregisterServer/route (using the router instance returned by new ToolRouter()) so API regressions are detected. Also update testToolCache to stop checking the private isExpired and instead assert a public API method or behavior; ensure recordTest is called with a failing result when required methods are absent.src/lib/mcp/batching/requestBatcher.ts-165-169 (1)
165-169:⚠️ Potential issue | 🟡 MinorNon-null assertions flagged by linter — use safe access instead.
Lines 169, 318, and 466 use
!non-null assertions that the CI linter flags as forbidden. Line 169 is immediately after aset()so it's safe in practice, but the others (this.executor!,this.toolExecutor!) rely on guards in a different async context.Proposed fix for line 169
- this.serverQueues.get(serverId)!.add(requestId); + const queue = this.serverQueues.get(serverId); + queue?.add(requestId);Proposed fix for line 318
+ if (!this.executor) { + throw new Error("Batch executor not set"); + } const results = await this.executor((and remove the
!)Also applies to: 316-318, 464-466
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/batching/requestBatcher.ts` around lines 165 - 169, The linter flags non-null assertions; replace them with safe access and explicit handling: for the serverQueues add path (where serverId was just set) replace this.serverQueues.get(serverId)! with a local const q = this.serverQueues.get(serverId); if (!q) throw new Error(...) or return, then q.add(requestId); for executor and toolExecutor (symbols this.executor and this.toolExecutor) remove the `!` and either guard their use with early checks (if (!this.executor) throw new Error('executor not initialized') ) or use conditional logic before calling methods so you never assume non-null across async boundaries.src/lib/mcp/mcpServerBase.ts-190-199 (1)
190-199:⚠️ Potential issue | 🟡 Minor
stop()does not handleonStop()failure — state becomes inconsistent.If a subclass's
onStop()throws,isRunningremainstrueand theserverStoppedevent is never emitted. Subsequentstop()calls will retryonStop(), which is reasonable, but consumers listening for the event will never be notified of the failure. Consider wrapping in try/finally or emitting an error event.Proposed fix
async stop(reason?: string): Promise<void> { if (!this.isRunning) { return; } - await this.onStop(); - this.isRunning = false; - - this.emit("serverStopped", { reason }); + try { + await this.onStop(); + } finally { + this.isRunning = false; + this.emit("serverStopped", { reason }); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/mcpServerBase.ts` around lines 190 - 199, The stop() method can leave isRunning true and never emit the stop event if this.onStop() throws; update stop() to wrap the await this.onStop() call in try/catch/finally: in finally ensure this.isRunning is set to false so state is consistent, in the try branch emit("serverStopped", { reason }) on success, and in the catch branch emit a failure/error event (e.g., emit("serverStopFailed" or emit("error"), including the thrown error and reason) then rethrow or handle per your error model; reference stop(), onStop(), isRunning and the emit("serverStopped") call when making the change.src/lib/mcp/caching/toolCache.ts-419-423 (1)
419-423:⚠️ Potential issue | 🟡 MinorReDoS risk:
?is not escaped and user-controlled patterns are compiled into aRegExp.The
patternToRegexescapes most special characters but misses?. A malicious or accidental pattern liketool:a]?????????????????????!could cause catastrophic backtracking. Consider using a stricter allowlist approach or escaping all regex-special characters (including?).Proposed fix
private patternToRegex(pattern: string): RegExp { - const escaped = pattern.replace(/[.+^${}()|[\]\\]/g, "\\$&"); + const escaped = pattern.replace(/[.+?^${}()|[\]\\]/g, "\\$&"); const regexPattern = escaped.replace(/\*/g, ".*"); return new RegExp(`^${regexPattern}$`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/caching/toolCache.ts` around lines 419 - 423, The patternToRegex function fails to escape the '?' character before compiling user-controlled patterns into a RegExp, exposing a ReDoS risk; update patternToRegex to escape all regex-special characters (including '?', and any others not currently covered) or switch to a safe allowlist/pattern builder so user input cannot create catastrophic backtracking; specifically modify the patternToRegex implementation (the escaped/regexPattern construction used in patternToRegex) to escape '?' (or use a comprehensive escape routine) before replacing '*' with '.*' and constructing the RegExp.src/lib/mcp/caching/toolCache.ts-271-284 (1)
271-284:⚠️ Potential issue | 🟡 Minor
getOrSetsilently fails forundefinedcached values.If
factory()returnsundefined, it gets cached. However,get()returnsundefinedfor both misses and cachedundefinedvalues (line 153 and 173). SubsequentgetOrSetcalls will re-invoke the factory every time, defeating the cache for legitimateundefinedresults.This is an edge-case but worth documenting or guarding. A sentinel value or separate
has()check beforeget()would fix it:Proposed fix
async getOrSet( key: string, factory: () => Promise<T> | T, ttl?: number, ): Promise<T> { - const existing = this.get(key); - if (existing !== undefined) { + if (this.has(key)) { + const existing = this.get(key); return existing; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/caching/toolCache.ts` around lines 271 - 284, getOrSet currently treats a cached undefined the same as a miss because get() returns undefined for both; update getOrSet to call this.has(key) first and if true return this.get(key) (allowing cached undefined to be returned) otherwise call factory(), then this.set(key, value, ttl) and return value. Reference the methods getOrSet, get, has, and set in ToolCache to locate and implement this change (alternatively you can implement a sentinel value storage, but the has() check is the preferred minimal fix).src/lib/mcp/mcpServerBase.ts-247-273 (1)
247-273:⚠️ Potential issue | 🟡 MinorUse
ErrorFactoryto create typed errors instead of plainError.The codebase uses
ErrorFactoryfor creating structured, categorized errors. Multiple validation failures here throw plainErrorobjects instead. Consider usingErrorFactory.invalidConfiguration()for tool definition validation errors (name, description, execute function requirements), or create a dedicatedErrorFactory.toolValidationFailed()method if more specific error handling is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/mcpServerBase.ts` around lines 247 - 273, Replace plain Error throws in validateTool (method validateTool on MCPServerBase handling MCPServerTool) with typed errors from ErrorFactory: use ErrorFactory.invalidConfiguration(...) for each validation failure (invalid name, too long, invalid pattern, missing/invalid description, missing execute function, duplicate registration via this.tools.has(tool.name)), or add and use a new ErrorFactory.toolValidationFailed(...) if you need a more specific category; include the original validation message (and tool.name where relevant) as the error details when calling ErrorFactory so existing callers can inspect error type and message.test/mcp/integration.test.ts-304-333 (1)
304-333:⚠️ Potential issue | 🟡 MinorAwait async shutdown to avoid dangling handles.
manager.shutdown()is awaited elsewhere; make this test async and await it to prevent leaks/flakiness.Suggested fix
- it("should validate server configuration correctly", () => { + it("should validate server configuration correctly", async () => { const manager = new ExternalServerManager(); @@ - manager.shutdown(); + await manager.shutdown(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mcp/integration.test.ts` around lines 304 - 333, The test currently calls manager.shutdown() synchronously which can leave async teardown running; change the spec to be async and await the shutdown call — locate the test using ExternalServerManager (the "should validate server configuration correctly" spec) and update it so it declares the test function async and invokes await manager.shutdown() at the end (ensuring the validateConfig checks remain unchanged).test/mcp/factory.test.ts-1-16 (1)
1-16:⚠️ Potential issue | 🟡 MinorMove feature tests into
test/suites/.This is a feature/unit test and should live under
test/suites/per repo convention (e.g.,test/suites/mcp/factory.test.ts).As per coding guidelines: "Create test suites in test/suites/ for feature-specific tests, test/integration/ for real provider integration tests. Use Vitest as test runner."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mcp/factory.test.ts` around lines 1 - 16, This test file is in the wrong folder; move the file to test/suites/mcp/factory.test.ts and update any relative imports or test runner config if necessary so paths still resolve (verify imports for createMCPServer, validateTool, getServerInfo, validateServerTools from ../../src/lib/mcp/factory.js remain correct after the move); ensure the test suite still uses Vitest (describe, it, expect, vi) and run the suite to confirm no path or module resolution errors.test/mcp/integration.test.ts-1-14 (1)
1-14:⚠️ Potential issue | 🟡 MinorRelocate integration tests to
test/integration/.These are integration tests and should be placed under
test/integration/(e.g.,test/integration/mcp.integration.test.ts).As per coding guidelines: "Create test suites in test/suites/ for feature-specific tests, test/integration/ for real provider integration tests. Use Vitest as test runner."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mcp/integration.test.ts` around lines 1 - 14, This file contains integration tests (uses describe and tests ExternalServerManager, ToolDiscoveryService, MCPRegistry, createMCPServer) and must be relocated to the integration tests folder; move the file to test/integration/mcp.integration.test.ts, update any test-runner or import paths if necessary, and ensure the test suite still imports ExternalServerManager, ToolDiscoveryService, MCPRegistry, and createMCPServer from their existing module paths so Vitest picks it up as an integration test.src/cli/commands/mcp.ts-1626-1633 (1)
1626-1633:⚠️ Potential issue | 🟡 MinorRemove non-null assertions on Map access at lines 1632 and 2649.
These non-null assertions are flagged by linters. While they are safe due to the preceding
has()check and initialization, use the nullish coalescing pattern to avoid them.Suggested fix
- toolsByServer.get(key)!.push(tool); + const bucket = toolsByServer.get(key) ?? []; + bucket.push(tool); + toolsByServer.set(key, bucket);- byCategory.get(category)!.push(entry); + const bucket = byCategory.get(category) ?? []; + bucket.push(entry); + byCategory.set(category, bucket);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/mcp.ts` around lines 1626 - 1633, The Map population uses non-null assertions on toolsByServer.get(...)! — replace those with a nullish-coalescing pattern: when retrieving the array use toolsByServer.get(key) ?? [] (or assign a local array via let arr = toolsByServer.get(key) ?? []; then push and set back if newly created) so you avoid the bang operator; update occurrences around the toolsByServer population (and the other similar access at the later usage) to use this pattern or ensure you set and retrieve the array via a safe variable instead of using the non-null assertion.src/lib/mcp/multiServerManager.ts-294-323 (1)
294-323:⚠️ Potential issue | 🟡 MinorReplace generic throws with ErrorFactory for consistent typed errors.
Multiple methods throw
new Error(...)directly. Please switch to ErrorFactory to align with typed-error conventions across the codebase.
As per coding guidelines: Use ErrorFactory for creating typed errors.Also applies to: 351-360, 487-490
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/multiServerManager.ts` around lines 294 - 323, The code currently throws raw Error objects in updateServer (throw new Error(`Server '${serverId}' not found`)), in createGroup (throw new Error(`Server '${serverId}' not found when creating group '${group.id}'`)) and at the other noted spots; replace those with the project ErrorFactory to produce typed errors (e.g., call ErrorFactory.create or the project's standard factory method) so callers get consistent error types; update the thrown instances in updateServer, createGroup and the other referenced methods (around the other ranges 351-360 and 487-490) to use ErrorFactory with the same message text and include any relevant context (serverId, group.id) when constructing the ErrorFactory error.test/mcp/cli-mcp-commands.test.ts-1-7 (1)
1-7:⚠️ Potential issue | 🟡 MinorPlace this feature test under
test/suites/per repo conventions.This is a feature-specific CLI suite, so it should live under
test/suites/(e.g.,test/suites/mcp/cli-mcp-commands.test.ts) instead oftest/mcp/.
As per coding guidelines: Create test suites in test/suites/ for feature-specific tests, test/integration/ for real provider integration tests. Use Vitest as test runner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mcp/cli-mcp-commands.test.ts` around lines 1 - 7, The test file test/mcp/cli-mcp-commands.test.ts is a feature-specific CLI suite and should be relocated under the repository's test/suites hierarchy; move the file to test/suites/mcp/cli-mcp-commands.test.ts (or create test/suites/mcp/ and place the test there), update any import paths if needed (e.g., references to MCPCommandFactory, or mocked SDK helpers) so imports still resolve, and ensure the test runner configuration (Vitest) picks up tests from test/suites.test/mcp/integration/mcp-enhancements.integration.test.ts-1-14 (1)
1-14:⚠️ Potential issue | 🟡 MinorMove this integration suite under
test/integration/(ortest/integration/mcp/).This is an integration suite but it lives under
test/mcp/integration, which conflicts with the repo’s test placement convention. Please relocate to the integration test directory to keep structure consistent.
As per coding guidelines: Create test suites in test/suites/ for feature-specific tests, test/integration/ for real provider integration tests. Use Vitest as test runner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mcp/integration/mcp-enhancements.integration.test.ts` around lines 1 - 14, The integration test file mcp-enhancements.integration.test.ts is placed under test/mcp/integration which breaks repo conventions; move the file to test/integration/ or test/integration/mcp/ (or into test/suites/ if it’s feature-specific) and update any internal imports or relative paths used by the test, ensure the filename and Vitest patterns still match (so Vitest picks up *.integration.test.ts), and run the test runner to verify no import errors remain.src/lib/mcp/routing/toolRouter.ts-265-270 (1)
265-270:⚠️ Potential issue | 🟡 MinorUse ErrorFactory for typed errors instead of
new Error.The router throws a generic
Errorwhen no servers are available; per guidelines, please construct typed errors via ErrorFactory for consistency and structured handling.
As per coding guidelines: Use ErrorFactory for creating typed errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/routing/toolRouter.ts` around lines 265 - 270, Replace the generic throw new Error with a typed error created by the project's ErrorFactory: where candidates are computed (the block using this.getCandidateServers(tool) in the ToolRouter/routing method), construct an ErrorFactory error (e.g., ErrorFactory.create or the project-specific factory method for "no healthy servers") including a unique error code and contextual metadata like tool.name, then throw that typed error instead of new Error so downstream error handling can rely on the structured type.src/lib/mcp/enhancedToolDiscovery.ts-178-195 (1)
178-195:⚠️ Potential issue | 🟡 MinorUse
withTimeout+ ErrorFactory for discovery timeout/error paths.The manual
Promise.race+new Error(...)sequence should be replaced with the sharedwithTimeouthelper and ErrorFactory-based error construction to keep async error handling consistent.
As per coding guidelines: Use ErrorFactory for creating typed errors. Wrap async operations with withTimeout utility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/enhancedToolDiscovery.ts` around lines 178 - 195, Replace the manual Promise.race timeout logic in EnhancedToolDiscovery with the shared withTimeout helper when calling client.listTools(), and replace direct new Error(...) constructions with ErrorFactory-created errors; specifically, call withTimeout(client.listTools(), timeout, ...) and use ErrorFactory to construct a timeout error and the "No tools returned from server" error so the discovery timeout and empty-result paths produce typed errors consistent with the rest of the codebase.src/lib/mcp/enhancedToolDiscovery.ts-206-213 (1)
206-213:⚠️ Potential issue | 🟡 MinorRemove non‑null assertions flagged by lint.
The non‑null assertions (
!) in discovery and search are explicitly forbidden by the static analysis gate. Please use local variables or fallback initialization instead.✅ Example fixes
- if (!this.serverToolsMap.has(serverId)) { - this.serverToolsMap.set(serverId, new Set()); - } - this.serverToolsMap.get(serverId)!.add(tool.name); + const toolSet = + this.serverToolsMap.get(serverId) ?? new Set<string>(); + toolSet.add(tool.name); + this.serverToolsMap.set(serverId, toolSet);- if (criteria.serverIds?.length) { + if (criteria.serverIds?.length) { + const serverIds = criteria.serverIds; results = results.filter((tool) => - criteria.serverIds!.includes(tool.serverId), + serverIds.includes(tool.serverId), ); } @@ - if (criteria.tags?.length) { + if (criteria.tags?.length) { + const tags = criteria.tags; results = results.filter((tool) => { const toolTags = tool.annotations?.tags ?? []; - return criteria.tags!.some((tag) => toolTags.includes(tag)); + return tags.some((tag) => toolTags.includes(tag)); }); } @@ - if (criteria.annotations) { + if (criteria.annotations) { + const annotations = criteria.annotations; results = results.filter((tool) => { if (!tool.annotations) { return false; } - for (const [key, value] of Object.entries(criteria.annotations!)) { + for (const [key, value] of Object.entries(annotations)) { const annotationKey = key as keyof MCPToolAnnotations; if (tool.annotations[annotationKey] !== value) { return false; } }Also applies to: 349-385
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/enhancedToolDiscovery.ts` around lines 206 - 213, The code uses non-null assertions on this.serverToolsMap.get(serverId)!; replace them by reading into a local variable and ensuring fallback initialization: e.g., const tools = this.serverToolsMap.get(serverId) ?? new Set<string>(); if (!this.serverToolsMap.has(serverId)) this.serverToolsMap.set(serverId, tools); tools.add(tool.name); apply the same pattern wherever non-null assertions are used in discovery/search (refer to usages around lines 349-385) so you never rely on ! and always operate on a defined Set.src/lib/mcp/multiServerManager.ts-261-271 (1)
261-271:⚠️ Potential issue | 🟡 MinorClean up round-robin counters when auto-removing empty groups.
When a group becomes empty during
removeServer, the group is deleted but its round-robin counter isn’t removed, leaving stale state behind.🧹 Suggested cleanup
if (group.servers.length === 0) { this.groups.delete(groupId); + this.roundRobinCounters.delete(groupId); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/multiServerManager.ts` around lines 261 - 271, The removeServer loop deletes empty groups from this.groups but doesn't clear their round-robin state, leaving stale entries in the round-robin map; update the cleanup inside the block that handles empty groups (in the removeServer flow) to also remove the corresponding entry from the round-robin counters (e.g., call this.roundRobinCounters.delete(groupId) or the equivalent method on whatever round-robin structure you use) so the group's counter is cleaned up when the group is deleted.src/lib/mcp/elicitationProtocol.ts-38-42 (1)
38-42:⚠️ Potential issue | 🟡 Minor
elicitation/timeoutmessage type declared but never handled inhandleMessage.
ElicitationProtocolMessageTypeincludes"elicitation/timeout", but theswitchinhandleMessage(lines 377-386) only handlesrequest,response, andcancel. If a timeout message arrives, it silently falls through with no action and no log, which will be difficult to debug.Either add a handler/log for the timeout case, or remove it from the union type if it's not expected as an inbound message.
Proposed fix (add explicit timeout handling)
case "elicitation/cancel": return this.handleCancel(message); + + default: + if (this.config.enableLogging) { + logger.warn(`[ElicitationProtocol] Unhandled method: ${message.method}`); + } }Also applies to: 377-386
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/elicitationProtocol.ts` around lines 38 - 42, ElicitationProtocolMessageType includes "elicitation/timeout" but handleMessage's switch (in function handleMessage) only handles "elicitation/request", "elicitation/response", and "elicitation/cancel", so add explicit handling for "elicitation/timeout" in handleMessage: update the switch to include a case "elicitation/timeout" that logs the timeout (using the same logger used elsewhere in this module) and performs any cleanup/transition that other timeouts would require (or call the existing timeout handler if one exists), or if timeout messages should never be received remove "elicitation/timeout" from the ElicitationProtocolMessageType union; reference the types ElicitationProtocolMessageType and the handleMessage switch to locate and apply the change.
🧹 Nitpick comments (17)
test/mcp/mcpCircuitBreaker.test.ts (2)
491-512:lastStateChangeassertion is trivially true under fake timers.With fake timers active and no call to
vi.advanceTimersByTime()between the initial read (line 497) and the post-failure read (line 508),Date.now()returns the same value for both, sotoBeGreaterThanOrEqualpasses even if the implementation never updateslastStateChange. Advancing the clock before the state-changing call would make this test meaningful.Suggested fix
const initialChange = breaker.getStats().lastStateChange; + // Advance time so the state change gets a different timestamp + vi.advanceTimersByTime(100); + try { await breaker.execute(async () => { throw new Error("fail"); }); } catch { // Expected } const stats = breaker.getStats(); - expect(stats.lastStateChange.getTime()).toBeGreaterThanOrEqual( + expect(stats.lastStateChange.getTime()).toBeGreaterThan( initialChange.getTime(), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mcp/mcpCircuitBreaker.test.ts` around lines 491 - 512, The test for MCPCircuitBreaker.getStats().lastStateChange is trivially true under fake timers; update the test to advance the fake clock so the timestamp can change: call vi.advanceTimersByTime(...) (or vi.setSystemTime(...) as appropriate) on the fake timers before invoking breaker.execute(...) (or immediately before re-reading breaker.getStats()) so that lastStateChange from MCPCircuitBreaker is actually updated and the expect comparing initialChange.getTime() to stats.lastStateChange.getTime() becomes meaningful.
97-148: Consider usingrejectspattern consistently instead of try/catch.Lines 101-105 already use the cleaner
await expect(...).rejects.toThrow(...)style. The subsequent tests in this block (lines 113-119, 132-138) and many later tests fall back to verbose try/catch with empty catch blocks. Usingrejects.toThrowthroughout would be more idiomatic Vitest and eliminate the// Expectedcomments.Example for lines 110-124:
Suggested refactor
it("should record failed calls", async () => { const breaker = new MCPCircuitBreaker("test"); - try { - await breaker.execute(async () => { - throw new Error("fail"); - }); - } catch { - // Expected - } + await expect( + breaker.execute(async () => { + throw new Error("fail"); + }), + ).rejects.toThrow("fail"); const stats = breaker.getStats(); expect(stats.failedCalls).toBe(1); breaker.destroy(); });This applies broadly to tests at lines 159-167, 182-190, 211-219, 242-248, 267-273, 298-304, 327-333, 387-393, 399-405, 419-426, 455-461, 478-484, 499-505, 640-646, 647-653, 677-683.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mcp/mcpCircuitBreaker.test.ts` around lines 97 - 148, Replace the verbose try/catch patterns in the failing-operations tests with the idiomatic Vitest rejects matcher: instead of wrapping breaker.execute(...) in try/catch and swallowing the error, use await expect(breaker.execute(() => { ... })).rejects.toThrow(...) so assertions are explicit and the tests remain concise; update the tests that call MCPCircuitBreaker.execute in the "failed operations" describe block (the tests asserting failedCalls and callFailure) to use this rejects pattern and remove the empty catch blocks and "// Expected" comments.src/lib/utils/schemaConversion.ts (2)
185-199:ensureTypeFieldmutates the caller's object in-place.Both the AI SDK path (line 157) and the plain-JSON-Schema path (line 162) hand the original object reference to
ensureTypeField, which writestypeandpropertiesdirectly onto it. If a caller retains a reference to the schema and inspects it later, or if the same schema object is processed again, it will have been silently modified.Shallow-clone before mutating to keep the function side-effect-free:
Proposed fix
function ensureTypeField( schema: Record<string, unknown>, ): Record<string, unknown> { + const result = { ...schema }; - if (!schema.type) { - schema.type = "object"; - if (!schema.properties) { - schema.properties = {}; - } + if (!result.type) { + result.type = "object"; + if (!result.properties) { + result.properties = {}; + } logger.debug("[SCHEMA-TYPE-FIX] Added missing type field to JSON Schema", { fixedType: "object", - hasProperties: !!schema.properties, + hasProperties: !!result.properties, }); } - return schema; + return result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/utils/schemaConversion.ts` around lines 185 - 199, ensureTypeField currently mutates the passed-in schema object in place; change it to be side-effect-free by shallow-cloning the input (e.g., create a newSchema from schema) and performing any additions on that clone instead of schema, setting newSchema.type = "object" and ensuring newSchema.properties = {} when missing, then call logger.debug with the same info and return the cloned newSchema; this preserves the original reference and prevents unexpected external mutations when callers reuse the schema.
146-152: Widen parameter type to match the three documented input forms.The function intentionally handles Zod schemas, AI SDK
jsonSchema()wrappers, and plain JSON Schema objects (per the JSDoc), but declares the parameter asZodUnknownSchemaonly. All callers currently work around this withas ZodUnknownSchemacasts.Consider changing the signature to
zodSchema: ZodUnknownSchema | Record<string, unknown>or defining an overload to make the three input types explicit and eliminate unsafe casts at call sites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/utils/schemaConversion.ts` around lines 146 - 152, The parameter type for convertZodToJsonSchema is too narrow (ZodUnknownSchema) even though the function accepts Zod schemas, AI SDK jsonSchema() wrapper objects, and plain JSON Schema objects; update the function signature to accept a wider union (e.g., zodSchema: ZodUnknownSchema | Record<string, unknown>) or add overloads that enumerate the three accepted input types so callers no longer need unsafe `as ZodUnknownSchema` casts, and ensure any internal type guards (the existing schema variable and checks) still correctly handle the widened type.test/continuous-test-suite-mcp.ts (1)
353-366: Unused import results —retryHandlerandrateLimiterare assigned but never read.The dynamic imports succeed (proving the modules exist) but the imported values are never inspected. If the intent is solely to verify the module loads, drop the assignment.
Proposed fix
try { - const retryHandler = await import("../src/lib/mcp/httpRetryHandler.js"); + await import("../src/lib/mcp/httpRetryHandler.js"); recordTest("httpRetryHandler module", true); } catch { recordTest("httpRetryHandler", true, true, "Not implemented"); } // Test rate limiter try { - const rateLimiter = await import("../src/lib/mcp/httpRateLimiter.js"); + await import("../src/lib/mcp/httpRateLimiter.js"); recordTest("httpRateLimiter module", true); } catch { recordTest("httpRateLimiter", true, true, "Not implemented"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/continuous-test-suite-mcp.ts` around lines 353 - 366, The dynamic imports assign to retryHandler and rateLimiter but those bindings are never used; change the lines to simply await the import (e.g., await import("../src/lib/mcp/httpRetryHandler.js"); and await import("../src/lib/mcp/httpRateLimiter.js");) so the modules are loaded without creating unused variables, keeping the surrounding try/catch and the recordTest("httpRetryHandler"...) / recordTest("httpRateLimiter"...) calls intact.src/lib/mcp/toolAnnotations.ts (1)
140-233: Annotation inference is substring-based, which can produce false positives.For example, a tool named
"resetPassword"matches both the destructive keyword"reset"and the read-only keyword"check"if its description contains"check credentials and reset". This is inherent to heuristic inference, but consider using word-boundary matching (e.g., whole-word checks) to reduce noise — especially sincereadOnlyHintanddestructiveHintbeing set simultaneously is flagged as conflicting byvalidateAnnotations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/toolAnnotations.ts` around lines 140 - 233, The inference in inferAnnotations uses substring matching on tool.name and tool.description which raises false positives; change the checks for readOnlyKeywords, destructiveKeywords, idempotentKeywords, and complexKeywords to perform whole-word/word-boundary matching (e.g., tokenize description and name or use regex word-boundaries) and normalize punctuation/casing first, and also add a simple conflict resolution (e.g., if destructiveHint is detected, do not set readOnlyHint) so inferAnnotations never sets conflicting hints; update the keyword checks that reference readOnlyKeywords, destructiveKeywords, idempotentKeywords, complexKeywords and the annotations.readOnlyHint / annotations.destructiveHint logic accordingly.src/lib/mcp/serverCapabilities.ts (1)
281-298: Inconsistent duplicate-registration behavior: resources silently overwrite, prompts throw.
registerResource(line 287) allows overwriting an existing URI without warning, whilevalidatePromptName(line 654) throws on duplicate prompt names. If overwriting resources is intentional, consider documenting it; otherwise, add a duplicate-URI check or emit a warning for consistency.Also applies to: 537-557
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/serverCapabilities.ts` around lines 281 - 298, registerResource currently overwrites existing entries silently while validatePromptName throws on duplicates; make the behavior consistent by adding an explicit duplicate-URI check in registerResource: if this.resources.has(resource.uri) either throw a descriptive Error (to match validatePromptName) or at minimum logger.warn and return this without overwriting, and avoid emitting "resourceRegistered" in that case; update the analogous code path referenced (the other occurrence around the 537-557 region) to use the same duplicate handling so both resources and prompts behave consistently.src/lib/mcp/agentExposure.ts (1)
198-365: Significant duplication betweenexposeAgentAsToolandexposeWorkflowAsTool.Both functions share ~90% of their logic (name generation, description building, annotation merging, timeout-wrapped execution, result wrapping). The differences are limited to the prefix default, a couple of metadata fields, and log labels. Consider extracting a shared
exposeAsToolhelper parameterized by source type to reduce the maintenance surface.Also applies to: 370-539
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/agentExposure.ts` around lines 198 - 365, Extract the duplicated logic in exposeAgentAsTool and exposeWorkflowAsTool into a shared helper (e.g., exposeAsTool) that accepts the exposable object, a sourceType string ("agent" | "workflow"), and configurable defaults (default prefix, nameTransformer, includeMetadataInDescription, wrapWithContext, executionTimeout, enableLogging, defaultAnnotations); move shared steps—name generation (baseName/toolName), description building (including metadata parts), annotation merging, input/output schema handling, timeout-wrapped execute creation (preserving logging and executionContext creation), and tool/return object construction—into that helper, and then implement exposeAgentAsTool and exposeWorkflowAsTool as thin wrappers that call exposeAsTool with their specific defaults and any small differences (e.g., metadata fields, log labels, prefix) while preserving the existing symbol names execute, tool, toolName, and returned structure.src/lib/mcp/elicitation/elicitationManager.ts (1)
479-479: Module-level singleton creates shared mutable state across consumers.
globalElicitationManageris instantiated at module load with default config. Any consumer importing this module gets the same instance, which means a handler set in one part of the app silently affects all others. This is fine if intentional (and documented), but worth noting that tests sharing this singleton can interfere with each other.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/elicitation/elicitationManager.ts` at line 479, The module-level instantiation export globalElicitationManager = new ElicitationManager() creates a shared mutable singleton that can leak state across consumers and tests; change the API to avoid eager singletons by either exporting a factory (e.g., createElicitationManager or newElicitationManager) or a controlled accessor (e.g., getElicitationManager(config?, reset?: boolean)) that returns a fresh or lazily-initialized instance instead of a global one; update call sites to call the factory/accessor and remove direct reliance on globalElicitationManager so each consumer or test can own its own ElicitationManager instance.src/lib/mcp/mcpServerBase.ts (2)
298-303: EmptyrequiresConfirmationblock is dead code.The comment explains the intent but the block is a no-op. Either remove it or add a TODO/FIXME to track the integration work, so it doesn't silently bypass the confirmation requirement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/mcpServerBase.ts` around lines 298 - 303, The empty conditional checking tool.annotations?.requiresConfirmation is dead code; either remove the no-op block or replace it with a clear TODO/FIXME and a minimal placeholder action so the intent isn't lost—e.g., add a FIXME comment referencing the HITL manager/ExternalServerManager and either log a warning or throw a NotImplemented/Assertion (so callers can't silently bypass confirmation) and include the symbol requiresConfirmation in the message to make locating it easy when implementing the integration.
278-360: Wraptool.execute()withwithTimeoutper coding guidelines.The async
tool.execute(params, context ?? {})call at line 305 lacks timeout protection, allowing misbehaving tools to hang indefinitely. The coding guidelines require wrapping async operations with thewithTimeoututility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/mcpServerBase.ts` around lines 278 - 360, The call to tool.execute in executeTool must be wrapped with the withTimeout utility to prevent hanging tools: replace await tool.execute(params, context ?? {}) with something like await withTimeout(() => tool.execute(params, context ?? {}), timeoutMs), where timeoutMs is taken from the tool config or a server default (e.g., tool.timeoutMs || this.config.toolTimeout || DEFAULT_TOOL_TIMEOUT); ensure withTimeout is imported and that timeouts propagate to the existing try/catch so errors from withTimeout are handled the same way and returned as ToolResult (affecting the branches that use isToolResult and the emitted "toolExecuted"/"toolError" events).src/lib/mcp/batching/requestBatcher.ts (1)
377-419: Server queue selection always picks the first Map entry — starvation risk.
selectBatchRequestswithgroupByServeralways takes fromthis.serverQueues.entries().next().value(line 382), which is the first-inserted server. If that server continuously receives new requests, other server queues starve. A round-robin index or least-queued-first selection would be fairer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/batching/requestBatcher.ts` around lines 377 - 419, selectBatchRequests currently always picks the first server queue from this.serverQueues which can cause starvation; change the server selection when this.config.groupByServer is true to choose fairly (e.g., round-robin or least-queued-first) instead of entries().next().value. Implement this by tracking a rotating pointer/last selected server id (add a field like lastServerId or lastServerKey) and on each call find the next server key after lastServerId (wrapping to start) or pick the server with the smallest requestIds.size, then pull up to this.config.maxBatchSize requests from that selected server's Set and update lastServerId (and delete empty queues) so other server queues get served fairly; keep the rest of the logic (pushing to batchRequests and deleting from this.pending and requestIds) unchanged.src/cli/commands/mcp.ts (2)
2955-3279: SplitexecuteAnnotateinto smaller helpers.This method exceeds the lint limit and is hard to reason about. Extract list-mode rendering and annotation application/validation into separate private helpers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/mcp.ts` around lines 2955 - 3279, The executeAnnotate method is too large; extract the list-mode rendering and the annotation building/validation/display into helpers. Create a private helper like renderAnnotateList(servers, argv) that encapsulates the entire "if (argv.list) { ... }" block (spinner, inferAnnotations loop, grouping by server, formatting/JSON output), and another helper applyAndValidateAnnotations(foundTool, argv) that builds annotations (parses argv.annotations, applies individual flags, handles argv.infer with inferAnnotations, runs validateAnnotations, supports argv.validate mode and returns the final annotations or throws/returns validation errors). Also factor out tool lookup into a small findTool(servers, toolName, serverId) function used by executeAnnotate, and replace the original blocks with calls to these helpers so executeAnnotate becomes a thin coordinator that calls renderAnnotateList, findTool, applyAndValidateAnnotations, then displays/persists results.
1407-1423: Wrap new async SDK/registry calls withwithTimeout.These new commands call into SDK/registry operations without timeouts, which can hang the CLI if a server stalls. Please wrap these awaits with the repo’s
withTimeoututility.As per coding guidelines: "Wrap async operations with withTimeout utility."
Also applies to: 1518-1524, 1697-1699, 2549-2562
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/mcp.ts` around lines 1407 - 1423, The executeServers method currently calls new async SDK methods directly (instantiating NeuroLink and awaiting listMCPServers) without using the withTimeout helper; change those awaits to use withTimeout (e.g., await withTimeout(sdk.listMCPServers(), timeout)) so the CLI won’t hang — update the call that sets servers and any other SDK/registry awaits in this file to use withTimeout, keeping the same error handling and ensuring the spinner (ora) is stopped on timeout; specifically modify executeServers (NeuroLink(), listMCPServers()) and apply the same wrapping to the other async SDK/registry calls referenced in the review.src/lib/mcp/elicitation/types.ts (1)
1-338: Consider relocating these reusable types tosrc/lib/types/and re-exporting.Project standards prefer shared/public types under
src/lib/types/*.ts, which helps avoid circular dependencies and keeps type organization consistent. Moving these types (e.g., tosrc/lib/types/elicitationTypes.ts) and re-exporting frommcp/elicitationwould align with that standard.
Based on learnings: Project standard: Place reusable/shared types under src/lib/types/.ts; test-only helper types under test/types/.ts; avoid declaring local types inside source implementation files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/elicitation/types.ts` around lines 1 - 338, Extract the reusable elicitation types into a new shared types module and re-export them from the original mcp/elicitation module for compatibility: move the definitions (ElicitationType, ElicitationRequest, ConfirmationElicitation, TextElicitation, SelectOption, SelectElicitation, MultiSelectElicitation, FormField, FormElicitation, FileElicitation, SecretElicitation, Elicitation, ElicitationResponse, ElicitationHandler, ElicitationManagerConfig, ElicitationContext) into a central types file, import JsonValue/JsonObject from the shared common types there, update project imports to reference the new shared types module, and add re-exports in the existing mcp/elicitation/types module so existing imports keep working; run the typechecker to ensure no circular deps or broken imports remain.src/lib/mcp/elicitationProtocol.ts (2)
392-404:handleRequesthas no error handling — exceptions won't produce protocol error responses.If
this.manager.request()throws (e.g., no handler registered, handler rejects), the raw exception propagates instead of returning a protocol-compliant error response message. Consider wrapping in try/catch to return anElicitationResponseMessagewithresponded: falseanderrorset.Additionally, per coding guidelines, async operations should use
withTimeoutand errors should useErrorFactory.Proposed fix (error handling)
private async handleRequest( message: ElicitationRequestMessage, ): Promise<ElicitationResponseMessage> { - const elicitation = protocolMessageToElicitation(message); - - // Use the manager to process the request - const response = await this.manager.request({ - ...elicitation, - timeout: elicitation.timeout ?? this.config.defaultTimeout, - }); - - return elicitationResponseToProtocol(response); + try { + const elicitation = protocolMessageToElicitation(message); + const response = await this.manager.request({ + ...elicitation, + timeout: elicitation.timeout ?? this.config.defaultTimeout, + }); + return elicitationResponseToProtocol(response); + } catch (error) { + return createElicitationResponse(message.id, { + responded: false, + error: error instanceof Error ? error.message : String(error), + }); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/elicitationProtocol.ts` around lines 392 - 404, handleRequest currently calls this.manager.request(...) without error handling so thrown exceptions leak instead of returning an ElicitationResponseMessage; wrap the manager call in try/catch and use withTimeout to enforce elicitation.timeout ?? this.config.defaultTimeout, then on success pass the response into elicitationResponseToProtocol, and on failure build a protocol error response object (ElicitationResponseMessage) with responded: false and error created via ErrorFactory (include message/stack/details) before converting via elicitationResponseToProtocol; ensure you still convert the original protocolMessageToElicitation at start and return the protocol-compliant error response on catch.
319-321: Default case returnsbase as Elicitation— bypasses discriminated union safety.If
typedoesn't match any known variant (e.g., a future protocol extension sends an unknown type), the returned object won't satisfy any variant of theElicitationdiscriminated union, which can cause downstreamswitchstatements to silently skip it or produce confusing behavior.Consider throwing or returning a well-defined error/fallback instead of the unsafe cast.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/elicitationProtocol.ts` around lines 319 - 321, The switch's default case unsafely returns base as Elicitation, bypassing the discriminated-union checks; replace that unsafe cast in the default branch with a safe failure path: either throw a descriptive Error (including base.type and any id/context) or return a well-defined fallback variant that conforms to the Elicitation union; update the switch in the function where this switch lives (the default branch handling of base / Elicitation) and ensure callers handle the thrown error or the new fallback appropriately.
| // Generate tool definitions | ||
| const toolDefs = | ||
| tools.length > 0 | ||
| ? tools | ||
| .map( | ||
| (toolName) => ` | ||
| // ${toolName} tool | ||
| server.setRequestHandler(CallToolRequestSchema, async (request) => { | ||
| if (request.params.name === "${toolName}") { | ||
| // TODO: Implement ${toolName} tool | ||
| return { | ||
| content: [{ | ||
| type: "text", | ||
| text: "${toolName} executed successfully", | ||
| }], | ||
| }; | ||
| } | ||
| throw new Error(\`Unknown tool: \${request.params.name}\`); | ||
| }); | ||
|
|
||
| server.setRequestHandler(ListToolsRequestSchema, async () => ({ | ||
| tools: [ | ||
| { | ||
| name: "${toolName}", | ||
| description: "TODO: Add description for ${toolName}", | ||
| inputSchema: { | ||
| type: "object", | ||
| properties: {}, | ||
| required: [], | ||
| }, | ||
| }, | ||
| ], | ||
| })); | ||
| `, | ||
| ) | ||
| .join("\n") | ||
| : ` | ||
| server.setRequestHandler(ListToolsRequestSchema, async () => ({ | ||
| tools: [ | ||
| { | ||
| name: "hello", | ||
| description: "A simple hello tool", | ||
| inputSchema: { | ||
| type: "object", | ||
| properties: { | ||
| name: { type: "string", description: "Name to greet" }, | ||
| }, | ||
| required: ["name"], | ||
| }, | ||
| }, | ||
| ], | ||
| })); | ||
|
|
||
| server.setRequestHandler(CallToolRequestSchema, async (request) => { | ||
| if (request.params.name === "hello") { | ||
| const args = request.params.arguments as { name?: string }; | ||
| return { | ||
| content: [{ | ||
| type: "text", | ||
| text: \`Hello, \${args.name || "World"}!\`, | ||
| }], | ||
| }; | ||
| } | ||
| throw new Error(\`Unknown tool: \${request.params.name}\`); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, understand the scaffold code structure at lines 1995-2059
sed -n '1995,2059p' src/cli/commands/mcp.tsRepository: juspay/neurolink
Length of output: 1827
🏁 Script executed:
# Check setRequestHandler usage patterns in the codebase
rg -n "setRequestHandler" src/ --type ts -B 2 -A 2Repository: juspay/neurolink
Length of output: 1863
🏁 Script executed:
# Look for MCP SDK documentation or type definitions
fd -e ts -e js node_modules/@modelcontextprotocol/sdk 2>/dev/null | head -20Repository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Check if there are any tests or examples showing correct MCP tool registration
rg -n "CallToolRequestSchema\|ListToolsRequestSchema" src/ --type ts -B 3 -A 3Repository: juspay/neurolink
Length of output: 42
TypeScript scaffold only registers the last tool due to handler override.
Each tool in the template registers its own CallToolRequestSchema and ListToolsRequestSchema handlers. When the template is generated with multiple tools, later registrations overwrite earlier ones, leaving only the final tool callable and listed. Consolidate to a single pair of handlers: one ListToolsRequestSchema handler listing all tools, and one CallToolRequestSchema handler with a switch statement routing to each tool.
Suggested fix
- const toolDefs =
- tools.length > 0
- ? tools
- .map(
- (toolName) => `
- // ${toolName} tool
- server.setRequestHandler(CallToolRequestSchema, async (request) => {
- if (request.params.name === "${toolName}") {
- // TODO: Implement ${toolName} tool
- return {
- content: [{
- type: "text",
- text: "${toolName} executed successfully",
- }],
- };
- }
- throw new Error(\`Unknown tool: \${request.params.name}\`);
- });
-
- server.setRequestHandler(ListToolsRequestSchema, async () => ({
- tools: [
- {
- name: "${toolName}",
- description: "TODO: Add description for ${toolName}",
- inputSchema: {
- type: "object",
- properties: {},
- required: [],
- },
- },
- ],
- }));
-`,
- )
- .join("\n")
- : `
+ const toolDefs =
+ tools.length > 0
+ ? `
+ server.setRequestHandler(ListToolsRequestSchema, async () => ({
+ tools: [
+${tools
+ .map(
+ (toolName) =>
+ ` { name: "${toolName}", description: "TODO: Add description for ${toolName}", inputSchema: { type: "object", properties: {}, required: [] } },`,
+ )
+ .join("\n")}
+ ],
+ }));
+
+ server.setRequestHandler(CallToolRequestSchema, async (request) => {
+ switch (request.params.name) {
+${tools
+ .map(
+ (toolName) => ` case "${toolName}":
+ return {
+ content: [{ type: "text", text: "${toolName} executed successfully" }],
+ };`,
+ )
+ .join("\n")}
+ default:
+ throw new Error(\`Unknown tool: \${request.params.name}\`);
+ }
+ });
+`
+ : `🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/commands/mcp.ts` around lines 1995 - 2059, The template currently
registers server.setRequestHandler repeatedly for CallToolRequestSchema and
ListToolsRequestSchema which causes later handlers to overwrite earlier ones;
replace the per-tool handler generation with a single
server.setRequestHandler(ListToolsRequestSchema, ...) that returns an array of
all tools (map the tools variable into name/description/inputSchema entries) and
a single server.setRequestHandler(CallToolRequestSchema, async (request) => {
switch (request.params.name) { case "toolA": /* toolA impl */; break; case
"toolB": /* toolB impl */; break; ... default: throw new Error(`Unknown tool:
${request.params.name}`); } }); ensure each tool’s implementation block (inside
the switch cases) returns the same response shape used before and remove the
multiple per-tool setRequestHandler calls so only these two handlers remain.
| async drain(): Promise<void> { | ||
| await this.flush(); | ||
|
|
||
| // Wait for all active batches to complete | ||
| while (this.activeBatches > 0) { | ||
| await new Promise((resolve) => setTimeout(resolve, 10)); | ||
| } | ||
| } |
There was a problem hiding this comment.
drain() busy-waits with no upper bound — can spin forever if a batch hangs.
The while (this.activeBatches > 0) polling loop has no timeout. If an executor never resolves, this spins indefinitely, blocking the caller and wasting CPU. Consider adding a deadline or using an event/promise-based approach.
Proposed fix (timeout-based)
- async drain(): Promise<void> {
- await this.flush();
-
- // Wait for all active batches to complete
- while (this.activeBatches > 0) {
- await new Promise((resolve) => setTimeout(resolve, 10));
- }
- }
+ async drain(timeoutMs = 30000): Promise<void> {
+ await this.flush();
+
+ const deadline = Date.now() + timeoutMs;
+ while (this.activeBatches > 0) {
+ if (Date.now() > deadline) {
+ throw new Error(`drain() timed out after ${timeoutMs}ms with ${this.activeBatches} active batches`);
+ }
+ await new Promise((resolve) => setTimeout(resolve, 10));
+ }
+ }📝 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.
| async drain(): Promise<void> { | |
| await this.flush(); | |
| // Wait for all active batches to complete | |
| while (this.activeBatches > 0) { | |
| await new Promise((resolve) => setTimeout(resolve, 10)); | |
| } | |
| } | |
| async drain(timeoutMs = 30000): Promise<void> { | |
| await this.flush(); | |
| const deadline = Date.now() + timeoutMs; | |
| while (this.activeBatches > 0) { | |
| if (Date.now() > deadline) { | |
| throw new Error(`drain() timed out after ${timeoutMs}ms with ${this.activeBatches} active batches`); | |
| } | |
| await new Promise((resolve) => setTimeout(resolve, 10)); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/mcp/batching/requestBatcher.ts` around lines 231 - 238, The drain()
method currently busy-waits on while (this.activeBatches > 0) and can hang
forever; change it to wait with an upper bound by replacing the polling loop
with a Promise-based wait that races the completion condition against a
configurable timeout/deadline. Specifically, after await this.flush(), create a
Promise that resolves when this.activeBatches becomes 0 (emit/resolve from the
code path that decrements activeBatches or subscribe to an EventEmitter) and use
Promise.race to reject or timeout after a provided maxWaitMs; ensure drain()
returns/rejects appropriately on timeout so callers won’t spin forever.
Reference the drain() method and the activeBatches counter (and flush()) when
making the change.
| private async executeBatch(): Promise<void> { | ||
| if (this.pending.size === 0) { | ||
| return; | ||
| } | ||
|
|
||
| // Check concurrent batch limit | ||
| if (this.activeBatches >= this.config.maxConcurrentBatches) { | ||
| // Reschedule for later | ||
| this.flushTimer = setTimeout(() => { | ||
| this.executeBatch(); | ||
| }, 10); | ||
| return; | ||
| } |
There was a problem hiding this comment.
flushTimer is shared and can be silently overwritten, losing scheduled flushes.
Multiple code paths assign to this.flushTimer without clearing the previous one:
- Line 182:
add()sets a delayed flush timer - Line 275:
scheduleFlush()clears it then usessetImmediate - Line 297:
executeBatch()sets a retry timer when at max concurrency - Line 371:
executeBatch()sets a follow-up timer for remaining requests
If two of these run concurrently (which they can since setImmediate/setTimeout interleave), one timer reference overwrites another, and the overwritten timeout callback becomes orphaned — it still fires but this.flushTimer no longer tracks it. This can cause duplicate batch executions or lost timer references.
Consider using separate timer variables for retry vs. queue-drain, or a flush-pending flag to coalesce.
Also applies to: 369-374
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/mcp/batching/requestBatcher.ts` around lines 289 - 301, The shared
this.flushTimer is being overwritten by multiple paths (add(), scheduleFlush(),
executeBatch()) which causes orphaned timers and lost scheduling; fix by
introducing distinct timer slots (e.g., this.retryTimer and this.drainTimer) or
a single flushPending boolean plus one canonical timer manager, ensure every
setter clears the specific previous timer before assigning and use those
distinct names inside executeBatch(), scheduleFlush(), and add() (and the
follow-up block around pending checks/activeBatches) so retry vs queue-drain
timers cannot clobber each other and callbacks remain tracked.
| export function createTimeoutMiddleware(timeoutMs: number): ToolMiddleware { | ||
| return async (tool, params, context, next) => { | ||
| const timeoutPromise = new Promise<never>((_, reject) => { | ||
| setTimeout( | ||
| () => | ||
| reject( | ||
| new Error(`Tool '${tool.name}' timed out after ${timeoutMs}ms`), | ||
| ), | ||
| timeoutMs, | ||
| ); | ||
| }); | ||
|
|
||
| return Promise.race([next(), timeoutPromise]); | ||
| }; |
There was a problem hiding this comment.
Use withTimeout + ErrorFactory to avoid dangling timers and typed errors.
createTimeoutMiddleware uses a raw Promise.race with setTimeout. The timer is never cleared, so it can keep the event loop alive after the tool finishes, and the error is untyped. Prefer the shared withTimeout helper with ErrorFactory.toolTimeout for cleanup and consistent error typing.
🔧 Suggested change
-import { logger } from "../utils/logger.js";
+import { logger } from "../utils/logger.js";
+import { ErrorFactory, withTimeout } from "../utils/errorHandling.js";
export function createTimeoutMiddleware(timeoutMs: number): ToolMiddleware {
return async (tool, params, context, next) => {
- const timeoutPromise = new Promise<never>((_, reject) => {
- setTimeout(
- () =>
- reject(
- new Error(`Tool '${tool.name}' timed out after ${timeoutMs}ms`),
- ),
- timeoutMs,
- );
- });
-
- return Promise.race([next(), timeoutPromise]);
+ return withTimeout(
+ next(),
+ timeoutMs,
+ ErrorFactory.toolTimeout(tool.name, timeoutMs),
+ );
};
}As per coding guidelines: Use ErrorFactory for creating typed errors. Wrap async operations with withTimeout utility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/mcp/toolIntegration.ts` around lines 309 - 322, Replace the raw
Promise.race/setTimeout pattern in createTimeoutMiddleware with the shared
withTimeout helper and instantiate the timeout error via
ErrorFactory.toolTimeout so timers are cleared and errors are typed;
specifically, wrap the call to next() with withTimeout(next(), timeoutMs, () =>
ErrorFactory.toolTimeout(tool.name, timeoutMs)) (or the project's withTimeout
signature) inside the returned ToolMiddleware to ensure the timer is cleaned up
and the thrown error is created by ErrorFactory.toolTimeout for consistent
typing and messages.
| export const validationMiddleware: ToolMiddleware = async ( | ||
| tool, | ||
| params, | ||
| context, | ||
| next, | ||
| ) => { | ||
| if (!tool.inputSchema) { | ||
| return next(); | ||
| } | ||
|
|
||
| const schema = tool.inputSchema as JsonObject; | ||
| const required = (schema.required as string[]) ?? []; | ||
| const properties = (schema.properties as JsonObject) ?? {}; | ||
|
|
||
| // Validate required parameters | ||
| const paramObj = (params ?? {}) as Record<string, unknown>; | ||
| const missing: string[] = []; | ||
|
|
||
| for (const req of required) { | ||
| if (paramObj[req] === undefined) { | ||
| missing.push(req); | ||
| } | ||
| } | ||
|
|
||
| if (missing.length > 0) { | ||
| // Try to elicit missing parameters | ||
| const formFields: FormField[] = missing.map((name) => { | ||
| const prop = properties[name] as JsonObject | undefined; | ||
| return { | ||
| name, | ||
| label: name, | ||
| type: (prop?.type as FormField["type"]) ?? "text", | ||
| required: true, | ||
| description: prop?.description as string | undefined, | ||
| }; | ||
| }); | ||
|
|
||
| const formResult = await context.elicitation.form( | ||
| `Missing required parameters for ${tool.name}`, | ||
| formFields, | ||
| ); | ||
|
|
||
| if (!formResult) { | ||
| return { | ||
| success: false, | ||
| error: `Missing required parameters: ${missing.join(", ")}`, | ||
| metadata: { missingParams: missing }, | ||
| }; | ||
| } | ||
|
|
||
| // Merge elicited values with params | ||
| Object.assign(paramObj, formResult); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the file structure and size
wc -l src/lib/mcp/toolIntegration.tsRepository: juspay/neurolink
Length of output: 94
🏁 Script executed:
# Check if the file exists and examine key sections
ls -la src/lib/mcp/toolIntegration.tsRepository: juspay/neurolink
Length of output: 140
🏁 Script executed:
# Read the validationMiddleware (lines 367-419)
sed -n '367,419p' src/lib/mcp/toolIntegration.tsRepository: juspay/neurolink
Length of output: 1436
🏁 Script executed:
# Read the executeTool method (lines 468-507)
sed -n '468,507p' src/lib/mcp/toolIntegration.tsRepository: juspay/neurolink
Length of output: 1161
🏁 Script executed:
# Search for executeTool call sites to understand how params are passed
rg -n "\.executeTool\(" src/lib/mcp/toolIntegration.ts -A 2
rg -n "executeTool\(" --type=ts -B 1 -A 2 | head -50Repository: juspay/neurolink
Length of output: 3209
🏁 Script executed:
# Read more of the validationMiddleware to see the complete implementation
sed -n '367,450p' src/lib/mcp/toolIntegration.tsRepository: juspay/neurolink
Length of output: 2193
🏁 Script executed:
# Search for createToolMiddlewareChain to understand middleware chaining
rg -n "createToolMiddlewareChain" src/lib/mcp/toolIntegration.ts -B 2 -A 10Repository: juspay/neurolink
Length of output: 837
🏁 Script executed:
# Search for ToolMiddleware type definition to understand the interface
rg -n "type ToolMiddleware" src/lib --type=ts -A 5Repository: juspay/neurolink
Length of output: 460
🏁 Script executed:
# Read the complete createToolMiddlewareChain implementation
sed -n '225,260p' src/lib/mcp/toolIntegration.tsRepository: juspay/neurolink
Length of output: 895
🏁 Script executed:
# Search for how next() is called in middleware chain
rg -n "executeNext\(\)" src/lib/mcp/toolIntegration.ts -B 2 -A 2Repository: juspay/neurolink
Length of output: 120
🏁 Script executed:
# Check if validationMiddleware passes params to next in any way
sed -n '367,425p' src/lib/mcp/toolIntegration.tsRepository: juspay/neurolink
Length of output: 1490
🏁 Script executed:
# Search for all middleware definitions
rg -n "ToolMiddleware.*=" src/lib/mcp/toolIntegration.ts | head -20Repository: juspay/neurolink
Length of output: 335
🏁 Script executed:
# Check if any other middleware expects params to be mutable
rg -n "const.*Middleware.*async" src/lib/mcp/toolIntegration.ts -A 15 | head -100Repository: juspay/neurolink
Length of output: 1410
🏁 Script executed:
# Look for test cases that call executeTool with null/undefined params
rg -n "executeTool.*\(.*null\|undefined" test/ -B 2 -A 2Repository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Check if params is used or expected to be mutable after middleware execution
sed -n '490,510p' src/lib/mcp/toolIntegration.tsRepository: juspay/neurolink
Length of output: 543
🏁 Script executed:
# Check test coverage for validation middleware with null/undefined params
rg -n "validationMiddleware" test/ -B 2 -A 5Repository: juspay/neurolink
Length of output: 1604
🏁 Script executed:
# Check if there are any integration tests that test param elicitation
rg -n "elicit" test/ --type=ts | head -20Repository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Read the full executeTool signature and understand the complete flow
sed -n '468,510p' src/lib/mcp/toolIntegration.tsRepository: juspay/neurolink
Length of output: 1172
🏁 Script executed:
# Check the MCPServerTool.execute signature to understand what it expects
rg -n "execute\(" src/lib/types/mcpTypes.ts -B 2 -A 5 | head -40Repository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Let me search for actual test cases of validationMiddleware execution
rg -n "validationMiddleware\|ToolIntegrationManager" test/ --type=ts | grep -E "(test|it|describe)" | head -20Repository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Check for any integration tests that use executeTool with params
rg -n "\.executeTool" test/ --type=ts -B 1 -A 1 | head -40Repository: juspay/neurolink
Length of output: 2705
🏁 Script executed:
# Verify MCPServerTool type and execute signature
rg -n "interface MCPServerTool\|type MCPServerTool" src/lib --type=ts -A 10 | head -50Repository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Search for MCPServerTool definition in types
rg -n "MCPServerTool" src/lib/types --type=ts -A 5 | head -100Repository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Check for execute method in tool definitions
rg -n "execute\s*\(" src/lib --type=ts -B 2 -A 2 | grep -E "(execute|MCPServerTool)" | head -30Repository: juspay/neurolink
Length of output: 2731
🏁 Script executed:
# Look at actual tool implementation to understand execute signature
fd ".*tool.*\.ts$" src/lib --type f | head -10Repository: juspay/neurolink
Length of output: 407
🏁 Script executed:
# Find the MCPServerTool interface or type definition
rg -n "interface MCPServerTool\|type MCPServerTool" src/lib --type=ts -A 20Repository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Look at mcpServerBase.ts to see how tool.execute is defined
sed -n '410,425p' src/lib/mcp/mcpServerBase.tsRepository: juspay/neurolink
Length of output: 642
🏁 Script executed:
# Search for where validationMiddleware is actually used
rg -n "validationMiddleware" src/lib --type=ts -B 1 -A 1Repository: juspay/neurolink
Length of output: 830
🏁 Script executed:
# Let me verify the complete flow one more time by examining how params flows through
# Look for any tests that specifically test validationMiddleware behavior
rg -n "test.*validationMiddleware\|it.*validationMiddleware" test/ --type=ts -A 10Repository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Check how ToolIntegrationManager is instantiated and used with middlewares
rg -n "ToolIntegrationManager\|\.use\(" src/lib --type=ts -B 2 -A 2 | head -50Repository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Verify the exact behavior by examining how the middleware chain passes params
sed -n '225,245p' src/lib/mcp/toolIntegration.tsRepository: juspay/neurolink
Length of output: 569
🏁 Script executed:
# Let me trace the exact object reference behavior by looking at a real scenario
# Create a simple test to understand the parameter passing
cat > /tmp/test_params.ts << 'EOF'
// Simulate the middleware chain behavior
type Middleware = (params: unknown, next: () => void) => void;
function createChain(middlewares: Middleware[]) {
return (params: unknown) => {
let index = 0;
const executeNext = () => {
if (index >= middlewares.length) {
console.log("Final params:", params);
return;
}
const middleware = middlewares[index++];
middleware(params, executeNext);
};
return executeNext();
};
}
// Simulate validationMiddleware behavior
const validationMiddleware: Middleware = (params, next) => {
const paramObj = (params ?? {}) as Record<string, unknown>;
paramObj.elicited = "value";
console.log("In middleware, paramObj:", paramObj);
console.log("Original params:", params);
return next();
};
const chain = createChain([validationMiddleware]);
// Test 1: with null params
console.log("=== Test with null ===");
chain(null);
// Test 2: with object params
console.log("\n=== Test with object ===");
chain({});
EOF
node /tmp/test_params.tsRepository: juspay/neurolink
Length of output: 318
🏁 Script executed:
# Get complete context around the middleware chain and executeTool
sed -n '495,510p' src/lib/mcp/toolIntegration.tsRepository: juspay/neurolink
Length of output: 426
🏁 Script executed:
# Check if there's any code that depends on params being null/undefined after executeTool call
rg -n "executeTool" src/lib --type=ts -A 5 | grep -E "(params|result)" | head -20Repository: juspay/neurolink
Length of output: 1633
🏁 Script executed:
# Verify that the suggested fix handles all code paths correctly
sed -n '468,510p' src/lib/mcp/toolIntegration.tsRepository: juspay/neurolink
Length of output: 1172
🏁 Script executed:
# Check if there are any type implications of the fix
rg -n "params.*unknown\|unknown.*params" src/lib/mcp/toolIntegration.ts | head -10Repository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Verify all call sites pass params correctly
rg -n "chain\(tool, params" src/lib/mcp/toolIntegration.tsRepository: juspay/neurolink
Length of output: 117
Elicited parameters are lost when params is null/undefined.
validationMiddleware merges elicited values into a local paramObj, but when callers pass null/undefined, this local object is not the one forwarded through the middleware chain. The original params value persists throughout, causing elicited values to be silently discarded. Normalize params to a mutable object in executeTool before passing to the middleware chain and tool execution.
Suggested fix
async executeTool(
toolName: string,
params: unknown,
context?: NeuroLinkExecutionContext,
): Promise<ToolResult | unknown> {
const tool = this.wrappedTools.get(toolName);
if (!tool) {
throw new Error(`Tool '${toolName}' not registered`);
}
+ const normalizedParams =
+ params && typeof params === "object" ? params : {};
const config = context?.config as Record<string, unknown> | undefined;
const serverId = config?.serverId as string | undefined;
// ... elicitation context setup ...
// Create middleware chain
if (this.middlewares.length === 0) {
- return tool.execute(params, enhancedContext);
+ return tool.execute(normalizedParams, enhancedContext);
}
const chain = createToolMiddlewareChain(this.middlewares);
- return chain(tool, params, enhancedContext, () =>
- tool.execute(params, enhancedContext),
+ return chain(tool, normalizedParams, enhancedContext, () =>
+ tool.execute(normalizedParams, enhancedContext),
);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/mcp/toolIntegration.ts` around lines 367 - 419, The middleware
currently merges elicited values into a local paramObj inside
validationMiddleware but callers pass null/undefined so those updates are never
forwarded; fix this by normalizing params to a mutable object in executeTool
before invoking the middleware chain and the tool handler (e.g., set params =
(params ?? {}) as Record<string, unknown> or create a shallow clone) so the same
mutable object is passed into validationMiddleware and then into the tool
execution; ensure executeTool passes that normalized object to next()/invoke so
elicited fields added to paramObj are preserved and delivered to the tool.
| getElicitationManager() { | ||
| // Dynamically import to avoid circular dependencies | ||
| const { globalElicitationManager } = require("./mcp/elicitation/index.js"); | ||
| return globalElicitationManager; | ||
| } | ||
|
|
||
| /** | ||
| * Register an elicitation handler for interactive tool input | ||
| * Handlers are called when tools need user input during execution | ||
| * @param handler - Function to handle elicitation requests | ||
| * @example | ||
| * ```typescript | ||
| * neurolink.registerElicitationHandler(async (request) => { | ||
| * switch (request.type) { | ||
| * case 'confirmation': | ||
| * return { confirmed: await confirmWithUser(request.message) }; | ||
| * case 'text': | ||
| * return { value: await promptUser(request.message) }; | ||
| * case 'select': | ||
| * return { value: await selectFromOptions(request.options) }; | ||
| * } | ||
| * }); | ||
| * ``` | ||
| */ | ||
| registerElicitationHandler( | ||
| handler: (request: unknown) => Promise<unknown>, | ||
| ): void { | ||
| const elicitationManager = this.getElicitationManager(); | ||
| elicitationManager.registerHandler(handler); | ||
| } | ||
|
|
||
| /** | ||
| * Get the multi-server manager for load balancing and coordination | ||
| * Allows managing multiple MCP servers with failover and load balancing | ||
| * @returns The global MultiServerManager instance | ||
| * @example | ||
| * ```typescript | ||
| * const multiServer = neurolink.getMultiServerManager(); | ||
| * | ||
| * // Create a server group with load balancing | ||
| * await multiServer.createServerGroup('ai-tools', { | ||
| * servers: ['openai-server', 'anthropic-server'], | ||
| * strategy: 'round-robin' | ||
| * }); | ||
| * ``` | ||
| */ | ||
| getMultiServerManager() { | ||
| const { globalMultiServerManager } = require("./mcp/multiServerManager.js"); | ||
| return globalMultiServerManager; | ||
| } | ||
|
|
||
| /** | ||
| * Get the enhanced tool discovery service | ||
| * Provides advanced search, filtering, and compatibility checking for tools | ||
| * @returns EnhancedToolDiscovery instance | ||
| * @example | ||
| * ```typescript | ||
| * const discovery = neurolink.getEnhancedToolDiscovery(); | ||
| * | ||
| * // Search for tools by criteria | ||
| * const results = await discovery.searchTools({ | ||
| * category: 'data-processing', | ||
| * capabilities: ['streaming', 'batch'], | ||
| * minReliability: 0.9 | ||
| * }); | ||
| * ``` | ||
| */ | ||
| getEnhancedToolDiscovery() { | ||
| const { EnhancedToolDiscovery } = require("./mcp/enhancedToolDiscovery.js"); | ||
| return new EnhancedToolDiscovery(this.toolRegistry); | ||
| } | ||
|
|
||
| /** | ||
| * Get the MCP registry client for discovering servers from registries | ||
| * Supports multiple registry sources (official, community, custom) | ||
| * @returns The global MCPRegistryClient instance | ||
| * @example | ||
| * ```typescript | ||
| * const registryClient = neurolink.getMCPRegistryClient(); | ||
| * | ||
| * // Search for servers | ||
| * const servers = await registryClient.searchServers({ | ||
| * query: 'database', | ||
| * categories: ['data', 'storage'] | ||
| * }); | ||
| * | ||
| * // Get a well-known server config | ||
| * const githubServer = registryClient.getWellKnownServer('github'); | ||
| * ``` | ||
| */ | ||
| getMCPRegistryClient() { | ||
| const { globalMCPRegistryClient } = require("./mcp/mcpRegistryClient.js"); | ||
| return globalMCPRegistryClient; | ||
| } | ||
|
|
||
| /** | ||
| * Expose a NeuroLink agent as an MCP tool | ||
| * This allows agents to be called by other systems via MCP | ||
| * @param agent - The agent to expose (must include id, name, description, and execute) | ||
| * @param options - Exposure configuration options (prefix, defaultAnnotations, etc.) | ||
| * @returns The exposed tool definition | ||
| * @example | ||
| * ```typescript | ||
| * const agent = { | ||
| * id: 'my-agent', | ||
| * name: 'My Agent', | ||
| * description: 'An agent that processes data', | ||
| * execute: async (params) => { ... } | ||
| * }; | ||
| * const tool = await neurolink.exposeAgentAsTool(agent, { | ||
| * prefix: 'agent_' | ||
| * }); | ||
| * ``` | ||
| */ | ||
| async exposeAgentAsTool( | ||
| agent: { | ||
| id: string; | ||
| name: string; | ||
| description: string; | ||
| execute: (params: unknown, context?: unknown) => Promise<unknown>; | ||
| }, | ||
| options?: { | ||
| prefix?: string; | ||
| includeMetadataInDescription?: boolean; | ||
| wrapWithContext?: boolean; | ||
| executionTimeout?: number; | ||
| enableLogging?: boolean; | ||
| }, | ||
| ) { | ||
| const agentExposure = await import("./mcp/agentExposure.js"); | ||
| return agentExposure.exposeAgentAsTool(agent, options); | ||
| } | ||
|
|
||
| /** | ||
| * Expose a workflow as an MCP tool | ||
| * This allows workflows to be called by other systems via MCP | ||
| * @param workflow - The workflow to expose (must include id, name, description, and execute) | ||
| * @param options - Exposure configuration options (prefix, defaultAnnotations, etc.) | ||
| * @returns The exposed tool definition | ||
| * @example | ||
| * ```typescript | ||
| * const workflow = { | ||
| * id: 'data-pipeline', | ||
| * name: 'Data Pipeline', | ||
| * description: 'Runs the data processing pipeline', | ||
| * execute: async (params) => { ... } | ||
| * }; | ||
| * const tool = await neurolink.exposeWorkflowAsTool(workflow, { | ||
| * prefix: 'workflow_' | ||
| * }); | ||
| * ``` | ||
| */ | ||
| async exposeWorkflowAsTool( | ||
| workflow: { | ||
| id: string; | ||
| name: string; | ||
| description: string; | ||
| execute: (params: unknown, context?: unknown) => Promise<unknown>; | ||
| steps?: Array<{ id: string; name: string; description?: string }>; | ||
| }, | ||
| options?: { | ||
| prefix?: string; | ||
| includeMetadataInDescription?: boolean; | ||
| wrapWithContext?: boolean; | ||
| executionTimeout?: number; | ||
| enableLogging?: boolean; | ||
| }, | ||
| ) { | ||
| const agentExposure = await import("./mcp/agentExposure.js"); | ||
| return agentExposure.exposeWorkflowAsTool(workflow, options); | ||
| } | ||
|
|
||
| /** | ||
| * Get the tool integration manager for middleware and elicitation | ||
| * Provides advanced tool wrapping with confirmation, timeout, retry, etc. | ||
| * @returns The global ToolIntegrationManager instance | ||
| * @example | ||
| * ```typescript | ||
| * const integration = neurolink.getToolIntegrationManager(); | ||
| * | ||
| * // Register a tool with middleware | ||
| * integration.registerTool(myTool, { | ||
| * timeout: 30000, | ||
| * retries: 3, | ||
| * requireConfirmation: true | ||
| * }); | ||
| * ``` | ||
| */ | ||
| getToolIntegrationManager() { | ||
| const { | ||
| globalToolIntegrationManager, | ||
| } = require("./mcp/toolIntegration.js"); | ||
| return globalToolIntegrationManager; | ||
| } | ||
|
|
||
| /** | ||
| * Convert NeuroLink tools to MCP format | ||
| * Useful for exposing local tools to external MCP clients | ||
| * @param tools - Array of NeuroLink tool definitions | ||
| * @param options - Conversion options | ||
| * @returns Array of MCP-formatted tools | ||
| * @example | ||
| * ```typescript | ||
| * const mcpTools = neurolink.convertToolsToMCPFormat([ | ||
| * { name: 'myTool', description: 'Does something', execute: async () => {} } | ||
| * ]); | ||
| * ``` | ||
| */ | ||
| convertToolsToMCPFormat( | ||
| tools: Array<{ | ||
| name: string; | ||
| description: string; | ||
| execute?: (params: unknown) => unknown; | ||
| }>, | ||
| options: { namespacePrefix?: string } = {}, | ||
| ) { | ||
| const { batchConvertToMCP } = require("./mcp/toolConverter.js"); | ||
| return batchConvertToMCP(tools, options); | ||
| } | ||
|
|
||
| /** | ||
| * Convert MCP tools to NeuroLink format | ||
| * Useful for importing tools from external MCP servers | ||
| * @param tools - Array of MCP tool definitions | ||
| * @param options - Conversion options | ||
| * @returns Array of NeuroLink-formatted tools | ||
| * @example | ||
| * ```typescript | ||
| * const neurolinkTools = neurolink.convertToolsFromMCPFormat(externalTools, { | ||
| * removeNamespacePrefix: 'external_' | ||
| * }); | ||
| * ``` | ||
| */ | ||
| convertToolsFromMCPFormat( | ||
| tools: Array<{ name: string; description: string; inputSchema?: unknown }>, | ||
| options: { removeNamespacePrefix?: string } = {}, | ||
| ) { | ||
| const { batchConvertToNeuroLink } = require("./mcp/toolConverter.js"); | ||
| return batchConvertToNeuroLink(tools, options); | ||
| } | ||
|
|
||
| /** | ||
| * Get tool annotations and safety information | ||
| * Provides insights about tool behavior, safety levels, and retry-ability | ||
| * @param toolName - Name of the tool to analyze | ||
| * @returns Tool annotation summary | ||
| * @example | ||
| * ```typescript | ||
| * const annotations = await neurolink.getToolAnnotations('deleteFile'); | ||
| * // Returns: { destructive: true, requiresConfirmation: true, safeToRetry: false } | ||
| * ``` | ||
| */ | ||
| async getToolAnnotations(toolName: string) { | ||
| const { inferAnnotations, getAnnotationSummary } = await import( | ||
| "./mcp/toolAnnotations.js" | ||
| ); | ||
| const toolInfo = this.toolRegistry.getToolInfo(toolName); | ||
| if (!toolInfo) { | ||
| return null; | ||
| } | ||
| // Infer annotations from the tool info, then get the summary | ||
| const annotations = inferAnnotations({ | ||
| name: toolInfo.tool.name, | ||
| description: toolInfo.tool.description ?? "", | ||
| }); | ||
| return { | ||
| annotations, | ||
| summary: getAnnotationSummary(annotations), | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for ESM configuration
echo "=== Checking package.json ==="
cat package.json | head -20
echo -e "\n=== Checking tsconfig.json ==="
fd -t f 'tsconfig.*\.json' -x cat {} \; 2>/dev/null | head -50Repository: juspay/neurolink
Length of output: 2330
🏁 Script executed:
#!/bin/bash
# Check for "type" field in package.json more thoroughly
echo "=== Checking for ESM type in package.json ==="
grep -i '"type"' package.json || echo "No 'type' field found"
# Also check if there's any conditional exports or module field
echo -e "\n=== Checking for module/exports fields ==="
grep -E '"(module|exports|main)"' package.json | head -10
# Now let's look at the actual implementation in neurolink.ts around the flagged lines
echo -e "\n=== Checking actual implementation in neurolink.ts ==="
wc -l src/lib/neurolink.ts
# Let's see around the getElicitationManager method
echo -e "\n=== Checking getElicitationManager and nearby methods ==="
rg -n "getElicitationManager|getMultiServerManager|convertToolsToMCPFormat" src/lib/neurolink.ts -A 2 | head -30Repository: juspay/neurolink
Length of output: 1414
🏁 Script executed:
#!/bin/bash
# Let's examine all require() calls in the flagged range (lines 6916-7184)
echo "=== All require() calls in flagged range ==="
sed -n '6916,7184p' src/lib/neurolink.ts | grep -n 'require(' | head -20
# Also check if import() is used elsewhere for comparison
echo -e "\n=== Methods using import() for comparison ==="
sed -n '6916,7184p' src/lib/neurolink.ts | grep -n 'import(' | head -10
# Check the very start of the file to see what style is used
echo -e "\n=== Import style at top of file ==="
head -50 src/lib/neurolink.ts | grep -E '^import |^export |require\('Repository: juspay/neurolink
Length of output: 1985
🏁 Script executed:
#!/bin/bash
# Check if createRequire is imported or defined anywhere in the file
echo "=== Checking for createRequire definition ==="
grep -n 'createRequire\|require.*=' src/lib/neurolink.ts | head -20
# Check the very beginning of the file more comprehensively
echo -e "\n=== First 100 lines of neurolink.ts for any require setup ==="
head -100 src/lib/neurolink.ts | grep -E 'import|require|createRequire'
# Check if there are any CommonJS patterns
echo -e "\n=== Check for Node.js module utility imports ==="
grep -n 'from.*module' src/lib/neurolink.ts
# Also verify the actual structure of the mcp files being required
echo -e "\n=== Check what's exported from the required files ==="
head -30 src/lib/mcp/elicitation/index.js 2>/dev/null || head -30 src/lib/mcp/elicitation/index.ts 2>/dev/null || echo "File not found or different format"Repository: juspay/neurolink
Length of output: 2474
🏁 Script executed:
#!/bin/bash
# Let's verify that require() will actually fail by checking if it's available in ESM context
echo "=== Check for any polyfills or workarounds for require ==="
grep -rn "global\.require\|globalThis\.require\|require\s*=" src/lib/neurolink.ts || echo "No require polyfills found"
# Let's see the exact context around one of the require() calls to understand the intent
echo -e "\n=== Full context of getElicitationManager method ==="
sed -n '6916,6926p' src/lib/neurolink.ts
# Compare with a method that uses import()
echo -e "\n=== Full context of exposeAgentAsTool (using import) ==="
sed -n '7045,7055p' src/lib/neurolink.ts
# Check the build output to see if there's any transpilation happening
echo -e "\n=== Check if there's any build process that handles require ==="
ls -la build.* Makefile webpack.* rollup.* 2>/dev/null || echo "No obvious build config found"
# Check if there's any runtime that provides require in ESM
echo -e "\n=== Check Node.js version compatibility ==="
grep -i 'engines\|node' package.json | head -5Repository: juspay/neurolink
Length of output: 1481
🏁 Script executed:
#!/bin/bash
# Check if the code is even being tested or if there's a build step that might transpile
echo "=== Check build/dist output structure ==="
ls -la dist/ 2>/dev/null | head -20 || echo "dist/ not found"
# Let's verify the TypeScript strictness settings
echo -e "\n=== Full tsconfig for lib compilation ==="
cat tsconfig.json | jq '.compilerOptions | {strict, noImplicitAny, module, moduleResolution, skipLibCheck}' 2>/dev/null || cat tsconfig.json | grep -E 'strict|noImplicitAny|module|skipLibCheck'
# Check if there are any tests for these methods
echo -e "\n=== Check for tests of getElicitationManager ==="
grep -r "getElicitationManager\|getMultiServerManager" test/ spec/ 2>/dev/null | head -5 || echo "No tests found or directories don't exist"
# Final confirmation: run TypeScript compiler in strict mode on just the flagged file
echo -e "\n=== TypeScript version and strict checking ==="
grep -i '"typescript"' package.jsonRepository: juspay/neurolink
Length of output: 334
🏁 Script executed:
#!/bin/bash
# Let's check the exact return types of the methods to understand if they can be async
echo "=== Return type analysis for require() methods ==="
sed -n '6916,6926p' src/lib/neurolink.ts | head -5
sed -n '6962,6966p' src/lib/neurolink.ts | head -3
sed -n '6974,6985p' src/lib/neurolink.ts | head -5
# Check methods using async import() to compare signatures
echo -e "\n=== Return type analysis for async import() methods ==="
sed -n '7045,7050p' src/lib/neurolink.ts | head -4
sed -n '7069,7074p' src/lib/neurolink.ts | head -4
# Final confirmation: does the TypeScript compiler actually accept bare require()?
echo -e "\n=== Check if require is available globally ==="
grep -n 'declare.*require\|types.*node' tsconfig.json package.json 2>/dev/nullRepository: juspay/neurolink
Length of output: 1091
🏁 Script executed:
#!/bin/bash
# Final confirmation: verify that without createRequire, require() will indeed fail
echo "=== Confirm: require() is undefined in ESM without createRequire ==="
node --input-type=module --eval "try { require('./test'); } catch(e) { console.log('Error:', e.message); }"
# Also check if dist/ compilation succeeds without fixing this
echo -e "\n=== Check if there's a dist build already ==="
if [ -d dist ]; then
echo "dist/ exists - checking compiled output"
grep -r "require(" dist/lib/neurolink.js 2>/dev/null | head -3 || echo "No require found in dist (may be transpiled)"
else
echo "No dist/ directory - code hasn't been built"
fiRepository: juspay/neurolink
Length of output: 249
Critical: require() is undefined in ESM and will cause runtime failure.
The codebase is configured as an ES module ("type": "module" in package.json, "module": "NodeNext" in tsconfig), but seven public SDK methods (lines 6918, 6963, 6975, 6990, 7133, 7154, 7175) use bare require() without setup. In ESM, require is undefined at runtime and will throw ReferenceError: require is not defined when these methods are called. Additionally, require() returns any, violating the strict TypeScript requirement ("No implicit any types allowed").
Use createRequire with type assertions to fix, or convert to async import() like the neighboring exposeAgentAsTool method (line 7047) already does. The latter is preferred for consistency within this file and avoids synchronous imports in an async context.
🔧 Recommended fix using createRequire
+import { createRequire } from "module";
+const require = createRequire(import.meta.url);
getElicitationManager() {
// Dynamically import to avoid circular dependencies
- const { globalElicitationManager } = require("./mcp/elicitation/index.js");
+ const { globalElicitationManager } = require("./mcp/elicitation/index.js") as typeof import("./mcp/elicitation/index.js");
return globalElicitationManager;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/neurolink.ts` around lines 6916 - 7184, Several public methods
(getElicitationManager, getMultiServerManager, getEnhancedToolDiscovery,
getMCPRegistryClient, getToolIntegrationManager, convertToolsToMCPFormat,
convertToolsFromMCPFormat) currently use require() which is undefined in ESM;
convert them to use dynamic import() like exposeAgentAsTool: make the methods
async, await import("./mcp/…"), then return the imported symbol (for singletons
return the named global object, for classes instantiate as before, and for
converters await the module then call
batchConvertToMCP/batchConvertToNeuroLink). Ensure signatures are updated to
async and any callers handle Promises, and keep the same runtime behavior (e.g.,
new EnhancedToolDiscovery(this.toolRegistry) after import).
✅ Actions performedFull review triggered. |
cdf2f3b to
5606713
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
🤖 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 |
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/demos/videos.md (1)
537-564:⚠️ Potential issue | 🟡 MinorVideo links point to the "legacy" location that this section recommends migrating away from.
The directory structure section (lines 537–564) labels
docs/visual-content/videos/as "Legacy video location" with a note to migrate todocs/demos/videos/. However, all the video links added in this file point to../visual-content/.... This creates a contradiction within the same document.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/demos/videos.md` around lines 537 - 564, The document shows docs/visual-content/videos/ as a legacy path but the actual video links still point to ../visual-content/..., so update the links to the canonical docs/demos/videos/ path (or conversely change the legacy note) to remove the contradiction; search for and replace occurrences of "../visual-content/" in videos.md with "../demos/videos/" (or update the "Legacy video location" label and migration note if you prefer keeping the old links) and verify all link targets and directory references (docs/demos/videos/ and docs/visual-content/videos/) are consistent throughout the file.src/lib/mcp/index.ts (1)
317-327:⚠️ Potential issue | 🟡 Minor
executeMCPerror message contradicts this PR's purpose.This function throws
"MCP execution not available - ecosystem removed", yet this PR is adding 14 new MCP ecosystem modules. If this stub is intentionally kept for a legacy API surface, the message is misleading to consumers. If it's stale, it should be updated or wired to the new ecosystem.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/index.ts` around lines 317 - 327, The throw in executeMCP currently states "MCP execution not available - ecosystem removed", which contradicts this PR that adds MCP ecosystem modules; either update the error to a clear deprecation/unsupported message or wire executeMCP to the new ecosystem. If you intend to keep this as a legacy stub, replace the misleading text with a concise unsupported/deprecated message (e.g., "executeMCP is unsupported in this build; use the new MCP ecosystem modules") and include guidance or a link/identifier for the new API; otherwise implement executeMCP to delegate to the new MCP registry/dispatcher you added (call the new ecosystem handler/registry and forward _name, _config, _args, _context and return its result) so consumers can invoke the new modules.
🟡 Minor comments (17)
test/mcp/mcpCircuitBreaker.test.ts-1-11 (1)
1-11:⚠️ Potential issue | 🟡 MinorTest file location doesn't match the project's testing convention.
The coding guidelines specify
test/suites/for feature-specific (unit) tests. This file sits undertest/mcp/, which is the same location used by the other newly-added MCP test files (factory.test.ts,cli-mcp-commands.test.ts, etc.), but that existing pattern itself deviates from the guideline.As per coding guidelines: "Create test suites in test/suites/ for feature-specific tests, test/integration/ for real provider integration tests."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mcp/mcpCircuitBreaker.test.ts` around lines 1 - 11, The test file is placed outside the project's testing convention; move this MCP unit test into the test/suites/ directory and update any import/require paths so imports of MCPCircuitBreaker and CircuitBreakerManager still resolve, ensuring the test file name remains descriptive (e.g., mcpCircuitBreaker.test.ts) and references the same exported symbols (MCPCircuitBreaker, CircuitBreakerManager) from mcpCircuitBreaker.js; also update any test-runner or CI patterns if they explicitly list test/mcp/ so the new location is picked up, and ensure other MCP tests follow the same test/suites/ convention for consistency.src/lib/utils/schemaConversion.ts-155-158 (1)
155-158:⚠️ Potential issue | 🟡 Minor
typeof null === "object"—schema.jsonSchemais not guarded againstnull.Since the function casts to
anyat line 148 andtypeof null === "object"istruein JavaScript, a caller passing{ jsonSchema: null }would slip through the guard, makingextractedequal tonull, andensureTypeField(null)would throw aTypeErrorwhen accessing.type.🛡️ Proposed fix
- if ("jsonSchema" in schema && typeof schema.jsonSchema === "object") { + if ("jsonSchema" in schema && schema.jsonSchema !== null && typeof schema.jsonSchema === "object") { const extracted = schema.jsonSchema as Record<string, unknown>; return ensureTypeField(extracted); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/utils/schemaConversion.ts` around lines 155 - 158, The current guard lets null pass because typeof null === "object"; update the check around schema.jsonSchema in the conversion logic so you only call ensureTypeField when schema.jsonSchema is a non-null object (e.g., schema.jsonSchema !== null && typeof schema.jsonSchema === "object"), otherwise skip or handle the null case appropriately; ensure the code path that calls ensureTypeField(extracted) only executes with a valid object to avoid a TypeError.docs/demos/videos.md-129-129 (1)
129-129:⚠️ Potential issue | 🟡 MinorMultiple sections link to the same video file with different claimed durations.
For example,
business-use-cases.mp4is linked at lines 129, 151, 208, 324, and 364 with durations ranging from 3:20 to 45:00. Similarly,developer-tools.mp4appears at lines 177, 222, 234, and 298 with durations from 4:45 to 6:00. If these are truly the same file, the different duration claims will confuse readers. Consider either using distinct video files per section or clarifying that these are timestamp references within the same video.Also applies to: 151-151, 208-208, 324-324, 364-364
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/demos/videos.md` at line 129, The doc links reuse the same video files but list different durations, which is confusing; locate all occurrences of business-use-cases.mp4 and developer-tools.mp4 in videos.md and either replace them with the correct distinct video filenames for each section or annotate the links with explicit timestamp ranges (e.g., start–end) and a single authoritative duration for the full file; update the displayed durations to match the actual file length or the annotated timestamp range so every section's link and duration are consistent (search for "business-use-cases.mp4" and "developer-tools.mp4" to find each occurrence).src/lib/mcp/caching/toolCache.ts-271-284 (1)
271-284:⚠️ Potential issue | 🟡 Minor
getOrSetcannot distinguish a cachedundefinedvalue from a cache miss.If the factory legitimately returns
undefined(whenTincludesundefined), the value will be cached but never returned on subsequent lookups — the factory will be re-invoked every time. This is a known limitation of the!== undefinedsentinel pattern. Consider documenting this constraint or using aSymbolsentinel for "not found."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/caching/toolCache.ts` around lines 271 - 284, getOrSet uses existing !== undefined to detect cache hits so a legitimately cached undefined value is treated as a miss; change detection to distinguish "no entry" from "value is undefined" by using a sentinel or presence check: update getOrSet (and related get/set logic) to use either a unique Symbol sentinel stored internally or a separate hasEntry/map that tracks keys (or wrap stored values in { found: true, value }) so that getOrSet can call get and reliably detect presence vs absence and return cached undefineds instead of re-invoking factory; reference getOrSet, get, and set to locate the change.src/lib/mcp/routing/toolRouter.ts-88-92 (1)
88-92:⚠️ Potential issue | 🟡 Minor
healthCheckIntervalis configured but never consumed.The config field
healthCheckInterval(default 30000) is accepted and stored but no timer or mechanism inToolRouteruses it. This is dead configuration that misleads consumers into thinking health checks are automatic.Either implement periodic health checking/affinity cleanup using this interval, or remove the field from the config interface to avoid confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/routing/toolRouter.ts` around lines 88 - 92, The config exposes healthCheckInterval but ToolRouter never uses it; either remove the field from the config interface or implement periodic health-check/affinity cleanup that respects it. To fix, in the ToolRouter class (e.g., constructor or start/init method) create a repeating timer using the configured healthCheckInterval (default 30000) that calls the existing health check and affinity-cleanup logic (invoke the methods that validate tool liveness and prune stale affinities), store the timer id on the instance, and clear it on shutdown/dispose to avoid leaks; alternatively, if you prefer not to add runtime behavior, delete the healthCheckInterval property from the config/interface and any related defaults so the API doesn't promise automatic checks.src/lib/mcp/serverCapabilities.ts-797-802 (1)
797-802:⚠️ Potential issue | 🟡 MinorRegex injection in
createPrompttemplate substitution via unescapedkey.
Object.entries(args)iterates keys from user-supplied arguments. If a key contains regex metacharacters (e.g.,$,(,+), thenew RegExp(...)on line 801 will either throw or match unintended patterns. Escape the key before interpolation:Proposed fix
for (const [key, value] of Object.entries(args)) { - text = text.replace(new RegExp(`\\{${key}\\}`, "g"), String(value)); + const escapedKey = key.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + text = text.replace(new RegExp(`\\{${escapedKey}\\}`, "g"), String(value)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/serverCapabilities.ts` around lines 797 - 802, In the generator function used by createPrompt, template substitution builds a RegExp from unescaped keys (args) which allows regex injection or errors; fix by escaping regex metacharacters in each key before constructing new RegExp (or replace using a literal-safe method such as splitting/joining on the substring `{${key}}`), ensuring you update the generator (args) loop that processes template and value substitution to use the escaped key or a non-regex string replace approach so keys with characters like $, (, +, etc. no longer break or behave unexpectedly.src/lib/mcp/mcpServerBase.ts-247-273 (1)
247-273:⚠️ Potential issue | 🟡 Minor
validateTooluses rawthrow new Error(...)instead ofErrorFactory.Per coding guidelines, use
ErrorFactoryfor creating typed errors. However,ErrorFactorycurrently lacks specific factory methods for tool validation errors (name validation, length limits, format validation, description requirement, etc.). Consider either:
- Adding new factory methods to
ErrorFactoryfor these validation scenarios, or- Creating a tool-specific error factory in the MCP module
Note:
executeTooldoes not follow the same pattern—it returnsToolResultobjects with error information rather than throwing errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/mcpServerBase.ts` around lines 247 - 273, The validateTool function currently throws raw Error instances; replace those throws with typed errors from a factory: either extend the global ErrorFactory with new methods like createToolNameError, createToolNameLengthError, createToolNameFormatError, createToolDescriptionError, and createToolExecuteError and use them inside validateTool, or add a local MCP-specific ToolErrorFactory with equivalent methods and call those from validateTool; ensure the error messages and types match existing conventions and mirror how other modules (e.g., executeTool which returns ToolResult errors) represent tool errors so consumers get consistent typed errors.test/mcp/integration.test.ts-1-6 (1)
1-6:⚠️ Potential issue | 🟡 MinorMove MCP integration tests under
test/integration/.This file sits in
test/mcp/, but integration tests should live undertest/integration/(e.g.,test/integration/mcp/integration.test.ts) to follow repo conventions. As per coding guidelines: "Create test suites in test/suites/ for feature-specific tests, test/integration/ for real provider integration tests. Use Vitest as test runner."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mcp/integration.test.ts` around lines 1 - 6, The integration test file is placed under test/mcp/ but must be moved to the integration tests folder; relocate the file test/mcp/integration.test.ts to test/integration/mcp/integration.test.ts, update any relative imports or test-runner paths that reference this file, and ensure the Vitest configuration (if it contains explicit test include globs) and any CI/test scripts include the new test path; keep the existing test contents (describe/it blocks) unchanged but ensure any module import paths within the file remain correct after the move.test/mcp/factory.test.ts-1-5 (1)
1-5:⚠️ Potential issue | 🟡 MinorRelocate factory tests to
test/suites/.These are feature/unit-style tests and should live under
test/suites/(e.g.,test/suites/mcp/factory.test.ts) per repo conventions. As per coding guidelines: "Create test suites in test/suites/ for feature-specific tests, test/integration/ for real provider integration tests. Use Vitest as test runner."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mcp/factory.test.ts` around lines 1 - 5, Move the feature/unit tests in test/mcp/factory.test.ts into the test/suites/ folder to follow repo conventions; create test/suites/mcp/factory.test.ts and relocate the entire test contents (including any imports referencing createMCPServer, createMCPServer factory tests, and related helpers) so imports paths remain valid or update them accordingly, ensuring Vitest remains the runner and any suite-level setup/teardown (e.g., mocks for createMCPServer) are preserved in the moved file.test/mcp/cli-mcp-commands.test.ts-1-7 (1)
1-7:⚠️ Potential issue | 🟡 MinorRelocate CLI MCP tests to
test/suites/.These are feature/unit-style tests and should live under
test/suites/(e.g.,test/suites/mcp/cli-mcp-commands.test.ts) per repo conventions. As per coding guidelines: "Create test suites in test/suites/ for feature-specific tests, test/integration/ for real provider integration tests. Use Vitest as test runner."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mcp/cli-mcp-commands.test.ts` around lines 1 - 7, Move the feature/unit tests from test/mcp/cli-mcp-commands.test.ts into the suites hierarchy (e.g., test/suites/mcp/cli-mcp-commands.test.ts) to follow repo conventions; after moving, update any relative imports or require paths inside the test (references to MCPCommandFactory and any mocked SDK helpers) so they resolve from the new location, and ensure the test runner (Vitest) picks up the new path (adjust test glob in config if necessary).test/mcp/integration/mcp-enhancements.integration.test.ts-1464-1471 (1)
1464-1471:⚠️ Potential issue | 🟡 MinorReduce timing-based flakiness in the token cache test.
A 10ms upper bound is brittle on slower CI runners. Consider loosening the threshold or asserting on behavior rather than elapsed time.
♻️ Suggested adjustment
- expect(duration).toBeLessThan(10); // Should be nearly instant + expect(duration).toBeLessThan(50); // Avoid CI timing flakes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mcp/integration/mcp-enhancements.integration.test.ts` around lines 1464 - 1471, The timing assertion in the test around storage.getTokens("server1") is brittle; replace the strict duration check with a more reliable assertion: either increase the threshold (e.g., to 200ms) or, preferably, assert cache behavior directly by spying/mocking the underlying fetch/load method used by storage (reference storage.getTokens and the underlying token fetcher) to ensure it was not invoked on the second call, and still assert retrieved?.accessToken === "cached_token". Update the test to use the mock/spy approach or a relaxed duration to avoid CI flakiness.test/mcp/integration/mcp-enhancements.integration.test.ts-1-14 (1)
1-14:⚠️ Potential issue | 🟡 MinorMove MCP enhancement integration tests under
test/integration/.This is an integration suite and should be placed under
test/integration/(e.g.,test/integration/mcp/mcp-enhancements.integration.test.ts) to match repo conventions. As per coding guidelines: "Create test suites in test/suites/ for feature-specific tests, test/integration/ for real provider integration tests. Use Vitest as test runner."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mcp/integration/mcp-enhancements.integration.test.ts` around lines 1 - 14, The integration test file "mcp-enhancements.integration.test.ts" (the "MCP Enhancements Integration Tests" suite) belongs under the repository's integration tests folder, so move that file from its current location into the integration test directory used for real-provider tests, then update any references: fix relative imports inside the test, update test-globs/configs (Vitest/vite/tsconfig includes or package.json test scripts) so the moved file is discovered, and ensure any CI/test workflows point to the new location.test/continuous-test-suite-mcp.ts-16-19 (1)
16-19:⚠️ Potential issue | 🟡 MinorLazy-load
NeuroLinkto allow running whendist/is missing.The top-level
../dist/index.jsimport will throw before your “skip when build missing” guard, so Part 1 tests won’t run without a build. Consider deferring the import until afterhasBuildis computed.🔧 Suggested fix (lazy import)
-import { NeuroLink } from "../dist/index.js"; +let NeuroLink: typeof import("../dist/index.js").NeuroLink | undefined; + +async function loadSDK(): Promise<typeof import("../dist/index.js").NeuroLink> { + if (!NeuroLink) { + ({ NeuroLink } = await import("../dist/index.js")); + } + return NeuroLink; +}- sdk = new NeuroLink(); + const SDK = await loadSDK(); + sdk = new SDK();Also applies to: 969-977
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/continuous-test-suite-mcp.ts` around lines 16 - 19, The top-level static import of NeuroLink causes a crash before the existing hasBuild guard runs; remove the static import of "../dist/index.js" and instead perform a lazy/dynamic import after computing hasBuild (e.g., after variable hasBuild is set) so that you only import when the build exists; specifically replace the top-level "import { NeuroLink } from '../dist/index.js'" with a late import using dynamic import() or require inside the branch that checks hasBuild (or assign NeuroLink from the resolved module before running Part 1 tests); apply the same change for the other occurrence around lines 969-977.src/cli/commands/mcp.ts-1626-1633 (1)
1626-1633:⚠️ Potential issue | 🟡 MinorAvoid non-null assertions flagged by static analysis.
Line 1632 and Line 2649 rely on
!even though the map access can be safely handled without it. This is flagged as forbidden.✅ Suggested fix
- toolsByServer.get(key)!.push(tool); + const list = toolsByServer.get(key); + if (list) { + list.push(tool); + } ... - byCategory.get(category)!.push(entry); + const list = byCategory.get(category); + if (list) { + list.push(entry); + }Also applies to: 2643-2650
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/mcp.ts` around lines 1626 - 1633, The code uses a non-null assertion when pushing into the Map (toolsByServer.get(key)!), which static analysis forbids; update the push logic to avoid `!` by retrieving the array with a safe fallback (e.g. const arr = toolsByServer.get(key) ?? []; arr.push(tool); toolsByServer.set(key, arr)) or by checking has() and using a local variable for the retrieved array before mutating it; apply the same pattern for the other occurrence that uses `!` (the block manipulating toolsByServer further down).src/lib/mcp/toolIntegration.ts-125-135 (1)
125-135:⚠️ Potential issue | 🟡 Minor
elicitationTimeoutis currently unused.The option is accepted but never applied to the default manager, so callers cannot change the default timeout.
✅ Suggested fix
- const { - elicitationManager = new ElicitationManager(), - autoConfirmDestructive = false, - elicitationTimeout = 60000, - enableLogging = true, - } = options; + const { + elicitationManager, + autoConfirmDestructive = false, + elicitationTimeout = 60000, + enableLogging = true, + } = options; + + const manager = + elicitationManager ?? + new ElicitationManager({ defaultTimeout: elicitationTimeout });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/toolIntegration.ts` around lines 125 - 135, The elicitationTimeout option is accepted but never applied to the default ElicitationManager; update the initialization so the provided elicitationTimeout is applied to elicitationManager (either by passing it into the ElicitationManager constructor or calling a setter like elicitationManager.setDefaultTimeout(elicitationTimeout) after construction) so callers can change the manager's default timeout; reference the variables elicitationTimeout and elicitationManager (or the ElicitationManager constructor/setDefaultTimeout method) and ensure this assignment occurs right after the manager is created.src/lib/mcp/enhancedToolDiscovery.ts-556-569 (1)
556-569:⚠️ Potential issue | 🟡 MinorVersion parsing doesn't guard against non-semver strings.
split(".").map(Number)silently producesNaNfor non-numeric segments (e.g."1.0.0-beta"), and comparisons withNaNare alwaysfalse, so version mismatches would go unreported.Proposed fix — add a guard
if (targetVersion && tool.version) { const toolVersion = tool.version.split(".").map(Number); const target = targetVersion.split(".").map(Number); + if (toolVersion.some(isNaN) || target.some(isNaN)) { + warnings.push( + `Non-standard version format: tool=${tool.version}, target=${targetVersion}`, + ); + } else if (toolVersion[0] !== target[0]) { - if (toolVersion[0] !== target[0]) { issues.push(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/enhancedToolDiscovery.ts` around lines 556 - 569, The version-parsing logic in enhancedToolDiscovery.ts uses tool.version and targetVersion with split(".").map(Number) which yields NaN for non-numeric segments (e.g. "1.0.0-beta") causing comparisons to silently fail; update the check in the block that defines toolVersion and target to validate parsed segments (e.g., ensure each mapped Number is finite and not NaN, and that toolVersion.length and target.length are sufficient) before doing numeric comparisons, and if validation fails push a warning/issue to warnings or issues respectively (mentioning the raw tool.version and targetVersion) so non-semver strings are handled instead of ignored; keep the existing Major/Minor comparison logic using the validated numeric arrays (toolVersion, target) and bail out early to avoid incorrect comparisons when parsing fails.src/lib/mcp/enhancedToolDiscovery.ts-446-466 (1)
446-466:⚠️ Potential issue | 🟡 Minor"moderate" classification is inconsistent with
getStatistics.
getToolsBySafetyLevel("moderate")(line 458-461) matches tools whereidempotentHint === trueregardless ofdestructiveHint/readOnlyHint, so a destructive-but-idempotent tool appears in both "dangerous" and "moderate" results. Meanwhile,getStatistics(lines 670-676) uses an exclusiveif / else if / elsechain, meaning the same tool would only land in "dangerous" there.Consider making the moderate filter exclusive to align with the statistics logic:
Proposed fix
case "moderate": return ( - (!annotations.destructiveHint && !annotations.readOnlyHint) || - annotations.idempotentHint === true + !annotations.destructiveHint && !annotations.readOnlyHint );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/enhancedToolDiscovery.ts` around lines 446 - 466, getToolsBySafetyLevel currently classifies a tool as "moderate" if idempotentHint === true regardless of destructive/readOnly flags, causing overlap with "dangerous"; change the "moderate" branch in getToolsBySafetyLevel so it is exclusive: require annotations.destructiveHint !== true and annotations.readOnlyHint !== true AND then include tools where annotations.idempotentHint === true or no safety hint is present (i.e., neither destructive nor readOnly nor idempotent set), matching the exclusive if/else-if/else behavior used in getStatistics; update the switch case for "moderate" to check the destructive/readOnly exclusion first and then the idempotent/neutral condition using annotations.idempotentHint, annotations.destructiveHint, and annotations.readOnlyHint.
🧹 Nitpick comments (19)
test/mcp/mcpCircuitBreaker.test.ts (2)
545-711:CircuitBreakerManagertests lack anafterEachsafety-net for cleanup.Each test in this block manually calls
manager.destroyAll()as the final statement. If an assertion throws before that call (e.g., theexpect(health.totalBreakers).toBe(2)assertion in the health summary test), the manager's internal breakers are never destroyed. In implementations that schedule internal timers (e.g.,resetTimeoutviasetTimeout), this leaks timer handles and can cause Vitest to warn about open handles or bleed state into subsequent tests.♻️ Suggested fix: add an `afterEach` guard
describe("CircuitBreakerManager", () => { + let manager: CircuitBreakerManager; + + afterEach(() => { + manager?.destroyAll(); + }); + describe("breaker management", () => { it("should create and retrieve breakers", () => { - const manager = new CircuitBreakerManager(); + manager = new CircuitBreakerManager(); // ... - manager.destroyAll(); }); // repeat for each test });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mcp/mcpCircuitBreaker.test.ts` around lines 545 - 711, Tests create CircuitBreakerManager instances but rely on per-test manual manager.destroyAll() which leaks timers if a test fails; make teardown resilient by declaring a shared mutable variable (e.g., let manager: CircuitBreakerManager | undefined) scoped to the describe block, assign a new instance to that variable in each it block (replace const manager = ... with manager = ...), and add an afterEach(() => manager?.destroyAll()) to always clean up; update any tests that create additional managers to either assign them to the shared variable or ensure they are destroyed in the same afterEach (e.g., track multiple managers in an array and clear them in afterEach by calling destroyAll() on each).
491-512:lastStateChangeassertion trivially passes with frozen fake timers.
vi.useFakeTimers()(installed inbeforeEach) freezesDate.now(). Since novi.advanceTimersByTime()is called before the failingexecute(),initialChangeandstats.lastStateChangeshare the same frozen timestamp, sotoBeGreaterThanOrEqualalways passes even if the implementation never updateslastStateChange. The test's intent — verifying that the timestamp actually advances on state change — is not exercised.♻️ Suggested fix: advance fake time before the state change
const initialChange = breaker.getStats().lastStateChange; + vi.advanceTimersByTime(100); // advance frozen clock so the state-change timestamp differs try { await breaker.execute(async () => { throw new Error("fail"); }); } catch { // Expected } const stats = breaker.getStats(); - expect(stats.lastStateChange.getTime()).toBeGreaterThanOrEqual( + expect(stats.lastStateChange.getTime()).toBeGreaterThan( initialChange.getTime(), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mcp/mcpCircuitBreaker.test.ts` around lines 491 - 512, The test currently uses vi.useFakeTimers() so initialChange and the post-failure stats share the same frozen timestamp; to properly assert lastStateChange advances, call vi.advanceTimersByTime(...) (or vi.setSystemTime(...) ) before triggering the failing breaker.execute() so the clock moves forward, then call breaker.getStats().lastStateChange and assert its time is greater than the initial value; update the test around MCPCircuitBreaker, getStats(), lastStateChange and execute() to advance fake time prior to the state-changing call.src/lib/utils/schemaConversion.ts (1)
185-199:ensureTypeFieldmutates its argument — side-effects on AI SDK wrapper and plain JSON schema paths; loghasPropertiesis alwaystrue.Two concerns:
Mutation side-effect: For path 1 (
extracted = schema.jsonSchema) and path 2 (the caller's schema object),ensureTypeFieldwrites directly into the caller's object. If the same schema instance is reused across calls, it will silently accumulatetype/propertiesfields. A shallow copy before mutation avoids this.Misleading debug log: After
if (!schema.properties) { schema.properties = {}; },schema.propertiesis guaranteed non-null, sohasProperties: !!schema.propertieslogstruein every code path inside the block. The intent was likely"addedProperties": !originalHasPropertiesor similar.♻️ Proposed fix
function ensureTypeField( schema: Record<string, unknown>, ): Record<string, unknown> { if (!schema.type) { - schema.type = "object"; - if (!schema.properties) { - schema.properties = {}; - } + const hadProperties = !!schema.properties; + const result = { ...schema, type: "object" }; + if (!result.properties) { + result.properties = {}; + } logger.debug("[SCHEMA-TYPE-FIX] Added missing type field to JSON Schema", { fixedType: "object", - hasProperties: !!schema.properties, + addedProperties: !hadProperties, }); - } - return schema; + return result; + } + return schema; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/utils/schemaConversion.ts` around lines 185 - 199, ensureTypeField currently mutates its input and logs misleading info; update it to operate on a shallow copy of the incoming schema (so callers and reused instances aren’t modified) and compute a boolean originalHasProperties = !!schema.properties before changing anything, then set copy.type = "object" and copy.properties = copy.properties ?? {} and call logger.debug with addedProperties: !originalHasProperties (or include originalHasProperties) and fixedType: "object"; return the modified copy instead of mutating the original.src/lib/mcp/caching/toolCache.ts (1)
377-417: O(n) linear scan for every eviction.
findLRU,findFIFO, andfindLFUall iterate the entire cache map. With the defaultmaxSizeof 1000 this is fine, but at scale (e.g.,maxSize= 100K) eviction becomes a hot path bottleneck sinceset()triggers eviction on every insert at capacity. Worth noting in the docs or considering a more efficient data structure (e.g., a doubly-linked list for LRU) if larger caches are anticipated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/caching/toolCache.ts` around lines 377 - 417, The eviction helpers findLRU, findFIFO, and findLFU perform O(n) scans over this.cache on every eviction; replace these scans with maintained auxiliary data structures to achieve O(1) or O(log n) eviction: for LRU update add/remove/move logic in set/get using a doubly-linked list (maintain node refs on each CacheEntry and a head/tail pointer), for FIFO maintain a simple queue (or tail pointer) that you push on insert and pop on eviction, and for LFU maintain frequency buckets or a min-heap keyed by accessCount and update bucket membership on get/set; update the methods that mutate the cache (set, get, delete, and eviction code) to keep these structures in sync and store node/bucket references on CacheEntry so you can evict without scanning the entire this.cache.src/lib/mcp/batching/requestBatcher.ts (1)
169-169: Non-null assertions flagged by linter.Static analysis flags
!assertions at lines 169, 318, and 466. While these are safe in the current control flow (each is guarded by a prior check), they violate the project's lint rules. Consider using optional chaining or explicit narrowing.Example fix for line 169
- this.serverQueues.get(serverId)!.add(requestId); + const queue = this.serverQueues.get(serverId); + if (queue) { + queue.add(requestId); + }Also applies to: 318-318, 466-466
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/batching/requestBatcher.ts` at line 169, Replace the non-null assertions on serverQueues at the three occurrences by explicitly narrowing or using optional chaining; e.g., instead of this.serverQueues.get(serverId)!.add(requestId) in the RequestBatcher code, fetch the queue into a const (const queue = this.serverQueues.get(serverId)) and call queue.add(requestId) only if queue is defined (if (queue) queue.add(requestId)), or use this.serverQueues.get(serverId)?.add(requestId); apply the same pattern to the other two spots flagged (the calls using .get(... )!.<method> at lines referenced) to satisfy the linter without changing control flow.src/lib/mcp/agentExposure.ts (2)
660-660: Module-level singleton is eagerly instantiated at import time.
globalAgentExposureManageris created unconditionally when the module is imported. This is a minor concern — if this module is tree-shaken or the singleton is never used, it's harmless. But if the constructor had side-effects or if this pattern proliferates across all 14 modules (the summary mentions several global singletons), startup cost accumulates.Consider a lazy getter pattern (
let instance; export function getGlobalAgentExposureManager() { ... }) for consistency with deferred initialization. This is optional given the lightweight constructor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/agentExposure.ts` at line 660, The module currently eagerly instantiates the singleton via "export const globalAgentExposureManager = new AgentExposureManager()", causing unconditional construction at import time; change this to a lazy-initializer pattern by replacing the exported constant with a module-scoped let (e.g., let globalAgentExposureManager) and an exported accessor function (e.g., export function getGlobalAgentExposureManager()) that constructs new AgentExposureManager() on first call and returns the instance thereafter; update all call sites to use getGlobalAgentExposureManager() (or export the accessor alongside the lazy variable) so initialization is deferred until actually needed.
198-365: Significant duplication betweenexposeAgentAsToolandexposeWorkflowAsTool.Both functions share nearly identical logic for name generation, description building, annotation merging, input schema defaulting, execution wrapping (timeout, logging, context), and tool construction. The only differences are the default prefix/timeout, a few metadata fields, and the "agent" vs "workflow" label.
Consider extracting a shared
exposeAsToolhelper parameterized by source type, with agent/workflow-specific overrides passed in. This would eliminate ~150 lines of duplication and ensure future changes (e.g., fixing the timeout leak) need to be applied only once.Also applies to: 370-539
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/agentExposure.ts` around lines 198 - 365, Extract the shared logic in exposeAgentAsTool and exposeWorkflowAsTool into a new helper (e.g., exposeAsTool) that is parameterized by sourceType ("agent" | "workflow"), sourceId, sourceName and accepts overrides for prefix/defaults, annotations, and metadata merges; move common steps (nameTransformer/baseName/toolName generation, description assembly with includeMetadataInDescription, annotation merging, inputSchema defaulting, execution wrapper creation including timeoutPromise, logging, wrapWithContext behavior and result/metadata shaping) into exposeAsTool, then implement exposeAgentAsTool and exposeWorkflowAsTool as thin wrappers that call exposeAsTool with the appropriate defaults (prefix, executionTimeout, tags like "agent" vs "workflow", and any extra metadata fields) and return the MCPServerTool shape (name/description/inputSchema/outputSchema/annotations/execute/metadata) so duplication around execute, annotations, and tool construction is removed and fixes (e.g., timeout handling) apply in one place.src/lib/mcp/mcpServerBase.ts (2)
365-372:isToolResulttype guard is overly permissive.Any object with a boolean
successproperty will be treated as aToolResult. This could misclassify arbitrary tool return values (e.g.,{ success: true, items: [...] }wheresuccessis a domain field, not aToolResultindicator). Consider tightening the guard by checking for an additional distinguishing field likemetadataor a sentinel property.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/mcpServerBase.ts` around lines 365 - 372, The isToolResult type guard is too permissive: any object with a boolean success will be treated as a ToolResult. Tighten isToolResult (in mcpServerBase.ts) to also check for a unique discriminator present on real ToolResult objects (e.g., a metadata property, a specific sentinel like __toolResult or a required fields array/object) so the guard verifies both typeof (result as ToolResult).success === "boolean" and the presence/shape of that extra discriminator; update ToolResult shape if needed and adjust code paths that call isToolResult to rely on the stricter check.
296-305:requiresConfirmationcheck is a no-op.Lines 300-303 check
tool.annotations?.requiresConfirmationbut do nothing — the comment says HITL integration is handled at a higher level. This dead branch should either be removed or wired to actually block execution (e.g., by emitting an event or throwing if no confirmation handler is registered). As-is it's misleading.Proposed fix — remove the dead branch
try { - // Check if confirmation is required - if (tool.annotations?.requiresConfirmation) { - // This would integrate with HITL manager - // The HITL integration is handled at a higher level (ExternalServerManager) - } - const result = await tool.execute(params, context ?? {});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/mcpServerBase.ts` around lines 296 - 305, The check for tool.annotations?.requiresConfirmation in mcpServerBase.ts is a dead branch that does nothing; either remove it or enforce confirmation. Fix by either deleting the if block around requiresConfirmation or wiring it to actual behavior: before calling tool.execute(params, context), emit a confirmation-required event or throw an error when no confirmation handler is present (integrating with ExternalServerManager or the HITL manager). Reference the symbols tool.annotations?.requiresConfirmation, tool.execute, and ExternalServerManager/ HITL manager in your change so reviewers can see you removed the no-op branch or implemented a confirmation emission/throw that blocks execution until handled.src/lib/mcp/serverCapabilities.ts (2)
303-322: Confusing API:registerResourceTemplateaccepts both apatternparameter and auriPatternfield insidetemplate.The
patternargument is used as the map key, whiletemplate.uriPatternis stored asurion the resource (line 313). If these diverge, template lookup by pattern will succeed but the storeduriwon't match the actual request URI. Consider using a single source of truth — either derive the key fromtemplate.uriPatternor drop the redundanturiPatternfield.Proposed simplification
registerResourceTemplate( - pattern: string, - template: Omit<RegisteredResource, "uri"> & { uriPattern: string }, + template: Omit<RegisteredResource, "uri"> & { uriPattern: string }, ): this { if (!this.config.resources) { throw new Error("Resource support is not enabled"); } - this.resourceTemplates.set(pattern, { + this.resourceTemplates.set(template.uriPattern, { ...template, uri: template.uriPattern, }); this.emit("resourceTemplateRegistered", { - pattern, + pattern: template.uriPattern, timestamp: new Date(), }); return this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/serverCapabilities.ts` around lines 303 - 322, The registerResourceTemplate API currently accepts a separate pattern parameter and a template with uriPattern, then stores pattern as the map key while using template.uriPattern to set the resource's uri, which can cause mismatches; update registerResourceTemplate (and any callers) to use a single source of truth by deriving the map key from template.uriPattern (or removing uriPattern and deriving template.uri from the pattern) so the stored uri and the map key always match; specifically modify registerResourceTemplate to compute key = template.uriPattern (or require pattern be equal to template.uriPattern and validate), set resourceTemplates.set(key, { ...template, uri: template.uriPattern }), and adjust emitted event payload to use that unified key to avoid divergence between resourceTemplates and template.uri.
491-500:validateResourceUrionly warns on invalid URIs but does not prevent registration.This means resources with completely malformed URIs will be silently registered and may cause confusion downstream. Consider at minimum documenting this permissive behavior, or offering a
strictmode that rejects invalid URIs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/serverCapabilities.ts` around lines 491 - 500, validateResourceUri currently only logs a warning for invalid URIs which allows malformed resources to be registered; update validateResourceUri to accept an optional strict boolean (e.g., validateResourceUri(uri: string, strict = false)) and when strict is true throw an Error for invalid URIs instead of just logger.warn, and update any callers (e.g., the resource registration method that invokes validateResourceUri) to opt into strict validation where registration should be rejected; also add a short JSDoc to validateResourceUri describing the permissive default and the strict behavior so callers know the contract.src/lib/neurolink.ts (2)
6940-6945: Type the elicitation handler to avoidunknownleakage.
registerElicitationHandlercurrently accepts(request: unknown) => Promise<unknown>, which bypasses the new elicitation type contracts. UseElicitationHandler(and related request/response types) to keep strict typing; ifgetElicitationManager()becomes async per the ESM fix, update this signature accordingly.Proposed typing tightening
+import type { ElicitationHandler } from "./mcp/elicitation/types.js"; ... - registerElicitationHandler( - handler: (request: unknown) => Promise<unknown>, - ): void { + registerElicitationHandler(handler: ElicitationHandler): void { const elicitationManager = this.getElicitationManager(); elicitationManager.registerHandler(handler); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 6940 - 6945, Change registerElicitationHandler to accept a strongly typed ElicitationHandler instead of (request: unknown)=>Promise<unknown>; update its signature to use the project's ElicitationHandler (and any specific ElicitationRequest/ElicitationResponse types) and pass that into elicitationManager.registerHandler. Also anticipate getElicitationManager becoming async per the ESM change: if it becomes async, make registerElicitationHandler async and await this.getElicitationManager() before calling elicitationManager.registerHandler. Ensure all references use the named types (ElicitationHandler, ElicitationRequest, ElicitationResponse) rather than unknown.
7030-7086: Wrap new async module loads with withTimeout/ErrorFactory.
exposeAgentAsToolandexposeWorkflowAsTooldynamically import modules without timeout or typed error handling. Per repo guidance, wrap these async operations withwithTimeoutand surface typed errors viaErrorFactory(and apply the same pattern to other new async imports in this block).As per coding guidelines: “Use ErrorFactory for creating typed errors. Wrap async operations with withTimeout utility. Implement graceful degradation with provider fallback.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 7030 - 7086, Wrap the dynamic imports in exposeAgentAsTool and exposeWorkflowAsTool with the withTimeout helper and create typed errors using ErrorFactory when the import fails or times out; specifically, replace the raw await import("./mcp/agentExposure.js") in both exposeAgentAsTool and exposeWorkflowAsTool with a withTimeout-wrapped import call and catch errors to throw an ErrorFactory-created error (include context like function name and module path), and apply the same pattern to any other new async imports in this block to enable graceful degradation/provider fallback.src/lib/mcp/elicitation/types.ts (1)
11-214: Centralize exported elicitation types under src/lib/types.These are shared/public API types used across MCP manager/protocol layers. The project standard calls for reusable types to live in src/lib/types to avoid circular deps; consider moving them into a dedicated types module (e.g., src/lib/types/elicitationTypes.ts) and re-exporting via src/lib/types/index.ts.
Based on learnings: “Project standard: Place reusable/shared types under src/lib/types/.ts; test-only helper types under test/types/.ts; avoid declaring local types inside source implementation files.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/elicitation/types.ts` around lines 11 - 214, The current file defines shared/public elicitation types locally — ElicitationType, ElicitationRequest, Elicitation (and variants like ConfirmationElicitation, TextElicitation, SelectElicitation, MultiSelectElicitation, FormElicitation, FileElicitation, SecretElicitation), SelectOption and FormField — which should be centralized into the shared types module; move these type declarations into a dedicated shared types module and export them from the central types index so other modules import from the single canonical location, update all imports to reference the central types index (removing the local definitions here) and ensure no circular deps by only exporting plain types (no runtime values) from that module.src/cli/commands/mcp.ts (1)
2955-3279:executeAnnotateexceeds the 300-line limit.Static analysis flags this method length. Consider extracting list/parse/validation/output into helpers to keep complexity manageable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/mcp.ts` around lines 2955 - 3279, The executeAnnotate function is too large (>300 lines); refactor by extracting logical sections into smaller helpers: move the "list mode" logic into a new listToolAnnotations(sdk, argv) function (handles spinner, inferAnnotations, grouping and formatted output), move the tool lookup into findTool(servers, toolName, serverId), move argument parsing/merging into buildAnnotationsFromArgs(argv, foundTool) (handles JSON parsing, flags, infer merge and calls mergeAnnotations), and move the display/confirm/validate block into displayAndMaybePersistAnnotations(foundTool, annotations, argv) (handles validate flag, printing summary and colorized fields). Keep executeAnnotate to orchestration only: instantiate NeuroLink, call listToolAnnotations when argv.list, call findTool and buildAnnotationsFromArgs, validate with validateAnnotations, then call displayAndMaybePersistAnnotations; re-use existing validateAnnotations, inferAnnotations, mergeAnnotations, getAnnotationSummary symbols.src/lib/mcp/multiServerManager.ts (1)
294-322: Use ErrorFactory for server/group-not-found errors, or create MCP-specific error factory.Four
throw new Error(...)statements (lines 298, 355, 359, 489) should use typed errors. ErrorFactory lacks server/group-specific methods; either use a generic method likeinvalidConfiguration()or create an MCP-specific error factory usingcreateErrorFactory()to align with the codebase pattern (used in PPT and RAG modules). Import ErrorFactory or NeuroLinkError accordingly.Error locations
- Line 298 (updateServer):
Server '${serverId}' not found- Line 355 (createGroup):
Group '${groupId}' not found- Line 359 (addServerToGroup):
Server '${serverId}' not found- Line 489 (setToolPreference):
Server '${serverId}' not found🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/multiServerManager.ts` around lines 294 - 322, Replace the plain throws in updateServer, createGroup, addServerToGroup, and setToolPreference with typed errors produced by the project's error factory: import/create an MCP-specific factory via createErrorFactory() (or reuse the existing ErrorFactory) and call an appropriate method (e.g., invalidConfiguration(...) or a new server/group-not-found helper) instead of `throw new Error(...)`; ensure each message includes the same contextual text (Server '...'/Group '...') and replace the four throw sites (`updateServer`, `createGroup`, `addServerToGroup`, `setToolPreference`) to use the factory-generated error so errors align with the codebase pattern.src/lib/mcp/enhancedToolDiscovery.ts (3)
206-212: Replace non-null assertion with a safe pattern.Static analysis flags
this.serverToolsMap.get(serverId)!on line 212. Since you check and set the map entry on lines 209-211, the assertion is technically safe, but you can eliminate it trivially:Proposed fix
- if (!this.serverToolsMap.has(serverId)) { - this.serverToolsMap.set(serverId, new Set()); - } - this.serverToolsMap.get(serverId)!.add(tool.name); + let serverTools = this.serverToolsMap.get(serverId); + if (!serverTools) { + serverTools = new Set(); + this.serverToolsMap.set(serverId, serverTools); + } + serverTools.add(tool.name);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/enhancedToolDiscovery.ts` around lines 206 - 212, Replace the non-null assertion on this.serverToolsMap.get(serverId)! by retrieving or creating the Set in a local variable and using that variable; for example, after ensuring an entry exists, do const serverSet = this.serverToolsMap.get(serverId) ?? new Set<string>(); serverSet.add(tool.name); this.serverToolsMap.set(serverId, serverSet); — update the block around this.toolRegistry.set(...) and the serverToolsMap handling to use serverSet instead of the non-null asserted get(...) call.
348-386: Replace non-null assertions in filter callbacks with narrowed closures.Lines 352, 367, and 378 use
criteria.serverIds!,criteria.tags!, andcriteria.annotations!inside filter callbacks, even though the outerifalready narrows the type. TypeScript doesn't carry the narrowing into the nested callback closure. A quick local binding eliminates the assertions cleanly:Proposed fix (example for serverIds; apply same pattern to tags and annotations)
if (criteria.serverIds?.length) { + const serverIds = criteria.serverIds; results = results.filter((tool) => - criteria.serverIds!.includes(tool.serverId), + serverIds.includes(tool.serverId), ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/enhancedToolDiscovery.ts` around lines 348 - 386, The filter callbacks in enhancedToolDiscovery use non-null assertions (criteria.serverIds!, criteria.tags!, criteria.annotations!) which TypeScript won't accept because the outer type narrowing isn't preserved into nested closures; fix by capturing narrowed values into local consts (e.g., const serverIds = criteria.serverIds; const tags = criteria.tags; const annotations = criteria.annotations) before calling results.filter and use those locals inside the callbacks, and when iterating annotation entries keep the annotationKey typing (key as keyof MCPToolAnnotations) as-is.
144-153: Constructor always instantiates aMultiServerManagereven if no multi-server features are used.When no
multiServerManageris passed, a fresh default instance is created. This may be intentional for standalone usage, but if the caller later provides their own manager elsewhere, the two instances won't share state. Consider lazy initialization or documenting that the optional parameter is the single point of integration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/enhancedToolDiscovery.ts` around lines 144 - 153, The constructor for EnhancedToolDiscovery unconditionally instantiates a MultiServerManager causing potential duplicate managers; change the implementation so the private multiServerManager is not created in the constructor but lazily instantiated or injectable: make multiServerManager optional (nullable/undefined), remove the "new MultiServerManager()" call in the constructor, add a private getOrCreateMultiServerManager() (or a public setMultiServerManager()/getMultiServerManager()) that creates the MultiServerManager on first use, and update any methods that reference this.multiServerManager to call the getter so a single shared instance is created only when needed; reference the EnhancedToolDiscovery constructor, the multiServerManager property, and any methods that currently use this.multiServerManager to apply the change.
| const candidates = this.getCandidateServers(tool); | ||
|
|
||
| if (candidates.length === 0) { | ||
| throw new Error(`No healthy servers available for tool: ${tool.name}`); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find ErrorFactory class definition
find . -type f -name "*.ts" -o -name "*.js" | xargs grep -l "class ErrorFactory" 2>/dev/null | head -20Repository: juspay/neurolink
Length of output: 115
🏁 Script executed:
#!/bin/bash
# Also search for ErrorFactory exports
rg "export.*ErrorFactory" --type ts --type js -A 3Repository: juspay/neurolink
Length of output: 801
🏁 Script executed:
#!/bin/bash
# Check the toolRouter.ts file around line 269
cat -n src/lib/mcp/routing/toolRouter.ts | sed -n '260,280p'Repository: juspay/neurolink
Length of output: 794
🏁 Script executed:
#!/bin/bash
# Look for existing ErrorFactory usage patterns in the codebase
rg "ErrorFactory\.create" --type ts --type js -B 2 -A 2 | head -50Repository: juspay/neurolink
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Read the ErrorFactory class definition
cat -n src/lib/utils/errorHandling.tsRepository: juspay/neurolink
Length of output: 36306
🏁 Script executed:
#!/bin/bash
# Search for how ErrorFactory is actually used in the codebase
rg "ErrorFactory\." --type ts --type js -B 2 -A 3 | head -100Repository: juspay/neurolink
Length of output: 7177
🏁 Script executed:
#!/bin/bash
# Check imports of ErrorFactory to understand usage patterns
rg "import.*ErrorFactory" --type ts --type js -A 2Repository: juspay/neurolink
Length of output: 997
🏁 Script executed:
#!/bin/bash
# Check toolRouter imports and context
head -n 50 src/lib/mcp/routing/toolRouter.tsRepository: juspay/neurolink
Length of output: 1499
🏁 Script executed:
#!/bin/bash
# Search for how "no healthy servers" errors are handled elsewhere
rg "healthy.*server|server.*available" --type ts --type js -B 2 -A 2Repository: juspay/neurolink
Length of output: 13081
🏁 Script executed:
#!/bin/bash
# Check if there's a suitable ErrorFactory method for server/routing errors
rg "static\s+\w+\(" src/lib/utils/errorHandling.ts | head -30Repository: juspay/neurolink
Length of output: 1505
Use ErrorFactory for typed errors instead of plain Error.
The suggestion to use ErrorFactory is valid per the coding guidelines, but the proposed ErrorFactory.create() method doesn't exist. ErrorFactory provides specific static methods like toolTimeout(), networkError(), memoryExhausted(), etc.
Since there's no predefined method for unavailable servers, create a new ErrorFactory static method (e.g., noHealthyServersAvailable()) following the existing pattern with ErrorCategory, ErrorSeverity, and error codes. Alternatively, use ErrorFactory.networkError() if this represents a connectivity issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/mcp/routing/toolRouter.ts` at line 269, Replace the plain throw new
Error in toolRouter (the line throwing `No healthy servers available for tool:
${tool.name}`) with a typed ErrorFactory call: either add a new static
ErrorFactory.noHealthyServersAvailable(toolName: string) method (follow the
pattern of existing ErrorFactory methods like toolTimeout() / networkError(),
using appropriate ErrorCategory, ErrorSeverity and a new error code) and call
that here, or if this is a connectivity issue call
ErrorFactory.networkError(...) with context; then update the throw site to throw
ErrorFactory.noHealthyServersAvailable(tool.name) (or the chosen
ErrorFactory.networkError call) so the code uses the typed ErrorFactory API.
5606713 to
3b2c756
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
🤖 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 full review |
✅ Actions performedFull review triggered. |
3b2c756 to
c473dea
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
🤖 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 full review |
murdore
changed the title
🤖 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 |
ae51d8f to
2cd4da0
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 |
2cd4da0 to
ddb73bb
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 |
There was a problem hiding this comment.
Pull request overview
Adds a large set of MCP “enhancement” modules (routing, caching, batching, annotations, elicitation, discovery, etc.) and wires them into the core SDK tool-execution path so they apply during generate()/stream(). Also extends LiteLLM provider tracing/cost attribution, expands pricing tables, and updates CLI/docs/tests to cover new capabilities.
Changes:
- Introduces new MCP enhancement modules (router/cache/batcher/annotations/converter/elicitation/discovery/server base/agent exposure) and exports them from the SDK.
- Wires custom tool execution through
NeuroLink.executeTool()(viaToolsManager.toolExecutor) so MCP enhancements apply consistently. - Improves LiteLLM streaming observability and updates model pricing + documentation/test utilities.
Reviewed changes
Copilot reviewed 48 out of 50 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/continuous-test-suite-tracing.ts | Adds provider-specific model env resolution after CLI overrides. |
| src/lib/utils/schemaConversion.ts | Improves schema conversion to support Zod, AI SDK wrappers, and plain JSON Schema; inlines $ref. |
| src/lib/utils/pricing.ts | Expands pricing coverage and adds cross-provider lookup for proxy providers. |
| src/lib/types/tools.ts | Adds optional annotations field for MCP tool metadata on ToolInfo. |
| src/lib/types/streamTypes.ts | Adds disableToolCache per-request option for streaming. |
| src/lib/types/index.ts | Re-exports MCPEnhancementsConfig. |
| src/lib/types/generateTypes.ts | Adds disableToolCache to generate/text-generation options. |
| src/lib/types/configTypes.ts | Adds NeurolinkConstructorConfig.mcp and defines MCPEnhancementsConfig. |
| src/lib/providers/litellm.ts | Adds OTel stream span + cost attribution and embedding APIs; tweaks OpenAI SDK init. |
| src/lib/mcp/toolIntegration.ts | Adds middleware + elicitation-aware tool wrapping and a manager abstraction. |
| src/lib/mcp/toolConverter.ts | Adds NeuroLink ↔ MCP tool format conversion utilities and helpers. |
| src/lib/mcp/toolAnnotations.ts | Adds MCP annotations types + inference/merge/validation helpers. |
| src/lib/mcp/routing/toolRouter.ts | Adds multi-strategy routing logic for multi-server tool execution. |
| src/lib/mcp/routing/index.ts | Exports routing types + router factory/defaults. |
| src/lib/mcp/mcpServerBase.ts | Introduces an abstract base class for building MCP servers with lifecycle + tool exec. |
| src/lib/mcp/index.ts | Exports new MCP enhancement modules and deprecates legacy executeMCP. |
| src/lib/mcp/enhancedToolDiscovery.ts | Adds discovery/search/filtering with annotation inference and multi-server integration. |
| src/lib/mcp/elicitationProtocol.ts | Adds JSON-RPC-like wire protocol types + adapter for elicitation messages. |
| src/lib/mcp/elicitation/types.ts | Defines elicitation request/response and context types. |
| src/lib/mcp/elicitation/index.ts | Public exports for elicitation types/manager. |
| src/lib/mcp/elicitation/elicitationManager.ts | Adds the elicitation request manager with timeouts/fallback handling. |
| src/lib/mcp/caching/toolCache.ts | Adds generic cache + tool-result cache wrapper with TTL/eviction strategies. |
| src/lib/mcp/caching/index.ts | Exports caching types and helpers. |
| src/lib/mcp/batching/requestBatcher.ts | Adds request batching with flush triggers, grouping, and timeouts. |
| src/lib/mcp/batching/index.ts | Exports batching types and helpers. |
| src/lib/mcp/agentExposure.ts | Adds utilities to expose agents/workflows as MCP tools. |
| src/lib/index.ts | Re-exports MCP enhancements from the main SDK entrypoint. |
| src/lib/core/modules/ToolsManager.ts | Routes custom tool execution through toolExecutor to enable MCP enhancements. |
| src/cli/loop/optionsSchema.ts | Adds CLI schema support for disableToolCache. |
| docs/visual-demos.md | Fixes/updates visual content links. |
| docs/reference/index.md | Adds MCP enhancements API reference entry. |
| docs/rag/TESTING.md | Fixes RAG configuration link path. |
| docs/rag/CONFIGURATION.md | Fixes RAG testing/API reference links. |
| docs/rag/CLI-COVERAGE.md | Fixes RAG configuration link path. |
| docs/index.md | Adds MCP Enhancements to “What’s New” table. |
| docs/features/rag.md | Fixes RAG doc cross-links to new filenames. |
| docs/features/mcp-enhancements-diagrams.md | Adds architecture diagrams for MCP enhancements. |
| docs/features/index.md | Adds MCP Enhancements to features index and highlights. |
| docs/demos/videos.md | Fixes demo video link paths. |
| docs/demos/index.md | Fixes demo image path reference. |
| docs/advanced/mcp-integration.md | Adds an “Advanced MCP Features” overview section. |
| README.md | Updates “What’s New” + adds MCP enhancements section and examples. |
| CLAUDE.md | Updates internal repo guide with MCP enhancements inventory and key files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| const cost = calculateCost(this.providerName, this.modelName, { | ||
| input: usage.promptTokens || 0, | ||
| output: usage.completionTokens || 0, | ||
| total: (usage.promptTokens || 0) + (usage.completionTokens || 0), | ||
| }); |
| "tool.result.status", | ||
| errorResult ? "error" : "success", | ||
| ); | ||
| customToolSpan.setStatus({ code: SpanStatusCode.OK }); |
ddb73bb to
66c9a39
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 |
66c9a39 to
239937d
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 |
239937d to
2d26474
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 |
…g, and wire into core SDK MCP Enhancement Modules (standalone): - ToolRouter: intelligent multi-server routing with 6 strategies (round-robin, latency, cost, capability, load-balance, priority) - ToolResultCache: LRU/FIFO/LFU caching with TTL, tag-based invalidation - RequestBatcher: automatic batching with configurable batch size/wait - Tool Annotations: auto-inference of readOnly, destructive, idempotent hints - ToolIntegration middleware: composable before/after tool execution hooks - EnhancedToolDiscovery: fuzzy search, tag/capability filtering Core SDK Wiring (new): - Add MCPEnhancementsConfig type for NeurolinkConstructorConfig.mcp - Wire ToolCache into executeToolInternal() with cache check/store - Wire Tool Annotations auto-inference into getAllAvailableTools() - Wire ToolIntegration middleware chain into executeToolInternal() - Wire RequestBatcher into executeTool() entry point - Add disableToolCache per-request option to GenerateOptions/StreamOptions - Add annotations field to ToolInfo type - Skip cache for destructive tools, support safe retry for idempotent tools - Add public APIs: useToolMiddleware(), getToolMiddlewares(), flushToolBatch(), getMCPEnhancementsConfig() - Cleanup all MCP modules in dispose() - Fix inferAnnotations word boundary matching for underscore/hyphen names Tests: - 16 vitest integration tests (mcp-wiring-integration.test.ts) - 14 continuous test suite tests (Part 1c: wired behavior) - All 88/88 continuous suite tests pass, 357 MCP vitest tests pass Documentation: - Comprehensive MCP enhancements guide with architecture diagrams - SDK wiring documentation with code examples - Updated CLAUDE.md and README.md
2d26474 to
8e7f05d
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 |
|
🎉 This PR is included in version 9.28.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
14 MCP enhancement modules + core SDK wiring + comprehensive test suite + LiteLLM support + pricing updates.
MCP Enhancement Modules (14 standalone modules, ~10K lines)
Core SDK Wiring
NeuroLink.executeTool()instead of callingtoolInfo.execute()directly, so cache/middleware/annotations apply duringgenerate()/stream()mcp: { cache, annotations, router, batcher, discovery, middleware }executeToolInternal(), skip for destructive tools, per-requestdisableToolCachegetAllAvailableTools(), readOnlyHint/destructiveHint from tool namesexecuteToolInternal(),useToolMiddleware()public APIgetToolAnnotations(),exposeAgentAsTool(),convertToolsToMCPFormat(), etc.LiteLLM Provider Enhancements
neurolink.provider.streamTextspan with cost attribution (matching OpenAI/Anthropic/Vertex/Bedrock pattern)--provider=litellmnow correctly picks upLITELLM_MODELenv varPricing Updates (18 → 57 models)
CLI (12 new subcommands)
mcp list,servers,tools,discover,create-server,annotate,install,add,test,exec,remove,registryTests (173 assertions, 44 functions)
Verification
Test plan
npx tsx test/continuous-test-suite-mcp.ts --provider=vertex— 172 pass, 0 failnpx tsx test/continuous-test-suite-mcp.ts --provider=litellm— 172 pass, 0 failpnpm run check— 0 TypeScript errorspnpm run lint(eslint) — 0 errorsneurolink.cost=0.045855on spans for claude-sonnet-4-5 via litellm