added mcp server#69
Open
igorkorsukov wants to merge 3 commits into
Open
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 36 minutes and 46 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 (12)
📝 WalkthroughWalkthroughThis pull request introduces the 🚥 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
Comment on lines
+106
to
+109
| if (fd == INVALID_SOCK) { | ||
| std::fprintf(stderr, "cannot connect to %s:%s\n", host, port); | ||
| return EXIT_FAILURE; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="framework/rcontrol/mcp/bridge/main.cpp"
# Basic sanity: confirm file exists and show line count
ls -l "$FILE"
wc -l "$FILE"
# Show the relevant regions around the cited lines and nearby cleanup/return paths
sed -n '1,220p' "$FILE" | nl -ba | sed -n '1,220p'
# Narrow focus around the specific ranges mentioned in the comment
sed -n '60,170p' "$FILE" | nl -ba | sed -n '60,170p'Repository: musescore/muse_framework
Length of output: 261
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="framework/rcontrol/mcp/bridge/main.cpp"
echo "=== lines 1-220 (line-numbered via awk) ==="
awk '{printf "%4d %s\n", NR, $0}' "$FILE" | sed -n '1,220p' | head -n 220
echo "=== focus: lines 1-40 ==="
awk '{printf "%4d %s\n", NR, $0}' "$FILE" | sed -n '1,40p'
echo "=== focus: lines 60-120 ==="
awk '{printf "%4d %s\n", NR, $0}' "$FILE" | sed -n '60,120p'
echo "=== focus: lines 120-170 ==="
awk '{printf "%4d %s\n", NR, $0}' "$FILE" | sed -n '120,170p'Repository: musescore/muse_framework
Length of output: 8265
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="framework/rcontrol/mcp/bridge/main.cpp"
echo "=== SIGPIPE / MSG_NOSIGNAL / SO_NOSIGPIPE in $FILE ==="
rg -n --hidden --no-ignore-vcs "SIGPIPE|MSG_NOSIGNAL|SO_NOSIGPIPE" "$FILE" || true
echo "=== Any signal( SIGPIPE ) usage across repo ==="
rg -n --hidden --no-ignore-vcs "SIGPIPE" . || trueRepository: musescore/muse_framework
Length of output: 1936
Fix missing WSACleanup() when connectTo() fails on Windows.
main()returnsEXIT_FAILUREimmediately onfd == INVALID_SOCK(lines 105-108), bypassing::WSACleanup()(lines 138-140).- Loop I/O failures (
sendAll()/readLine())breakbutmain()still returnsEXIT_SUCCESS(lines 121-135, 141). - POSIX
sendAll()uses::send(..., 0)(line 82) with no SIGPIPE suppression; writing to a closed socket can terminate the process.
Suggested fix
- if (fd == INVALID_SOCK) {
- std::fprintf(stderr, "cannot connect to %s:%s\n", host, port);
- return EXIT_FAILURE;
- }
+ if (fd == INVALID_SOCK) {
+ std::fprintf(stderr, "cannot connect to %s:%s\n", host, port);
+#if defined(_WIN32)
+ ::WSACleanup();
+#endif
+ return EXIT_FAILURE;
+ }🤖 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/bridge/main.cpp` around lines 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.
Comment on lines
+122
to
+131
| if (!sendAll(fd, request + "\n")) { | ||
| std::fprintf(stderr, "send failed\n"); | ||
| break; | ||
| } | ||
|
|
||
| std::string response; | ||
| if (!reader.readLine(response)) { | ||
| std::fprintf(stderr, "connection closed by server\n"); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Communication errors currently exit with success code.
After send failed / connection closed by server, the loop breaks but main() still returns EXIT_SUCCESS (Line 142). This makes automation think the bridge succeeded.
Suggested fix
+ bool hadError = false;
while (std::getline(std::cin, request)) {
@@
if (!sendAll(fd, request + "\n")) {
std::fprintf(stderr, "send failed\n");
+ hadError = true;
break;
}
@@
if (!reader.readLine(response)) {
std::fprintf(stderr, "connection closed by server\n");
+ hadError = true;
break;
}
@@
- return EXIT_SUCCESS;
+ return hadError ? EXIT_FAILURE : EXIT_SUCCESS;Also applies to: 142-142
🧰 Tools
🪛 Clang (14.0.6)
[warning] 127-127: variable 'response' is not initialized
(cppcoreguidelines-init-variables)
🤖 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/bridge/main.cpp` around lines 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.
Comment on lines
+48
to
+57
| m_mcpServer->onToolsCallRequest([this](const std::string& name, | ||
| const JsonObject& args, | ||
| const McpServer::ToolsCallResultHandler& onResult) | ||
| { | ||
| LOGDA() << "Tools call: " << name; | ||
|
|
||
| dispatcher()->dispatch(name); | ||
|
|
||
| onResult(ToolResult()); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the bind address/interface for the TCP transport (localhost vs all interfaces)
rg -nP -C3 '(listen|bind|QHostAddress|2212)' --type=cpp -g '*tcptransport*'Repository: musescore/muse_framework
Length of output: 824
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect mcpcontroller.cpp around init() and tool handler registration
sed -n '1,140p' framework/rcontrol/mcp/mcpcontroller.cpp | nl -ba | sed -n '1,140p'
# 2) Find dispatcher() and dispatch(name) implementations/usages
rg -n "dispatcher\(\)->dispatch\(" framework/rcontrol/mcp/mcpcontroller.cpp framework/rcontrol -S
rg -n "dispatch\(" framework/rcontrol -S --type=cpp
# 3) Locate tool list / makeToolsList / Tool definitions and whether advertised tools are known
rg -n "makeToolsList|ToolsList|advertised tools|play\"|stop\"" framework/rcontrol/mcp -S
rg -n "struct Tool|class Tool|struct ToolResult|ToolResult" framework/rcontrol/mcp -S
# 4) Inspect TCP transport init/handler registration ordering if referenced by mcpcontroller.cpp
rg -n "init\(|register|onToolsCallRequest|ToolsCallRequest|m_mcpServer->init\(" framework/rcontrol/mcp -SRepository: musescore/muse_framework
Length of output: 111
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect mcpcontroller.cpp around init() and tool handler setup
echo "---- mcpcontroller.cpp (head/around init/tool handler) ----"
cat -n framework/rcontrol/mcp/mcpcontroller.cpp | sed -n '1,220p'
# 2) Find dispatcher()->dispatch implementation/usages
echo "---- search for dispatcher()->dispatch and dispatch(name) ----"
rg -n "dispatcher\(\)->dispatch\(" framework/rcontrol -S --type=cpp
rg -n "\bdispatch\(" framework/rcontrol -S --type=cpp
# 3) Find Tool/ToolResult and tools list generation (advertised tools)
echo "---- search for Tool / ToolResult / makeToolsList / play stop ----"
rg -n "makeToolsList|ToolsList|ToolResult|struct Tool|class Tool" framework/rcontrol/mcp -S --type=cpp
rg -n "\"play\"|\"stop\"|play|stop" framework/rcontrol/mcp -S --type=cpp
# 4) Inspect tcptransport + any other transport bindings used by MCP controller
echo "---- tcptransport binding/address ----"
cat -n framework/rcontrol/mcp/transport/tcptransport.cpp | sed -n '1,160p'Repository: musescore/muse_framework
Length of output: 9470
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect MCP server init() and how it starts transport / uses handlers
echo "---- mcpserver.cpp init() / transport start ordering ----"
rg -n "void McpServer::init|McpServer::init|start\(" framework/rcontrol/mcp/mcpserver.cpp --type=cpp
sed -n '1,260p' framework/rcontrol/mcp/mcpserver.cpp | cat
# 2) Inspect Tool / ToolResult definitions
echo "---- mcptypes.h Tool / ToolResult ----"
sed -n '1,140p' framework/rcontrol/mcp/mcptypes.h | cat
# 3) Inspect ToolsCall dispatch path in mcpserver.cpp (how it calls m_onToolsCall and serializes errors)
echo "---- mcpserver.cpp onToolsCall* plumbing ----"
rg -n "onToolsCall|ToolsCall" framework/rcontrol/mcp/mcpserver.cpp --type=cpp
sed -n '140,260p' framework/rcontrol/mcp/mcpserver.cpp | cat
# 4) Inspect dispatcher() implementation and dispatch(name) behavior
echo "---- search for dispatcher() and dispatch signature ----"
rg -n "\bdispatcher\(\)" framework -S --type=cpp
rg -n "dispatch\(\s*const\s+.*std::string|dispatch\(\s*const\s+std::string|dispatch\(\s*.*QString|dispatch\(\s*.*std::u16string" framework -S --type=cpp
# Print any likely dispatch definition hits (small excerpts only)
rg -n "dispatch\(" framework/rcontrol -S --type=cppRepository: musescore/muse_framework
Length of output: 24695
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Find dispatcher implementation (C++) ----"
rg -n "class Dispatcher|Dispatcher::dispatch|dispatch\(const std::string|dispatch\(const QString|unReg\(" framework -S --type=cpp
# Narrow to likely files
fd -t f dispatcher --search-path framework || true
fd -t f "dispatcherapi.*" framework/actions -S || true
# Show dispatcher API header/impl if present
rg -n "Q_INVOKABLE void dispatch|DispatcherApi::dispatch" framework/actions -S --type=cpp
sed -n '1,120p' framework/actions/api/dispatcherapi.h 2>/dev/null || true
sed -n '1,120p' framework/actions/api/dispatcherapi.cpp 2>/dev/null || true
echo "---- Find dispatcher()->reg mapping for arbitrary string action codes ----"
rg -n "dispatcher\(\)->reg\(.*,[ ]*\"([^\"]+)\"" framework -S --type=cpp | head -n 40
echo "---- Inspect dispatcher() type used in McpController (mcpcontroller.h) ----"
sed -n '1,120p' framework/rcontrol/mcp/mcpcontroller.h | catRepository: musescore/muse_framework
Length of output: 13265
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Inspect actions dispatcher implementation ----"
sed -n '1,240p' framework/actions/internal/actionsdispatcher.cpp | cat
echo "---- Inspect iactionsdispatcher.h for action code types/signatures ----"
sed -n '1,200p' framework/actions/iactionsdispatcher.h | cat
echo "---- Inspect ActionData / ActionCode types (search) ----"
rg -n "using ActionCode|typedef .*ActionCode|struct ActionCode|class ActionData|struct ActionData|ActionQuery" framework/actions -S --type=cppRepository: musescore/muse_framework
Length of output: 12759
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "dispatcher\(\)->reg\(.*\"play\"|dispatcher\(\)->reg\(.*\"stop\"|dispatch\(\"play\"|dispatch\(\"stop\"" framework -S --type=cpp | head -n 80
rg -n "\"play\"|\"stop\"" framework/rcontrol/mcp -S --type=cppRepository: musescore/muse_framework
Length of output: 783
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,45p' framework/rcontrol/mcp/mcpcontroller.cpp | catRepository: musescore/muse_framework
Length of output: 1323
Restrict MCP tool names and return real failures (TCP 2212 is exposed)
params.nameis passed straight intodispatcher()->dispatch(name), andActionsDispatcherwill execute any registered action code matching that string (not limited to the advertised toolsplay/stop). WithTcpTransportlistening onQHostAddress::Any:2212, a remote client can invoke unintended internal actions.- The MCP handler always replies
onResult(ToolResult())(withToolResult::isErrordefaulting tofalse), even whenActionsDispatcherfinds no registered handler (it only logs and returns). McpController::init()callsm_mcpServer->init()(starts the transport) before settingonToolsListRequest/onToolsCallRequest, creating a race where early requests can error with “No tools … handler”.
Move tool handler registration before starting the transport, allow-list name against makeToolsList() (reject anything else), and set ToolResult.isError=true when rejecting or when the dispatch target isn’t handled.
🧰 Tools
🪛 Cppcheck (2.20.0)
[style] 54-54: The function 'make_arg1' is never used.
(unusedFunction)
[style] 57-57: The function 'async_isConnected' is never used.
(unusedFunction)
[style] 57-57: The function 'disableWaitPendingsOnSend' is never used.
(unusedFunction)
🤖 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 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.
Comment on lines
+86
to
+88
| if (!m_server->listen(QHostAddress::Any, 2212)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Bind to loopback instead of all interfaces.
listen(QHostAddress::Any, 2212) exposes the MCP command dispatcher on every network interface with no authentication. Since the dispatcher invokes application actions (play/stop today, broader RCommand infrastructure planned), any host on the network could drive the application. The bridge client connects to 127.0.0.1 by default, so binding to loopback is sufficient and closes this exposure.
🔒️ Proposed fix
- if (!m_server->listen(QHostAddress::Any, 2212)) {
+ if (!m_server->listen(QHostAddress::LocalHost, 2212)) {
return false;
}🧰 Tools
🪛 Clang (14.0.6)
[warning] 87-87: redundant boolean literal in conditional return statement
(readability-simplify-boolean-expr)
🤖 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 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.
4757276 to
b7603dd
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
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.
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: