added mcp server#69
Merged
Merged
Conversation
igorkorsukov
force-pushed
the
w/rcmd/rcmd_step2
branch
from
79be830 to
4757276
Compare
|
Warning Review limit reached
More reviews will be available in 39 minutes and 9 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThis pull request introduces the rcontrol module implementing MCP over TCP: CMake build integration and option, a standalone bridge client executable, MCP contracts (ITransport and types), a Qt TCP transport, a JSON-RPC McpServer, an McpController registering play/stop tools and dispatching calls, and module lifecycle wiring to initialize the controller in GUI mode. All edits add new files and CMake wiring; existing source files are unchanged. 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 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 `@framework/rcontrol/mcp/bridge/main.cpp`:
- Around line 122-131: The error branches after sendAll(...) failure and
reader.readLine(...) failure currently break the loop but let main() return
EXIT_SUCCESS; modify those branches to propagate a non-zero failure exit: either
call std::exit(EXIT_FAILURE) or set a local error flag (e.g., bool had_error =
true) when sendAll(fd, request + "\n") fails or when reader.readLine(response)
fails, and then return EXIT_FAILURE from main() (or check the flag before
returning) so failures return a non-success status; adjust references: sendAll,
reader.readLine, main, EXIT_SUCCESS/EXIT_FAILURE.
- Around line 106-109: The connect failure path in main (when fd ==
INVALID_SOCK) returns immediately without calling WSACleanup() on Windows and
overall exit paths can skip cleanup or incorrectly return EXIT_SUCCESS after I/O
errors; ensure main always calls ::WSACleanup() before any return on Windows
(add cleanup in the connectTo() failure branch and before any
EXIT_SUCCESS/EXIT_FAILURE return), convert I/O error handling in the
sendAll()/readLine() loop to return a failure exit code after performing cleanup
(or set a status variable and break to a single exit path that calls
::WSACleanup()), and modify sendAll() to avoid SIGPIPE on POSIX (e.g., use
MSG_NOSIGNAL or set SO_NOSIGPIPE where available) so writes to closed sockets
don't terminate the process.
- Around line 83-85: The send call in sendAll (the ::send(fd, data.data() +
sent, static_cast<int>(data.size() - sent), 0) path) can raise SIGPIPE and kill
the process when the peer disconnects; change the send invocation to suppress
SIGPIPE (use MSG_NOSIGNAL on platforms that support it) or alternatively set the
socket to ignore SIGPIPE via setsockopt or install a SIG_IGN handler before
writing, and ensure the error path still checks m <= 0 and returns false; update
the ::send call/site in sendAll to pass MSG_NOSIGNAL (or platform-guarded
equivalent) so that a broken pipe results in EPIPE returned to code instead of a
fatal signal.
In `@framework/rcontrol/mcp/mcpcontroller.cpp`:
- Around line 39-58: Move the registration of the tools handlers so they are set
before starting the transport: in McpController::init register
m_mcpServer->onToolsListRequest (using makeToolsList and the onResult callback)
and m_mcpServer->onToolsCallRequest (logging name, calling
dispatcher()->dispatch(name), and returning ToolResult via onResult) first, then
call m_mcpServer->init(); this ensures incoming tools/list and tools/call
requests are handled instead of hitting the null-handler.
- Around line 48-57: Move the MCP request handlers to be registered before
starting the transport in McpController::init() (i.e., set
m_mcpServer->onToolsListRequest and m_mcpServer->onToolsCallRequest prior to
calling m_mcpServer->init()), and harden the onToolsCallRequest callback so it
first validates the incoming name against the allow-list returned by
makeToolsList() and rejects anything not present by returning a ToolResult with
isError=true; if the name is allowed, call dispatcher()->dispatch(name) but
detect whether the dispatch actually handled the action (handle the case where
ActionsDispatcher has no handler) and return a ToolResult with isError=true on
that failure instead of always returning a default success ToolResult(), and
include a clear error message in the ToolResult for rejected/failed calls.
In `@framework/rcontrol/mcp/mcpserver.cpp`:
- Around line 140-150: The generated JSON Schema puts each entry in
schema.properties directly onto schemaObj instead of nesting them under a
"properties" key and always emits minimum/maximum as empty strings; update the
block that builds schemaObj (the code handling schema.properties and
propertyObj) to create a JsonObject propertiesObj, populate
propertiesObj[property.first] = propertyObj, then assign schemaObj["properties"]
= propertiesObj (keeping schemaObj["type"] = schema.type), and only set
propertyObj["minimum"] and propertyObj["maximum"] when the corresponding
property.second.minimum/maximum are present (emit them as numeric values rather
than property.second.minimum.toString()) before assigning obj["inputSchema"] =
schemaObj.
- Around line 79-81: The notification branch currently calls
onResponse(JsonObject()) which sends an empty `{}`; change the logic in the
block handling method.startsWith(u"notifications/") so it only invokes
onNotifications(method) and does not call onResponse for JSON-RPC notifications
(i.e. messages without an id). Update the surrounding dispatch code that calls
onResponse (or the ResponseHandler contract) to accept a "no response" outcome
(e.g. by allowing a null/optional/absent payload) so transports don't serialize
an empty JsonObject back to the client.
In `@framework/rcontrol/mcp/mcptypes.h`:
- Around line 22-25: The file uses std::map but doesn't include <map>, relying
on a transitive include; add a direct `#include` <map> alongside the existing
includes at the top of framework/rcontrol/mcp/mcptypes.h so std::map (used in
the types/typedefs around the mcptypes declarations) is guaranteed to be
available and avoid fragile transitive dependency issues.
- Around line 54-59: Property.type is uninitialized causing undefined behavior
when read (e.g., by to_string(property.second.type) in mcpserver.cpp
onToolsList); fix it by giving Property a safe default for `type` (either a
default member initializer in struct Property like `type = DataType::Unknown` or
a default constructor that sets `type` to a known valid enum value, or if no
sentinel exists use `static_cast<DataType>(0)`), leaving other members unchanged
so aggregate construction no longer yields indeterminate `type`.
In `@framework/rcontrol/mcp/transport/tcptransport.cpp`:
- Around line 86-90: The code hardcodes port 2212 in TcpTransport; extract it to
a single named constant (e.g., constexpr quint16 kTcpTransportPort = 2212) and
use that constant in m_server->listen(QHostAddress::Any, kTcpTransportPort) and
in the LOGI() message instead of the literal; place the constant in a shared
header or common config accessible to both TcpTransport (tcptransport.cpp /
TcpTransport) and the bridge client so both use the same symbol and avoid drift.
- Around line 75-83: TcpTransport currently leaves m_connection as a dangling
pointer when TcpConnection self-deletes (its constructor connects disconnected →
deleteLater), causing lockout and double-free in stop(); fix by resetting
m_connection to nullptr when the TcpConnection object is destroyed: in the
newConnection lambda (or where you create TcpConnection), connect the
TcpConnection instance's destroyed() (or a custom closed/destroyed signal) to a
slot/lambda that sets m_connection = nullptr; also make stop() robust by
checking m_connection != nullptr before deleting and setting it to nullptr after
deletion (reference symbols: m_connection, TcpConnection, TcpTransport::stop(),
QTcpServer::newConnection).
- Around line 86-88: The transport currently binds the MCP server to all
interfaces via m_server->listen(QHostAddress::Any, 2212) which exposes the
command dispatcher; change the bind to loopback (use QHostAddress::LocalHost) so
the server only listens on localhost/127.0.0.1 (keep port 2212) — update the
call in tcptransport.cpp where m_server->listen is invoked (class/method:
TCPTransport / the constructor or init where m_server is created) to use the
loopback address instead of QHostAddress::Any.
In `@framework/rcontrol/mcp/transport/tcptransport.h`:
- Around line 45-58: TcpTransport currently owns raw pointers m_server and
m_connection but has no destructor, so objects deleted without stop() leak; add
a destructor TcpTransport::~TcpTransport() that calls stop() to ensure m_server
and m_connection are cleaned up, and implement the destructor in the .cpp so
stop() runs on destruction (ensure stop() safely handles nullptrs and repeated
calls).
- Around line 22-43: The header TcpConnection uses QObject (class TcpConnection
: public QObject) and QByteArray (member m_buffer) but doesn't include <QObject>
or <QByteArray>, relying on transitive includes; update tcptransport.h to
explicitly include the Qt headers for QObject and QByteArray so the declaration
of class TcpConnection and member m_buffer are valid regardless of
transitive/conditional includes (i.e., add includes for QObject and QByteArray
at the top of tcptransport.h and avoid relying on global/types/bytearray.h).
In `@framework/rcontrol/rcontrolmodule.cpp`:
- Around line 44-47: RControlContext::onInit currently always calls
m_mcpController->init(), which starts a TCP listener on port 2212; change
RControlContext::onInit(const IApplication::RunMode&) to only call
m_mcpController->init() for appropriate run modes (e.g., GUI/Server modes) by
checking the incoming RunMode value (compare against the enum values used by
IApplication::RunMode) and skip init for ConsoleApp/AudioPluginRegistration
modes; update the method to return without calling m_mcpController->init() when
the run mode is not in the allowed list, and also verify the app's module
registration logic to ensure the rcontrol module is not added to m_modules for
ConsoleApp and AudioPluginRegistration so it won’t be iterated by
BaseApplication::doSetup().
🪄 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: ASSERTIVE
Plan: Pro
Run ID: d767429a-df7b-49ba-8ba4-82871ca4e84d
📒 Files selected for processing (15)
framework/CMakeLists.txtframework/cmake/MuseDeclareOptions.cmakeframework/cmake/MuseModules.cmakeframework/rcontrol/CMakeLists.txtframework/rcontrol/mcp/bridge/main.cppframework/rcontrol/mcp/itransport.hframework/rcontrol/mcp/mcpcontroller.cppframework/rcontrol/mcp/mcpcontroller.hframework/rcontrol/mcp/mcpserver.cppframework/rcontrol/mcp/mcpserver.hframework/rcontrol/mcp/mcptypes.hframework/rcontrol/mcp/transport/tcptransport.cppframework/rcontrol/mcp/transport/tcptransport.hframework/rcontrol/rcontrolmodule.cppframework/rcontrol/rcontrolmodule.h
igorkorsukov
force-pushed
the
w/rcmd/rcmd_step2
branch
from
4757276 to
b7603dd
Compare
kryksyh
approved these changes
Jun 1, 2026
igorkorsukov
force-pushed
the
w/rcmd/rcmd_step2
branch
from
b7603dd to
645ed61
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
framework/rcontrol/mcp/transport/tcptransport.cpp (2)
93-95:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBind to loopback instead of all interfaces.
listen(QHostAddress::Any, DEFAULT_PORT)exposes the MCP command dispatcher on every network interface with no authentication; any host on the network could driveplay/stopand futureRCommandactions. The bridge client connects to localhost, soQHostAddress::LocalHostis sufficient.🔒️ Proposed fix
- if (!m_server->listen(QHostAddress::Any, DEFAULT_PORT)) { + if (!m_server->listen(QHostAddress::LocalHost, DEFAULT_PORT)) { return false; }🤖 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 `@framework/rcontrol/mcp/transport/tcptransport.cpp` around lines 93 - 95, The code currently binds the TCP server to all interfaces via m_server->listen(QHostAddress::Any, DEFAULT_PORT), exposing the MCP command dispatcher; change the listen call to bind only to loopback by using QHostAddress::LocalHost (i.e., update the call on m_server where listen(...) is invoked) so the bridge client can still connect on localhost while preventing external hosts from accessing the port.
82-90:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDangling
m_connectionafter client disconnect → connection lockout and double free.Line 38 wires
disconnectedtodeleteLater, destroying theTcpConnection, butm_connectionis never reset tonullptr. After a client disconnects, theif (m_connection)guard (Line 83) sees a dangling non-null pointer and permanently rejects new connections, andstop()(Line 105) callsdelete m_connectionon freed memory.🐛 Proposed fix
QTcpSocket* socket = m_server->nextPendingConnection(); m_connection = new TcpConnection(socket, m_onRequest); + QObject::connect(m_connection, &QObject::destroyed, m_server, [this]() { + m_connection = nullptr; + });🤖 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 `@framework/rcontrol/mcp/transport/tcptransport.cpp` around lines 82 - 90, m_connection becomes a dangling pointer when a TcpConnection is deleted via its disconnected->deleteLater path, causing new connections to be rejected and stop() to double-delete; fix by resetting m_connection to nullptr when the connection object is destroyed or disconnected and by guarding/de-nullifying in stop(). Specifically, in the newConnection lambda (and/or inside TcpConnection construction) connect the TcpConnection's destroyed or disconnected signal to a slot/lambda that sets m_connection = nullptr; also update stop() to check m_connection before deleting and set it to nullptr after deletion to avoid double-free (references: m_connection, TcpConnection, QTcpServer::newConnection, disconnected, stop()).framework/rcontrol/mcp/mcpcontroller.cpp (1)
47-56:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftAllow-list the tool name and surface real failures.
nameflows straight intodispatcher()->dispatch(name), which will execute any registered action code matching that string, not just the advertisedplay/stop. Combined with the TCP transport listening on all interfaces, a remote client can invoke unintended internal actions. Additionally, the handler always returnsonResult(ToolResult())withisError=false, even when no action handler exists (the dispatcher just logs and returns), so the client cannot detect failures.Validate
nameagainstmakeToolsList()and reject unknown/unhandled calls withToolResult.isError=true.🤖 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 `@framework/rcontrol/mcp/mcpcontroller.cpp` around lines 47 - 56, The lambda registered with m_mcpServer->onToolsCallRequest currently forwards any incoming name directly to dispatcher()->dispatch(name) and always returns onResult(ToolResult()) which hides failures; change it to first validate the requested name against the allowed set from makeToolsList() (reject and return a ToolResult with isError=true and an explanatory message when name is not in the allow-list), and when dispatching, capture success/failure from dispatcher()->dispatch (or catch exceptions it may throw) and return a ToolResult with isError=true and error details on failure, otherwise return a success ToolResult; keep references to the same symbols (m_mcpServer->onToolsCallRequest, dispatcher()->dispatch, makeToolsList, ToolResult, onResult) so the logic is easy to locate and update.framework/rcontrol/mcp/mcpserver.cpp (2)
80-82:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not send a response to JSON-RPC notifications.
Per JSON-RPC 2.0, notifications (no
id) must not receive a reply.onResponse(JsonObject())serializes an empty{}back over the transport, which is a malformed message and can desync the newline-delimited framing on the client.🐛 Proposed fix
} else if (method.startsWith(u"notifications/")) { onNotifications(method); - onResponse(JsonObject()); } else {This requires the
onResponse/transport chain to tolerate "no response" for notifications; confirm an empty payload is not written to the socket.🤖 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 `@framework/rcontrol/mcp/mcpserver.cpp` around lines 80 - 82, The code currently calls onResponse(JsonObject()) for JSON-RPC notifications in the branch that matches method.startsWith(u"notifications/"), which sends an invalid empty response; remove the onResponse(JsonObject()) invocation from that branch so notifications only call onNotifications(method) and do not produce any reply, and then verify the onResponse/transport path (the writer used by onResponse) tolerates a "no response" case (i.e., does not write an empty payload or newline) when no response object is provided.
143-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
inputSchemaproperties must be nested under a"properties"key.Each property is assigned directly onto
schemaObjalongside"type", producing{"type":"object","<prop>":{...}}, which is not a valid JSON Schema. Properties must live under a"properties"object. Also,minimum/maximumare always emitted viatoString(), so unset bounds serialize as empty strings — emit them only when present. Latent today (tools use emptyInputSchema()), but breaks as soon as a tool declares properties.🐛 Proposed fix
const InputSchema& schema = tool.inputSchema; JsonObject schemaObj; schemaObj["type"] = schema.type; + JsonObject propertiesObj; for (const auto& property : schema.properties) { JsonObject propertyObj; propertyObj["type"] = to_string(property.second.type); propertyObj["description"] = property.second.description; propertyObj["minimum"] = property.second.minimum.toString(); propertyObj["maximum"] = property.second.maximum.toString(); - schemaObj[property.first] = propertyObj; + propertiesObj[property.first] = propertyObj; } + schemaObj["properties"] = propertiesObj; obj["inputSchema"] = schemaObj;🤖 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 `@framework/rcontrol/mcp/mcpserver.cpp` around lines 143 - 149, The loop currently assigns each property directly onto schemaObj, producing invalid JSON Schema; change it to collect properties into a nested JsonObject (e.g., create a local JsonObject propertiesObj and set propertiesObj[property.first] = propertyObj) and after the loop assign schemaObj["properties"] = propertiesObj; also avoid always emitting empty strings for bounds by only adding propertyObj["minimum"] and propertyObj["maximum"] when the corresponding property.second.minimum / property.second.maximum are actually present (check presence via the type's "is set" or non-empty check before calling toString()).
🤖 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 `@framework/rcontrol/mcp/mcpcontroller.h`:
- Around line 22-46: The header McpController declares std::vector<Tool>
makeToolsList() but does not include <vector>, relying on transitive includes;
add a direct `#include` <vector> at the top of
framework/rcontrol/mcp/mcpcontroller.h so the declaration of makeToolsList and
the use of std::vector (and Tool from mcptypes.h) are valid without depending on
other headers.
In `@framework/rcontrol/mcp/mcpserver.cpp`:
- Line 63: init() currently discards the bool result of m_transport->start(), so
if start() fails (e.g., listen() fails) the server appears initialized but
accepts no connections; update init() to check the return value of
m_transport->start() and handle failure by either returning/propagating an error
(e.g., make init() return bool or throw) or at minimum logging the failure and
aborting initialization, referencing m_transport->start() and the init() caller
path so callers know initialization failed; ensure any cleanup (e.g., not
leaving partially initialized members) runs when start() returns false.
---
Duplicate comments:
In `@framework/rcontrol/mcp/mcpcontroller.cpp`:
- Around line 47-56: The lambda registered with m_mcpServer->onToolsCallRequest
currently forwards any incoming name directly to dispatcher()->dispatch(name)
and always returns onResult(ToolResult()) which hides failures; change it to
first validate the requested name against the allowed set from makeToolsList()
(reject and return a ToolResult with isError=true and an explanatory message
when name is not in the allow-list), and when dispatching, capture
success/failure from dispatcher()->dispatch (or catch exceptions it may throw)
and return a ToolResult with isError=true and error details on failure,
otherwise return a success ToolResult; keep references to the same symbols
(m_mcpServer->onToolsCallRequest, dispatcher()->dispatch, makeToolsList,
ToolResult, onResult) so the logic is easy to locate and update.
In `@framework/rcontrol/mcp/mcpserver.cpp`:
- Around line 80-82: The code currently calls onResponse(JsonObject()) for
JSON-RPC notifications in the branch that matches
method.startsWith(u"notifications/"), which sends an invalid empty response;
remove the onResponse(JsonObject()) invocation from that branch so notifications
only call onNotifications(method) and do not produce any reply, and then verify
the onResponse/transport path (the writer used by onResponse) tolerates a "no
response" case (i.e., does not write an empty payload or newline) when no
response object is provided.
- Around line 143-149: The loop currently assigns each property directly onto
schemaObj, producing invalid JSON Schema; change it to collect properties into a
nested JsonObject (e.g., create a local JsonObject propertiesObj and set
propertiesObj[property.first] = propertyObj) and after the loop assign
schemaObj["properties"] = propertiesObj; also avoid always emitting empty
strings for bounds by only adding propertyObj["minimum"] and
propertyObj["maximum"] when the corresponding property.second.minimum /
property.second.maximum are actually present (check presence via the type's "is
set" or non-empty check before calling toString()).
In `@framework/rcontrol/mcp/transport/tcptransport.cpp`:
- Around line 93-95: The code currently binds the TCP server to all interfaces
via m_server->listen(QHostAddress::Any, DEFAULT_PORT), exposing the MCP command
dispatcher; change the listen call to bind only to loopback by using
QHostAddress::LocalHost (i.e., update the call on m_server where listen(...) is
invoked) so the bridge client can still connect on localhost while preventing
external hosts from accessing the port.
- Around line 82-90: m_connection becomes a dangling pointer when a
TcpConnection is deleted via its disconnected->deleteLater path, causing new
connections to be rejected and stop() to double-delete; fix by resetting
m_connection to nullptr when the connection object is destroyed or disconnected
and by guarding/de-nullifying in stop(). Specifically, in the newConnection
lambda (and/or inside TcpConnection construction) connect the TcpConnection's
destroyed or disconnected signal to a slot/lambda that sets m_connection =
nullptr; also update stop() to check m_connection before deleting and set it to
nullptr after deletion to avoid double-free (references: m_connection,
TcpConnection, QTcpServer::newConnection, disconnected, stop()).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 8705d00a-d958-4f02-aed1-403f2e86f4b7
📒 Files selected for processing (12)
framework/rcontrol/CMakeLists.txtframework/rcontrol/mcp/bridge/main.cppframework/rcontrol/mcp/itransport.hframework/rcontrol/mcp/mcpcontroller.cppframework/rcontrol/mcp/mcpcontroller.hframework/rcontrol/mcp/mcpserver.cppframework/rcontrol/mcp/mcpserver.hframework/rcontrol/mcp/mcptypes.hframework/rcontrol/mcp/transport/tcptransport.cppframework/rcontrol/mcp/transport/tcptransport.hframework/rcontrol/rcontrolmodule.cppframework/rcontrol/rcontrolmodule.h
Comment on lines
+22
to
+46
| #include <memory> | ||
|
|
||
| #include "modularity/ioc.h" | ||
| #include "global/iapplication.h" | ||
| #include "actions/iactionsdispatcher.h" | ||
|
|
||
| #include "mcptypes.h" | ||
|
|
||
| namespace muse::rcontrol::mcp { | ||
| class McpServer; | ||
| class McpController : public Contextable | ||
| { | ||
| GlobalInject<IApplication> application; | ||
| ContextInject<actions::IActionsDispatcher> dispatcher = { this }; | ||
|
|
||
| public: | ||
| McpController(const modularity::ContextPtr& iocCtx); | ||
| ~McpController(); | ||
|
|
||
| void init(); | ||
| void deinit(); | ||
|
|
||
| private: | ||
|
|
||
| std::vector<Tool> makeToolsList() const; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm <vector> is not included directly in mcpcontroller.h or mcptypes.h
rg -nP '`#include`\s+<vector>' framework/rcontrol/mcp/mcpcontroller.h framework/rcontrol/mcp/mcptypes.h || echo "no direct <vector> include found"Repository: musescore/muse_framework
Length of output: 101
Add a direct #include <vector>.
framework/rcontrol/mcp/mcpcontroller.h declares std::vector<Tool> makeToolsList() const;, but neither mcpcontroller.h nor mcptypes.h includes <vector>, so it relies on transitive includes.
♻️ Proposed fix
`#include` <memory>
+#include <vector>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #include <memory> | |
| #include "modularity/ioc.h" | |
| #include "global/iapplication.h" | |
| #include "actions/iactionsdispatcher.h" | |
| #include "mcptypes.h" | |
| namespace muse::rcontrol::mcp { | |
| class McpServer; | |
| class McpController : public Contextable | |
| { | |
| GlobalInject<IApplication> application; | |
| ContextInject<actions::IActionsDispatcher> dispatcher = { this }; | |
| public: | |
| McpController(const modularity::ContextPtr& iocCtx); | |
| ~McpController(); | |
| void init(); | |
| void deinit(); | |
| private: | |
| std::vector<Tool> makeToolsList() const; | |
| `#include` <memory> | |
| `#include` <vector> | |
| `#include` "modularity/ioc.h" | |
| `#include` "global/iapplication.h" | |
| `#include` "actions/iactionsdispatcher.h" | |
| `#include` "mcptypes.h" | |
| namespace muse::rcontrol::mcp { | |
| class McpServer; | |
| class McpController : public Contextable | |
| { | |
| GlobalInject<IApplication> application; | |
| ContextInject<actions::IActionsDispatcher> dispatcher = { this }; | |
| public: | |
| McpController(const modularity::ContextPtr& iocCtx); | |
| ~McpController(); | |
| void init(); | |
| void deinit(); | |
| private: | |
| std::vector<Tool> makeToolsList() const; |
🧰 Tools
🪛 Clang (14.0.6)
[error] 22-22: 'memory' file not found
(clang-diagnostic-error)
🤖 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 `@framework/rcontrol/mcp/mcpcontroller.h` around lines 22 - 46, The header
McpController declares std::vector<Tool> makeToolsList() but does not include
<vector>, relying on transitive includes; add a direct `#include` <vector> at the
top of framework/rcontrol/mcp/mcpcontroller.h so the declaration of
makeToolsList and the use of std::vector (and Tool from mcptypes.h) are valid
without depending on other headers.
| }); | ||
| }); | ||
|
|
||
| m_transport->start(); |
There was a problem hiding this comment.
Handle start() failure in init().
m_transport->start() returns bool (false when listen() fails, e.g. port in use), but the result is discarded, so the server silently appears initialized while accepting no connections. Log or propagate the failure.
🐛 Proposed fix
- m_transport->start();
+ if (!m_transport->start()) {
+ LOGE() << "Failed to start transport";
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| m_transport->start(); | |
| if (!m_transport->start()) { | |
| LOGE() << "Failed to start transport"; | |
| } |
🤖 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 `@framework/rcontrol/mcp/mcpserver.cpp` at line 63, init() currently discards
the bool result of m_transport->start(), so if start() fails (e.g., listen()
fails) the server appears initialized but accepts no connections; update init()
to check the return value of m_transport->start() and handle failure by either
returning/propagating an error (e.g., make init() return bool or throw) or at
minimum logging the failure and aborting initialization, referencing
m_transport->start() and the init() caller path so callers know initialization
failed; ensure any cleanup (e.g., not leaving partially initialized members)
runs when start() returns false.
645ed61 to
0d85412
Compare
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.
The rcontrol module (remote control) is a module for integration with external systems, for example, for integration with AI using MCP. But it can also be something else.
Added basic integration for MCP
How does this work:
AI (Cursor, Cloude Desktop) -> [stdio] -> mcpbridge -> [tcp] -> (app) mcpserver
The MCP Server itself in the application consists of: