fix(mcp): fix HTTP MCP server health check and connection test #1375
Conversation
Summary of ChangesHello @mateuszruszkowski, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses critical issues with HTTP MCP server health checks and connection testing by modernizing the communication protocol. It transitions from simple GET requests to more robust POST requests with a complete MCP initialize payload, ensuring broader compatibility with various MCP server implementations, including those that previously returned 400 Bad Request errors. Additionally, it improves support for SSE-based servers, enhances debugging capabilities through detailed logging, and refines error reporting, leading to more reliable and informative server interactions. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
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. 📝 WalkthroughWalkthroughSwitches MCP health checks and connection tests from GET to POST with a standardized initialize payload; adds MCP client metadata constants, header redaction, SSE-aware response reading with bounded reads, MCP JSON-RPC response typing, and improved diagnostic/error-body logging and parsing; adds i18n support and mappings for MCP messages in the renderer. Changes
Sequence Diagram(s)sequenceDiagram
participant Frontend as Frontend (IPC main)
participant HTTP as HTTP Layer
participant MCP as MCP Server
rect rgba(200,150,255,0.5)
Note over Frontend,MCP: Health check (POST initialize)
Frontend->>HTTP: POST /... with createMcpInitRequest (protocolVersion, roots.listChanged, clientInfo)
HTTP->>MCP: Forward request
MCP-->>HTTP: Response (status, headers, body/stream)
HTTP-->>Frontend: Return response
alt status 200 OK
Frontend->>Frontend: Log status, redact headers, parse body
else non-OK
Frontend->>Frontend: readResponseWithSSESupport, truncate/log error body
end
end
rect rgba(150,200,255,0.5)
Note over Frontend,MCP: Connection test (SSE detection + JSON fallback)
Frontend->>HTTP: POST test with Accept: text/event-stream and initialize payload
HTTP->>MCP: Forward request
MCP-->>HTTP: SSE stream or JSON response
HTTP-->>Frontend: Return headers + body/stream
alt Content-Type == text/event-stream
Frontend->>Frontend: readResponseWithSSESupport -> parse "data:" lines as JSON
alt initialize result found
Frontend->>Frontend: Report success
else
Frontend->>Frontend: Fallback: call tools.list for verification
end
else JSON response
Frontend->>Frontend: Parse JSON, verify initialize result
end
Frontend->>Frontend: Log diagnostics and return connection result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request effectively addresses the problem of HTTP 400 errors with MCP servers that do not support GET requests by switching to POST with a proper MCP initialize request. It also improves logging for debugging connection issues and handles text/event-stream for SSE-based servers. The changes enhance the robustness and debuggability of the MCP health check and connection test functions. However, there are some areas for improvement regarding logging practices for sensitive information, error handling, and type safety.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/ipc-handlers/mcp-handlers.ts`:
- Around line 377-378: The code currently logs raw responseText (variable
responseText) at info level via appLog.info which may expose sensitive data;
change the call to appLog.debug and avoid logging full raw content—either log a
safely truncated/redacted preview (e.g., first N chars with redaction) or
replace the text with a placeholder if sensitive, and ensure the logging line
using responseText.substring(...) is updated to use appLog.debug and a sanitized
preview instead of direct raw output.
- Around line 312-313: The handlers currently log full header values via
appLog.info('[MCP Test] Headers:', JSON.stringify(headers)), which can expose
sensitive tokens; update the MCP test logging to redact sensitive header values
before logging (e.g., replace values for common auth keys like Authorization,
Cookie, X-Api-Key, etc.) or log only header names/keys instead of values;
implement or reuse a helper (e.g., redactHeaders or the same redaction logic
used in checkHttpHealth) and call it where appLog.info logs headers for
server.url and headers so no plaintext secrets are written.
- Around line 108-109: The logs currently output full headers (see
appLog.info('[MCP Health] Checking HTTP server:', server.url) and
appLog.info('[MCP Health] Headers:', JSON.stringify(headers))), which can leak
sensitive tokens; add a header-sanitization step (e.g., create a redactHeaders
or sanitizeHeaders function) that strips or masks known sensitive keys
(Authorization, Cookie, X-Api-Key, etc.) or only whitelists safe headers, then
replace the direct JSON.stringify(headers) call with
JSON.stringify(redactHeaders(headers)) so server.url is still logged but
sensitive values are redacted before calling appLog.info.
- Around line 380-406: The SSE detection is too permissive and hides parse
errors; change the if-condition to only treat the response as SSE when
contentType.includes('text/event-stream') OR the responseText clearly looks like
an SSE stream (e.g., startsWith('event:') or contains '\ndata:'), not merely
startsWith('data:'), and update the JSON parse failure branch in the dataLines
handling to emit a warning with details (use appLog.warn and include server.id,
responseTime and the raw responseText or jsonStr) instead of silently returning
success; still you may return a success result for confirmed SSE transports but
ensure the warning is logged when parsing fails (refer to contentType,
responseText, dataLines, jsonStr, appLog, server.id, responseTime).
- Around line 413-420: Remove the redundant if (response.ok) conditional in the
non-JSON 200 response branch and return the success payload directly; locate the
block that currently returns { serverId: server.id, success: true, message:
'Server responded (non-JSON)', responseTime } (the branch handling a 200
non-JSON/SSE response) and simplify it by eliminating the extra response.ok
check so control flow is clear and unreachable checks are gone.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟠 NEEDS REVISION
🟠 Needs revision - 5 issue(s) require attention.
Branch is out of date with base branch. Update branch first - if no conflicts arise, you can merge. If merge conflicts arise, resolve them and run follow-up review again. 5 issue(s) must be addressed (2 required, 3 recommended). 1 non-blocking suggestion(s) to consider.
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Low | Based on lines changed |
| Security Impact | Medium | Based on security findings |
| Scope Coherence | Good | Based on structural review |
🚨 Blocking Issues (Must Fix)
- Branch Out of Date: PR branch is behind the base branch and needs to be updated
Findings Summary
- High: 2 issue(s)
- Medium: 3 issue(s)
- Low: 1 issue(s)
Generated by Auto Claude PR Review
Findings (6 selected of 6 total)
🟠 [afbce1ccf241] [HIGH] [Potential] Sensitive authentication headers logged to persistent file
📁 apps/frontend/src/main/ipc-handlers/mcp-handlers.ts:109
The code logs the full headers object which may contain authentication tokens. Lines 104-106 merge server.headers (which can contain 'Authorization: Bearer ...' tokens) into the headers object, then line 109 logs the entire headers JSON to persistent log files. These logs persist on disk (~/Library/Logs/Auto-Claude/ on macOS) and can be included in debug reports, potentially exposing credentials.
Suggested fix:
Filter out sensitive headers before logging: const safeHeaders = { ...headers }; delete safeHeaders['Authorization']; delete safeHeaders['authorization']; delete safeHeaders['x-api-key']; appLog.info('[MCP Health] Headers:', JSON.stringify(safeHeaders));
🟠 [ce07adfe60d2] [HIGH] [Potential] Sensitive authentication headers logged to persistent file (duplicate)
📁 apps/frontend/src/main/ipc-handlers/mcp-handlers.ts:313
Same vulnerability as sec-1 but in testHttpConnection function. Line 313 logs the full headers object including any authentication tokens from server.headers to persistent log files.
Suggested fix:
Apply same fix as sec-1 - filter sensitive headers before logging.
🔵 [274dbd5b86ed] [LOW] [Potential] Redundant response.ok check contains dead code
📁 apps/frontend/src/main/ipc-handlers/mcp-handlers.ts:414
Line 344 checks if (!response.ok) and returns early for all non-OK responses. Therefore, by line 414, response.ok is guaranteed to be true. The if (response.ok) check is redundant and the else branch (lines 422-428) is dead code that can never execute.
Suggested fix:
Remove the redundant condition: simply return the success object unconditionally since we know response.ok is true at this point.
🟡 [5948d4dfae46] [MEDIUM] [Potential] Permissive success for unparseable SSE responses may mask issues
📁 apps/frontend/src/main/ipc-handlers/mcp-handlers.ts:391
The code returns success: true in two questionable cases: (1) When SSE data lines exist but JSON parsing fails (lines 388-396), and (2) When SSE response has no data lines at all (lines 398-405). This could mask real MCP protocol issues - a user sees 'Connected successfully' but may experience failures when actually using the server.
Suggested fix:
Consider returning a warning message that indicates partial verification: 'Server responded (SSE format, MCP protocol not verified)' to alert users that full protocol compliance wasn't confirmed.
🟡 [17c4e6e33ca0] [MEDIUM] [Potential] Duplicated initRequest object definition (DRY violation)
📁 apps/frontend/src/main/ipc-handlers/mcp-handlers.ts:112
The initRequest object is defined identically in two places: lines 112-126 in checkHttpHealth() and lines 316-330 in testHttpConnection(). Both have identical jsonrpc, id, method, protocolVersion, capabilities, and clientInfo. A third variant exists at lines 563-575 with a different client name. This violates DRY and means protocol changes must be updated in multiple places.
Suggested fix:
Extract to a shared factory function: function createMcpInitRequest() { return { jsonrpc: '2.0', id: 1, method: 'initialize', params: { ... } }; }
🟡 [43bec3ce74df] [MEDIUM] [Potential] Response body logged may contain sensitive data
📁 apps/frontend/src/main/ipc-handlers/mcp-handlers.ts:378
Line 378 logs the first 500 characters of the response text. While truncated, MCP server responses could contain sensitive data like tokens or API keys in some configurations.
Suggested fix:
Consider removing this logging in production or making it debug-level only: if (process.env.NODE_ENV === 'development') { appLog.debug(...); }
This review was generated by Auto Claude.
mateuszruszkowski
force-pushed
the
fix/mcp-http-health-check
branch
2 times, most recently
from
94e344f to
cc647ac
Compare
AndyMik90
force-pushed
the
develop
branch
2 times, most recently
from
67a743f to
e83e445
Compare
mateuszruszkowski
force-pushed
the
fix/mcp-http-health-check
branch
from
cc647ac to
465fe58
Compare
mateuszruszkowski
force-pushed
the
fix/mcp-http-health-check
branch
from
465fe58 to
f6ba70d
Compare
- Change health check from GET to POST with MCP initialize request (many MCP servers including Z.AI don't support GET) - Add text/event-stream to Accept header for SSE-based servers - Add detailed logging for debugging connection issues - Include proper capabilities in initialize request Fixes HTTP 400 Bad Request errors when testing Z.AI and other HTTP-based MCP servers. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Security improvements: - Add redactSensitiveHeaders() function to sanitize auth tokens, API keys, and cookies before logging - Change sensitive data logging from appLog.info to appLog.debug level - Redact Authorization, X-API-Key, Cookie, X-Auth-Token, Bearer headers Code quality improvements: - Extract MCP_CLIENT_NAME and MCP_CLIENT_VERSION constants for consistency - Define McpJsonRpcResponse interface instead of using 'any' type - Add debug logging in empty catch blocks for better troubleshooting - Use optional chaining (data?.error, data?.result) for null safety SSE detection improvements: - Refine SSE detection logic to require content-type hint when response starts with 'data:' to reduce false positives - Change SSE parse failure from appLog.info to appLog.warn with context - Add warning when server responds with non-JSON format (may not be MCP compliant) Code cleanup: - Remove redundant response.ok check (already verified at line 344) Addresses review comments from: - Gemini Code Assist (5 high priority, 6 medium priority) - CodeRabbit (5 issues/nitpicks) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/frontend/src/main/ipc-handlers/mcp-handlers.ts (3)
106-110: Use platform abstraction function instead of directprocess.platformcheck.Per coding guidelines, use
isWindows()from the platform abstraction module instead of checkingprocess.platformdirectly.♻️ Suggested fix
+import { isWindows } from '../platform'; // adjust import path as needed function areArgsSafe(args: string[] | undefined): boolean { if (!args || args.length === 0) return true; // Check for dangerous interpreter flags if (args.some(arg => DANGEROUS_FLAGS.has(arg))) return false; // On Windows with shell: true, check for shell metacharacters that could enable injection - if (process.platform === 'win32') { + if (isWindows()) { if (args.some(arg => SHELL_METACHARACTERS.some(char => arg.includes(char)))) { return false; } } return true; }
276-279: Use platform abstraction function instead of directprocess.platformcheck.Per coding guidelines, use
isWindows()from the platform abstraction module.♻️ Suggested fix
- const command = process.platform === 'win32' ? 'where' : 'which'; + const command = isWindows() ? 'where' : 'which';
591-596: Use platform abstraction function instead of directprocess.platformcheck.Per coding guidelines, use
isWindows()from the platform abstraction module.♻️ Suggested fix
// On Windows, use shell: true to properly handle .cmd/.bat scripts like npx const proc = spawn(server.command!, args, { stdio: ['pipe', 'pipe', 'pipe'], timeout: 15000, // OS-level timeout for reliable process termination - shell: process.platform === 'win32', // Required for Windows to run npx.cmd + shell: isWindows(), // Required for Windows to run npx.cmd });
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/ipc-handlers/mcp-handlers.ts`:
- Around line 394-401: The block in mcp-handlers.ts that logs the fetched
errorBody currently uses appLog.error (inside the MCP test response handling);
change that to appLog.debug so detailed response bodies (which may contain
sensitive data) are only emitted at debug level, and ensure the existing
substring(0, 500) truncation remains; update the same pattern in the analogous
checkHttpHealth handler if present and keep the try/catch around response.text()
unchanged.
- Around line 199-205: The current try block logs the fetched error body at
error level using appLog.error which may expose sensitive data; change the
logging call that currently does appLog.error('[MCP Health] Error response:',
errorBody.substring(0, 500)) to use appLog.debug instead (keep the same message
and substring truncation), so the sensitive response.text() output is only
emitted at debug level; leave the existing catch behavior (appLog.debug('[MCP
Health] Failed to get error response text:', e)) unchanged.
Address CodeRabbit review comments - error response bodies may contain sensitive information, so use appLog.debug instead of appLog.error. Co-Authored-By: Claude <noreply@anthropic.com>
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/main/ipc-handlers/mcp-handlers.ts (1)
449-505: Use i18n keys for new user‑facing messages.These newly added
messagestrings are user-facing and must be i18n keys (e.g.,mcp:connection.connectedSse) with translation handled in the renderer. Please replace the hardcoded English strings accordingly. Based on learnings, only new strings should be addressed here. As per coding guidelines, use i18n translation keys.🔤 Example (apply consistently)
- message: 'Connected successfully (SSE transport)', + message: 'mcp:connection.connectedSse',
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/ipc-handlers/mcp-handlers.ts`:
- Around line 67-83: The McpJsonRpcResponse interface narrows the json-rpc "id"
to number which is incorrect per the JSON-RPC spec; update the id property on
McpJsonRpcResponse to accept string | number | null (i.e., id: string | number |
null) and adjust any dependent type usages or casts that assume a number (search
for McpJsonRpcResponse and places that read response.id) so the code safely
handles string/number/null ids.
- Around line 388-427: The response.text() call can hang on SSE streams after
the earlier clearTimeout(timeout); detect SSE via the contentType variable
(contains 'text/event-stream') and, instead of response.text(), perform a
bounded read from response.body: get a reader via response.body.getReader(),
race reader.read() against a short timeout (e.g., few seconds), build
responseText from the first chunk (or empty string if timed out), then call
reader.cancel() and close the stream; for non-SSE keep the existing
response.text() path. Update usages of responseText and ensure any temporary
per-request timeout/AbortController is cleaned up; reference contentType,
response.body, responseText, reader.read(), reader.cancel() and timeout in your
changes.
- Extract initRequest to createMcpInitRequest() factory function (DRY) - Add warning for SSE responses without data lines - Update messages to indicate MCP protocol verification status Addresses review feedback: - DRY violation: duplicated initRequest (line 112, 316) - Permissive SSE success: now logs warning and indicates partial verification Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/frontend/src/main/ipc-handlers/mcp-handlers.ts (2)
616-628: Consider usingcreateMcpInitRequest()for consistency.The command-based test builds the initialize request inline rather than using the
createMcpInitRequest()factory. While the client info is aligned via constants, using the factory would ensure the full payload (includingcapabilitiesandprotocolVersion) stays synchronized.♻️ Suggested refactor
// Send MCP initialize request - const initRequest = JSON.stringify({ - jsonrpc: '2.0', - id: 1, - method: 'initialize', - params: { - protocolVersion: '2024-11-05', - capabilities: {}, - clientInfo: { - name: MCP_CLIENT_NAME, - version: MCP_CLIENT_VERSION, - }, - }, - }) + '\n'; + const initRequest = JSON.stringify(createMcpInitRequest()) + '\n';
128-132: Pre-existing: Directprocess.platformchecks should use platform abstraction.Per coding guidelines, use platform abstraction functions like
isWindows()from the platform module instead ofprocess.platform === 'win32'. This applies to lines 128, 284, and 594.This is pre-existing code not introduced by this PR, so can be deferred to a future cleanup. Based on learnings, pre-existing issues can be addressed in separate passes.
♻️ Duplicate comments (3)
apps/frontend/src/main/ipc-handlers/mcp-handlers.ts (3)
89-91: JSON-RPCidtype is too narrow.Per the JSON-RPC 2.0 specification,
idcan be a string, number, or null. The currentnumbertype may cause type mismatches with compliant servers.♻️ Suggested fix
interface McpJsonRpcResponse { jsonrpc: string; - id: number; + id: number | string | null; result?: {
443-481: Returningsuccess: truefor unverified responses may mislead users.When the server responds but doesn't return valid MCP JSON-RPC (SSE parse failure at line 445, no data lines at line 459, non-JSON at line 478), the function returns
success: true. While the messages indicate "MCP protocol not verified", thesuccessfield is what drives UI indicators.For a "Full MCP connection test", consider returning
success: falsewith a distinct status or adding averified: booleanfield to distinguish "server reachable" from "MCP protocol confirmed".💡 Suggested approach
+ // SSE response but can't parse - reachable but not verified return { serverId: server.id, - success: true, + success: false, - message: 'Server responded (SSE format, MCP protocol not verified)', + message: 'Server reachable but MCP protocol could not be verified', + error: 'SSE response could not be parsed as MCP JSON-RPC', responseTime, };Alternatively, add a
verifiedfield to the result type to preserve backward compatibility while providing clearer status.
382-421: Potential hang when reading infinite SSE streams.The timeout is cleared at line 382 before calling
response.text()at line 420. For servers returning infinite SSE streams (valid fortext/event-stream), this call will block indefinitely with no timeout protection.Since this is a connection test, implement a bounded read for SSE responses.
🐛 Suggested fix (bounded SSE read)
clearTimeout(timeout); const responseTime = Date.now() - startTime; appLog.info('[MCP Test] Response status:', response.status, response.statusText); // ... error handling ... // Check content type - SSE servers may return event stream const contentType = response.headers.get('content-type') || ''; appLog.info('[MCP Test] Content-Type:', contentType); let data: McpJsonRpcResponse | undefined; - const responseText = await response.text(); + let responseText = ''; + + // For SSE streams, use bounded read to prevent infinite blocking + if (contentType.includes('text/event-stream') && response.body) { + const reader = response.body.getReader(); + const decoder = new TextDecoder(); + const readTimeout = setTimeout(() => reader.cancel(), 5000); + try { + const { value, done } = await reader.read(); + responseText = value ? decoder.decode(value) : ''; + } catch { + // Read cancelled or failed + } finally { + clearTimeout(readTimeout); + await reader.cancel().catch(() => {}); + } + } else { + responseText = await response.text(); + } appLog.debug('[MCP Test] Response text (first 500):', responseText.substring(0, 500));
- Add bounded SSE read to prevent hanging on infinite streams - Fix JSON-RPC id type to match spec (string | number | null) - Use createMcpInitRequest() factory in command-based test for DRY - Return success: false for unverified MCP responses to improve UX Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/main/ipc-handlers/mcp-handlers.ts (1)
587-615: Missing timeout protection ontools/listfallback request.The
tools/listfetch at line 595 doesn't have anAbortControllertimeout. If the server hangs on this request, it could block indefinitely since the original timeout was already cleared at line 458.🐛 Suggested fix
// Try to list tools as additional verification + const toolsController = new AbortController(); + const toolsTimeout = setTimeout(() => toolsController.abort(), 10000); const toolsRequest = { jsonrpc: '2.0', id: 2, method: 'tools/list', params: {}, }; - const toolsResponse = await fetch(server.url, { - method: 'POST', - headers, - body: JSON.stringify(toolsRequest), - }); + try { + const toolsResponse = await fetch(server.url, { + method: 'POST', + headers, + body: JSON.stringify(toolsRequest), + signal: toolsController.signal, + }); + clearTimeout(toolsTimeout); - let tools: string[] = []; - if (toolsResponse.ok) { - const toolsData = await toolsResponse.json(); - if (toolsData.result?.tools) { - tools = toolsData.result.tools.map((t: { name: string }) => t.name); + let tools: string[] = []; + if (toolsResponse.ok) { + const toolsData = await toolsResponse.json(); + if (toolsData.result?.tools) { + tools = toolsData.result.tools.map((t: { name: string }) => t.name); + } } - } - return { - serverId: server.id, - success: true, - message: tools.length > 0 ? `Connected successfully, ${tools.length} tools available` : 'Connected successfully', - tools, - responseTime, - }; + return { + serverId: server.id, + success: true, + message: tools.length > 0 ? `Connected successfully, ${tools.length} tools available` : 'Connected successfully', + tools, + responseTime, + }; + } catch { + clearTimeout(toolsTimeout); + // tools/list failed but initialize worked, still consider it a success + return { + serverId: server.id, + success: true, + message: 'Connected successfully', + responseTime, + }; + }
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/ipc-handlers/mcp-handlers.ts`:
- Around line 65-84: Update the SENSITIVE_HEADER_KEYS constant to include more
common auth-related header names (examples: 'set-cookie', 'proxy-authorization',
'x-access-token', 'x-refresh-token', 'access-token', 'refresh-token',
'authentication', 'api-key' (alias), 'x-amz-security-token',
'x-aws-security-token', 'oauth', 'token') and any other project-specific tokens;
keep using redactSensitiveHeaders(headers) and its case-insensitive check
(lowerKey.includes(...)) so new entries are picked up, and consider adding
explicit exact-match entries if you want to avoid accidental substring matches.
Add more common auth-related headers to SENSITIVE_HEADER_KEYS: - Standard: authorization, cookie, bearer - Tokens: token, api-key, apikey, oauth, session - AWS: x-amz-security, x-aws-security - Secrets: secret, password, credential - Auth variants: x-auth, authentication Uses substring matching so 'token' covers x-access-token, x-refresh-token, x-auth-token, etc. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/ipc-handlers/mcp-handlers.ts`:
- Around line 115-188: readResponseWithSSESupport currently creates timeout
promises via setTimeout but never clears their timers when the read completes
first, leaving lingering timers; fix by capturing each setTimeout return value
(e.g., initialTimeoutId for the initial timeoutPromise and shortTimeoutId for
each shortTimeoutPromise inside the loop) and calling clearTimeout(...) to
cancel the timer as soon as the corresponding Promise.race resolves in favor of
the reader.read() path (both for the initial race and for each additionalResult
race), ensuring you still await/handle the reader results and preserving the
existing reader.cancel() cleanup.
- Around line 520-594: Replace hardcoded user-facing MCP test messages in the
mcP handling flow (inside the logic around readResponseWithSSESupport / isSSE
handling where responses return objects typed McpJsonRpcResponse and logs via
appLog) with i18n t() calls using new keys (e.g., mcp:connection.unverified,
mcp:sse.parseError, mcp:sse.noDataLines, mcp:response.invalidJson); create the
matching locale entries for those keys (including an errors.parseError variant
that accepts an { error } interpolation) and update the returned message and
error strings to use t('mcp:...') (interpolating errorMessage where needed) so
UI strings are localized instead of hardcoded.
The timeout promises were never cleared when the read wins first. Now properly clearing timeouts after Promise.race resolves to avoid extra timers on repeated health checks. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/main/ipc-handlers/mcp-handlers.ts (1)
237-241: Pre-existing directprocess.platformcheck.Per coding guidelines, platform detection should use abstraction functions like
isWindows()from the platform module. This check (and similar ones at lines 393 and 707) pre-dates this PR but should be addressed in a follow-up to maintain consistency.
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/ipc-handlers/mcp-handlers.ts`:
- Around line 177-181: The SSE read loop currently breaks when
responseText.includes('\n\n') || responseText.includes('data:'), which can
prematurely stop on a partial 'data:' line; change the termination logic in the
loop that reads responseText to require both the 'data:' line and the SSE
terminator (i.e., ensure responseText contains 'data:' AND the '\n\n' delimiter
before breaking), or more robustly check that '\n\n' follows the most recent
'data:' occurrence; update the break condition accordingly in the SSE read loop
that references responseText.
♻️ Duplicate comments (1)
apps/frontend/src/main/ipc-handlers/mcp-handlers.ts (1)
553-594: Localize new user-facing MCP connection test messages.These hardcoded strings are displayed to users in the health status UI. Per coding guidelines, all user-facing text in the frontend must use i18n translation keys from
react-i18next:
'Server reachable but MCP protocol could not be verified''SSE response could not be parsed as MCP JSON-RPC''SSE response did not contain expected data lines''Response was not valid JSON'Create translation keys (e.g.,
mcp:connection.unverified,mcp:sse.parseError) and uset()calls. For dynamic content, use interpolation:t('mcp:errors.parseError', { error: errorMessage }).
| // Stop if we have complete SSE data lines | ||
| if (responseText.includes('\n\n') || responseText.includes('data:')) { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
SSE read termination condition may exit prematurely.
The condition responseText.includes('data:') will terminate the loop as soon as any data: prefix appears, potentially before the complete SSE message (including the trailing \n\n delimiter) is received. This could result in truncated JSON payloads.
Consider requiring both the data line and its terminator:
♻️ Suggested refinement
// Stop if we have complete SSE data lines
- if (responseText.includes('\n\n') || responseText.includes('data:')) {
+ if (responseText.includes('\n\n') || responseText.match(/data:.*\n/)) {
break;
}🤖 Prompt for AI Agents
In `@apps/frontend/src/main/ipc-handlers/mcp-handlers.ts` around lines 177 - 181,
The SSE read loop currently breaks when responseText.includes('\n\n') ||
responseText.includes('data:'), which can prematurely stop on a partial 'data:'
line; change the termination logic in the loop that reads responseText to
require both the 'data:' line and the SSE terminator (i.e., ensure responseText
contains 'data:' AND the '\n\n' delimiter before breaking), or more robustly
check that '\n\n' follows the most recent 'data:' occurrence; update the break
condition accordingly in the SSE read loop that references responseText.
- Fix SSE read termination to require complete data line (data:...\n) instead of breaking on partial 'data:' prefix - Add mcp.json translation files (en/fr) for MCP connection messages - Register 'mcp' namespace in i18n configuration - Add getMcpMessage helper in AgentTools to translate backend messages - Map known MCP backend messages to translation keys for localization Addresses: - CodeRabbit: SSE read termination may exit prematurely - CodeRabbit: Localize user-facing MCP connection test messages Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/ipc-handlers/mcp-handlers.ts`:
- Around line 37-42: MCP_CLIENT_VERSION is hard-coded; replace it so the app
reads the real runtime version (e.g., use Electron's app.getVersion() or import
package.json.version) and set MCP_CLIENT_VERSION dynamically where MCP client
info is built (referencing the MCP_CLIENT_VERSION constant and any usage in
clientInfo). Ensure the Electron import or package.json read happens in the main
process context and falls back to a sensible default if unavailable, then
propagate that value wherever MCP_CLIENT_NAME and MCP_CLIENT_VERSION are used.
In `@apps/frontend/src/renderer/components/AgentTools.tsx`:
- Around line 642-659: getMcpMessage currently misses the backend timeout text
and lets many backend strings bypass i18n; update the mapping in getMcpMessage
to use the exact backend text ('Connection timed out' instead of 'Connection
timeout'), add explicit mappings for other known backend messages (e.g.,
'Authentication failed', 'Connection refused - server may be down', 'Server not
found - check URL'), and add a startsWith branch for variants like 'Connected
successfully, X tools available' to map to a translation key with params;
finally ensure the default fallback returns a localized key (e.g.,
t('mcp:connection.unknown') or t('mcp:connection.unknown', { message })) so all
user-facing MCP messages go through t().
| /** | ||
| * MCP client name constant for consistent identification across all requests | ||
| */ | ||
| const MCP_CLIENT_NAME = 'auto-claude-client'; | ||
| const MCP_CLIENT_VERSION = '1.0.0'; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid hard‑coding the MCP client version.
1.0.0 will drift from the actual app version and can mislead diagnostics. Prefer sourcing it from Electron’s app metadata (or package.json) so clientInfo stays accurate.
♻️ Suggested refactor
-import { ipcMain } from 'electron';
+import { ipcMain, app } from 'electron';
@@
-const MCP_CLIENT_VERSION = '1.0.0';
+const MCP_CLIENT_VERSION = app.getVersion();🤖 Prompt for AI Agents
In `@apps/frontend/src/main/ipc-handlers/mcp-handlers.ts` around lines 37 - 42,
MCP_CLIENT_VERSION is hard-coded; replace it so the app reads the real runtime
version (e.g., use Electron's app.getVersion() or import package.json.version)
and set MCP_CLIENT_VERSION dynamically where MCP client info is built
(referencing the MCP_CLIENT_VERSION constant and any usage in clientInfo).
Ensure the Electron import or package.json read happens in the main process
context and falls back to a sensible default if unavailable, then propagate that
value wherever MCP_CLIENT_NAME and MCP_CLIENT_VERSION are used.
| // Map MCP backend messages to i18n keys for localization | ||
| const getMcpMessage = (message: string | undefined): string => { | ||
| if (!message) return ''; | ||
| // Map known backend messages to translation keys | ||
| const messageKeyMap: Record<string, string> = { | ||
| 'Server reachable but MCP protocol could not be verified': 'mcp:connection.unverified', | ||
| 'SSE response could not be parsed as MCP JSON-RPC': 'mcp:sse.parseError', | ||
| 'SSE response did not contain expected data lines': 'mcp:sse.noDataLines', | ||
| 'Response was not valid JSON': 'mcp:response.invalidJson', | ||
| 'Server returned error': 'mcp:connection.serverError', | ||
| 'MCP error': 'mcp:connection.mcpError', | ||
| 'Connected successfully': 'mcp:connection.success', | ||
| 'Connection timeout': 'mcp:connection.timeout', | ||
| 'Connection failed': 'mcp:connection.failed', | ||
| }; | ||
| const key = messageKeyMap[message]; | ||
| return key ? t(key) : message; | ||
| }; |
There was a problem hiding this comment.
Fix timeout mapping and cover unmapped MCP messages.
getMcpMessage maps 'Connection timeout', but the backend returns 'Connection timed out', so the timeout case won’t localize. Other backend messages (e.g., 'Authentication failed', 'Connection refused - server may be down', 'Server not found - check URL', and 'Connected successfully, X tools available') will also bypass localization. Consider aligning strings or returning structured codes + params to keep all user‑facing text localized.
💡 Minimal fix for the timeout mismatch
- 'Connection timeout': 'mcp:connection.timeout',
+ 'Connection timed out': 'mcp:connection.timeout',As per coding guidelines, ensure all user-facing MCP messages resolve through i18n keys.
📝 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.
| // Map MCP backend messages to i18n keys for localization | |
| const getMcpMessage = (message: string | undefined): string => { | |
| if (!message) return ''; | |
| // Map known backend messages to translation keys | |
| const messageKeyMap: Record<string, string> = { | |
| 'Server reachable but MCP protocol could not be verified': 'mcp:connection.unverified', | |
| 'SSE response could not be parsed as MCP JSON-RPC': 'mcp:sse.parseError', | |
| 'SSE response did not contain expected data lines': 'mcp:sse.noDataLines', | |
| 'Response was not valid JSON': 'mcp:response.invalidJson', | |
| 'Server returned error': 'mcp:connection.serverError', | |
| 'MCP error': 'mcp:connection.mcpError', | |
| 'Connected successfully': 'mcp:connection.success', | |
| 'Connection timeout': 'mcp:connection.timeout', | |
| 'Connection failed': 'mcp:connection.failed', | |
| }; | |
| const key = messageKeyMap[message]; | |
| return key ? t(key) : message; | |
| }; | |
| // Map MCP backend messages to i18n keys for localization | |
| const getMcpMessage = (message: string | undefined): string => { | |
| if (!message) return ''; | |
| // Map known backend messages to translation keys | |
| const messageKeyMap: Record<string, string> = { | |
| 'Server reachable but MCP protocol could not be verified': 'mcp:connection.unverified', | |
| 'SSE response could not be parsed as MCP JSON-RPC': 'mcp:sse.parseError', | |
| 'SSE response did not contain expected data lines': 'mcp:sse.noDataLines', | |
| 'Response was not valid JSON': 'mcp:response.invalidJson', | |
| 'Server returned error': 'mcp:connection.serverError', | |
| 'MCP error': 'mcp:connection.mcpError', | |
| 'Connected successfully': 'mcp:connection.success', | |
| 'Connection timed out': 'mcp:connection.timeout', | |
| 'Connection failed': 'mcp:connection.failed', | |
| }; | |
| const key = messageKeyMap[message]; | |
| return key ? t(key) : message; | |
| }; |
🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/components/AgentTools.tsx` around lines 642 - 659,
getMcpMessage currently misses the backend timeout text and lets many backend
strings bypass i18n; update the mapping in getMcpMessage to use the exact
backend text ('Connection timed out' instead of 'Connection timeout'), add
explicit mappings for other known backend messages (e.g., 'Authentication
failed', 'Connection refused - server may be down', 'Server not found - check
URL'), and add a startsWith branch for variants like 'Connected successfully, X
tools available' to map to a translation key with params; finally ensure the
default fallback returns a localized key (e.g., t('mcp:connection.unknown') or
t('mcp:connection.unknown', { message })) so all user-facing MCP messages go
through t().
2 participants
Closes #1422
Summary
text/event-streamto Accept header for SSE-based serversProblem
HTTP 400 Bad Request errors when testing Z.AI and other HTTP-based MCP servers that don't support GET requests.
Solution
Use POST with proper MCP initialize request for health checks, which is the correct protocol for MCP servers.
Changes
apps/frontend/src/main/ipc-handlers/mcp-handlers.ts: UpdatecheckHttpHealthfunctionTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.