Enhance MembraneMCPServer #2938
Merged
Merged
Conversation
…n MCP tool's getExchanges
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:
📝 WalkthroughWalkthroughCentralizes MCP argument integer-range validation and widens an exception constructor; extends getExchanges to support offset paging and an optional maxResponseSize that produces either a normal page or a byte-budgeted, truncated response. ChangesInteger Validation Refactoring & Size-Limited MCP Responses
Sequence Diagram(s)sequenceDiagram
participant Client
participant Membrane as MembraneMCPServer
participant SessionMgr as McpSessionManager
participant Validator as MCPUtil.validateIntRange
participant ExchangeSupport as ExchangeToolSupport
Client->>Membrane: POST JSON-RPC (getExchanges)
Membrane->>SessionMgr: requireReadySession(sessionId)
Membrane->>Validator: validate offset/limit/maxResponseSize
Membrane->>ExchangeSupport: parseQuery(args)
Membrane->>ExchangeSupport: findPage(exchanges, query)
ExchangeSupport-->>Membrane: ExchangePage
alt maxResponseSize provided
Membrane->>ExchangeSupport: buildSizedPageResponse(page, maxResponseSize)
ExchangeSupport-->>Membrane: sized payload (truncated) + hasMore/nextOffset
Membrane-->>Client: return size-limited response
else
Membrane->>ExchangeSupport: buildFullPageResponse(page)
ExchangeSupport-->>Membrane: full payload + hasMore/nextOffset
Membrane-->>Client: return normal JSON exchange page
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
|
This pull request needs "/ok-to-test" from an authorized committer. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java (1)
276-282:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove the duplicated
getOptionalPorthelper.This class contains the same
getOptionalPort(MCPToolsCall)method twice at lines 276–278 and 280–282, which will cause a compilation error.🐛 Proposed fix
- private static `@Nullable` Integer getOptionalPort(MCPToolsCall call) { - return call.getArgument("port") == null ? null : getOptionalIntArgument(call, "port", -1, 1, 65535); - } - private static `@Nullable` Integer getOptionalPort(MCPToolsCall call) { return call.getArgument("port") == null ? null : getOptionalIntArgument(call, "port", -1, 1, 65535); }🤖 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 `@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java` around lines 276 - 282, Remove the duplicated helper method getOptionalPort(MCPToolsCall) so there is only one definition; locate both identical methods named getOptionalPort in MembraneMCPServer and delete one of them, leaving the single implementation that returns call.getArgument("port") == null ? null : getOptionalIntArgument(call, "port", -1, 1, 65535); (ensure getOptionalIntArgument and MCPToolsCall references remain unchanged).
🤖 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
`@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java`:
- Line 257: Change the response-size tracking variables in the capped-response
path from int to long: declare maxResponseSize as long (use -1L for the
sentinel) and update any local counters/accumulators used to sum response bytes
(e.g., the variables that accumulate bytes in the capped-response logic) to long
as well; adjust the call to getOptionalSizeArgument by adding/using a
long-returning variant or cast its result to long, and update all comparisons
and arithmetic to use long literals (e.g., 1L, Long.MAX_VALUE) so the size cap
checks (the accumulation and the maxResponseSize comparison) cannot overflow.
---
Outside diff comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java`:
- Around line 276-282: Remove the duplicated helper method
getOptionalPort(MCPToolsCall) so there is only one definition; locate both
identical methods named getOptionalPort in MembraneMCPServer and delete one of
them, leaving the single implementation that returns call.getArgument("port") ==
null ? null : getOptionalIntArgument(call, "port", -1, 1, 65535); (ensure
getOptionalIntArgument and MCPToolsCall references remain unchanged).
🪄 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: 20377016-c431-4367-abfd-2964a64d81e8
📒 Files selected for processing (2)
core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MCPUtil.javacore/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java
Collaborator
Author
|
/ok-to-test |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java (2)
336-364: 🏗️ Heavy liftConsider incremental size tracking to improve performance.
The current implementation serializes the full response for each exchange added (line 347), resulting in O(n²) serialization complexity. For 100 exchanges, this means approximately 5,050 serialization operations.
While the current approach is correct and handles all edge cases properly (including the excellent error message at line 351), an incremental size-tracking approach could significantly improve performance for users who specify
maxResponseSize.The tradeoff is complexity: incremental tracking requires careful accounting of JSON structure, escaping, and delimiters (as attempted in earlier code versions). The current approach prioritizes correctness and maintainability over performance.
Given that
maxResponseSizeis optional and the feature is new, deferring this optimization is reasonable. However, if this feature sees heavy use with large page sizes, the performance impact could become noticeable.🤖 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 `@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java` around lines 336 - 364, The loop currently calls responseSize(createExchangePageResponse(...)) each iteration causing O(n²) serialization; replace this with incremental size tracking: precompute the fixed-size overhead of the outer response (fields from createExchangePageResponse except the exchanges array) once, then for each added entry use requireExchangeDescription(...) and compute its serialized byte size (e.g., ObjectMapper.writeValueAsBytes or JsonGenerator to bytes) plus 1 byte for the comma/delimiter, add that to a running total (trackedSize), and compare trackedSize to maxResponseSize instead of serializing the whole candidate; keep the same removal/exception behavior when exceeding maxResponseSize and still call createExchangePageResponse only once at the end, referencing describedExchanges, createExchangePageResponse, responseSize, requireExchangeDescription, nextOffset, and maxResponseSize to locate and update the logic.
210-210: 💤 Low valueConsider mentioning response-size limits in the tool description.
The description mentions "offsets for follow-up calls" but doesn't mention the new
maxResponseSizeparameter that allows clients to cap response sizes. Consider adding ", and optional response-size limits" for completeness.🤖 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 `@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java` at line 210, Update the tool description string in MembraneMCPServer (the description used for the "Gets recent HTTP exchanges..." tool) to mention the new maxResponseSize parameter by appending text like ", and optional response-size limits" so the description now references sanitized headers, optional bodies, request filters, offsets for follow-up calls, and optional response-size limits.
🤖 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.
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java`:
- Around line 336-364: The loop currently calls
responseSize(createExchangePageResponse(...)) each iteration causing O(n²)
serialization; replace this with incremental size tracking: precompute the
fixed-size overhead of the outer response (fields from
createExchangePageResponse except the exchanges array) once, then for each added
entry use requireExchangeDescription(...) and compute its serialized byte size
(e.g., ObjectMapper.writeValueAsBytes or JsonGenerator to bytes) plus 1 byte for
the comma/delimiter, add that to a running total (trackedSize), and compare
trackedSize to maxResponseSize instead of serializing the whole candidate; keep
the same removal/exception behavior when exceeding maxResponseSize and still
call createExchangePageResponse only once at the end, referencing
describedExchanges, createExchangePageResponse, responseSize,
requireExchangeDescription, nextOffset, and maxResponseSize to locate and update
the logic.
- Line 210: Update the tool description string in MembraneMCPServer (the
description used for the "Gets recent HTTP exchanges..." tool) to mention the
new maxResponseSize parameter by appending text like ", and optional
response-size limits" so the description now references sanitized headers,
optional bodies, request filters, offsets for follow-up calls, and optional
response-size limits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 64b51325-5ea3-462b-9a92-d674904e2adb
📒 Files selected for processing (1)
core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java
…d error handling
… session handling, paging, and filtering in MCP tools.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/mcp/ExchangeToolSupport.java (1)
135-157:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
longfor capped-response byte accounting.Lines 138-157 and 209-258 still total response bytes in
int. SincemaxResponseSizeallows values up toInteger.MAX_VALUE, these additions can wrap negative near the limit and let an oversized response slip past the cap. This is the same overflow issue that was previously flagged before the logic moved into this helper.Proposed fix
- int prefixBytes = measureEscapedJsonStringContentSize(EXCHANGES_PAYLOAD_PREFIX); - int separatorBytes = measureEscapedJsonStringContentSize(EXCHANGES_PAYLOAD_SEPARATOR); - int minimumResponseSize = responseEnvelope.fixedBytes() + prefixBytes + measureExchangePageSuffixBytes(false, null); + long prefixBytes = measureEscapedJsonStringContentSize(EXCHANGES_PAYLOAD_PREFIX); + long separatorBytes = measureEscapedJsonStringContentSize(EXCHANGES_PAYLOAD_SEPARATOR); + long minimumResponseSize = responseEnvelope.fixedBytes() + prefixBytes + measureExchangePageSuffixBytes(false, null); @@ - int exchangesBytes = 0; + long exchangesBytes = 0L; @@ - int additionalExchangeBytes = measureExchangeBytes(description, describedExchanges.isEmpty(), separatorBytes); + long additionalExchangeBytes = measureExchangeBytes(description, describedExchanges.isEmpty(), separatorBytes); @@ - int trackedSize = responseEnvelope.fixedBytes() + long trackedSize = responseEnvelope.fixedBytes() + prefixBytes + exchangesBytes + additionalExchangeBytes + measureExchangePageSuffixBytes(hasMore, candidateNextOffset); @@ - private int measureExchangeBytes(Map<String, Object> description, boolean firstExchange, int separatorBytes) { + private long measureExchangeBytes(Map<String, Object> description, boolean firstExchange, long separatorBytes) { return measureEscapedJsonStringContentSize(serializeJson(description)) + (firstExchange ? 0 : separatorBytes); } @@ - private int measureExchangePageSuffixBytes(boolean hasMore, `@Nullable` Integer nextOffset) { + private long measureExchangePageSuffixBytes(boolean hasMore, `@Nullable` Integer nextOffset) { return measureEscapedJsonStringContentSize(buildExchangePageSuffix(hasMore, nextOffset)); } @@ - private int measureEscapedJsonStringContentSize(String value) { + private long measureEscapedJsonStringContentSize(String value) { try { return OM.writeValueAsBytes(value).length - 2; } catch (JsonProcessingException e) { throw new RuntimeException("Failed to serialize JSON string content", e); } } @@ - private static int utf8Size(String value) { + private static long utf8Size(String value) { return value.getBytes(StandardCharsets.UTF_8).length; } @@ - private record TextResponseEnvelope(int prefixBytes, int suffixBytes) { - private int fixedBytes() { + private record TextResponseEnvelope(long prefixBytes, long suffixBytes) { + private long fixedBytes() { return prefixBytes + suffixBytes; } }Also applies to: 209-258, 283-286
🤖 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 `@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/ExchangeToolSupport.java` around lines 135 - 157, The code uses int for cumulative byte accounting which can overflow near Integer.MAX_VALUE; change all size/count variables used for summing and comparisons to long (e.g., prefixBytes, separatorBytes, minimumResponseSize, exchangesBytes, additionalExchangeBytes, trackedSize) and ensure comparisons against maxResponseSize are done safely by promoting maxResponseSize to long (or casting it to long) so the check in the loop in ExchangeToolSupport (around measureTextResponseEnvelope, measureEscapedJsonStringContentSize, measureExchangePageSuffixBytes, describeExchangeOrThrow, measureExchangeBytes, nextOffset) cannot overflow; apply the same long-based fix to the other similar summations referenced in the file (the other exchange-page accounting blocks).
🤖 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.
Duplicate comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/ExchangeToolSupport.java`:
- Around line 135-157: The code uses int for cumulative byte accounting which
can overflow near Integer.MAX_VALUE; change all size/count variables used for
summing and comparisons to long (e.g., prefixBytes, separatorBytes,
minimumResponseSize, exchangesBytes, additionalExchangeBytes, trackedSize) and
ensure comparisons against maxResponseSize are done safely by promoting
maxResponseSize to long (or casting it to long) so the check in the loop in
ExchangeToolSupport (around measureTextResponseEnvelope,
measureEscapedJsonStringContentSize, measureExchangePageSuffixBytes,
describeExchangeOrThrow, measureExchangeBytes, nextOffset) cannot overflow;
apply the same long-based fix to the other similar summations referenced in the
file (the other exchange-page accounting blocks).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 83e61806-afab-4655-b4ba-e4c67f2a021c
📒 Files selected for processing (3)
core/src/main/java/com/predic8/membrane/core/interceptor/mcp/ExchangeToolSupport.javacore/src/main/java/com/predic8/membrane/core/interceptor/mcp/McpSessionManager.javacore/src/main/java/com/predic8/membrane/core/interceptor/mcp/MembraneMCPServer.java
…essionLookup`, and clean up unused methods
…r improved accuracy in size tracking and validation
…ith optional bodies
…factor MCP schemas to use it
predic8
approved these changes
May 18, 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
Improvements