added commands register#87
Conversation
|
Warning Review limit reached
More reviews will be available in 37 minutes and 8 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ 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 (15)
📝 WalkthroughWalkthroughThe pull request renames the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/rcommand/internal/commanddispatcher.cpp (1)
44-53:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
clientandcallbackbefore registering a command.Registration currently dereferences
clientand later assumescallbackis callable. A nullclientor emptycallbackcauses runtime failure paths.Proposed fix
void CommandDispatcher::onRequest(Commandable* client, const Command& command, const CallBack& callback) { + if (!client || !callback) { + LOGW() << "invalid command registration"; + return; + } + IF_ASSERT_FAILED(m_clients.find(command) == m_clients.end()) { LOGW() << "command already registered: " << command; return; } m_clients[command] = { client, callback }; client->setDispatcher(this); }🤖 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/rcommand/internal/commanddispatcher.cpp` around lines 44 - 53, The CommandDispatcher::onRequest method does not validate that the client parameter is non-null before using it in client->setDispatcher(this), and does not validate that the callback parameter is valid before storing it in m_clients. Add null checks for the client pointer and validate that callback is not empty before proceeding with the registration. If either validation fails, log an appropriate error message and return early to prevent runtime failures from dereferencing invalid pointers or storing invalid callables.
🤖 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/rcommand/CMakeLists.txt`:
- Around line 23-35: The public interface header `icommandsregister.h` is
missing from the target_sources list for the muse_rcommand target in the
CMakeLists.txt file. Add `icommandsregister.h` to the target_sources PRIVATE
section alongside the other public interface headers like `icommanddispatcher.h`
and `imodulecommands.h` to ensure consistency with other public headers and to
properly include it in any header export or install metadata.
In `@framework/rcommand/commandtypes.h`:
- Around line 52-56: The DataType type member variable in the Arg struct lacks a
deterministic default initialization, causing undefined behavior when Arg
objects are default-constructed (such as when std::map constructs values
implicitly). Initialize the DataType type field to a sensible default value
(typically the first enum value in the DataType enum or an explicitly defined
default enum value) by adding an initializer in the struct member declaration to
ensure predictable behavior during default initialization and serialization.
In `@framework/rcommand/internal/commandsregister.cpp`:
- Around line 25-33: Add null pointer checks to both the reg() and unreg()
methods in the CommandsRegister class to prevent crashes when a null module
pointer is passed. In the reg() method, verify that the module pointer is not
null before calling moduleName() and inserting into the m_modules map.
Similarly, in the unreg() method, add a null check before calling moduleName()
and erasing from the map. Return early or skip the operation if the module is
null to prevent undefined behavior.
- Around line 38-40: In the for loop iterating over m_modules, the
commandInfos() method is being called twice on the same module.second object
within the insert statement. To optimize this, extract the return value of
module.second->commandInfos() into a local const reference variable before the
insert call, and then use that reference's begin() and end() methods in the
insert statement instead of calling commandInfos() twice.
In `@framework/rcontrol/mcp/mcpcontroller.cpp`:
- Around line 97-106: The tool's inputSchema is being initialized as empty with
InputSchema() instead of being populated with the actual argument schema from
CommandInfo::inputSchema. Uncomment and properly implement the schema mapping
loop that iterates through info.inputSchema.args, creating Property objects for
each argument with the name, type, description, minimum, and maximum fields
populated from the corresponding CommandInfo argument metadata, then add these
properties to the tool's inputSchema before pushing the tool to the tools
collection.
- Around line 71-74: The code at the dispatch call site fire-and-forgets the
commandsDispatcher()->dispatch(commandQuery(...)) call and immediately invokes
onResult(ToolResult()) without waiting for completion or capturing any errors.
Modify this code to wait for the dispatcher to complete execution, capture any
errors or failures from the dispatch operation (including unknown command
paths), and pass an appropriate ToolResult to onResult that reflects success or
failure based on the dispatcher's actual result rather than always returning a
successful ToolResult.
- Around line 42-54: The commandToToolName and commandQuery functions use a
non-bijective encoding scheme where converting "/" to "_" causes collisions (for
example, "a/b_c" and "a/b/c" both map to the same string), and the args
parameter in commandQuery is unused, causing tool-call arguments to be dropped.
Implement a proper bijective encoding scheme in both commandToToolName and
commandQuery functions that can reliably convert between "/" and "_" without
collisions (consider escaping or using an alternative delimiter encoding), and
pass the args parameter into the CommandQuery constructor when constructing the
return value in commandQuery.
---
Outside diff comments:
In `@framework/rcommand/internal/commanddispatcher.cpp`:
- Around line 44-53: The CommandDispatcher::onRequest method does not validate
that the client parameter is non-null before using it in
client->setDispatcher(this), and does not validate that the callback parameter
is valid before storing it in m_clients. Add null checks for the client pointer
and validate that callback is not empty before proceeding with the registration.
If either validation fails, log an appropriate error message and return early to
prevent runtime failures from dereferencing invalid pointers or storing invalid
callables.
🪄 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: 067bc26f-0a43-45bb-b50b-fca48c73f226
📒 Files selected for processing (15)
framework/rcommand/CMakeLists.txtframework/rcommand/commandable.hframework/rcommand/commandtypes.hframework/rcommand/icommanddispatcher.hframework/rcommand/icommandsregister.hframework/rcommand/imodulecommands.hframework/rcommand/internal/commanddispatcher.cppframework/rcommand/internal/commanddispatcher.hframework/rcommand/internal/commandsregister.cppframework/rcommand/internal/commandsregister.hframework/rcommand/rcommandmodule.cppframework/rcommand/rcommandmodule.hframework/rcontrol/mcp/mcpcontroller.cppframework/rcontrol/mcp/mcpcontroller.hframework/rcontrol/mcp/mcptypes.h
| target_sources(muse_rcommand PRIVATE | ||
| rcommandmodule.cpp | ||
| rcommandmodule.h | ||
| ircommanddispatcher.h | ||
| rcommandable.h | ||
| rcommandtypes.h | ||
| icommanddispatcher.h | ||
| imodulecommands.h | ||
| commandable.h | ||
| commandtypes.h | ||
|
|
||
| internal/rcommanddispatcher.cpp | ||
| internal/rcommanddispatcher.h | ||
| internal/commanddispatcher.cpp | ||
| internal/commanddispatcher.h | ||
| internal/commandsregister.cpp | ||
| internal/commandsregister.h | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check how muse_rcommand headers are surfaced/installed and whether icommandsregister.h is covered.
rg -n "target_sources\\(muse_rcommand|icommandsregister\\.h|FILE_SET|install\\(TARGETS\\s+muse_rcommand" -C3Repository: musescore/muse_framework
Length of output: 1455
🏁 Script executed:
cat -n framework/rcommand/CMakeLists.txtRepository: musescore/muse_framework
Length of output: 1519
🏁 Script executed:
# Search for install rules and header export patterns in this module
rg -n "install|FILE_SET|PUBLIC|INTERFACE" framework/rcommand/CMakeLists.txt -A 2Repository: musescore/muse_framework
Length of output: 50
🏁 Script executed:
# Check the muse_create_module function to understand how headers are handled
rg -n "muse_create_module" --type cmake -A 5 | head -50Repository: musescore/muse_framework
Length of output: 2696
🏁 Script executed:
# Find muse_create_module definition
rg -n "macro|function" --type cmake | rg "muse_create_module" -B 2 -A 10Repository: musescore/muse_framework
Length of output: 2094
🏁 Script executed:
# Check if icommandsregister.h exists
ls -la framework/rcommand/ | grep -i icommandsRepository: musescore/muse_framework
Length of output: 138
🏁 Script executed:
# Look for cmake function definitions that might handle module creation
fd -name "*.cmake" -type f | head -20Repository: musescore/muse_framework
Length of output: 299
🏁 Script executed:
sed -n '45,129p' framework/cmake/MuseCreateModule.cmakeRepository: musescore/muse_framework
Length of output: 2908
🏁 Script executed:
# Also check content of icommandsregister.h to confirm it's a public interface
cat -n framework/rcommand/icommandsregister.hRepository: musescore/muse_framework
Length of output: 1488
🏁 Script executed:
# Search for install rules in parent CMakeLists or related cmake files
rg -n "install.*DIRECTORY|FILE_SET.*HEADERS|PUBLIC_HEADER" framework/CMakeLists.txt buildscripts/cmake/ -A 3 | head -100Repository: musescore/muse_framework
Length of output: 50
🏁 Script executed:
# Check if there's a parent CMakeLists that handles module installation
cat -n framework/CMakeLists.txt | head -60Repository: musescore/muse_framework
Length of output: 2048
🏁 Script executed:
# Verify if icommandsregister.h is accessible as a public include without being in target_sources
rg -n "icommandsregister" -A 2 -B 2 | head -80Repository: musescore/muse_framework
Length of output: 751
🏁 Script executed:
# Search for install rules that might use target_sources or install headers
rg -n "install.*FILES|install.*DIRECTORY" buildscripts/ --type cmake | head -40Repository: musescore/muse_framework
Length of output: 774
🏁 Script executed:
# Check if there's a top-level CMakeLists with install rules
find . -maxdepth 2 -name CMakeLists.txt -exec grep -l "install" {} \;Repository: musescore/muse_framework
Length of output: 50
🏁 Script executed:
# Look for any cmake that might derive install from target_sources
rg -n "PUBLIC_HEADER|FILE_SET" buildscripts/cmake/ --type cmakeRepository: musescore/muse_framework
Length of output: 50
🏁 Script executed:
# Check if other interface headers (.h files not paired with .cpp) are in target_sources
rg -n "\.h" framework/rcommand/CMakeLists.txt | grep "target_sources" -A 20Repository: musescore/muse_framework
Length of output: 50
🏁 Script executed:
# Look for patterns: do interface-only headers get added to target_sources elsewhere?
rg -l "\.h" framework/*/CMakeLists.txt | xargs grep -l "icommand\|idispatcher\|imodule" | head -5Repository: musescore/muse_framework
Length of output: 192
🏁 Script executed:
# Check rcommand module structure - count .h vs .cpp files
find framework/rcommand -name "*.h" -o -name "*.cpp" | sortRepository: musescore/muse_framework
Length of output: 519
Add icommandsregister.h to the module's public header set.
This is a public interface header (marked MODULE_GLOBAL_INTERFACE) and is imported by downstream modules (rcontrol/mcp/mcpcontroller.h). Other public interface headers in this module (icommanddispatcher.h, imodulecommands.h) are explicitly listed in target_sources, so icommandsregister.h should be added for consistency and to ensure it's included in any header export/install metadata.
🤖 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/rcommand/CMakeLists.txt` around lines 23 - 35, The public interface
header `icommandsregister.h` is missing from the target_sources list for the
muse_rcommand target in the CMakeLists.txt file. Add `icommandsregister.h` to
the target_sources PRIVATE section alongside the other public interface headers
like `icommanddispatcher.h` and `imodulecommands.h` to ensure consistency with
other public headers and to properly include it in any header export or install
metadata.
| static std::string commandToToolName(const Command& command) | ||
| { | ||
| std::string path = command.path(); | ||
| muse::strings::replace(path, "/", "_"); | ||
| return path; | ||
| } | ||
|
|
||
| static CommandQuery commandQuery(const std::string& name, const JsonObject& args) | ||
| { | ||
| std::string path = name; | ||
| muse::strings::replace(path, "_", "/"); | ||
| return CommandQuery(path); | ||
| } |
There was a problem hiding this comment.
Make tool-name conversion reversible and forward MCP arguments into CommandQuery.
The /↔_ replacement is not bijective (a/b_c and a/b/c collide), and the args parameter (Line 49) is currently unused. This can dispatch the wrong command and silently drop tool-call parameters.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 42-42: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 49-49: use a trailing return type for this function
(modernize-use-trailing-return-type)
🤖 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 42 - 54, The
commandToToolName and commandQuery functions use a non-bijective encoding scheme
where converting "/" to "_" causes collisions (for example, "a/b_c" and "a/b/c"
both map to the same string), and the args parameter in commandQuery is unused,
causing tool-call arguments to be dropped. Implement a proper bijective encoding
scheme in both commandToToolName and commandQuery functions that can reliably
convert between "/" and "_" without collisions (consider escaping or using an
alternative delimiter encoding), and pass the args parameter into the
CommandQuery constructor when constructing the return value in commandQuery.
| commandsDispatcher()->dispatch(commandQuery(name, args)); | ||
|
|
||
| onResult(ToolResult()); | ||
| }); |
There was a problem hiding this comment.
Wait for dispatcher completion before replying to MCP tool calls.
This path fire-and-forgets dispatch(...) and immediately returns ToolResult() (isError=false). Dispatcher failures (including unknown command paths) are never propagated to MCP callers.
🤖 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 71 - 74, The code at
the dispatch call site fire-and-forgets the
commandsDispatcher()->dispatch(commandQuery(...)) call and immediately invokes
onResult(ToolResult()) without waiting for completion or capturing any errors.
Modify this code to wait for the dispatcher to complete execution, capture any
errors or failures from the dispatch operation (including unknown command
paths), and pass an appropriate ToolResult to onResult that reflects success or
failure based on the dispatcher's actual result rather than always returning a
successful ToolResult.
| tool.inputSchema = InputSchema(); | ||
| // for (const auto& arg : info.inputSchema.args) { | ||
| // Property property; | ||
| // property.name = String::fromStdString(arg.first); | ||
| // property.type = String::fromStdString(arg.second.type); | ||
| // property.description = String::fromStdString(arg.second.description); | ||
| // property.minimum = String::fromStdString(arg.second.minimum); | ||
| // property.maximum = String::fromStdString(arg.second.maximum); | ||
| // } | ||
| tools.push_back(std::move(tool)); |
There was a problem hiding this comment.
Publish real argument schema from CommandInfo::inputSchema instead of an empty schema.
Each tool currently exposes InputSchema() regardless of command metadata, while schema mapping code is left commented out. MCP clients cannot reliably infer required arguments/types for registered commands.
🤖 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 97 - 106, The tool's
inputSchema is being initialized as empty with InputSchema() instead of being
populated with the actual argument schema from CommandInfo::inputSchema.
Uncomment and properly implement the schema mapping loop that iterates through
info.inputSchema.args, creating Property objects for each argument with the
name, type, description, minimum, and maximum fields populated from the
corresponding CommandInfo argument metadata, then add these properties to the
tool's inputSchema before pushing the tool to the tools collection.
igorkorsukov
force-pushed
the
w/rcmd/rcmd_step3
branch
from
af635de to
cc57642
Compare
igorkorsukov
force-pushed
the
w/rcmd/rcmd_step3
branch
from
cc57642 to
1b2136f
Compare
2 participants
Summary by CodeRabbit
Release Notes
New Features
Refactor