Repository navigation
Support move semantics in mcp_write_result and optimize MCP notifications and headers - #3025
Open
HarshPopat23 wants to merge 1 commit into
Open
HarshPopat23 wants to merge 1 commit into
HarshPopat23 wants to merge 1 commit into
Conversation
…ions and headers - Provide an overload mcp_write_result(..., JSON &&result, ...) in mcp_results.h and results.cc to eliminate deep clones of multi-megabyte payloads when callers pass an rvalue. - In notifications.cc, resolve the log level enum once into a local variable instead of parsing twice and unconditionally dereferencing mcp_resolve_log_level. - In parameter_headers.cc, eliminate repeated temporary JSON allocations for JavaScript safe integer boundaries by using scalar comparisons for integers and static constants for integral fallback. - Add unit tests verifying move semantics and safe integer boundary validations. Signed-off-by: HarshPopat23 <musichk61@gmail.com>
🤖 Augment PR SummaryAdds a move-aware 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/core/mcp/results.cc">
<violation number="1" location="src/core/mcp/results.cc:1167">
P2: In-place decoration can reallocate `result`, invalidating `identifier` when it aliases a result property; `jsonrpc_make_success` then reads a dangling reference. Copy and use the identifier before decorating.</violation>
<violation number="2" location="src/core/mcp/results.cc:1180">
P3: The lvalue compatibility wrapper deep-copies the entire result before the existing validation path runs. Large invalid results now incur unnecessary allocation/traversal and can fail during copying before reporting the invalid identifier, method, or result; validate first, then copy only after validation succeeds.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| } | ||
| auto payload{result}; | ||
| decorate_result(version, payload, server_info); | ||
| decorate_result(version, result, server_info); |
There was a problem hiding this comment.
P2: In-place decoration can reallocate result, invalidating identifier when it aliases a result property; jsonrpc_make_success then reads a dangling reference. Copy and use the identifier before decorating.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/core/mcp/results.cc, line 1167:
<comment>In-place decoration can reallocate `result`, invalidating `identifier` when it aliases a result property; `jsonrpc_make_success` then reads a dangling reference. Copy and use the identifier before decorating.</comment>
<file context>
@@ -1164,11 +1164,21 @@ auto mcp_write_result(std::ostream &stream, const MCPProtocolVersion version,
}
- auto payload{result};
- decorate_result(version, payload, server_info);
+ decorate_result(version, result, server_info);
if (cache_policy) {
- decorate_cacheable_result(version, payload, *cache_policy, method);
</file context>
| const std::optional<MCPCachePolicy> cache_policy, | ||
| const std::optional<MCPImplementation> server_info) | ||
| -> void { | ||
| auto payload{result}; |
There was a problem hiding this comment.
P3: The lvalue compatibility wrapper deep-copies the entire result before the existing validation path runs. Large invalid results now incur unnecessary allocation/traversal and can fail during copying before reporting the invalid identifier, method, or result; validate first, then copy only after validation succeeds.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/core/mcp/results.cc, line 1180:
<comment>The lvalue compatibility wrapper deep-copies the entire result before the existing validation path runs. Large invalid results now incur unnecessary allocation/traversal and can fail during copying before reporting the invalid identifier, method, or result; validate first, then copy only after validation succeeds.</comment>
<file context>
@@ -1164,11 +1164,21 @@ auto mcp_write_result(std::ostream &stream, const MCPProtocolVersion version,
+ const std::optional<MCPCachePolicy> cache_policy,
+ const std::optional<MCPImplementation> server_info)
+ -> void {
+ auto payload{result};
+ mcp_write_result(stream, version, method, identifier, std::move(payload),
+ cache_policy, server_info);
</file context>
This branch has not been deployed
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
1 participant
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
This PR addresses three performance and safety optimizations in the Model Context Protocol (MCP) implementation:
Move Semantics for MCP Result Writing:
esources/read\ or \ ools/list\ payloads.
Eliminate Duplicate Log Level Enum Resolution:
esolved_level\ when checking \mcp_log_level_enabled, avoiding redundant lookups and unverified pointer dereferences.
Avoid Temporary JSON Allocations in Parameter Headers:
Testing