fix: probe POST-only Streamable HTTP MCP servers before marking them dead - #2776
fix: probe POST-only Streamable HTTP MCP servers before marking them dead#2776whnb773 wants to merge 1 commit into
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe MCP health check supports configurable HTTP methods and request bodies. It retries failed GET probes with a JSON-RPC ChangesMCP HTTP health probing
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: 🔵 Low · up to The change enables healthy POST-only MCP servers to pass preflight checks, avoiding false unhealthy states. The PR is mergeable with owner awareness that the regression test should more fully validate fallback ordering, headers, and initialize payload fields. Sequence Diagram(s)sequenceDiagram
participant MCPHealthCheck
participant requestHttp
participant MCPServer
MCPHealthCheck->>requestHttp: Send GET probe
requestHttp->>MCPServer: Forward GET request
MCPServer-->>requestHttp: Return 404
MCPHealthCheck->>requestHttp: Send POST with initialize body
requestHttp->>MCPServer: Forward JSON-RPC initialize request
MCPServer-->>requestHttp: Return successful response
requestHttp-->>MCPHealthCheck: Return successful probe
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
…dead The preflight probe in mcp-health-check only ever sent a bare GET to the server URL. Some Streamable HTTP MCP servers route POST exclusively and answer any GET with 404 — api.telnyx.com/v2/mcp is one — so the probe failed permanently against a perfectly healthy server. 404 is not in HEALTHY_HTTP_CODES, so every probe failed, the backoff compounded to the 10-minute ceiling, and the hook blocked every tool call for that server before it left the machine while `claude mcp list` still reported it Connected. Replay a failed GET as a real JSON-RPC initialize POST and accept that as proof of life. Whitelisting 404 was the alternative, but it would mask genuine outages on every other server. Adds a regression test with a POST-only server that 404s all GETs and validates the initialize body; it fails without this change. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
|
|
||
| function requestHttp(urlString, headers, timeoutMs) { | ||
| function requestHttp(urlString, headers, timeoutMs, options = {}) { |
There was a problem hiding this comment.
The modified requestHttp function now spans roughly 53 lines, and the new async test callback at tests/hooks/mcp-health-check.test.js:1027-1105 spans roughly 79 lines. Splitting request construction and the fixture-server setup into focused helpers would satisfy the repository's under-50-line function requirement and reduce the cost of extending this probe.
File Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/hooks/mcp-health-check.js
Line: 256
Comment:
**Probe functions exceed size limit**
The modified `requestHttp` function now spans roughly 53 lines, and the new async test callback at `tests/hooks/mcp-health-check.test.js:1027-1105` spans roughly 79 lines. Splitting request construction and the fixture-server setup into focused helpers would satisfy the repository's under-50-line function requirement and reduce the cost of extending this probe.
**File Used:** AGENTS.md ([source](AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/hooks/mcp-health-check.test.js`:
- Around line 1049-1055: Strengthen the initialize request validation in the
fixture by requiring valid params.protocolVersion, params.capabilities, and
params.clientInfo in addition to jsonrpc and method. Reject missing or malformed
required fields with the existing HTTP 400 response, and only allow the fallback
to return HTTP 200 for a complete JSON-RPC initialize payload.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6539aa28-f49a-46a1-97a4-f9dd9d5d0bdd
📒 Files selected for processing (2)
scripts/hooks/mcp-health-check.jstests/hooks/mcp-health-check.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (20)
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}
📄 CodeRabbit inference engine (.cursor/rules/common-coding-style.md)
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}: Always create new objects, never mutate existing ones. Use immutable patterns to prevent hidden side effects and enable safe concurrency
Organize code into many small files (200-400 lines typical, 800 lines max) organized by feature/domain rather than by type
Always handle errors explicitly at every level and never silently swallow errors
Always validate all user input before processing at system boundaries
Use schema-based validation where available
Fail fast with clear error messages when validation fails
Never trust external data (API responses, user input, file content)
Ensure code is readable and well-named
Keep functions small (less than 50 lines)
Keep files focused (less than 800 lines)
Avoid deep nesting (more than 4 levels)
Do not use hardcoded values; use constants or configuration instead
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
No hardcoded secrets (API keys, passwords, tokens) - validate before any commit
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}: All user inputs must be validated
Enable CSRF protection on all state-changing endpoints
Verify authentication and authorization for all protected endpoints
Implement rate limiting on all endpoints to prevent abuse
Ensure error messages do not leak sensitive data in responses
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,sql}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Use parameterized queries to prevent SQL injection
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,ts,jsx,tsx,html,php,java,cs,rb,go}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Implement XSS prevention by sanitizing HTML output
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp,properties,yml,yaml,json,env,config}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
NEVER hardcode secrets in source code - ALWAYS use environment variables or a secret manager
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript-coding-style.md)
**/*.{ts,tsx,js,jsx}: Use spread operator for immutable updates in TypeScript/JavaScript instead of direct mutation
Use async/await with try-catch for error handling in TypeScript/JavaScript
Use Zod for schema-based input validation in TypeScript/JavaScript
No console.log statements in production code; use proper logging libraries instead
**/*.{ts,tsx,js,jsx}: Auto-format JavaScript/TypeScript files using Prettier after edit
Warn aboutconsole.logstatements in edited files
Check all modified files forconsole.logstatements before session ends
**/*.{ts,tsx,js,jsx}: Use the ApiResponse interface pattern with generic type parameter:interface ApiResponse<T> { success: boolean; data?: T; error?: string; meta?: { total: number; page: number; limit: number; } }
Implement custom React hooks following the pattern: export a named function with use prefix, generic type parameters, and proper useEffect cleanup for side effects
**/*.{ts,tsx,js,jsx}: Never hardcode secrets; always use environment variables for sensitive credentials like API keys
Throw an error when required environment variables are not configured to fail fast and ensure security prerequisites are metUse Playwright as the E2E testing framework for critical user flows in TypeScript/JavaScript
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: Write tests before implementation (test-driven development); target 80%+ coverage
Achieve minimum 80% test coverage across all three layers: Unit, Integration, and E2E
Use AAA structure (Arrange / Act / Assert) in tests with descriptive test names that explain behavior under test
Files:
tests/hooks/mcp-health-check.test.js
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts,jsx,tsx}: Always create new objects and never mutate in place; return new copies instead
Keep files between 200–400 lines typical, with a maximum of 800 lines
Extract helpers when a file exceeds 200 lines
Handle errors explicitly at every level; never swallow errors silently
Validate all user input before processing; use schema-based validation where available
Never trust external data (API responses, file content, query params); always validate
All user inputs must be validated and sanitized
Error messages must be scrubbed of sensitive internals
Use readable, well-named identifiers in all code
Keep functions under 50 lines
Keep files under 800 lines
Avoid nesting deeper than 4 levels
Implement comprehensive error handling in all code
Do not hardcode values; use constants or environment configuration instead
Do not use in-place mutation; always return new objects or state
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,ts,jsx,tsx,json,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not hardcode secrets, API keys, passwords, or tokens
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts}: Use parameterized queries for all database writes (no string interpolation)
Auth/authz must be checked server-side for every sensitive path
Rate limiting must be applied to all public endpoints
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
HTML output must be sanitized where applicable
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,ts,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Required environment variables must be validated at startup
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,jsx,ts,tsx,py,java,kt,go,rs,cpp,c,h,cs,rb,php}
📄 CodeRabbit inference engine (AGENTS.md)
Test-Driven — Write tests before implementation, 80%+ coverage required
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Security-First — Never compromise on security; validate all inputs
Immutability — Always create new objects, never mutate existing ones
- No hardcoded secrets (API keys, passwords, tokens)
- All user inputs validated
- Authentication/authorization verified
- Error messages don't leak sensitive data
Immutability (CRITICAL): Always create new objects, never mutate. Return new copies with changes applied.
Error handling: Handle errors at every level. Provide user-friendly messages in UI code. Log detailed context server-side. Never silently swallow errors.
Input validation: Validate all user input at system boundaries. Use schema-based validation. Fail fast with clear messages. Never trust external data.
Minimum coverage: 80%
TDD workflow (mandatory):
Commit format:<type>: <description>— Types: feat, fix, refactor, docs, test, chore, perf, ci
API response format: Consistent envelope with success indicator, data payload, error message, and pagination metadata.
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{sql,js,jsx,ts,tsx,py,java,kt,go,rs,php}
📄 CodeRabbit inference engine (AGENTS.md)
- SQL injection prevention (parameterized queries)
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{test,spec}.{js,jsx,ts,tsx,py,java,kt,go,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{js,jsx,ts,tsx,py,java,kt,go,rs}: 1. Unit tests — Individual functions, utilities, components
2. Integration tests — API endpoints, database operations
3. E2E tests — Critical user flows
Files:
tests/hooks/mcp-health-check.test.js
{package.json,*.config.js,scripts/**/*.js}
📄 CodeRabbit inference engine (CLAUDE.md)
Package manager detection should support npm, pnpm, yarn, and bun, with configuration via CLAUDE_PACKAGE_MANAGER environment variable or project config.
Files:
scripts/hooks/mcp-health-check.js
scripts/**/*.js
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure cross-platform support for Windows, macOS, and Linux via Node.js scripts in the scripts/ directory.
Files:
scripts/hooks/mcp-health-check.js
{scripts,bin}/**
⚙️ CodeRabbit configuration file
{scripts,bin}/**: Focus on command injection, unsafe subprocess usage, path traversal, SSRF, secret exposure, and missing tests for new CLI behavior.
Files:
scripts/hooks/mcp-health-check.js
🧠 Learnings (2)
📚 Learning: 2026-06-27T23:49:19.839Z
Learnt from: gaurav0107
Repo: affaan-m/ECC PR: 2373
File: tests/hooks/observe-signal-timeout.test.js:0-0
Timestamp: 2026-06-27T23:49:19.839Z
Learning: In tests under tests/hooks that require a Python runtime to run, the test should fail fast when Python isn’t available (or prerequisites aren’t met). Do not treat a missing Python runtime as test.skip, as an expected/allowed condition, or as a passing state; instead, explicitly fail (e.g., throw/return a rejected promise or use a test runner fail/expect that marks the test as failed) so reviewers can’t accidentally mask environment issues.
Applied to files:
tests/hooks/mcp-health-check.test.js
📚 Learning: 2026-07-14T03:26:12.530Z
Learnt from: thejesh23
Repo: affaan-m/ECC PR: 2517
File: tests/hooks/pre-bash-tmux-reminder.test.js:21-25
Timestamp: 2026-07-14T03:26:12.530Z
Learning: In this repository, do not flag `console.log` usage as a guideline violation in hook test files under `tests/hooks/*.test.js`. These tests intentionally use `console.log` for pass/fail output because the repo’s console-based runner (`tests/run-all.js`) is used and there is no Jest/Mocha dependency. Outside this specific hook-test path, follow the normal logging guidelines.
Applied to files:
tests/hooks/mcp-health-check.test.js
🪛 ast-grep (0.45.1)
tests/hooks/mcp-health-check.test.js
[warning] 1033-1064: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(
serverScript,
[
"const fs = require('fs');",
"const http = require('http');",
"const portFile = process.argv[2];",
"const server = http.createServer((req, res) => {",
" if (req.method !== 'POST') {",
" res.writeHead(404, { 'Content-Type': 'application/json' });",
" res.end(JSON.stringify({ error: 'not found' }));",
" return;",
" }",
" let body = '';",
" req.on('data', chunk => { body += chunk; });",
" req.on('end', () => {",
" let parsed = null;",
" try { parsed = JSON.parse(body); } catch { parsed = null; }",
" if (!parsed || parsed.jsonrpc !== '2.0' || parsed.method !== 'initialize') {",
" res.writeHead(400, { 'Content-Type': 'application/json' });",
" res.end(JSON.stringify({ error: 'expected a JSON-RPC initialize body' }));",
" return;",
" }",
" res.writeHead(200, { 'Content-Type': 'application/json' });",
" res.end(JSON.stringify({ jsonrpc: '2.0', id: parsed.id, result: {} }));",
" });",
"});",
"server.listen(0, '127.0.0.1', () => {",
" fs.writeFileSync(portFile, String(server.address().port));",
"});",
"setInterval(() => {}, 1000);"
].join('\n')
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
🔇 Additional comments (1)
scripts/hooks/mcp-health-check.js (1)
256-277: LGTM!Also applies to: 306-324, 512-524
| " let parsed = null;", | ||
| " try { parsed = JSON.parse(body); } catch { parsed = null; }", | ||
| " if (!parsed || parsed.jsonrpc !== '2.0' || parsed.method !== 'initialize') {", | ||
| " res.writeHead(400, { 'Content-Type': 'application/json' });", | ||
| " res.end(JSON.stringify({ error: 'expected a JSON-RPC initialize body' }));", | ||
| " return;", | ||
| " }", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate all required initialize payload fields.
The fixture only checks jsonrpc and method. A fallback that omits or corrupts params.protocolVersion, params.capabilities, or params.clientInfo still passes this test. Assert the complete payload contract before returning HTTP 200.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/hooks/mcp-health-check.test.js` around lines 1049 - 1055, Strengthen
the initialize request validation in the fixture by requiring valid
params.protocolVersion, params.capabilities, and params.clientInfo in addition
to jsonrpc and method. Reject missing or malformed required fields with the
existing HTTP 400 response, and only allow the fallback to return HTTP 200 for a
complete JSON-RPC initialize payload.
53de186 to
5a378fd
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/hooks/mcp-health-check.test.js`:
- Around line 1040-1057: Harden the HTTP fixture in the GET-to-POST fallback
test by counting GET requests and rejecting POST requests unless a preceding GET
has returned 404. In the POST handler, require the expected JSON/SSE
Content-Type and Accept headers, and verify Content-Length matches the received
request body before accepting the JSON-RPC initialize payload.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 05d50f4d-5ae3-46ce-9dab-b04be6abdb5b
📒 Files selected for processing (2)
scripts/hooks/mcp-health-check.jstests/hooks/mcp-health-check.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (20)
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}
📄 CodeRabbit inference engine (.cursor/rules/common-coding-style.md)
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}: Always create new objects, never mutate existing ones. Use immutable patterns to prevent hidden side effects and enable safe concurrency
Organize code into many small files (200-400 lines typical, 800 lines max) organized by feature/domain rather than by type
Always handle errors explicitly at every level and never silently swallow errors
Always validate all user input before processing at system boundaries
Use schema-based validation where available
Fail fast with clear error messages when validation fails
Never trust external data (API responses, user input, file content)
Ensure code is readable and well-named
Keep functions small (less than 50 lines)
Keep files focused (less than 800 lines)
Avoid deep nesting (more than 4 levels)
Do not use hardcoded values; use constants or configuration instead
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
No hardcoded secrets (API keys, passwords, tokens) - validate before any commit
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}: All user inputs must be validated
Enable CSRF protection on all state-changing endpoints
Verify authentication and authorization for all protected endpoints
Implement rate limiting on all endpoints to prevent abuse
Ensure error messages do not leak sensitive data in responses
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,sql}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Use parameterized queries to prevent SQL injection
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,ts,jsx,tsx,html,php,java,cs,rb,go}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Implement XSS prevention by sanitizing HTML output
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp,properties,yml,yaml,json,env,config}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
NEVER hardcode secrets in source code - ALWAYS use environment variables or a secret manager
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript-coding-style.md)
**/*.{ts,tsx,js,jsx}: Use spread operator for immutable updates in TypeScript/JavaScript instead of direct mutation
Use async/await with try-catch for error handling in TypeScript/JavaScript
Use Zod for schema-based input validation in TypeScript/JavaScript
No console.log statements in production code; use proper logging libraries instead
**/*.{ts,tsx,js,jsx}: Auto-format JavaScript/TypeScript files using Prettier after edit
Warn aboutconsole.logstatements in edited files
Check all modified files forconsole.logstatements before session ends
**/*.{ts,tsx,js,jsx}: Use the ApiResponse interface pattern with generic type parameter:interface ApiResponse<T> { success: boolean; data?: T; error?: string; meta?: { total: number; page: number; limit: number; } }
Implement custom React hooks following the pattern: export a named function with use prefix, generic type parameters, and proper useEffect cleanup for side effects
**/*.{ts,tsx,js,jsx}: Never hardcode secrets; always use environment variables for sensitive credentials like API keys
Throw an error when required environment variables are not configured to fail fast and ensure security prerequisites are metUse Playwright as the E2E testing framework for critical user flows in TypeScript/JavaScript
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: Write tests before implementation (test-driven development); target 80%+ coverage
Achieve minimum 80% test coverage across all three layers: Unit, Integration, and E2E
Use AAA structure (Arrange / Act / Assert) in tests with descriptive test names that explain behavior under test
Files:
tests/hooks/mcp-health-check.test.js
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts,jsx,tsx}: Always create new objects and never mutate in place; return new copies instead
Keep files between 200–400 lines typical, with a maximum of 800 lines
Extract helpers when a file exceeds 200 lines
Handle errors explicitly at every level; never swallow errors silently
Validate all user input before processing; use schema-based validation where available
Never trust external data (API responses, file content, query params); always validate
All user inputs must be validated and sanitized
Error messages must be scrubbed of sensitive internals
Use readable, well-named identifiers in all code
Keep functions under 50 lines
Keep files under 800 lines
Avoid nesting deeper than 4 levels
Implement comprehensive error handling in all code
Do not hardcode values; use constants or environment configuration instead
Do not use in-place mutation; always return new objects or state
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,ts,jsx,tsx,json,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not hardcode secrets, API keys, passwords, or tokens
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts}: Use parameterized queries for all database writes (no string interpolation)
Auth/authz must be checked server-side for every sensitive path
Rate limiting must be applied to all public endpoints
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
HTML output must be sanitized where applicable
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,ts,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Required environment variables must be validated at startup
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{js,jsx,ts,tsx,py,java,kt,go,rs,cpp,c,h,cs,rb,php}
📄 CodeRabbit inference engine (AGENTS.md)
Test-Driven — Write tests before implementation, 80%+ coverage required
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Security-First — Never compromise on security; validate all inputs
Immutability — Always create new objects, never mutate existing ones
- No hardcoded secrets (API keys, passwords, tokens)
- All user inputs validated
- Authentication/authorization verified
- Error messages don't leak sensitive data
Immutability (CRITICAL): Always create new objects, never mutate. Return new copies with changes applied.
Error handling: Handle errors at every level. Provide user-friendly messages in UI code. Log detailed context server-side. Never silently swallow errors.
Input validation: Validate all user input at system boundaries. Use schema-based validation. Fail fast with clear messages. Never trust external data.
Minimum coverage: 80%
TDD workflow (mandatory):
Commit format:<type>: <description>— Types: feat, fix, refactor, docs, test, chore, perf, ci
API response format: Consistent envelope with success indicator, data payload, error message, and pagination metadata.
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{sql,js,jsx,ts,tsx,py,java,kt,go,rs,php}
📄 CodeRabbit inference engine (AGENTS.md)
- SQL injection prevention (parameterized queries)
Files:
tests/hooks/mcp-health-check.test.jsscripts/hooks/mcp-health-check.js
**/*.{test,spec}.{js,jsx,ts,tsx,py,java,kt,go,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{js,jsx,ts,tsx,py,java,kt,go,rs}: 1. Unit tests — Individual functions, utilities, components
2. Integration tests — API endpoints, database operations
3. E2E tests — Critical user flows
Files:
tests/hooks/mcp-health-check.test.js
{package.json,*.config.js,scripts/**/*.js}
📄 CodeRabbit inference engine (CLAUDE.md)
Package manager detection should support npm, pnpm, yarn, and bun, with configuration via CLAUDE_PACKAGE_MANAGER environment variable or project config.
Files:
scripts/hooks/mcp-health-check.js
scripts/**/*.js
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure cross-platform support for Windows, macOS, and Linux via Node.js scripts in the scripts/ directory.
Files:
scripts/hooks/mcp-health-check.js
{scripts,bin}/**
⚙️ CodeRabbit configuration file
{scripts,bin}/**: Focus on command injection, unsafe subprocess usage, path traversal, SSRF, secret exposure, and missing tests for new CLI behavior.
Files:
scripts/hooks/mcp-health-check.js
🧠 Learnings (2)
📚 Learning: 2026-06-27T23:49:19.839Z
Learnt from: gaurav0107
Repo: affaan-m/ECC PR: 2373
File: tests/hooks/observe-signal-timeout.test.js:0-0
Timestamp: 2026-06-27T23:49:19.839Z
Learning: In tests under tests/hooks that require a Python runtime to run, the test should fail fast when Python isn’t available (or prerequisites aren’t met). Do not treat a missing Python runtime as test.skip, as an expected/allowed condition, or as a passing state; instead, explicitly fail (e.g., throw/return a rejected promise or use a test runner fail/expect that marks the test as failed) so reviewers can’t accidentally mask environment issues.
Applied to files:
tests/hooks/mcp-health-check.test.js
📚 Learning: 2026-07-14T03:26:12.530Z
Learnt from: thejesh23
Repo: affaan-m/ECC PR: 2517
File: tests/hooks/pre-bash-tmux-reminder.test.js:21-25
Timestamp: 2026-07-14T03:26:12.530Z
Learning: In this repository, do not flag `console.log` usage as a guideline violation in hook test files under `tests/hooks/*.test.js`. These tests intentionally use `console.log` for pass/fail output because the repo’s console-based runner (`tests/run-all.js`) is used and there is no Jest/Mocha dependency. Outside this specific hook-test path, follow the normal logging guidelines.
Applied to files:
tests/hooks/mcp-health-check.test.js
🪛 ast-grep (0.45.1)
tests/hooks/mcp-health-check.test.js
[warning] 1033-1064: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(
serverScript,
[
"const fs = require('fs');",
"const http = require('http');",
"const portFile = process.argv[2];",
"const server = http.createServer((req, res) => {",
" if (req.method !== 'POST') {",
" res.writeHead(404, { 'Content-Type': 'application/json' });",
" res.end(JSON.stringify({ error: 'not found' }));",
" return;",
" }",
" let body = '';",
" req.on('data', chunk => { body += chunk; });",
" req.on('end', () => {",
" let parsed = null;",
" try { parsed = JSON.parse(body); } catch { parsed = null; }",
" if (!parsed || parsed.jsonrpc !== '2.0' || parsed.method !== 'initialize') {",
" res.writeHead(400, { 'Content-Type': 'application/json' });",
" res.end(JSON.stringify({ error: 'expected a JSON-RPC initialize body' }));",
" return;",
" }",
" res.writeHead(200, { 'Content-Type': 'application/json' });",
" res.end(JSON.stringify({ jsonrpc: '2.0', id: parsed.id, result: {} }));",
" });",
"});",
"server.listen(0, '127.0.0.1', () => {",
" fs.writeFileSync(portFile, String(server.address().port));",
"});",
"setInterval(() => {}, 1000);"
].join('\n')
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
🔇 Additional comments (2)
tests/hooks/mcp-health-check.test.js (1)
1051-1055: Validate all requiredinitializepayload fields.The fixture still accepts missing or malformed
params.protocolVersion,params.capabilities, andparams.clientInfo. This duplicates existing review feedback.scripts/hooks/mcp-health-check.js (1)
256-308: LGTM!Also applies to: 310-323, 541-553
| "const server = http.createServer((req, res) => {", | ||
| " if (req.method !== 'POST') {", | ||
| " res.writeHead(404, { 'Content-Type': 'application/json' });", | ||
| " res.end(JSON.stringify({ error: 'not found' }));", | ||
| " return;", | ||
| " }", | ||
| " let body = '';", | ||
| " req.on('data', chunk => { body += chunk; });", | ||
| " req.on('end', () => {", | ||
| " let parsed = null;", | ||
| " try { parsed = JSON.parse(body); } catch { parsed = null; }", | ||
| " if (!parsed || parsed.jsonrpc !== '2.0' || parsed.method !== 'initialize') {", | ||
| " res.writeHead(400, { 'Content-Type': 'application/json' });", | ||
| " res.end(JSON.stringify({ error: 'expected a JSON-RPC initialize body' }));", | ||
| " return;", | ||
| " }", | ||
| " res.writeHead(200, { 'Content-Type': 'application/json' });", | ||
| " res.end(JSON.stringify({ jsonrpc: '2.0', id: parsed.id, result: {} }));", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the GET-to-POST fallback request contract.
The fixture accepts a POST without proving that a GET occurred first. It also accepts missing Content-Type, Accept, and Content-Length headers. A direct POST implementation or a header regression will pass this test.
Count the GET request. Reject POST until the GET has returned 404. Validate the JSON/SSE headers and that Content-Length matches the received body.
Proposed test hardening
+ "let getRequests = 0;",
"const server = http.createServer((req, res) => {",
" if (req.method !== 'POST') {",
+ " getRequests++;",
" res.writeHead(404, { 'Content-Type': 'application/json' });",
" res.end(JSON.stringify({ error: 'not found' }));",
" return;",
" }",
" let body = '';",
" req.on('data', chunk => { body += chunk; });",
" req.on('end', () => {",
+ " const validHeaders = req.headers['content-type'] === 'application/json'",
+ " && String(req.headers.accept || '').includes('application/json')",
+ " && String(req.headers.accept || '').includes('text/event-stream')",
+ " && Number(req.headers['content-length']) === Buffer.byteLength(body);",
+ " if (getRequests !== 1 || !validHeaders) {",
+ " res.writeHead(400, { 'Content-Type': 'application/json' });",
+ " res.end(JSON.stringify({ error: 'expected GET fallback and MCP headers' }));",
+ " return;",
+ " }",As per coding guidelines: “Integration tests — API endpoints, database operations.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/hooks/mcp-health-check.test.js` around lines 1040 - 1057, Harden the
HTTP fixture in the GET-to-POST fallback test by counting GET requests and
rejecting POST requests unless a preceding GET has returned 404. In the POST
handler, require the expected JSON/SSE Content-Type and Accept headers, and
verify Content-Length matches the received request body before accepting the
JSON-RPC initialize payload.
Source: Coding guidelines
| let result = await requestHttp(config.url, config.headers || {}, timeoutMs); | ||
|
|
||
| if (!result.ok) { | ||
| const posted = await requestHttp(config.url, config.headers || {}, timeoutMs, { | ||
| method: 'POST', | ||
| body: mcpInitializeBody() | ||
| }); |
There was a problem hiding this comment.
When an endpoint accepts connections but never returns response headers, the GET probe uses the full configured timeout and the fallback POST then receives another full timeout. With a 400 ms timeout, the PreToolUse integration runs took 842.821 ms and 839.582 ms. Use a shared deadline or pass only the remaining budget to the POST fallback so an unavailable MCP server does not delay the tool call for roughly twice the configured timeout.
Artifacts
Executable local integration harness source
- Captured numbered source of the isolated harness that starts a local HTTP server which accepts GET and POST requests without responding, invokes the hook, and records elapsed time; it provides the exact executable test used.
PreToolUse hung endpoint timing before capture
- Captured first execution of the local hung-endpoint integration harness with a 400 ms timeout; it shows GET and POST and 842.821 ms total elapsed time, confirming two sequential timeout windows.
PreToolUse hung endpoint timing repeat capture
- Captured independent repeat execution of the same local hung-endpoint integration harness with a 400 ms timeout; it shows GET and POST and 839.582 ms total elapsed time, reproducing the doubled probe duration.
Current HTTP fallback probe source lines
- Captured exact current numbered lines 540–560 from the hook; they show the awaited GET followed by an awaited fallback POST with the same timeout, which explains the observed delay.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/hooks/mcp-health-check.js
Line: 542-548
Comment:
**HTTP fallback consumes two timeout windows**
When an endpoint accepts connections but never returns response headers, the GET probe uses the full configured timeout and the fallback POST then receives another full timeout. With a 400 ms timeout, the PreToolUse integration runs took 842.821 ms and 839.582 ms. Use a shared deadline or pass only the remaining budget to the POST fallback so an unavailable MCP server does not delay the tool call for roughly twice the configured timeout.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.1 participant
Problem
The preflight probe in
scripts/hooks/mcp-health-check.jsonly ever sends a bareGETto the server URL. Some Streamable HTTP MCP servers routePOSTexclusively and answer anyGETwith 404. Telnyx is one:404 isn't in
HEALTHY_HTTP_CODES, so every probe failed against a completely healthy server. The failure count compounded to theMAX_BACKOFF_MSceiling and the hook blocked every tool call for that server before it left the machine — whileclaude mcp liststill reported it✔ Connected. The user-visible symptom is:with a
mcp-health-cache.jsonentry showingfailureCount: 10, lastError: "HTTP 404"that never recovers, because each retry repeats the same invalid probe.Fix
requestHttp()takes an optional method/body, andprobeServer()replays a failedGETas a real JSON-RPCinitializePOSTbefore declaring the server unreachable. The GET stays the first attempt, so nothing changes for servers that already answer it.Adding 404 to
HEALTHY_HTTP_CODESwas the cheaper alternative, but it would mask genuine outages on every other server — a 404 usually is a bad URL. Speaking the protocol is the honest check.Tests
Adds a regression test that stands up a POST-only server returning 404 for every GET and asserting the probe body is a valid JSON-RPC
initialize. It fails without the change:and passes with it.
node tests/hooks/mcp-health-check.test.js-> 23 passed, 0 failed. Fullnode tests/run-all.js-> 2580 passed, 0 failed. ESLint clean.🤖 Generated with Claude Code