Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/24908 )
Change subject: KUDU-3806: add the MCP server ...................................................................... Patch Set 2: (19 comments) Thank you for the initial round of review Alexey! http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/mcp_test_util.h File src/kudu/tools/mcp_test_util.h: http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/mcp_test_util.h@44 PS1, Line 44: // The parsed MCP tool result from a 'tools/call' JSON-RPC response. This : // deliberately mirrors (rather than reuses) the server's ToolOutcome in : // tool_action_mcp.cc: that is the value before JSON serialization, this is the : // value parsed back off the wire. Keeping the test's parse-side type : // > This seems duplicated: there is another definition in tool_action_mcp.cc ca Intentional. They're the two ends of the round-trip: ToolOutcome (tool_action_mcp.cc) is the server-side value before JSON serialization, while ToolResult here is what the test parses back off the wire. They only share fields because that's all the MCP tool-result format carries (a text block + isError), and the swapped field order is just a side effect of them being written independently. Keeping the test's parse-side type independent of the server's type is what makes these helpers an independent oracle of the wire format (see the file comment), so a framing bug in the server can't be masked by a helper that reuses the server's own type. I've added a comment on ToolResult spelling this out so it doesn't read as an accidental clone. http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp-itest.cc File src/kudu/tools/tool_action_mcp-itest.cc: http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp-itest.cc@24 PS1, Line 24: #include <sys/wait.h> : #include <unistd.h> : : #include <cerrno> > nit: for some of these, find corresponding C++ counterparts and move into t Done - moved <stdint.h> -> <cstdint> and <stdlib.h> -> <cstdlib> into the C++ block. <sys/wait.h> (the W* macros, waitpid) and <unistd.h> (::read/::write) are POSIX-only with no C++ counterpart, so they stay in the C block above. http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp-itest.cc@90 PS1, Line 90: // parses the tool result. 'arguments_json' is the raw JSON object for : // params.arguments. : ToolResult CallTool(const string& tool_name, const string& arguments_json) { : > Why to have this defined in multiple places? Is it possible to define it o Moved these into src/kudu/tools/tool_action_mcp-internal.h . http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp-itest.cc@684 PS1, Line 684: << "child interpreted '" << flag_like << "' as a flag: " << tr.text; : } : } : : } // namespace tools : } // namespace kudu : : : : > Any particular reason of using EXPECT_xxx instead of ASSERT_xxx when tr.is_ No particular one, changed all to ASSERT. http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp-test.cc File src/kudu/tools/tool_action_mcp-test.cc: http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp-test.cc@299 PS1, Line 299: : JsonReader reader(line); : NO_FATALS(AssertValidEnvelope(line, &reader)); > Any particular reason of using EXECT_xxx instead of ASSERT_xx here? Done http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp-test.cc@411 PS1, Line 411: scan = t; > Why EXPECT_FALSE, not ASSERT_FALSE? Would the rest of the test case make s Done http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc File src/kudu/tools/tool_action_mcp.cc: http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc@20 PS1, Line 20: #include <sys/wait. > nit: convert into '#include <cstdlib>' and move into the section below? Done http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc@93 PS1, Line 93: : // Interprets a finished child tool process into an MCP tool result; see the : // declaration in tool_action_mcp-internal.h for the full contract. : To > One more definition of the same entity? Why do we need so many? Ah yes, moved this into src/kudu/tools/tool_action_mcp-internal.h http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc@169 PS1, Line 169: // stdio, finds a handshake-only server, and falls back to negotiating down to a : // version in this set. So this is a capability gap, not a breakage. : bool IsSupportedProtocolVersion(c > This is how versions are defined in MCP? Wow, that's a piece of innovation modelcontextprotocol.info is an unofficial mirror; the canonical spec lives at modelcontextprotocol.io. Versioning section: https://modelcontextprotocol.io/docs/2026-07-28/learn/versioning Regarding the version based behaviour, I've added explanation comment section here, and the missing latest version 2026-07-28 todo. http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc@202 PS1, Line 202: jw->Uint64(id->GetUint64()); : } else if (id->IsDouble()) { : jw->Double( > Isn't this a pure data loss? If so, wouldn't it make sense to track it som Valid yes. Now ClassifyRequest validates the id type. Added also a new test: TEST(ToolActionMcpTest, NonScalarIdReturnsInvalidRequestWithNullId) . http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc@261 PS1, Line 261: > nit for here and elsewhere with lambda captures: is it possible to limit th Done - narrowed this one to [&protocol_version]. The other BuildResultResponse call sites (BuildResultResponse, BuildErrorResponse, BuildToolsListResponse, BuildToolResultResponse) each capture 1–2 of their own function params and the lambdas are invoked synchronously in place, so I left them as [&] http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc@296 PS1, Line 296: xposure registry + reflection into MCP too > Why this is handled differently from 'allow_writes' above? Both are suppos Absolutely, fixed it thanks! http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc@427 PS1, Line 427: cleanly, falling back > IIUC, this case cannot be properly handed by safe_strto64() and requires sa Done http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc@429 PS1, Line 429: teFlagDefault(JsonWriter* jw, > Here and elsewhere safe_strtoxxx() is used: aren't we supposed to handle an It basically degrades to emitting the default verbatim as a JSON string, which keeps the schema well-formed. http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc@1225 PS1, Line 1225: list" > nit for here and elsewhere: please use the same approach for STL classes th Done http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc@1253 PS1, Line 1253: nrpc":"2.0", echoes the request id exactly as : // sent, and contains EITHER "result" OR "error", never both. Blank lines are : // ignored. The loop termina > What's the purpose of this strange behavior? Why to keep this nonsensical Yes makes sense. Moved this binary patch checking into RunMcpServe before RunMcpServeLoop. http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc@1270 PS1, Line 1270: in, ost > There are two more characters commonly attributed to the "space" class in Done http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc@1305 PS1, Line 1305: > I'm a bit concerned that unexpected stream errors might entail data loss an Done http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc@1327 PS1, Line 1327: rapidjson::Value* root = reader > Server-level master addresses? What are those and how are they different f Reworded the comment. -- To view, visit http://gerrit.cloudera.org:8080/24908 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I59146a2fd4f5b8d61d3e494cc88974b396ed6856 Gerrit-Change-Number: 24908 Gerrit-PatchSet: 2 Gerrit-Owner: Marton Greber <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Yan-Daojiang <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Tue, 22 Sep 2026 12:22:16 +0000 Gerrit-HasComments: Yes
