Skip to content

Fix nil handling in item_type method and add parameter specs#37

Merged
patvice merged 2 commits intopatvice:mainfrom
MadBomber:nil_item_type
Jul 7, 2025
Merged

Fix nil handling in item_type method and add parameter specs#37
patvice merged 2 commits intopatvice:mainfrom
MadBomber:nil_item_type

Conversation

@MadBomber
Copy link
Contributor

This commit updates the item_type method to safely handle cases where the @items variable may be nil, preventing potential exceptions. The change utilizes the safe navigation operator &. to ensure the method returns nil instead of raising an error when @items is not properly initialized.

Additionally, this commit introduces a set of RSpec tests for the RubyLLM::MCP::Parameter class. These tests cover various cases for the item_type method, ensuring it behaves correctly regarding the presence or absence of the type key in the @items hash as well as validating the class initialization. This enhancement improves code robustness and test coverage.

In Model Context Protocol (MCP), having a nil value for the item type in a Parameter object has specific implications:

MCP Parameter Schema Context

In MCP, parameters are defined using JSON Schema, and the items property is used specifically for array-type parameters to define the schema of array elements.

What nil item_type means:

  1. Non-array parameter - If @Items["type"] is nil, it most likely means this parameter is not an array type. The items property is only relevant for parameters with "type": "array".
  2. Missing or incomplete schema - It could indicate:
  • The parameter schema doesn't include an items definition
  • The parameter is a primitive type (string, number, boolean, etc.) rather than an array
  • The schema is malformed or incomplete
  1. Expected behavior - For non-array parameters, item_type returning nil is actually correct behavior since there's no array element type to describe.

Example MCP Parameter Schemas:

// Array parameter (would have item_type)
{
"type": "array",
  "items": {
    "type": "string"  // This would make item_type return :string
  }
}

// Non-array parameter (item_type should be nil)
{
  "type": "string"  // No "items" property, so item_type is nil
}

The nil guard is correct because:

  • It prevents crashes when checking item_type on non-array parameters
  • It follows the MCP specification where items is only meaningful for array types
  • It allows the code to gracefully handle both array and non-array parameters

The nil guard makes your Parameter class more robust when dealing with the variety of parameter types that MCP supports.

This commit updates the `item_type` method to safely handle cases
where the `@items` variable may be nil, preventing potential
exceptions. The change utilizes the safe navigation operator `&.`
to ensure the method returns nil instead of raising an error when
`@items` is not properly initialized.

Additionally, this commit introduces a set of RSpec tests for the
`RubyLLM::MCP::Parameter` class. These tests cover various cases
for the `item_type` method, ensuring it behaves correctly
regarding the presence or absence of the `type` key in the
`@items` hash as well as validating the class initialization.
This enhancement improves code robustness and test coverage.

In Model Context Protocol (MCP), having a nil value for the item type in a Parameter object has specific implications:

MCP Parameter Schema Context

In MCP, parameters are defined using JSON Schema, and the items property is used specifically for array-type parameters to
define the schema of array elements.

What nil item_type means:

1. Non-array parameter - If @Items["type"] is nil, it most likely means this parameter is not an array type. The items
property is only relevant for parameters with "type": "array".
2. Missing or incomplete schema - It could indicate:
- The parameter schema doesn't include an items definition
- The parameter is a primitive type (string, number, boolean, etc.) rather than an array
- The schema is malformed or incomplete
3. Expected behavior - For non-array parameters, item_type returning nil is actually correct behavior since there's no array
element type to describe.

Example MCP Parameter Schemas:

```json
// Array parameter (would have item_type)
{
"type": "array",
  "items": {
    "type": "string"  // This would make item_type return :string
  }
}

// Non-array parameter (item_type should be nil)
{
  "type": "string"  // No "items" property, so item_type is nil
}
```

The nil guard is correct because:

- It prevents crashes when checking item_type on non-array parameters
- It follows the MCP specification where items is only meaningful for array types
- It allows the code to gracefully handle both array and non-array parameters

The nil guard makes your Parameter class more robust when dealing with the variety of parameter types that MCP supports.

def item_type
@items["type"].to_sym
@items&.[]("type")&.to_sym
Copy link
Owner

@patvice patvice Jul 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible to use dig on this, that's the pattern that used in other locations in the codebase

@items&.dig("type")&.to_sym


RSpec.describe RubyLLM::MCP::Parameter do
describe "#item_type" do
context "when @items is nil" do
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These context block was just on it statments - can you collapse them down or bucket some it statements together

# frozen_string_literal: true

RSpec.describe RubyLLM::MCP::Parameter do
describe "#item_type" do
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at your specs, we are missing the case that kicked off the issue in the first place. An array type without a item type attached. Possible to add that case?

@patvice
Copy link
Owner

patvice commented Jul 6, 2025

The premise of this PR makes sense, and I added a few comments for feedback.

You do have a failing spec that would need to be corrected, otherwise should be good to merge with some corrections and passing specs.

Failures:

  1) RubyLLM::MCP::Parameter#initialize creates a parameter with custom values
     Failure/Error: expect(parameter.desc).to eq("Test description")

     NoMethodError:
       undefined method `desc' for #<RubyLLM::MCP::Parameter:0x00007f222efa1688 @name="test_param", @type=:array, @description="Test description", @required=false, @properties={}, @union_type=nil, @default=nil>

             expect(parameter.desc).to eq("Test description")
                             ^^^^^
     # ./spec/ruby_llm/mcp/parameter_spec.rb:50:in `block (3 levels) in <top (required)>'
     # ./vendor/bundle/ruby/3.1.0/gems/webmock-3.25.1/lib/webmock/rspec.rb:39:in `block (2 levels) in <top (required)>'

@MadBomber
Copy link
Contributor Author

The test suite as many failures I think because it has some manual steps that must be performed w/r/t to either the vcr or live server setups.

@patvice
Copy link
Owner

patvice commented Jul 7, 2025

Really! VCR specs are only used when we have tests that are communicated to an LLM service. We do use a MCP server that is ran in bun to test a lot of the client to server interactions, maybe that's the issue.

Maybe just focus on fixing RubyLLM::MCP::Parameter spec and leave the rest, that should allow CI to pass

@patvice
Copy link
Owner

patvice commented Jul 7, 2025

If you are having trouble - I'm happy to go in an fix up the spec on my machine, let me know

@patvice patvice merged commit 441512c into patvice:main Jul 7, 2025
9 of 10 checks passed
@patvice
Copy link
Owner

patvice commented Jul 7, 2025

Hey @MadBomber - had a moment this morning and when ahead and got your PR passing. :shipit:

Will end up putting all of this into a v0.5 release which I should be getting out today. Thanks again for the contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants