ruby-sdk icon indicating copy to clipboard operation
ruby-sdk copied to clipboard

Unintuitive use of meta programming for :server context in tool call

Open sergioisidoro opened this issue 6 months ago • 4 comments

I've spent a good deal of time trying to figure out why :server_context was not being passed into the tool call I was using *args and **kargs to debug things and still could not figure out why I could not get the server context in tool calls. I was going crazy.

Finally went looking at the code, and it seems to do some reflection and change its behaviour in a bit strange ways.

        call_params = tool_call_parameters(tool)

        if call_params.include?(:server_context)
          tool.call(**arguments.transform_keys(&:to_sym), server_context:).to_h
        else
          tool.call(**arguments.transform_keys(&:to_sym)).to_h
        end

Since context is used for authentication and authorization this is a bit strange to me. Why not always pass :server_context to a tool call and introduce complexity with this introspection, that only works if the signature explicitly mentions the parameter? 🤔

sergioisidoro avatar Jun 03 '25 22:06 sergioisidoro

It should probably be better documented, but I'd personally prefer if the server_context is not passed as a useless argument if I don't need to use it in the tool...

Local running MCP servers don't need this property since the tools can just read the Environment Variables or local config files directly.

kfischer-okarin avatar Jun 04 '25 14:06 kfischer-okarin

That's fair, but I think the method of reflection is brittle and breaks a few common patterns of ruby (eg. *args **kwargs). Also something to have in consideration would be to use defensive programing, ensuring that the tool call cannot in any instance pass the argument server_contex as a parameter to the tool (possibly overriding parameters used for permission checks)

Using positional arguments or keyword arguments would be a defensive approach. ie call(:tool_params, :server_context)

And even in local MCP servers, relying on the environment in the tool call does not sound super appropriate, as you may want to have shared context across all tools. Calling environment in the tool call would have a significant code replication across all tools

sergioisidoro avatar Jun 04 '25 15:06 sergioisidoro

Quick update: It seems that everything works as expected in the tool call in the tests of this repo, even when I add more extensive tests with *args, **kwargs, and other variations.

I'm a bit lost here as to why server_context is not passed on some occasions. My best guess is that my environment tool_call_method_def does not return the same results reliably. If I remove the if condition mentioned above from the gem in my bundle, everything works as expected.

Running this on ruby:3.3.5 without sorbet


Edit: The only way to get this to work with with splats, is by doing this

    def self.call(*args, server_context: nil, **tool_arguments)

Which is unpleasant, to say the least.

sergioisidoro avatar Jun 04 '25 16:06 sergioisidoro

Sounds like the metaprogramming implementation can be definitely improved then... 👍🏻 if the tool call has a kwargs splat it should include server_context in there definitely

kfischer-okarin avatar Jun 05 '25 05:06 kfischer-okarin

@sergioisidoro How do you feel about the latest state of things on main?

atesgoral avatar Jul 14 '25 22:07 atesgoral

@atesgoral I still need to test how everything works, but from a quick glance at main it seems #54 addresses all my concerns beautifully :) I'll close.

Btw, a bit outside of the scope of this ticket, but appying the same pattern of #54 toresources_read_handler would also be very nice. At the moment I have to use function generators to pass server context to resoruce read handler (to ensure resource permission scopes in the reader)

    server.resources_read_handler do |params|
      [
        # current_project is part of the server context
        Mcp::V1::Resources::TeamResources.build_listen_to_team_resources_handler(current_project.id).call(params)
      ].flatten.compact
    end

sergioisidoro avatar Jul 15 '25 15:07 sergioisidoro