Replace rubydex_mcp server with Ruby implementation#863
Conversation
f13e11d to
6a9ca41
Compare
rubydex_mcp server with Ruby implementation
6a9ca41 to
db9f6ba
Compare
Assisted-By: devx/0b82d122-3a3f-4089-9195-b78154de2f9a
0f6b7c8 to
077eb83
Compare
Assisted-By: devx/0b82d122-3a3f-4089-9195-b78154de2f9a
077eb83 to
d88a604
Compare
| end | ||
|
|
||
| begin | ||
| option_parser.parse!(ARGV) |
There was a problem hiding this comment.
FWIW, one of the things I regret in the LSP was using parse! instead of the non-raising version parse. It makes it more difficult to make backwards incompatible changes to accepted flags, even for development related stuff.
| exit(2) | ||
| end | ||
|
|
||
| path = ARGV.fetch(0, ".") |
There was a problem hiding this comment.
Better to use absolute paths when possible.
| path = ARGV.fetch(0, ".") | |
| path = ARGV.fetch(0, Dir.pwd) |
| require "rubydex/mcp_server/tools/codebase_stats_tool" | ||
| require "rubydex/mcp_server/tools/find_constant_references_tool" | ||
| require "rubydex/mcp_server/tools/get_declaration_tool" | ||
| require "rubydex/mcp_server/tools/get_descendants_tool" | ||
| require "rubydex/mcp_server/tools/get_file_declarations_tool" | ||
| require "rubydex/mcp_server/tools/search_declarations_tool" |
There was a problem hiding this comment.
So we don't have to keep adding to the list.
| require "rubydex/mcp_server/tools/codebase_stats_tool" | |
| require "rubydex/mcp_server/tools/find_constant_references_tool" | |
| require "rubydex/mcp_server/tools/get_declaration_tool" | |
| require "rubydex/mcp_server/tools/get_descendants_tool" | |
| require "rubydex/mcp_server/tools/get_file_declarations_tool" | |
| require "rubydex/mcp_server/tools/search_declarations_tool" | |
| Dir.glob("rubydex/mcp_server/tools/**/*.rb").each { |f| require f } |
| SERVER_INSTRUCTIONS = <<~TEXT | ||
| Rubydex provides semantic Ruby code intelligence. | ||
|
|
||
| ONLY use these tools for Ruby files (.rb, .rbi, .rbs) -- never for Rust, JavaScript, or other languages. |
There was a problem hiding this comment.
Now I'm wondering if the reference to other language names will make it better or worse at picking up the MCP 😂
| Decision guide: | ||
| - Know a name? -> search_declarations (fuzzy search by name) | ||
| - Have an exact fully qualified name? -> get_declaration (full details with docs, ancestors, members) | ||
| - Need reverse hierarchy? -> get_descendants (what inherits from this class/module) | ||
| - Refactoring a class/module/constant? -> find_constant_references (all precise usages across codebase) | ||
| - Exploring a file? -> get_file_declarations (structural overview) | ||
| - Want general statistics? -> codebase_stats (size and composition) |
There was a problem hiding this comment.
Is this maybe a better fit for the individual tool descriptions?
| end | ||
| end | ||
|
|
||
| class Server |
There was a problem hiding this comment.
IMO, it would be easier to follow the architecture if we merged Server and State into a single object and kept things like error codes and convenience objects inside protocol.
| INVALID_PARAMS = -32_602 | ||
| INTERNAL_ERROR = -32_603 | ||
|
|
||
| #: (Hash | Array | untyped) -> Hash | Array[Hash]? |
There was a problem hiding this comment.
Can the MCP send asynchronous messages to the client like an LSP can? If so, we're making the same mistake we originally made in the LSP here: implementing a handle method that returns the response to the sent to the client.
That approach makes it really difficult to have a single operation (like a tool call) both send an asynchronous notification and return a response.
That's why in the LSP we have an outgoing queue with a dispatcher thread. Any request/notification can enqueue as many messages as desired to be sent to the client.
| has_id = request.key?(:id) || request.key?("id") | ||
| id = request.key?(:id) ? request[:id] : request["id"] | ||
| method = request[:method] || request["method"] | ||
| params = request.key?(:params) ? request[:params] : request["params"] |
There was a problem hiding this comment.
Do we need to check if it's string or symbol? If we control the JSON parsing, we should be able to use symbolize_names: true and standardize everything on symbols.
| #: (Hash) -> Hash | ||
| def call_tool(params) | ||
| tool_name = params[:name] || params["name"] | ||
| tool = TOOLS.find { |candidate| candidate.to_h.fetch(:name) == tool_name } |
There was a problem hiding this comment.
Instead of an array, we should probably use a hash and make this a simple lookup.
|
|
||
| #: -> void | ||
| def open | ||
| @input.each_line do |line| |
There was a problem hiding this comment.
Is this using JSONRPC? The separator should be \r\n\r\n and not a single line. So you need something like gets("\r\n\r\n"). And the first line you get is the content length, so after the gets you need to read the specified number of bytes.
For example, something like this:
headers = io.gets("\r\n\r\n")
raise unless headers
length = headers[/Content-Length: (\d+)/i, 1]
raise unless length
raw_message = io.read(length)
message = JSON.parse(raw_message)
Summary
Adds a Ruby implementation of
rubydex_mcpwith a small internal MCP/JSON-RPC protocol layer.The existing Rust MCP crate and build plumbing are left in place. If this PR is accepted, I'll remove it in a follow up PR.
Parity with the Rust MCP server
Matches the existing Rust MCP server for:
search_declarations,get_declaration,get_descendants,find_constant_references,get_file_declarations,codebase_statsManually checked against
rust/target/debug/rubydex_mcpusing the same fixture and tool calls for the six tools.Known difference:
Follow-up work