Add HTTP/Streamable HTTP transport for MCP servers - #732
Merged
Merged
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds HTTP/Streamable HTTP transport for remote MCP servers (headers, httpOptions, retryConfig, rateLimiting, OAuth 2.1/PKCE), expands MCP client/server tooling and types, migrates examples to a NeuroLink-based API, updates CI/docs, and adds extensive tests and documentation for the new transport. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as NeuroLink Client
participant RateLimiter as HTTPRateLimiter
participant OAuth as NeuroLinkOAuthProvider
participant Retry as withHTTPRetry
participant HTTP as HTTP Transport
participant Server as Remote MCP Server
Client->>RateLimiter: acquire()
alt token granted
RateLimiter-->>Client: granted
else queued
RateLimiter-->>Client: wait then granted
end
Client->>OAuth: getAuthorizationHeader(serverId)
alt token valid
OAuth-->>Client: "Bearer <token>"
else refresh needed
OAuth->>Server: token refresh
Server-->>OAuth: new token
OAuth-->>Client: "Bearer <token>"
end
Client->>Retry: withHTTPRetry(() => HTTP.request(...))
Retry->>HTTP: perform POST /mcp (with headers)
HTTP->>Server: request
alt 200 OK
Server-->>HTTP: response
HTTP-->>Retry: response
Retry-->>Client: success
else 429 / Retry-After
Server-->>HTTP: 429 + Retry-After
HTTP->>RateLimiter: handleRateLimitResponse(headers)
Retry->>Retry: backoff + sleep
Retry->>HTTP: retry request
else 503 / network error
Retry->>Retry: exponential backoff + jitter
Retry->>HTTP: retry
else non-retryable
Retry-->>Client: error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Copilot
AI
changed the title
[WIP] Add HTTP/Streamable HTTP transport support for MCP servers
Add HTTP/Streamable HTTP transport for MCP servers
Dec 30, 2025
murdore
force-pushed
the
copilot/add-http-streamable-support
branch
from
January 1, 2026 04:25
bc7ca6f to
551f99b
Compare
murdore
marked this pull request as ready for review
January 1, 2026 04:25
Contributor
🤖 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 |
murdore
force-pushed
the
copilot/add-http-streamable-support
branch
from
January 1, 2026 20:11
551f99b to
cf844bb
Compare
Contributor
✅ 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 |
Contributor
🤖 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 |
murdore
force-pushed
the
copilot/add-http-streamable-support
branch
from
January 1, 2026 20:35
cf844bb to
0407256
Compare
Contributor
🤖 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 |
murdore
requested changes
Jan 2, 2026
Contributor
|
@coderabbitai Can you go through all the changes and verify them against the project standards defined in agents.md, clineules, memory bank and other places? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 74558 🏁 Script executed: Length of output: 14374 🏁 Script executed: Length of output: 179 🏁 Script executed: Length of output: 8312 @murdore I've completed a comprehensive verification of PR #732 against the project standards. Here's my detailed analysis: ✅ Verification Summary: EXCELLENT - Exceeds All Standards🎯 Commit Format Compliance ✅
📝 Code Quality Standards ✅TypeScript Strict Mode ✅// Proper type definitions
export type MCPTransportType = "stdio" | "sse" | "websocket" | "http" | ...
// Type-safe headers
headers?: Record<string, string>;
// Proper error handling with types
private static async createHTTPTransport(
config: MCPServerInfo,
): Promise<NetworkTransportResult>Error Handling Patterns ✅// Meaningful error messages with validation
if (!config.url) {
throw new Error("URL is required for HTTP transport");
}
// Try-catch with proper error propagation
catch (error) {
throw new Error(
`Invalid HTTP URL: ${error instanceof Error ? error.message : String(error)}`
);
}Modular Architecture ✅🧪 Test Coverage ✅ EXCEEDS TARGETRequirement: 90% coverage target Test Files Added: Coverage Levels:
📚 Documentation Standards ✅12+ files updated/added (~1,500+ lines): Documentation Quality:
🔒 Security Patterns ✅// ✅ No hardcoded secrets - uses environment variables
headers: {
Authorization: `Bearer ${process.env.GITHUB_TOKEN}`
}
// ✅ OAuth 2.1 with PKCE (industry standard)
auth: {
type: "oauth2",
clientId: process.env.CLIENT_ID,
// PKCE flow for enhanced security
}
// ✅ Rate limiting to prevent abuse
rateLimiting: {
requestsPerMinute: 60,
maxBurst: 10
}
// ✅ Circuit breaker pattern for resilience
globalCircuitBreakerManager🏗️ Architecture & Design ✅Follows Factory Pattern: // Proper extension of BaseProvider pattern
private static async createHTTPTransport(
config: MCPServerInfo
): Promise<NetworkTransportResult>
// Uses official MCP SDK transport
import { StreamableHTTPClientTransport } from
"@modelcontextprotocol/sdk/client/streamableHttp.js";Enterprise Features:
📊 Overall Quality Metrics
🎉 Final Verdict: APPROVEDStrengths:
Compliance Summary:
Recommendation:✅ MERGE APPROVED - This PR exceeds all project standards and demonstrates exceptional software engineering practices. The implementation is production-ready with enterprise-grade features, comprehensive testing, and excellent documentation. |
Contributor
🤖 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 |
murdore
force-pushed
the
copilot/add-http-streamable-support
branch
from
January 2, 2026 13:27
f95642c to
af35f35
Compare
Contributor
🤖 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 |
murdore
force-pushed
the
copilot/add-http-streamable-support
branch
from
January 2, 2026 15:58
af35f35 to
538dd7a
Compare
Contributor
🤖 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 |
murdore
force-pushed
the
copilot/add-http-streamable-support
branch
from
January 2, 2026 16:10
538dd7a to
1a171d0
Compare
Contributor
🤖 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 |
murdore
force-pushed
the
copilot/add-http-streamable-support
branch
from
January 2, 2026 17:47
1a171d0 to
f7fb513
Compare
Contributor
🤖 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 |
murdore
force-pushed
the
copilot/add-http-streamable-support
branch
from
January 2, 2026 18:09
f7fb513 to
3897aa4
Compare
Contributor
🤖 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 |
Contributor
|
@coderabbitai ful review |
|
@murdore I'll conduct a full review of all the changes in PR #732. This will include detailed analysis of:
I'll review each file and provide specific feedback where needed. ✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
murdore
force-pushed
the
copilot/add-http-streamable-support
branch
from
January 2, 2026 18:43
3897aa4 to
7545b21
Compare
Contributor
🤖 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 |
Add comprehensive HTTP transport support following MCP 2025 specification:
- Add HTTP transport type to MCPTransportType and MCPServerInfo
- Implement StreamableHTTPClientTransport integration in MCPClientFactory
- Add transport-specific validation (stdio requires command, HTTP/SSE/WebSocket require url)
- Support custom headers for authentication (Bearer, API keys, Basic auth)
- httpOptions: connectionTimeout, requestTimeout, idleTimeout, keepAliveTimeout
- retryConfig: maxAttempts, initialDelay, maxDelay, exponential backoff
- rateLimiting: requestsPerMinute, maxBurst with token bucket algorithm
- OAuth 2.1 with PKCE support for enterprise authentication
- src/lib/mcp/httpRateLimiter.ts - Token bucket rate limiting
- src/lib/mcp/httpRetryHandler.ts - Exponential backoff retry logic
- src/lib/mcp/auth/ - OAuth 2.1 client provider and token storage
- examples/http-transport-mcp.ts - TypeScript usage examples
- docs/MCP-HTTP-TRANSPORT.md - Comprehensive HTTP transport documentation
- test/unit/mcp/httpTransport.test.ts - MCPClientFactory HTTP validation
- test/unit/mcp/httpRateLimiter.test.ts - Rate limiter unit tests
- test/unit/mcp/httpRetryHandler.test.ts - Retry handler unit tests
- test/sdk/mcp/httpTransportSdk.test.ts - SDK-level HTTP transport tests
- test/integration/mcp/httpTransportIntegration.test.ts - Integration tests
- test/integration/mcp/realHttpServers.test.ts - Real server tests (DeepWiki, Semgrep)
- Updated 12+ documentation files with HTTP transport configuration
- Added httpOptions, retryConfig, rateLimiting reference tables
- Added OAuth 2.1 authentication examples
- Updated SDK API reference with new parameters
- MCPServerInfo: headers, httpOptions, retryConfig, rateLimiting, auth fields
- HTTPTransportOptions, RetryConfig, RateLimitConfig types
- OAuth2Config type with PKCE supporti
murdore
force-pushed
the
copilot/add-http-streamable-support
branch
from
January 2, 2026 19:36
7545b21 to
35c8b8b
Compare
Contributor
🤖 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 |
Contributor
|
@coderabbitai full review and verify if all the comments are resolved |
murdore
approved these changes
Jan 2, 2026
🧩 Analysis chain🏁 Script executed: Length of output: 175294 🏁 Script executed: Length of output: 10281 🏁 Script executed: Length of output: 2666 🏁 Script executed: Length of output: 547 @murdore Here’s the full review status for PR #732 and the resolution check of all review comments. I re-ran repository checks on the PR branch (copilot/add-http-streamable-support) to verify each thread. Summary
Resolved
Pending or needs follow-up
Suggested patches
// src/lib/types/tools.ts
// New compatibility alias to keep sessionId optional for public API
export type PublicToolContext = Omit<SDKToolContext, "sessionId"> & {
sessionId?: string;
};
// src/lib/index.ts
export type { PublicToolContext as ToolContext } from "./types/tools.js";
// src/lib/mcp/mcpClientFactory.ts (createHTTPTransport)
const limiter = config.rateLimiting
? globalRateLimiterManager.getLimiter(config.id, {
requestsPerWindow: config.rateLimiting.requestsPerMinute ?? 60,
windowMs: 60_000,
maxBurst: config.rateLimiting.maxBurst ?? 10,
useTokenBucket: config.rateLimiting.useTokenBucket ?? true,
refillRate: (config.rateLimiting.requestsPerMinute ?? 60) / 60,
})
: undefined;
const fetchWithEnhancements = this.createEnhancedFetch(
config,
httpOptions.requestTimeout,
oauthProvider,
limiter,
);
// then update signature and body of createEnhancedFetch:
private static createEnhancedFetch(
config: MCPServerInfo,
timeoutMs: number,
oauthProvider?: NeuroLinkOAuthProvider,
limiter?: HTTPRateLimiter /* or ReturnType<typeof globalRateLimiterManager.getLimiter> */,
): typeof fetch {
const fetchWithTimeout = this.createTimedFetch(timeoutMs);
return async (input: RequestInfo | URL, init?: RequestInit) => {
if (limiter) await limiter.acquire();
// oauth header handling...
return fetchWithTimeout(input, init);
};
}
// test/unit/mcp/httpRetryHandler.test.ts
import {
DEFAULT_HTTP_RETRY_CONFIG,
isRetryableStatusCode,
isRetryableHTTPError,
withHTTPRetry,
} from "../../../src/lib/mcp/httpRetryHandler.js";
import type { HTTPRetryConfig } from "../../../src/lib/types/mcpTypes.js";
// Remove local DEFAULT_RETRY_CONFIG and helper re-implementations.
// Adjust assertions to the production config fields: initialDelay, maxDelay, backoffMultiplier.
// src/lib/mcp/auth/oauthClientProvider.ts (sketch)
import { withTimeout } from "../../utils/withTimeout.js"; // or local utility
// In exchangeCode/refreshTokens/revokeTokens:
const res = await withTimeout(fetch(url, init), 15000);
// After successful exchange:
this.pendingStates.delete(state);
this.pendingChallenges.delete(state); // ensure cleanup regardless of codeVerifier source
// src/lib/types/hitlTypes.ts
// Replace
export interface HITLManager { ... }
// With
export type HITLManager = {
isEnabled(): boolean;
requiresConfirmation(toolName: string, args?: unknown): boolean;
// ...
};
// Remove the duplicate declaration block.
// .mcp-servers.example.json (internal-http-server entry)
"description": "⚠️ WARNING: SSL verification disabled — NEVER use in production. For internal dev/test networks with self-signed certs only"
// test/integration/mcp/realHttpServers.test.ts (in finally)
if (transport && typeof transport.close === "function") {
try { await transport.close(); } catch {}
}
// In the summary test:
const connectedCount = Object.values(results).filter(r => r.connected).length;
expect(connectedCount).toBeGreaterThan(0);Are all comments resolved?
Would you like me to open follow-up issues for the non-blocking items (OAuth hardening, test cleanups, rate limiter polish), or should we address them in this PR? ✏️ Learnings added
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/timeout-usage.js (1)
7-7: Update documentation to reflect actual timeout format.The comment states "Using different timeout formats" but the file now exclusively uses numeric milliseconds throughout. Consider updating to "Using different timeout values" or "Using timeout values in milliseconds" for accuracy.
🔎 Proposed documentation fix
* This example demonstrates: - * - Using different timeout formats + * - Using different timeout values * - Handling timeout errors
♻️ Duplicate comments (2)
.mcp-servers.example.json (1)
199-218: SSL verification warning still needs strengthening.The
rejectUnauthorized: falsesetting at line 210 was flagged in a previous review. While the description mentions "relaxed SSL verification," a more prominent warning would help prevent copy-paste security issues in production.memory-bank/mcp-auto-discovery.md (1)
176-214: Past review comment still applies: Remove duplicated Transport Types section.This duplication was already flagged in a previous review. The "Transport Types (NEW - MCP 2025)" section appears twice in the document. Please remove the redundant copy to prevent maintenance issues and version drift.
🧹 Nitpick comments (14)
.github/workflows/ci.yml (1)
31-32: Use a commit SHA for AnimMouse/setup-ffmpeg to improve reproducibility.The
AnimMouse/setup-ffmpeg@v1action is actively maintained (last updated March 7, 2025) and includes a version input. However, replace the@v1tag with a specific commit SHA to ensure deterministic builds, especially if the action is later updated. The current verification step confirms default ffmpeg versions meet your needs.src/lib/mcp/auth/oauthClientProvider.ts (1)
385-396: Consider TTL-based cleanup for pending PKCE challenges.The current cleanup logic only clears entries when exceeding 100 items, without tracking timestamps. This could allow stale PKCE challenges to accumulate if authorization flows are started but never completed.
🔎 Suggested improvement
Track creation timestamps to enable TTL-based cleanup:
private pendingChallenges: Map<string, PKCEChallenge & { createdAt: number }> = new Map(); cleanupPendingRequests(): void { const now = Date.now(); const maxAge = 10 * 60 * 1000; // 10 minutes for (const [state, challenge] of this.pendingChallenges) { if (now - challenge.createdAt > maxAge) { this.pendingChallenges.delete(state); this.pendingStates.delete(state); } } }test/integration/mcp/httpTransportIntegration.test.ts (1)
312-319: Promise may hang if server address is unexpected.If
server.address()returnsnull(server not listening) or a string (Unix socket), the promise never resolves. While unlikely with port 0, adding a fallback improves robustness.🔎 Suggested defensive handling
server.listen(0, () => { const addr = server.address(); if (addr && typeof addr === "object") { const url = `http://localhost:${addr.port}`; resolve({ server, url }); + } else { + server.close(); + throw new Error("Failed to get server address"); } });docs/MCP-HTTP-TRANSPORT.md (1)
39-41: Security note: Avoid committing actual tokens in example configurations.The example shows a placeholder token format
ghp_xxxxxxxxxxxxxxxxxxxxwhich looks like a real GitHub token pattern. Consider using a more obviously fake placeholder likeYOUR_GITHUB_TOKENto prevent accidental copy-paste of real credentials.🔎 Suggested clarification
"headers": { - "Authorization": "Bearer ghp_xxxxxxxxxxxxxxxxxxxx" + "Authorization": "Bearer YOUR_GITHUB_TOKEN" },src/lib/types/externalMcp.ts (1)
389-437: Consider documenting the toolsMap/config.tools data duplication strategy.The RuntimeMCPServerInfo type maintains tool information in two places:
toolsMap: Map<string, ExternalMCPToolInfo>(line 428)config: MCPServerInfowhich includestools: MCPToolInfo[]While the comment at line 427 indicates this is "legacy compatibility," having dual sources of truth creates risk of inconsistency. Consider:
- Adding explicit documentation about which source is authoritative
- Documenting the synchronization strategy between these fields
- Creating a tracking issue for the eventual migration away from this dual representation
📝 Suggested documentation addition
/** Legacy compatibility - maintain tools map for now */ toolsMap: Map<string, ExternalMCPToolInfo>; + // NOTE: toolsMap and config.tools represent the same data in different formats. + // toolsMap is the authoritative source during runtime. config.tools should be + // kept in sync when tools are discovered/updated. This dual representation will + // be removed in Phase 2 of the zero-conversion architecture migration. /** Cached tools array for ZERO conversion - MCP format */ toolsArray?: Array<{test/integration/mcp/realHttpServers.test.ts (1)
102-111: Consider closing transport in cleanup.The
finallyblock closes theclientbut doesn't explicitly close thetransport. While the SDK may handle this internally, explicitly closing the transport ensures proper resource cleanup.🔎 Suggested fix to close transport
} finally { // Cleanup if (client) { try { await client.close(); } catch { // Ignore cleanup errors } } + if (transport) { + try { + await transport.close(); + } catch { + // Ignore cleanup errors + } + } }docs/reference/configuration.md (1)
12-12: Migration note appears to have a duplicate reference.The migration note states that configuration remains identical for both
generate()andgenerate(), which seems like a copy-paste artifact. It likely should reference two different methods or be simplified.src/lib/mcp/mcpClientFactory.ts (2)
72-89: Rate limiter token acquired at client creation, not per-request.Per PR comments, rate limiting is acquired once during client creation but not wired into per-request fetch operations. For HTTP transport, tool invocations happen after client creation and should respect rate limits.
Consider integrating rate limiter acquisition into
createEnhancedFetch(lines 519-554) so each request respects the configured rate limits, not just the initial connection.🔎 Suggested approach
private static createEnhancedFetch( config: MCPServerInfo, timeoutMs: number, oauthProvider?: NeuroLinkOAuthProvider, ): typeof fetch { const fetchWithTimeout = this.createFetchWithTimeout(timeoutMs); return async (input: RequestInfo | URL, init?: RequestInit) => { + // Acquire rate limit token before each request + if (config.rateLimiting) { + const rateLimiter = globalRateLimiterManager.getLimiter(config.id, { + requestsPerWindow: config.rateLimiting.requestsPerMinute ?? 60, + windowMs: 60000, + maxBurst: config.rateLimiting.maxBurst ?? 10, + useTokenBucket: config.rateLimiting.useTokenBucket ?? true, + refillRate: (config.rateLimiting.requestsPerMinute ?? 60) / 60, + }); + await rateLimiter.acquire(); + } + // If OAuth is configured, ensure we have valid tokens // ... rest of the function
559-588: OAuth provider setup lacks timeout protection.Per coding guidelines, async operations should be wrapped with timeout utilities. The
setupAuthProvidermethod creates an OAuth provider but doesn't protect against slow or hanging token operations.Consider wrapping the OAuth provider creation or adding timeout configuration for token operations within
NeuroLinkOAuthProvider.test/continuous-test-suite.ts (2)
3034-3044: Missingtransport.close()in cleanup.Per PR comments on real HTTP MCP integration tests, ensure both
transport.close()andclient.close()are called to avoid resource leaks. Currently onlyclient.close()is called.🔎 Proposed fix
} finally { // Cleanup + if (transport) { + try { + await transport.close(); + } catch { + // Ignore cleanup errors + } + } if (client) { try { await client.close(); } catch { // Ignore cleanup errors } } }
3122-3131: Same cleanup issue for fetch client transport.The
fetchTransportshould also be closed in the finally block alongsidefetchClient.🔎 Proposed fix
} finally { + if (fetchTransport) { + try { + await fetchTransport.close(); + } catch { + // Ignore cleanup errors + } + } if (fetchClient) { try { await fetchClient.close(); } catch { // Ignore cleanup errors } } }test/types/mcp.ts (1)
38-54: Consider renamingHTTPRetryConfigto avoid collision with production type.Per PR comments, this test-specific
HTTPRetryConfighas different field names (baseDelayMs,maxDelayMs,exponentialBackoff,jitterFactor) compared to the productionHTTPRetryConfiginsrc/lib/types/mcpTypes.ts(initialDelay,maxDelay,backoffMultiplier,retryableStatusCodes).This naming collision can cause confusion and potential import errors. Consider renaming to
TestHTTPRetryConfigorMockHTTPRetryConfigfor clarity.🔎 Proposed fix
-export type HTTPRetryConfig = { +export type TestHTTPRetryConfig = { /** Maximum number of retry attempts (default: 3) */ maxAttempts: number; /** Base delay between retries in milliseconds (default: 1000) */ baseDelayMs: number; // ... rest of fields };src/lib/mcp/externalServerManager.ts (1)
15-32: HTTP transport config wiring inExternalServerManagerlooks correct; consider aligning transport enums
- File-based configs now correctly support both stdio (via
command) and HTTP/SSE/WebSocket (viaurl) thanks to the updatedisValidExternalMCPServerConfigand the enhancedexternalConfigmapping (includingheaders,httpOptions,retryConfig, andrateLimiting).validateConfigcorrectly enforces:
command+argsforstdiourlforhttp,sse, andwebsocketaddServer’stempConfignow carries through all HTTP‑specific fields before validation and into the runtimeconfig, so downstream components see the full configuration.One optional clean‑up:
MCPTransportTypealso declares"ws" | "tcp" | "unix", butvalidateConfigonly accepts"stdio" | "sse" | "websocket" | "http". If"ws","tcp", or"unix"are intended for future use, you might either:
- Add them to the allowed set here, or
- Trim them from
MCPTransportTypeuntil they’re supported.Not a blocker for the HTTP work.
Also applies to: 84-133, 239-339, 441-505, 551-585, 692-716
src/lib/mcp/index.ts (1)
73-83: Consider using ErrorFactory for typed errors.The
executeMCPfunction throws a genericErrorinstead of using theErrorFactorypattern recommended in the coding guidelines for this module path.As per coding guidelines,
src/lib/**/*.tsshould use ErrorFactory for creating typed errors.🔎 Example using ErrorFactory pattern
export async function executeMCP<T = unknown>( _name: string, _config: unknown, _args: unknown, _context?: { sessionId?: string; userId?: string; }, ): Promise<T> { - throw new Error("MCP execution not available - ecosystem removed"); + throw ErrorFactory.createNotImplementedError("MCP execution not available - ecosystem removed"); }
| with: | ||
| ffmpeg-version: "7.0.2" | ||
| github-token: ${{ secrets.GITHUB_TOKEN }} | ||
| uses: AnimMouse/setup-ffmpeg@v1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check GitHub stats for the AnimMouse/setup-ffmpeg action
echo "=== Checking AnimMouse/setup-ffmpeg repository stats ==="
gh repo view AnimMouse/setup-ffmpeg --json stargazerCount,pushedAt,isArchived,description,updatedAt
echo ""
echo "=== Checking recent releases ==="
gh release list --repo AnimMouse/setup-ffmpeg --limit 10
echo ""
echo "=== Checking repository visibility and activity ==="
gh repo view AnimMouse/setup-ffmpeg --json forkCount,watchersRepository: juspay/neurolink
Length of output: 460
Consider the risks of low community adoption for this action in a production release workflow.
The AnimMouse/setup-ffmpeg action is actively maintained (recent commits through December 2025), but has extremely low community adoption (11 stars, 3 forks, 1 watcher) and has not released a new version since February 2023 despite recent repository activity. While the change removes unnecessary inputs, switching to a sparsely-used action in your release workflow introduces risk. If issues arise, community support and alternative solutions will be limited. Strongly recommend either monitoring this action closely after deployment or considering a more widely-adopted FFmpeg setup alternative.
🤖 Prompt for AI Agents
In .github/workflows/release.yml around line 35, the workflow currently uses
AnimMouse/setup-ffmpeg@v1 which has very low community adoption; either replace
it with a more widely-adopted FFmpeg setup action (pin to a popular, actively
maintained alternative) or mitigate risk by pinning the current action to a
specific commit/sha, adding a fallback installation step (e.g., apt/yum or
another setup action) if it fails, and adding post-deploy monitoring/alerts to
detect and roll back release jobs that fail due to the action.
Comment on lines
+1005
to
+1054
| ## OpenRouter Configuration {#openrouter} | ||
|
|
||
| OpenRouter provides access to 300+ AI models from 60+ providers through a single unified API with automatic failover and cost optimization. | ||
|
|
||
| ### Basic Setup | ||
|
|
||
| ```bash | ||
| export OPENROUTER_API_KEY="sk-or-v1-your-api-key" | ||
| ``` | ||
|
|
||
| ### Optional Configuration | ||
|
|
||
| ```bash | ||
| # Attribution for OpenRouter dashboard | ||
| export OPENROUTER_REFERER="https://yourapp.com" | ||
| export OPENROUTER_APP_NAME="Your App Name" | ||
|
|
||
| # Default model | ||
| export OPENROUTER_MODEL="anthropic/claude-3-5-sonnet" | ||
| ``` | ||
|
|
||
| ### Supported Models | ||
|
|
||
| OpenRouter supports 300+ models including: | ||
|
|
||
| - `anthropic/claude-3-5-sonnet` (default) - Best overall quality | ||
| - `openai/gpt-4o` - Excellent code generation | ||
| - `google/gemini-2.0-flash` - Fast and cost-effective | ||
| - `meta-llama/llama-3.1-70b-instruct` - Best open source | ||
|
|
||
| ### Usage Example | ||
|
|
||
| ```typescript | ||
| import { NeuroLink } from "@juspay/neurolink"; | ||
|
|
||
| const neurolink = new NeuroLink(); | ||
|
|
||
| const result = await neurolink.generate({ | ||
| input: { text: "Explain quantum computing" }, | ||
| provider: "openrouter", | ||
| model: "anthropic/claude-3-5-sonnet", | ||
| temperature: 0.7, | ||
| maxTokens: 500, | ||
| }); | ||
| ``` | ||
|
|
||
| ### Complete Guide | ||
|
|
||
| For comprehensive OpenRouter setup including model selection, cost optimization, and best practices, see the [OpenRouter Provider Guide](./providers/openrouter.md). | ||
|
|
There was a problem hiding this comment.
Fix relative link to OpenRouter provider guide
From docs/getting-started/provider-setup.md, the OpenRouter guide link:
[OpenRouter Provider Guide](./providers/openrouter.md)will resolve to docs/getting-started/providers/openrouter.md, which likely does not exist if the guide lives under docs/providers/.
Update the path to go up one level:
Suggested fix
-For comprehensive OpenRouter setup including model selection, cost optimization, and best practices, see the [OpenRouter Provider Guide](./providers/openrouter.md).
+For comprehensive OpenRouter setup including model selection, cost optimization, and best practices, see the [OpenRouter Provider Guide](../providers/openrouter.md).🤖 Prompt for AI Agents
In docs/getting-started/provider-setup.md around lines 1005 to 1054 the relative
link [OpenRouter Provider Guide](./providers/openrouter.md) incorrectly resolves
to docs/getting-started/providers/openrouter.md; change the link to point one
level up by updating it to ../providers/openrouter.md so it correctly resolves
to docs/providers/openrouter.md. Ensure no other occurrences of the same
mistaken ./providers path remain in this file.
Comment on lines
+411
to
+534
| ## Remote HTTP MCP Servers | ||
|
|
||
| NeuroLink supports connecting to remote MCP servers over HTTP/Streamable HTTP transport with authentication, retry logic, and rate limiting. | ||
|
|
||
| ### Configuring Remote HTTP Servers | ||
|
|
||
| ```typescript | ||
| const ai = new NeuroLink({ | ||
| providers: [ | ||
| { name: "anthropic", config: { apiKey: process.env.ANTHROPIC_API_KEY } }, | ||
| ], | ||
| mcpServers: [ | ||
| // Remote API with Bearer token | ||
| { | ||
| name: "remote-api", | ||
| transport: "http", | ||
| url: "https://api.example.com/mcp", | ||
| headers: { | ||
| Authorization: `Bearer ${process.env.API_TOKEN}`, | ||
| }, | ||
| httpOptions: { | ||
| connectionTimeout: 30000, | ||
| requestTimeout: 60000, | ||
| }, | ||
| retryConfig: { | ||
| maxAttempts: 3, | ||
| initialDelay: 1000, | ||
| maxDelay: 30000, | ||
| }, | ||
| }, | ||
|
|
||
| // Remote server with API key | ||
| { | ||
| name: "external-tools", | ||
| transport: "http", | ||
| url: "https://tools.example.com/mcp", | ||
| headers: { | ||
| "X-API-Key": process.env.TOOLS_API_KEY, | ||
| }, | ||
| rateLimiting: { | ||
| requestsPerMinute: 60, | ||
| maxBurst: 10, | ||
| }, | ||
| }, | ||
|
|
||
| // OAuth 2.1 protected server | ||
| { | ||
| name: "oauth-protected", | ||
| transport: "http", | ||
| url: "https://secure.example.com/mcp", | ||
| auth: { | ||
| type: "oauth2", | ||
| oauth: { | ||
| clientId: process.env.OAUTH_CLIENT_ID, | ||
| clientSecret: process.env.OAUTH_CLIENT_SECRET, | ||
| tokenEndpoint: "https://auth.example.com/oauth/token", | ||
| scopes: ["mcp:read", "mcp:write"], | ||
| usePKCE: true, | ||
| }, | ||
| }, | ||
| }, | ||
| ], | ||
| }); | ||
| ``` | ||
|
|
||
| ### HTTP Transport Configuration Options | ||
|
|
||
| | Option | Type | Description | | ||
| | -------------------------------- | --------- | ----------------------------------------- | | ||
| | `transport` | `"http"` | Transport type for remote servers | | ||
| | `url` | `string` | URL of the remote MCP endpoint | | ||
| | `headers` | `object` | HTTP headers for authentication | | ||
| | `httpOptions.connectionTimeout` | `number` | Connection timeout in ms (default: 30000) | | ||
| | `httpOptions.requestTimeout` | `number` | Request timeout in ms (default: 60000) | | ||
| | `httpOptions.idleTimeout` | `number` | Idle timeout in ms (default: 120000) | | ||
| | `httpOptions.keepAliveTimeout` | `number` | Keep-alive timeout in ms (default: 30000) | | ||
| | `retryConfig.maxAttempts` | `number` | Max retry attempts (default: 3) | | ||
| | `retryConfig.initialDelay` | `number` | Initial retry delay in ms (default: 1000) | | ||
| | `retryConfig.maxDelay` | `number` | Max retry delay in ms (default: 30000) | | ||
| | `retryConfig.backoffMultiplier` | `number` | Backoff multiplier (default: 2) | | ||
| | `rateLimiting.requestsPerMinute` | `number` | Rate limit per minute | | ||
| | `rateLimiting.maxBurst` | `number` | Max burst requests | | ||
| | `rateLimiting.useTokenBucket` | `boolean` | Use token bucket algorithm | | ||
|
|
||
| ### Authentication Types | ||
|
|
||
| **Bearer Token:** | ||
|
|
||
| ```typescript | ||
| { | ||
| headers: { | ||
| "Authorization": "Bearer YOUR_TOKEN" | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| **API Key:** | ||
|
|
||
| ```typescript | ||
| { | ||
| headers: { | ||
| "X-API-Key": "your-api-key" | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| **OAuth 2.1 with PKCE:** | ||
|
|
||
| ```typescript | ||
| { | ||
| auth: { | ||
| type: "oauth2", | ||
| oauth: { | ||
| clientId: "your-client-id", | ||
| clientSecret: "your-client-secret", | ||
| tokenEndpoint: "https://auth.example.com/oauth/token", | ||
| scopes: ["mcp:read", "mcp:write"], | ||
| usePKCE: true | ||
| } | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| See [MCP HTTP Transport Guide](../../MCP-HTTP-TRANSPORT.md) for complete documentation. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find the relevant source files
echo "=== Finding HTTP-related source files ==="
find . -type f -name "*.ts" -o -name "*.js" | grep -E "(http|mcp)" | grep -v node_modules | grep -v dist | sortRepository: juspay/neurolink
Length of output: 1527
🏁 Script executed:
#!/bin/bash
# Check the type definitions file first
echo "=== Type definitions (mcpTypes.ts) ==="
cat ./src/lib/types/mcpTypes.ts | head -200Repository: juspay/neurolink
Length of output: 6180
🏁 Script executed:
#!/bin/bash
# Check httpRateLimiter implementation
echo "=== Checking httpRateLimiter.ts ==="
wc -l ./src/lib/mcp/httpRateLimiter.ts
cat -n ./src/lib/mcp/httpRateLimiter.ts | head -80Repository: juspay/neurolink
Length of output: 3304
🏁 Script executed:
#!/bin/bash
# Search for RateLimitConfig type definition
echo "=== Searching for RateLimitConfig type ==="
rg -n "type RateLimitConfig|interface RateLimitConfig" ./src/lib/types/Repository: juspay/neurolink
Length of output: 259
🏁 Script executed:
#!/bin/bash
# Check both RateLimitConfig definitions
echo "=== From mcpTypes.ts ==="
sed -n '940,950p' ./src/lib/types/mcpTypes.ts
echo ""
echo "=== From typeAliases.ts ==="
sed -n '375,395p' ./src/lib/types/typeAliases.ts
echo ""
echo "=== Looking for TokenBucketRateLimitConfig definition ==="
rg -n "TokenBucketRateLimitConfig" ./src/lib/types/ -A 10Repository: juspay/neurolink
Length of output: 3137
🏁 Script executed:
#!/bin/bash
# Check how rateLimiting is used in the HTTP transport implementation
echo "=== Search for usage of rateLimiting configuration ==="
rg -n "rateLimiting" ./src/lib/mcp/ -B 2 -A 5 | head -100Repository: juspay/neurolink
Length of output: 4943
🏁 Script executed:
#!/bin/bash
# Look at the MCPServerInfo rateLimiting field definition more carefully
echo "=== MCPServerInfo rateLimiting field definition ==="
sed -n '115,130p' ./src/lib/types/mcpTypes.tsRepository: juspay/neurolink
Length of output: 619
🏁 Script executed:
#!/bin/bash
# Verify httpOptions field names in implementation
echo "=== MCPHTTPTransportOptions type definition ==="
sed -n '885,895p' ./src/lib/types/mcpTypes.ts
echo ""
echo "=== Search for httpOptions usage in mcpClientFactory ==="
rg -n "httpOptions" ./src/lib/mcp/mcpClientFactory.ts -B 2 -A 5 | head -60Repository: juspay/neurolink
Length of output: 1530
🏁 Script executed:
#!/bin/bash
# Final verification - check the complete MCPHTTPTransportOptions definition
echo "=== Complete MCPHTTPTransportOptions definition ==="
rg -n "export type MCPHTTPTransportOptions" ./src/lib/types/mcpTypes.ts -A 10Repository: juspay/neurolink
Length of output: 533
🏁 Script executed:
#!/bin/bash
# Check if the referenced MCP-HTTP-TRANSPORT.md file exists
echo "=== Checking for MCP-HTTP-TRANSPORT.md ==="
find . -name "MCP-HTTP-TRANSPORT.md" -o -name "*MCP*HTTP*TRANSPORT*"
echo ""
echo "=== Checking docs directory structure ==="
ls -la ./docs/ | head -20
echo ""
echo "=== Check if file is referenced at root level ==="
ls -la ./docs/ | grep -i mcpRepository: juspay/neurolink
Length of output: 2111
Add missing requestsPerHour field to the rate limiting configuration table.
The HTTP transport configuration fields are well-documented and match the implementation. However, the rateLimiting.requestsPerHour option defined in MCPServerInfo is missing from the HTTP Transport Configuration Options table. Add this field to the table:
| Option | Type | Description |
| rateLimiting.requestsPerHour | number | Rate limit per hour (optional) |
All other documented fields (httpOptions.*, retryConfig.*, and the remaining rateLimiting.* options) are correctly implemented and match their descriptions.
🤖 Prompt for AI Agents
In docs/guides/mcp/server-catalog.md around lines 411 to 534, the rate-limiting
table is missing the requestsPerHour option; update the HTTP Transport
Configuration Options table to add a row for `rateLimiting.requestsPerHour` with
Type `number` and Description `Rate limit per hour (optional)` so the docs match
the MCPServerInfo implementation.
Comment on lines
+28
to
+35
| - **HTTP/Streamable HTTP Transport for MCP** – Connect to remote MCP servers via HTTP with authentication headers, retry logic, and rate limiting. → [HTTP Transport Guide](MCP-HTTP-TRANSPORT.md) | ||
| - 🧠 **Gemini 3 Preview Support** - Full support for gemini-3-flash-preview and gemini-3-pro-preview with extended thinking capabilities | ||
| - **Structured Output with Zod Schemas** – Type-safe JSON generation with automatic validation using `schema` + `output.format: "json"` in `generate()`. → [Structured Output Guide](features/structured-output.md) | ||
| - **CSV File Support** – Attach CSV files to prompts for AI-powered data analysis with auto-detection. → [CSV Guide](features/multimodal-chat.md#csv-file-support) | ||
| - **PDF File Support** – Process PDF documents with native visual analysis for Vertex AI, Anthropic, Bedrock, AI Studio. → [PDF Guide](features/pdf-support.md) | ||
| - **LiteLLM Integration** – Access 100+ AI models from all major providers through unified interface. → [Setup Guide](LITELLM-INTEGRATION.md) | ||
| - **SageMaker Integration** – Deploy and use custom trained models on AWS infrastructure. → [Setup Guide](SAGEMAKER-INTEGRATION.md) | ||
| - **OpenRouter Integration** – Access 300+ models from OpenAI, Anthropic, Google, Meta, and more through a single unified API. → [Setup Guide](getting-started/providers/openrouter.md) |
There was a problem hiding this comment.
Minor grammar fix: use hyphen for compound adjective.
Line 34 should use "custom-trained" (hyphenated) as a compound adjective modifying "models".
🔎 Suggested fix
-- **SageMaker Integration** – Deploy and use custom trained models on AWS infrastructure. → [Setup Guide](SAGEMAKER-INTEGRATION.md)
+- **SageMaker Integration** – Deploy and use custom-trained models on AWS infrastructure. → [Setup Guide](SAGEMAKER-INTEGRATION.md)📝 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.
Suggested change
| - **HTTP/Streamable HTTP Transport for MCP** – Connect to remote MCP servers via HTTP with authentication headers, retry logic, and rate limiting. → [HTTP Transport Guide](MCP-HTTP-TRANSPORT.md) | |
| - 🧠 **Gemini 3 Preview Support** - Full support for gemini-3-flash-preview and gemini-3-pro-preview with extended thinking capabilities | |
| - **Structured Output with Zod Schemas** – Type-safe JSON generation with automatic validation using `schema` + `output.format: "json"` in `generate()`. → [Structured Output Guide](features/structured-output.md) | |
| - **CSV File Support** – Attach CSV files to prompts for AI-powered data analysis with auto-detection. → [CSV Guide](features/multimodal-chat.md#csv-file-support) | |
| - **PDF File Support** – Process PDF documents with native visual analysis for Vertex AI, Anthropic, Bedrock, AI Studio. → [PDF Guide](features/pdf-support.md) | |
| - **LiteLLM Integration** – Access 100+ AI models from all major providers through unified interface. → [Setup Guide](LITELLM-INTEGRATION.md) | |
| - **SageMaker Integration** – Deploy and use custom trained models on AWS infrastructure. → [Setup Guide](SAGEMAKER-INTEGRATION.md) | |
| - **OpenRouter Integration** – Access 300+ models from OpenAI, Anthropic, Google, Meta, and more through a single unified API. → [Setup Guide](getting-started/providers/openrouter.md) | |
| - **HTTP/Streamable HTTP Transport for MCP** – Connect to remote MCP servers via HTTP with authentication headers, retry logic, and rate limiting. → [HTTP Transport Guide](MCP-HTTP-TRANSPORT.md) | |
| - 🧠 **Gemini 3 Preview Support** - Full support for gemini-3-flash-preview and gemini-3-pro-preview with extended thinking capabilities | |
| - **Structured Output with Zod Schemas** – Type-safe JSON generation with automatic validation using `schema` + `output.format: "json"` in `generate()`. → [Structured Output Guide](features/structured-output.md) | |
| - **CSV File Support** – Attach CSV files to prompts for AI-powered data analysis with auto-detection. → [CSV Guide](features/multimodal-chat.md#csv-file-support) | |
| - **PDF File Support** – Process PDF documents with native visual analysis for Vertex AI, Anthropic, Bedrock, AI Studio. → [PDF Guide](features/pdf-support.md) | |
| - **LiteLLM Integration** – Access 100+ AI models from all major providers through unified interface. → [Setup Guide](LITELLM-INTEGRATION.md) | |
| - **SageMaker Integration** – Deploy and use custom-trained models on AWS infrastructure. → [Setup Guide](SAGEMAKER-INTEGRATION.md) | |
| - **OpenRouter Integration** – Access 300+ models from OpenAI, Anthropic, Google, Meta, and more through a single unified API. → [Setup Guide](getting-started/providers/openrouter.md) |
🧰 Tools
🪛 LanguageTool
[grammar] ~34-~34: Use a hyphen to join words.
Context: ...er Integration** – Deploy and use custom trained models on AWS infrastructure. → ...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
In docs/index.md around lines 28 to 35, the phrase "custom trained models" on
line 34 should be hyphenated as "custom-trained models"; update that compound
adjective to use a hyphen so it reads "Deploy and use custom-trained models on
AWS infrastructure." and keep the existing link intact.
Comment on lines
+1526
to
+1600
| ### Adding Remote HTTP MCP Servers | ||
|
|
||
| Connect to remote MCP servers via HTTP transport with authentication, retry, and rate limiting: | ||
|
|
||
| ```typescript | ||
| import { NeuroLink } from "@juspay/neurolink"; | ||
|
|
||
| const neurolink = new NeuroLink(); | ||
|
|
||
| // Add HTTP MCP server with full configuration | ||
| await neurolink.addMCPServer("remote-api", { | ||
| transport: "http", | ||
| url: "https://api.example.com/mcp", | ||
| headers: { | ||
| Authorization: "Bearer YOUR_API_TOKEN", | ||
| "X-Custom-Header": "value", | ||
| }, | ||
| httpOptions: { | ||
| connectionTimeout: 30000, | ||
| requestTimeout: 60000, | ||
| idleTimeout: 120000, | ||
| keepAliveTimeout: 30000, | ||
| }, | ||
| retryConfig: { | ||
| maxAttempts: 3, | ||
| initialDelay: 1000, | ||
| maxDelay: 30000, | ||
| backoffMultiplier: 2, | ||
| }, | ||
| rateLimiting: { | ||
| requestsPerMinute: 60, | ||
| maxBurst: 10, | ||
| useTokenBucket: true, | ||
| }, | ||
| }); | ||
|
|
||
| // Add HTTP server with OAuth 2.1 | ||
| await neurolink.addMCPServer("oauth-api", { | ||
| transport: "http", | ||
| url: "https://api.enterprise.com/mcp", | ||
| auth: { | ||
| type: "oauth2", | ||
| oauth: { | ||
| clientId: "your-client-id", | ||
| clientSecret: "your-client-secret", | ||
| authorizationUrl: "https://auth.provider.com/authorize", | ||
| tokenUrl: "https://auth.provider.com/token", | ||
| redirectUrl: "http://localhost:8080/callback", | ||
| scope: "mcp:read mcp:write", | ||
| usePKCE: true, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| // Use the remote server's tools in AI generation | ||
| const result = await neurolink.generate({ | ||
| input: { text: "Use the remote API to perform analysis" }, | ||
| provider: "google-ai", | ||
| }); | ||
| ``` | ||
|
|
||
| **HTTP Configuration Options:** | ||
|
|
||
| | Option | Type | Description | | ||
| | -------------- | ------ | ------------------------------------------- | | ||
| | `transport` | string | Must be `"http"` for HTTP transport | | ||
| | `url` | string | Remote MCP endpoint URL | | ||
| | `headers` | object | Custom HTTP headers | | ||
| | `httpOptions` | object | Connection timeout settings | | ||
| | `retryConfig` | object | Retry with exponential backoff | | ||
| | `rateLimiting` | object | Rate limiting configuration | | ||
| | `auth` | object | Authentication (OAuth 2.1, Bearer, API Key) | | ||
|
|
||
| See [MCP HTTP Transport Guide](./MCP-HTTP-TRANSPORT.md) for complete documentation. | ||
|
|
There was a problem hiding this comment.
Remove duplicate documentation section.
The AI summary notes that this "Adding Remote HTTP MCP Servers" section appears twice in the diff. Please remove the duplicate to maintain documentation clarity and avoid confusion.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~1596-~1596: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... | | rateLimiting | object | Rate limiting configuration | | `auth...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🤖 Prompt for AI Agents
In docs/SDK-CUSTOM-TOOLS.md around lines 1526 to 1600 there is a duplicated
"Adding Remote HTTP MCP Servers" section (same examples, table and links
repeated); remove the redundant copy so the section appears only once, keep the
most complete/updated instance (retain code examples, HTTP Configuration Options
table and the MCP-HTTP-TRANSPORT.md link), verify surrounding headings flow
correctly after deletion and run a quick markdown lint or preview to ensure no
leftover duplicate anchors or broken references.
Comment on lines
+134
to
+137
| async clearAll(): Promise<void> { | ||
| this.tokens.clear(); | ||
| await this.saveToFile(); | ||
| } |
There was a problem hiding this comment.
clearAll() may overwrite file without loading existing tokens.
Unlike other mutation methods (saveTokens, deleteTokens, hasTokens), clearAll() doesn't call loadTokens() first. If called before any other operation, it will overwrite an existing token file with an empty object without ever loading its contents.
Consider adding await this.loadTokens(); before clearing, or document this as intentional behavior.
🔎 Proposed fix
async clearAll(): Promise<void> {
+ await this.loadTokens();
this.tokens.clear();
await this.saveToFile();
}📝 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.
Suggested change
| async clearAll(): Promise<void> { | |
| this.tokens.clear(); | |
| await this.saveToFile(); | |
| } | |
| async clearAll(): Promise<void> { | |
| await this.loadTokens(); | |
| this.tokens.clear(); | |
| await this.saveToFile(); | |
| } |
🤖 Prompt for AI Agents
In src/lib/mcp/auth/tokenStorage.ts around lines 134 to 137, clearAll() clears
tokens and immediately saves to file without loading existing tokens first;
update the method to call await this.loadTokens() before this.tokens.clear() so
it loads current state prior to mutating and saving (or alternatively document
that clearAll intentionally overwrites without loading if that behavior is
desired).
Comment on lines
+56
to
+80
| /** | ||
| * Refill tokens based on elapsed time since last refill | ||
| * Tokens are added at the configured refillRate (tokens per second) | ||
| */ | ||
| private refillTokens(): void { | ||
| const now = Date.now(); | ||
| const elapsedMs = now - this.lastRefill; | ||
| const elapsedSeconds = elapsedMs / 1000; | ||
|
|
||
| // Calculate tokens to add based on elapsed time and refill rate | ||
| const tokensToAdd = elapsedSeconds * this.config.refillRate; | ||
|
|
||
| if (tokensToAdd >= 1) { | ||
| // Only refill if at least one token should be added | ||
| const previousTokens = this.tokens; | ||
| this.tokens = Math.min(this.config.maxBurst, this.tokens + tokensToAdd); | ||
| this.lastRefill = now; | ||
|
|
||
| if (this.tokens > previousTokens) { | ||
| mcpLogger.debug( | ||
| `[HTTPRateLimiter] Refilled tokens: ${previousTokens.toFixed(2)} -> ${this.tokens.toFixed(2)} (+${tokensToAdd.toFixed(2)})`, | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Tighten HTTPRateLimiter behavior/docs (queue bound & refillRate edge cases)
Two small inconsistencies worth addressing:
-
Doc vs implementation mismatch
- The JSDoc on
acquirementions@throws Error if the wait queue is too long, but there is no queue-length check and the method never throws based on queue size. Either implement a simple bound (e.g.,if (this.waitQueue.length >= MAX_QUEUE) reject(...)) or update the comment to avoid promising an exception path that does not exist.
- The JSDoc on
-
Defensive guard for invalid
refillRateprocessQueuecomputeswaitTimeMsas(tokensNeeded / this.config.refillRate) * 1000. IfrefillRateis set to0or a negative value viaupdateConfig, this becomesInfinity/NaNand results in an effectively unbounded sleep.- Consider clamping
refillRateto a small positive minimum on construction/update (e.g.,this.config.refillRate = Math.max(this.config.refillRate, 0.001)) or early-returning with a log whenrefillRate <= 0.
These are minor and non‑blocking but will make the limiter more robust and its docs accurate.
Example minimal fixes
- /**
- * Acquire a token, waiting if necessary
- * This is the primary method for rate-limited operations
- *
- * @returns Promise that resolves when a token is acquired
- * @throws Error if the wait queue is too long
- */
+ /**
+ * Acquire a token, waiting if necessary.
+ * This is the primary method for rate-limited operations.
+ *
+ * @returns Promise that resolves when a token is acquired
+ */
async acquire(): Promise<void> {
@@
updateConfig(config: Partial<RateLimitConfig>): void {
- Object.assign(this.config, config);
+ Object.assign(this.config, config);
+ if (this.config.refillRate <= 0) {
+ mcpLogger.warn(
+ "[HTTPRateLimiter] Invalid refillRate <= 0; clamping to 0.001",
+ );
+ this.config.refillRate = 0.001;
+ }
mcpLogger.info(`[HTTPRateLimiter] Configuration updated:`, config);
}Also applies to: 89-107, 119-149, 261-276, 296-306
Comment on lines
+15
to
+21
| export const DEFAULT_HTTP_RETRY_CONFIG: HTTPRetryConfig = { | ||
| maxAttempts: 3, | ||
| initialDelay: 1000, | ||
| maxDelay: 30000, | ||
| backoffMultiplier: 2, | ||
| retryableStatusCodes: [408, 429, 500, 502, 503, 504], | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for system-wide retry/backoff constants
rg -n "DEFAULT.*RETRY|DEFAULT.*BACKOFF|DEFAULT.*DELAY" --type ts -g '!node_modules' -g '!*.test.ts' -C2Repository: juspay/neurolink
Length of output: 7132
🏁 Script executed:
# Check what calculateBackoffDelay references and where it's defined
rg -n "calculateBackoffDelay" --type ts -g '!node_modules' -C3Repository: juspay/neurolink
Length of output: 6697
🏁 Script executed:
# Search for withTimeout utility to verify if it should be used
rg -n "withTimeout" --type ts -g '!node_modules' -C3Repository: juspay/neurolink
Length of output: 5602
🏁 Script executed:
# Check the httpRetryHandler file to see actual usage and any ErrorFactory/withTimeout
cat -n src/lib/mcp/httpRetryHandler.tsRepository: juspay/neurolink
Length of output: 7273
Align DEFAULT_HTTP_RETRY_CONFIG with SYSTEM_LIMITS constants for consistency.
The configuration values (maxAttempts: 3, initialDelay: 1000ms, maxDelay: 30000ms, backoffMultiplier: 2) match SYSTEM_LIMITS defaults perfectly. However, unlike DEFAULT_RETRY_CONFIG in src/lib/utils/retryHandler.ts which references SYSTEM_LIMITS constants directly, this file hardcodes the values. Import and use SYSTEM_LIMITS constants to maintain consistency across the codebase and avoid value duplication.
🤖 Prompt for AI Agents
In src/lib/mcp/httpRetryHandler.ts around lines 15 to 21, the
DEFAULT_HTTP_RETRY_CONFIG is hardcoding values that duplicate SYSTEM_LIMITS
defaults; import SYSTEM_LIMITS from the same module used by DEFAULT_RETRY_CONFIG
(e.g., src/lib/utils/retryHandler.ts's SYSTEM_LIMITS export) and replace
maxAttempts, initialDelay, maxDelay, and backoffMultiplier with the
corresponding SYSTEM_LIMITS constants (use the exact constant names used
elsewhere), and add the import at the top of the file so the config references
SYSTEM_LIMITS instead of literal numbers.
Comment on lines
+57
to
+58
| // Tool system types are exported directly from tools.js via index.ts | ||
| // Do not re-export here to avoid duplicate export conflicts |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && git ls-files | head -20Repository: juspay/neurolink
Length of output: 128
🏁 Script executed:
# First, let's look at the sdkTypes.ts file
cat -n src/lib/types/sdkTypes.ts | head -100Repository: juspay/neurolink
Length of output: 3203
🏁 Script executed:
# Check if GenerateResult is used elsewhere in the codebase
rg -n "GenerateResult" --type=ts -B2 -A2 | head -50Repository: juspay/neurolink
Length of output: 3152
🏁 Script executed:
# Check index.ts to understand export structure
cat -n src/lib/index.ts | head -60Repository: juspay/neurolink
Length of output: 2036
🏁 Script executed:
# Look for any backward-compatible re-exports or aliases
rg -n "export.*GenerateResult\|export.*GenerateApiResult" --type=tsRepository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Check if there's a package.json with exports field (indicates public API)
cat -n package.json | grep -A 20 '"exports"'Repository: juspay/neurolink
Length of output: 777
🏁 Script executed:
# Check if sdkTypes is explicitly part of the public API or internal
rg -n "sdkTypes" README.md CHANGELOG.md docs/ --type=mdRepository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Look at the actual generateTypes definition to understand the type
cat -n src/lib/types/generateTypes.ts | head -80Repository: juspay/neurolink
Length of output: 3209
🏁 Script executed:
# Check if there's a CHANGELOG or version info indicating major release
cat -n CHANGELOG.md 2>/dev/null | head -50 || echo "No CHANGELOG found"Repository: juspay/neurolink
Length of output: 2675
🏁 Script executed:
# Check package.json version
cat package.json | grep '"version"'Repository: juspay/neurolink
Length of output: 82
Export renaming breaks the public /types API entry point.
The GenerateResult type is renamed to GenerateApiResult in sdkTypes.ts, which is explicitly exported as a public API path ("./types") in package.json. Consumers importing from neurolink/types will break on this change. While the main entry point (neurolink) maintains backward compatibility, this creates an inconsistent public API contract in a minor version release.
Add a backward-compatible re-export in sdkTypes.ts:
export type { GenerateApiResult };
export type { GenerateApiResult as GenerateResult }; // Backward compatibilityAlternatively, bump to a major version (9.0.0) if this breaking change is intentional.
🤖 Prompt for AI Agents
In src/lib/types/sdkTypes.ts around lines 57-58, the type GenerateResult was
renamed to GenerateApiResult which breaks the public "/types" API; restore
backward compatibility by adding a re-export that aliases GenerateApiResult back
to GenerateResult (i.e., export the new name and also export it as the old
name), or if the rename is intended to be breaking, update the package version
to a major bump (9.0.0) and document the breaking change.
Comment on lines
+764
to
+768
| trackingServer.close(); | ||
|
|
||
| // Should have made at least one request | ||
| expect(requestCount).toBeGreaterThanOrEqual(0); | ||
| }); |
There was a problem hiding this comment.
Weak assertion always passes.
The assertion expect(requestCount).toBeGreaterThanOrEqual(0) will always pass since requestCount is initialized to 0. This doesn't actually verify retry behavior. Consider asserting a specific expected request count.
🔎 Suggested fix
- // Should have made at least one request
- expect(requestCount).toBeGreaterThanOrEqual(0);
+ // Should have made at least one request for testConnection
+ expect(requestCount).toBeGreaterThanOrEqual(1);📝 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.
Suggested change
| trackingServer.close(); | |
| // Should have made at least one request | |
| expect(requestCount).toBeGreaterThanOrEqual(0); | |
| }); | |
| trackingServer.close(); | |
| // Should have made at least one request for testConnection | |
| expect(requestCount).toBeGreaterThanOrEqual(1); | |
| }); |
🤖 Prompt for AI Agents
In test/integration/mcp/httpTransportIntegration.test.ts around lines 764 to
768, the assertion `expect(requestCount).toBeGreaterThanOrEqual(0)` is too weak
(always true); update it to assert actual retry behavior — either expect at
least one request by using `toBeGreaterThan(0)` or, better, compute the expected
number of requests from the retry configuration (e.g. expectedRequests =
configuredRetries + 1) and assert `expect(requestCount).toBe(expectedRequests)`
so the test fails when retry logic changes.
Contributor
|
🎉 This PR is included in version 8.29.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
8 of 10 tasks
Merged
6 tasks done
5 of 8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Address Code Review Comments
Addressing reviewer feedback to follow project conventions:
Changes Made
1. Documentation Files (memory-bank/)
interfacedeclarations totypein systemPatterns.mdinterfacedeclarations totypein techContext.md2. Centralized Type Definitions (src/lib/types/mcpTypes.ts)
PKCEChallengetype for OAuth 2.1 PKCE flowTokenResponsetype for OAuth token responsesRateLimiterStatstype for rate limiter monitoring3. Source Files
src/lib/mcp/auth/oauthClientProvider.tsto import PKCEChallenge and TokenResponse from mcpTypes.tssrc/lib/mcp/httpRateLimiter.tsto import RateLimiterStats from mcpTypes.ts4. Test Files
test/types/httpTransportTest.tsfor test-specific typestest/integration/mcp/httpTransportIntegration.test.tsto import from centralized typestest/unit/mcp/httpRateLimiter.test.tsto import from centralized typestest/unit/mcp/httpRetryHandler.test.tsto import from centralized typesProject Conventions Followed
interfacedeclarations - onlytypedeclarationssrc/lib/types/directorytest/types/directoryOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.