MCPServer for Membrane#2930
Merged
christiangoerdes merged 30 commits intomasterfrom May 5, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new MCP JSON‑RPC subsystem (JSONRPCRequest/Response models, typed MCP request/response/notification classes, and MCPResponse abstractions) and a MembraneMCPServer interceptor; updates ROADMAP.md; and removes explicit Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTP as "HTTP Endpoint"
participant MCP as "MembraneMCPServer"
participant RPC as "JSONRPCRequest/Response"
participant Router as "Router/ExchangeStore"
Client->>HTTP: POST /mcp (application/json)
HTTP->>MCP: forward Exchange (body InputStream)
MCP->>RPC: JSONRPCRequest.parse(InputStream)
alt initialize
MCP->>MCP: MCPInitialize.from(...)
MCP->>Router: gather server info / proxies / stats
Router-->>MCP: info/results
MCP->>RPC: build MCPInitializeResponse -> JSONRPCResponse
MCP->>HTTP: 200 + application/json body
else tools/list
MCP->>Router: list tools / proxies
Router-->>MCP: list data
MCP->>RPC: build MCPToolsListResponse -> JSONRPCResponse
MCP->>HTTP: 200 + application/json body
else tools/call
MCP->>MCP: MCPToolsCall.from(...)
MCP->>Router: execute tool (listProxies/getExchanges/getStatistics)
Router-->>MCP: tool result
MCP->>RPC: build MCPToolsCallResponse (or toolError)
MCP->>HTTP: 200 + application/json body
else notification
MCP-->>HTTP: 204 No Content
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/ROADMAP.md (1)
17-19: Consider clarifying thejsonRPCProtectiondescription.The
maxBatchSizenotation could be clearer. The syntaxmaxBatchSize = 0,1 -> No Batch, n = n-Batchesis somewhat ambiguous:
- Is
0,1showing two distinct values or a default value?- What does
n = n-Batchesmean specifically?Consider reformulating for clarity:
- jsonRPCProtection: - maxBatchSize: - 0 or 1: Disable batch requests - n (n > 1): Allow batches up to n requests - allow/block list for methodsOr provide a brief example configuration to clarify the intended usage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ROADMAP.md` around lines 17 - 19, Clarify the jsonRPCProtection section by replacing the ambiguous "maxBatchSize = 0,1 -> No Batch, n = n-Batches" with explicit semantics for the maxBatchSize option: state that maxBatchSize = 0 or 1 disables batch requests, and maxBatchSize = n (where n > 1) allows batches up to n requests; keep the note about allow/block list for methods and optionally add a short example configuration showing maxBatchSize set to 1 and to 5 to illustrate the difference; reference the jsonRPCProtection and maxBatchSize keys so the change is made in that block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/ROADMAP.md`:
- Line 14: Replace the explicit `@coderabbitai` mention with a permanent roadmap
task: either remove the line if already completed or reword it as a general
action item like "Document all MEMBRANE_* environment variables found in the
codebase, including usage examples" (or "Create a comprehensive list of
environment variables with usage examples"); if the work is still pending,
create/link a dedicated issue for tracking instead of keeping an `@-mention` in
the roadmap to ensure permanence and clarity.
---
Nitpick comments:
In `@docs/ROADMAP.md`:
- Around line 17-19: Clarify the jsonRPCProtection section by replacing the
ambiguous "maxBatchSize = 0,1 -> No Batch, n = n-Batches" with explicit
semantics for the maxBatchSize option: state that maxBatchSize = 0 or 1 disables
batch requests, and maxBatchSize = n (where n > 1) allows batches up to n
requests; keep the note about allow/block list for methods and optionally add a
short example configuration showing maxBatchSize set to 1 and to 5 to illustrate
the difference; reference the jsonRPCProtection and maxBatchSize keys so the
change is made in that block.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro
Run ID: c6833872-5845-44cc-adef-477b2bc3bc9c
📒 Files selected for processing (1)
docs/ROADMAP.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java`:
- Around line 116-122: The request map in MembraneMCPServer currently exposes
full request body and headers (built from
e.getRequest().getBodyAsStringDecoded() and e.getRequest().getHeader()); change
this to redact sensitive data before sending to MCP: replace the body value with
a redacted placeholder or only include its metadata (e.g., length, content-type)
and replace headers with a sanitized/allowlisted map (copy only safe header
names or mask values for Authorization, Cookie, Set-Cookie, API keys, etc.) or
provide a toggle to include full content. Update the code that sets
request.put("body", ...) and request.put("headers", ...) to produce the
sanitized values (add a helper like sanitizeRequestBody/sanitizeHeaders in
MembraneMCPServer to centralize logic and reference e.getRequest() when building
the redacted map).
- Around line 75-77: The default branch in the switch inside MembraneMCPServer
currently logs unknown methods and returns null which causes createResponse() to
emit no content; instead detect when the incoming JSON-RPC request has an id
(request.getId() != null) and return a JSON-RPC error response for "Method not
found" (use the same JSON-RPC error structure used elsewhere in this class)
rather than null; update the default -> log.info("Unknown MCP Request: {}",
request) branch to construct and return that error response so createResponse()
can serialize a protocol-level error for callers.
- Around line 43-50: handleRequest currently only converts IOException into a
RuntimeException-safe path, causing parse/validation errors from
JSONRPCRequest.parse and processMCPRequest to bubble up as HTTP 500s; catch
parsing/validation/processing exceptions around the JSONRPCRequest.parse(...)
and processMCPRequest(...) calls in MembraneMCPServer.handleRequest, convert
them into a JSON-RPC error envelope (use the same createResponse(...) pathway to
build the HTTP response) and return RETURN so MCP clients receive a proper
JSON-RPC error response instead of an exception; reference JSONRPCRequest.parse,
processMCPRequest, createResponse and the Exchange exc/request handling when
implementing the catch and error-envelope creation.
- Around line 99-104: The code streams
getRouter().getExchangeStore().getAllExchangesAsList() with no limit causing
unbounded export; change it to only include the most recent 100 exchanges (e.g.
compute the list size and take a subList of Math.max(0, size-100) to size or use
stream().skip(Math.max(0, size-100))) before mapping to
MembraneMCPServer::getExchangeDescription, keep the filter(Objects::nonNull) and
toList(), and consider extracting the limit (100) to a named constant for
clarity.
- Around line 85-96: The getStatistics branch currently calls
getRouter().getStatistics() but discards the result and returns null; modify the
switch case in MembraneMCPServer (the req.getName() dispatcher) so that the
"getStatistics" case wraps the result of getRouter().getStatistics() into an
MCPToolsCallResponse and returns it (similar to how listProxies(req) and
getExchanges(req) return responses), and also update the tools/list
advertisement to include "getStatistics" so the tool is advertised.
In `@core/src/main/java/com/predic8/membrane/core/jsonrpc/JSONRPCRequest.java`:
- Around line 38-40: JSONRPCRequest currently collapses missing "id" and
explicit "id": null into the same state by only storing Object id; add a boolean
field (e.g., idPresent) to the JSONRPCRequest class and ensure Jackson sets it
when parsing (use a `@JsonSetter`("id") method or `@JsonCreator` to set both id and
idPresent), update isNotification() to return true only when idPresent is false,
and modify constructors/setId methods to allow callers to explicitly indicate
presence vs. an explicit null (e.g., provide a setId(Object id, boolean present)
or an overloaded constructor that accepts idPresent) so explicit null ids are
preserved and treated correctly.
In `@core/src/main/java/com/predic8/membrane/core/jsonrpc/JSONRPCResponse.java`:
- Around line 45-83: JSON-RPC responses must always emit "id" and must
distinguish between "no result" and "result:null", so change JSONRPCResponse to
stop using the class-level `@JsonInclude`(NON_NULL) and instead (1) always
serialize id (remove omission or annotate id to ALWAYS include), (2) add a
response-kind flag (e.g. boolean hasResult or enum responseKind) that is set by
the factory methods success(Object id, Object result) and error(Object id, int
code, String message, Object data), and (3) drive serialization of result/error
from that flag so that when responseKind==SUCCESS the "result" field is emitted
even if its value is null, and when responseKind==ERROR the "error" field is
emitted and "result" omitted; update JSONRPCResponse.success(...) and
JSONRPCResponse.error(...) to set the flag accordingly and adjust any
getters/annotations so Jackson emits the correct fields per response kind.
In `@core/src/main/java/com/predic8/membrane/core/mcp/MCPRequest.java`:
- Around line 38-49: The MCPRequest constructor currently accepts
notification-style JSON-RPC requests (id == null) which lets methods like
initialize, tools/list, and tools/call be treated as regular requests; update
the MCPRequest(JSONRPCRequest request, String expectedMethod) constructor to
reject notifications by validating that request.getId() is not null (after the
method check) and throw an IllegalArgumentException with a clear message if it
is null so only calls expecting responses are constructed.
In `@core/src/main/java/com/predic8/membrane/core/mcp/MCPToolsCall.java`:
- Around line 96-99: The current MCPToolsCall.toString() includes full
caller-supplied argument values which may leak sensitive data; update the
toString() method in class MCPToolsCall to avoid printing argument contents and
instead include only safe metadata such as getId(), name, and either the
argument count or a list of argument names/keys (not values) derived from the
arguments field; locate the toString() implementation and replace the arguments
concatenation with something like "argumentsCount=" +
(arguments==null?0:arguments.size()) or a names-only summary to ensure no raw
argument values are logged.
In `@core/src/main/java/com/predic8/membrane/core/mcp/MCPToolsListResponse.java`:
- Around line 210-218: The constructor Tool(String name, String description,
Map<String, Object> inputSchema) currently only null-checks name but allows
inputSchema (a documented required field) to be null and the setters (e.g.,
setName, setInputSchema) also permit clearing required fields; update the
constructor to use Objects.requireNonNull(inputSchema, "inputSchema must not be
null") and change any setters for required fields (setName, setInputSchema) to
reject null (throw NPE or use Objects.requireNonNull) so required fields cannot
be set to null; apply the same non-null enforcement to the other overloaded
constructor/method block referenced (lines ~220-227) to keep behavior
consistent.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro
Run ID: c1032164-0cd4-4120-98ea-d5f2bf2f1c25
📒 Files selected for processing (15)
core/src/main/java/com/predic8/membrane/core/exchangestore/ClientStatistics.javacore/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.javacore/src/main/java/com/predic8/membrane/core/jsonrpc/JSONRPCRequest.javacore/src/main/java/com/predic8/membrane/core/jsonrpc/JSONRPCResponse.javacore/src/main/java/com/predic8/membrane/core/mcp/MCPInitialize.javacore/src/main/java/com/predic8/membrane/core/mcp/MCPInitializeResponse.javacore/src/main/java/com/predic8/membrane/core/mcp/MCPInitialized.javacore/src/main/java/com/predic8/membrane/core/mcp/MCPNotification.javacore/src/main/java/com/predic8/membrane/core/mcp/MCPRequest.javacore/src/main/java/com/predic8/membrane/core/mcp/MCPResponse.javacore/src/main/java/com/predic8/membrane/core/mcp/MCPToolsCall.javacore/src/main/java/com/predic8/membrane/core/mcp/MCPToolsCallResponse.javacore/src/main/java/com/predic8/membrane/core/mcp/MCPToolsList.javacore/src/main/java/com/predic8/membrane/core/mcp/MCPToolsListResponse.javadocs/ROADMAP.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/ROADMAP.md
predic8
changed the title
|
This pull request needs "/ok-to-test" from an authorized committer. |
…onsistent handling
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java (1)
144-161:⚠️ Potential issue | 🟠 Major
responsemap is always empty.Line 151 allocates a
responsemap, line 159 puts it on the result, but nothing is ever inserted into it — so every exchange descriptor reportsresponse: {}, even though the early return at line 145 guarantees a non-null response is present. The status code, headers, body length, etc. that you presumably intended to expose are missing.🐛 Suggested fix (also tightens the existing redaction concern)
request.put("method", e.getRequest().getMethod()); request.put("path", e.getRequest().getUri()); - request.put("body", e.getRequest().getBodyAsStringDecoded()); - request.put("headers", e.getRequest().getHeader()); + request.put("bodyLength", e.getRequest().getHeader().getContentLength()); + request.put("contentType", e.getRequest().getHeader().getContentType()); + // headers/body intentionally redacted; add an allowlist if exposure is required + + response.put("statusCode", e.getResponse().getStatusCode()); + response.put("contentType", e.getResponse().getHeader().getContentType()); exc.put("request", request); exc.put("response", response); return exc;The sensitive-data exposure on
request.body/request.headers(lines 155–156) is still unaddressed from the previous review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java` around lines 144 - 161, getExchangeDescription currently creates a response Map but never fills it, so populate the response Map returned by getExchangeDescription using the existing AbstractExchange e.getResponse() (e.g. include status code via e.getResponse().getStatus()/getStatusCode(), headers via e.getResponse().getHeader(), body or body length via e.getResponse().getBodyAsStringDecoded() or getBody().getContentLength()) and then put it into exc; also address sensitive-data concerns by redacting or truncating request body and request headers before putting them into the request Map (refer to the local variables exc, request, response and the method getExchangeDescription to locate the change).
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java (1)
207-217: Tools list omitsgetStatistics; clean up the dead commented schema.
getStatisticsis dispatched intoolsCall(line 129) but is not advertised here, so MCP clients won't discover it. The commented-outMap.of("type","object","properties",…,"required",…)block at lines 213–215 should either be wired into one of the tools as the properinputSchemaor removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java` around lines 207 - 217, The tools list returned by MembraneMCPServer (in the method that builds MCPToolsListResponse.from(...)) omits the getStatistics tool advertised by toolsCall and contains a dead commented schema; add a .withTool(...) entry for "getStatistics" using MCPToolsListResponse.Tool so clients can discover it (include a concise description and an appropriate inputSchema if needed), and either remove the leftover commented Map.of(...) schema or wire that schema into the correct tool's inputSchema (e.g., attach it to getStatistics or listProxies via MCPToolsListResponse.Tool's inputSchema) so the dead comment is cleaned up and the advertised tools match toolsCall.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java`:
- Around line 225-229: In MembraneMCPServer.update the getCapabilities() method
to use the MCP-standard tools key "listChanged" instead of the non-standard
"lastExchanges": change the entry in the HashMap created by getCapabilities()
(method getCapabilities in class MembraneMCPServer) to put("tools",
Map.of("listChanged", false)) so the server advertises the correct boolean
capability name to clients while keeping the same boolean value semantics.
---
Duplicate comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java`:
- Around line 144-161: getExchangeDescription currently creates a response Map
but never fills it, so populate the response Map returned by
getExchangeDescription using the existing AbstractExchange e.getResponse() (e.g.
include status code via e.getResponse().getStatus()/getStatusCode(), headers via
e.getResponse().getHeader(), body or body length via
e.getResponse().getBodyAsStringDecoded() or getBody().getContentLength()) and
then put it into exc; also address sensitive-data concerns by redacting or
truncating request body and request headers before putting them into the request
Map (refer to the local variables exc, request, response and the method
getExchangeDescription to locate the change).
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java`:
- Around line 207-217: The tools list returned by MembraneMCPServer (in the
method that builds MCPToolsListResponse.from(...)) omits the getStatistics tool
advertised by toolsCall and contains a dead commented schema; add a
.withTool(...) entry for "getStatistics" using MCPToolsListResponse.Tool so
clients can discover it (include a concise description and an appropriate
inputSchema if needed), and either remove the leftover commented Map.of(...)
schema or wire that schema into the correct tool's inputSchema (e.g., attach it
to getStatistics or listProxies via MCPToolsListResponse.Tool's inputSchema) so
the dead comment is cleaned up and the advertised tools match toolsCall.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro
Run ID: 5ea96ac5-b5af-4a60-a43a-a8d78ae10bf9
📒 Files selected for processing (1)
core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java (4)
131-143:⚠️ Potential issue | 🟠 Major
getStatisticsbranch discards the result and returns HTTP 204.Line 138 invokes
getRouter().getStatistics()but the value is dropped on the floor (noyield/return, just an expression statement), so control falls through toreturn nullat line 142 and the client gets no body. Wrap the result inMCPToolsCallResponse(and also advertise the tool intools/list).🐛 Proposed fix
case "getExchanges" -> { return getExchanges(req); } - case "getStatistics" -> getRouter().getStatistics(); - default -> log.info("Unknown tools call: " + req.getName()); + case "getStatistics" -> { + return MCPToolsCallResponse.from(req) + .withJson(Map.of("statistics", getRouter().getStatistics())); + } + default -> log.info("Unknown tools call: {}", req.getName()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java` around lines 131 - 143, The getStatistics branch currently calls getRouter().getStatistics() but discards its result, causing a 204 response; change the "getStatistics" case in MembraneMCPServer.switch (alongside existing listProxies/getExchanges branches) to return a new MCPToolsCallResponse wrapping getRouter().getStatistics() (i.e., yield/return the statistics result instead of just calling it), and ensure the "getStatistics" tool is advertised in the tools/list output so clients know the tool exists.
145-151:⚠️ Potential issue | 🟠 MajorUnbounded exchange export.
getAllExchangesAsList()is streamed without a cap even though the tool description advertises "last 100". This can produce very large payloads and amplifies the sensitive-data exposure flagged below.⚙️ Proposed fix
return MCPToolsCallResponse.from(req) .withJson(Map.of("exchanges", getRouter().getExchangeStore() .getAllExchangesAsList().stream() .map(MembraneMCPServer::getExchangeDescription) - .filter(Objects::nonNull).toList())); + .filter(Objects::nonNull) + .limit(100) + .toList()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java` around lines 145 - 151, The current getExchanges method streams getRouter().getExchangeStore().getAllExchangesAsList() with no bound; change MembraneMCPServer.getExchanges to limit the exported exchanges to the most recent 100 entries before mapping/serialization (e.g. obtain the full list once, compute a subList of the last 100 items if size>100, then stream that sublist and apply map(MembraneMCPServer::getExchangeDescription).filter(Objects::nonNull)). Ensure the limiting happens before mapping to avoid huge payloads.
153-170:⚠️ Potential issue | 🟠 Major
responsemap is created and attached but never populated; also leaks request body/headers.Two issues in
getExchangeDescription:
- Bug:
responseis allocated at line 160 and put intoexcat line 168, but no fields (status, body, headers, etc.) are ever placed into it — every exchange will report"response": {}. Either populate it frome.getResponse()or drop it.- Sensitive-data exposure: the request body and full header map are returned verbatim to the MCP/LLM client. This can leak
Authorization,Cookie, API keys, and PII. Redact or allowlist headers, and gate body inclusion behind explicit configuration.🐛 Proposed sketch
request.put("method", e.getRequest().getMethod()); request.put("path", e.getRequest().getUri()); - request.put("body", e.getRequest().getBodyAsStringDecoded()); - request.put("headers", e.getRequest().getHeader()); + request.put("headers", sanitizeHeaders(e.getRequest().getHeader())); + // Optionally include body only when explicitly enabled / sanitized. + + var resp = e.getResponse(); + response.put("statusCode", resp.getStatusCode()); + response.put("headers", sanitizeHeaders(resp.getHeader())); exc.put("request", request); exc.put("response", response);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java` around lines 153 - 170, getExchangeDescription currently creates and attaches an empty response map and returns full request body/headers unredacted; fix by populating the response map from e.getResponse() (status, body, headers) or remove it if not needed, but when adding headers/body implement redaction/allowlist and gate body inclusion behind a configuration flag. Specifically, in getExchangeDescription use e.getResponse() to fill "response" (e.g., status code, body via getBodyAsStringDecoded, and headers via getHeader) or omit the map entirely; for both request and response replace returning the raw header map with a filtered allowlist (e.g., only Host, Content-Type, Content-Length) and do not include bodies unless a new config flag (e.g., includeExchangeBodies) is true — if enabled, ensure sensitive patterns (Authorization, Cookie, API-Key) are removed from headers and bodies before adding to exc.
234-238:⚠️ Potential issue | 🟠 Major
toolscapability key should belistChanged, notlastExchanges.The MCP spec defines
capabilities.toolsas{ "listChanged": <bool> }to indicate whether the server emitsnotifications/tools/list_changed.lastExchangesis non-standard and strict clients may ignore the entiretoolscapability advertisement.🐛 Proposed fix
- capabilities.put("tools", Map.of("lastExchanges", false)); + capabilities.put("tools", Map.of("listChanged", false));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java` around lines 234 - 238, The capabilities map returned by MembraneMCPServer.getCapabilities() uses a non-standard "lastExchanges" key under "tools"; change that inner key to "listChanged" (keeping the boolean value, e.g., false) so capabilities.put("tools", Map.of("listChanged", false)); to conform to the MCP spec and ensure clients correctly detect support for notifications/tools/list_changed.
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java (2)
214-226: Drop commented-out scaffolding and consider advertisinggetStatistics.The dangling block comment at lines 222-224 is dead code; either inline the proper input schema for the tools or remove it. Also, once the
getStatisticsbranch intoolsCallreturns a real response, advertise it here so MCP clients discover it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java` around lines 214 - 226, The toolsList method in MembraneMCPServer contains dead commented scaffolding and should be cleaned up: remove the commented block (lines within toolsList) or replace it with a real input schema for the tool if intended, and add an MCPToolsListResponse.Tool entry advertising "getStatistics" (matching the toolsCall.getStatistics branch) once that branch returns a real response so clients can discover it; update the MCPToolsListResponse.Tool construction in toolsList to include the new "getStatistics" tool with an appropriate description and input schema using MCPToolsListResponse.Tool.
86-91: Remove the unusedcreateResponse(MCPResponse<?>)overload.All call sites in
handleRequestandprocessMCPRequestpassJSONRPCResponsetocreateResponse(), either viacreateErrorResponse(...)or directly fromprocessMCPRequest(). ThecreateResponse(MCPResponse<?>)method at lines 86–91 is never invoked and merely delegates to theJSONRPCResponseoverload, making it dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java` around lines 86 - 91, Remove the dead overload createResponse(MCPResponse<?> mcpResponse): it is never called (all callers in handleRequest and processMCPRequest pass a JSONRPCResponse via createErrorResponse(...) or directly) and only delegates to the JSON-RPC overload; delete the method declaration createResponse(MCPResponse<?>) and any associated imports, leaving the existing createResponse(JSONRPCResponse) overload as the single implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java`:
- Around line 138-139: The default branch in MembraneMCPServer (inside the
switch handling req.getName()) currently just logs unknown tool calls and
returns null which causes createResponse() to emit a 204; change this to build
and return a proper MCP JSON-RPC error envelope (e.g. method-not-found or
invalid-params) when req.getName() is unrecognized: create an error response
object referencing the incoming req.getId() and req.getName(), populate an error
code and descriptive message, and return that instead of null so callers receive
an explicit error rather than an empty 204.
- Around line 228-232: Remove the unnecessary checked exception declaration from
the initialize method: delete "throws IOException" from the signature of
initialize(JSONRPCRequest request) since constructing MCPInitializeResponse and
calling withCapabilities() / withServerInfo() do not throw checked IOExceptions;
update callers (e.g., processMCPRequest) if they currently propagate or catch
IOException only because of initialize to simplify their signatures/handling
accordingly.
---
Duplicate comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java`:
- Around line 131-143: The getStatistics branch currently calls
getRouter().getStatistics() but discards its result, causing a 204 response;
change the "getStatistics" case in MembraneMCPServer.switch (alongside existing
listProxies/getExchanges branches) to return a new MCPToolsCallResponse wrapping
getRouter().getStatistics() (i.e., yield/return the statistics result instead of
just calling it), and ensure the "getStatistics" tool is advertised in the
tools/list output so clients know the tool exists.
- Around line 145-151: The current getExchanges method streams
getRouter().getExchangeStore().getAllExchangesAsList() with no bound; change
MembraneMCPServer.getExchanges to limit the exported exchanges to the most
recent 100 entries before mapping/serialization (e.g. obtain the full list once,
compute a subList of the last 100 items if size>100, then stream that sublist
and apply
map(MembraneMCPServer::getExchangeDescription).filter(Objects::nonNull)). Ensure
the limiting happens before mapping to avoid huge payloads.
- Around line 153-170: getExchangeDescription currently creates and attaches an
empty response map and returns full request body/headers unredacted; fix by
populating the response map from e.getResponse() (status, body, headers) or
remove it if not needed, but when adding headers/body implement
redaction/allowlist and gate body inclusion behind a configuration flag.
Specifically, in getExchangeDescription use e.getResponse() to fill "response"
(e.g., status code, body via getBodyAsStringDecoded, and headers via getHeader)
or omit the map entirely; for both request and response replace returning the
raw header map with a filtered allowlist (e.g., only Host, Content-Type,
Content-Length) and do not include bodies unless a new config flag (e.g.,
includeExchangeBodies) is true — if enabled, ensure sensitive patterns
(Authorization, Cookie, API-Key) are removed from headers and bodies before
adding to exc.
- Around line 234-238: The capabilities map returned by
MembraneMCPServer.getCapabilities() uses a non-standard "lastExchanges" key
under "tools"; change that inner key to "listChanged" (keeping the boolean
value, e.g., false) so capabilities.put("tools", Map.of("listChanged", false));
to conform to the MCP spec and ensure clients correctly detect support for
notifications/tools/list_changed.
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java`:
- Around line 214-226: The toolsList method in MembraneMCPServer contains dead
commented scaffolding and should be cleaned up: remove the commented block
(lines within toolsList) or replace it with a real input schema for the tool if
intended, and add an MCPToolsListResponse.Tool entry advertising "getStatistics"
(matching the toolsCall.getStatistics branch) once that branch returns a real
response so clients can discover it; update the MCPToolsListResponse.Tool
construction in toolsList to include the new "getStatistics" tool with an
appropriate description and input schema using MCPToolsListResponse.Tool.
- Around line 86-91: Remove the dead overload createResponse(MCPResponse<?>
mcpResponse): it is never called (all callers in handleRequest and
processMCPRequest pass a JSONRPCResponse via createErrorResponse(...) or
directly) and only delegates to the JSON-RPC overload; delete the method
declaration createResponse(MCPResponse<?>) and any associated imports, leaving
the existing createResponse(JSONRPCResponse) overload as the single
implementation.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro
Run ID: a4057b3a-0b38-48b3-8c7f-9603fe0d69db
📒 Files selected for processing (1)
core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java (1)
230-234: Drop commented-out scaffolding.The dangling
//block at lines 230-232 (and the similar one at line 192) is dead documentation that will rot. Either turn it into a real input schema for one of the tools or remove it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java` around lines 230 - 234, Remove the dead commented scaffolding in MembraneMCPServer.java: delete the dangling commented Map.of(...) block (the lines containing "// Map.of("type", "object", ...)" and the similar block near line 192) so no stale commented JSON/schema remains; if you intended an input schema instead, replace those comments with a real schema construction and wire it into the existing MCP tool setup (refer to the Map.of("type","object", "properties", Map.of("query", Map.of("type","string")), "required", List.of("query")) snippet and the surrounding code in class MembraneMCPServer to ensure the schema is either removed or implemented).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java`:
- Around line 166-176: The response map is never populated before being put into
the exchange map in MembraneMCPServer (vars request/response), so populate
response with the response status, headers and body from e.getResponse()
similarly to the request: add entries like "status" (response status code),
"headers" (response headers) and "body" (decoded response body) to the response
map before exc.put("response", response); keep any existing header/body
redaction logic in mind if present.
- Around line 116-121: The info-level log in MembraneMCPServer is printing the
entire JSON-RPC request (log.info("Unknown MCP Request: {}", request)) which can
expose user-supplied params; change the info log to only include safe
identifiers (e.g., request.getMethod() and request.getId()) and move the full
request serialization to a debug-level log (e.g., log.debug(..., request)) so
sensitive payloads are not logged at INFO; modify the default branch in the
request-dispatching code in MembraneMCPServer accordingly while leaving the
existing error(request.getId(), ERR_METHOD_NOT_FOUND, ...) behavior intact.
- Around line 188-205: The pattern-switch in MembraneMCPServer that switches on
p currently lists case ServiceProxy s before case SOAPProxy sp, so SOAPProxy
instances match ServiceProxy and never reach the SOAPProxy branch; reorder the
case labels so case SOAPProxy sp appears before case ServiceProxy s (or add an
explicit guard in the ServiceProxy case to skip SOAPProxy instances) and keep
the APIProxy case where it is; update the switch in the method that contains the
switch-on-p (the switch handling APIProxy, ServiceProxy, SOAPProxy) so wsdl and
serviceName from SOAPProxy are populated as intended.
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java`:
- Around line 230-234: Remove the dead commented scaffolding in
MembraneMCPServer.java: delete the dangling commented Map.of(...) block (the
lines containing "// Map.of("type", "object", ...)" and
the similar block near line 192) so no stale commented JSON/schema remains; if
you intended an input schema instead, replace those comments with a real schema
construction and wire it into the existing MCP tool setup (refer to the
Map.of("type","object", "properties", Map.of("query", Map.of("type","string")),
"required", List.of("query")) snippet and the surrounding code in class
MembraneMCPServer to ensure the schema is either removed or implemented).
🪄 Autofix (Beta)
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: CHILL
Plan: Pro
Run ID: 4120c87a-bc58-476b-9995-79f79b51489f
📒 Files selected for processing (1)
core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java
… null IDs, improving MCP server error handling and response generation.
…tion and customize serialization
…for cleanup and consistency
Collaborator
|
/ok-to-test |
…arity and logging for notifications
… serialization handling
… parsing, normalization, and error handling
…stry`, `McpToolDefinition`, and `McpToolHandler`. Minor cleanup and refactoring in MCP initialization and request handling.
…ayload sanitation and session state management
…ation, session management, and improved error handling.
…clean up imports
…e lifecycle handling, error scenarios, and MCP tool behaviors
…gumentsException`, improve protocol version validation, and enhance test coverage.
predic8
commented
May 4, 2026
…t keys instead of full argument details.
…ss for improved modularity and code reuse.
…DER` handling, session-based request processing, and enhanced error handling. Update corresponding unit tests for validation.
christiangoerdes
approved these changes
May 5, 2026
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
2 participants
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.
Summary by CodeRabbit
New Features
Documentation
Refactor