diff --git a/CHANGELOG.md b/CHANGELOG.md index e525ecf..1cea5c6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,27 @@ +## [0.3.0] - 2026-07-03 + +### Added + +- **Resource discovery DX** improvements, closing gaps hit when an MCP agent had to guess a + related resource's name rather than discover it: + - The `resource_schema` tool now names, for each relationship, the `target_resource` it + resolves to (callable via `list` / `get`). A `scheduled_notifications.notification` link is + thus discoverably the `notifications` resource instead of a name to guess. Additive and + backward compatible: it is omitted when the target can't be resolved (e.g. a polymorphic + link), so existing relationship consumers are unaffected. + - An unknown resource name now raises a "did you mean" message — the closest registered + name(s) via Ruby's stdlib `DidYouMean::SpellChecker` (with a dependency-free edit-distance + fallback), plus the full list when the catalog is short. It flows unchanged to the caller as + a clean `InvalidParams` tool error via the existing `resolve_descriptor` path. + +### Changed + +- The transport controller now logs a WARN when a POST arrives with no matching session + (`mcp_render_session_not_found`): a greppable, id/token-free line recording only whether a + session-id header was present, so a non-shared `cache_store` misconfiguration surfaces in a + satellite's own logs instead of only as a client-side 404. The logger defaults to + `Rails.logger` and is overridable via the new `mcp_logger` controller hook. + ## [0.2.0] - 2026-07-02 ### Added diff --git a/lib/mcp_toolkit.rb b/lib/mcp_toolkit.rb index f34cce7..5e9528d 100644 --- a/lib/mcp_toolkit.rb +++ b/lib/mcp_toolkit.rb @@ -38,6 +38,8 @@ # time/conversions - Time#iso8601 (Serializer::Base timestamps; # Time.zone.parse in ListExecutor) # date_time/conversions - DateTime#iso8601 (token expires_at may be a DateTime) +# string/inflections - String#pluralize (ResourceSchema resolves a link's +# singular name to the plural registered resource name) require "active_support" require "active_support/core_ext/object/blank" require "active_support/core_ext/hash/keys" @@ -45,6 +47,7 @@ require "active_support/core_ext/enumerable" require "active_support/core_ext/time/conversions" require "active_support/core_ext/date_time/conversions" +require "active_support/core_ext/string/inflections" # The version constant is needed eagerly by the gemspec (before the loader is set # up), so it stays an explicit require rather than an autoload. diff --git a/lib/mcp_toolkit/registry.rb b/lib/mcp_toolkit/registry.rb index 6e56725..3a5c301 100644 --- a/lib/mcp_toolkit/registry.rb +++ b/lib/mcp_toolkit/registry.rb @@ -43,7 +43,7 @@ def required_scope_for(resource) end def fetch(name) - find(name) or raise(UnknownResource, "unknown resource: #{name.inspect}") + find(name) or raise(UnknownResource, McpToolkit::UnknownResourceMessage.new(name, resource_names).build) end def find(name) diff --git a/lib/mcp_toolkit/resource_schema.rb b/lib/mcp_toolkit/resource_schema.rb index b7b6d51..8eb880e 100644 --- a/lib/mcp_toolkit/resource_schema.rb +++ b/lib/mcp_toolkit/resource_schema.rb @@ -21,13 +21,17 @@ class McpToolkit::ResourceSchema COMPUTED_TYPE = "computed" STANDARD_FILTERS = %w[ids updated_since limit offset].freeze - def self.call(resource) - new(resource).call + # `registry` is used to resolve each relationship to the registered resource it + # points at (see #relationships). Defaults to the process-wide registry; the + # `resource_schema` tool passes the active config's registry explicitly. + def self.call(resource, registry: McpToolkit.registry) + new(resource, registry:).call end - def initialize(resource) + def initialize(resource, registry: McpToolkit.registry) @resource = resource @model = resource.model + @registry = registry end def call @@ -43,7 +47,7 @@ def call private - attr_reader :resource, :model + attr_reader :resource, :model, :registry def attributes resource.attribute_names.map { |name| attribute_schema(name) } @@ -85,13 +89,68 @@ def filterable_column_for(attribute_name) end def relationships - resource.association_descriptors.map do |association| - { - name: association.links_key, - kind: association.type.to_s, - polymorphic: association.polymorphic || false - } + resource.association_descriptors.map { |association| relationship_schema(association) } + end + + # One relationship entry. Beyond the link key/kind/polymorphic flag it now also + # names the `target_resource` — the registered resource this link resolves to, + # callable via `list`/`get` — so e.g. a `scheduled_notifications.notification` + # link is discoverably the `notifications` resource rather than a name to guess. + # It is omitted (additive/backward-compatible) when the target can't be resolved + # (e.g. a polymorphic link). + def relationship_schema(association) + target = target_resource_for(association) + { + name: association.links_key, + kind: association.type.to_s, + polymorphic: association.polymorphic || false, + target_resource: target&.name + }.compact + end + + # The registered resource an association points at, or nil. Polymorphic links + # have no single target, so they are left unresolved (the `polymorphic` flag and + # the record's `{id:, type:}` link value already carry the target type). Prefers + # an explicit target serializer's model, then falls back to matching the link's + # (pluralized) name against registered resource names. + def target_resource_for(association) + return nil if association.polymorphic + + target_from_serializer(association) || target_from_name(association) + end + + def target_from_serializer(association) + serializer = association.serializer + return nil unless serializer.respond_to?(:model_class) + + target_model = safe_model_class(serializer) + return nil unless target_model + + registry.resources.find { |candidate| candidate.model == target_model } + end + + def target_from_name(association) + candidate_names(association).each do |candidate| + match = registry.find(candidate) + return match if match end + nil + end + + # Names to try against the registry for a link, closest spelling first: the link + # key and association name as declared, then their pluralizations (resources are + # registered under plural names, so a singular `notification` link resolves to + # the `notifications` resource). + def candidate_names(association) + [association.links_key, association.name.to_s].flat_map do |base| + [base, base.pluralize] + end.uniq + end + + def safe_model_class(serializer) + serializer.model_class + rescue StandardError + nil end # Reads an attribute's DB column type, tolerating models that don't expose diff --git a/lib/mcp_toolkit/tools/resource_schema.rb b/lib/mcp_toolkit/tools/resource_schema.rb index 6350620..cdd43c6 100644 --- a/lib/mcp_toolkit/tools/resource_schema.rb +++ b/lib/mcp_toolkit/tools/resource_schema.rb @@ -8,7 +8,8 @@ class McpToolkit::Tools::ResourceSchema < McpToolkit::Tools::Base Describe a single read-only resource in detail. Pass the resource name as `resource` (use the `resources` tool to discover names). Returns: - attributes: every field in the response, each with its `type` and a value `format` hint - - relationships: associated resources emitted in the record's `links` + - relationships: associated resources emitted in the record's `links`; each names the + `target_resource` it resolves to (callable via `list`/`get`) - standard_filters: ids, updated_since, limit, offset (accepted by the `list` tool) - filters: the per-attribute equality filter keys the `list` tool accepts The `attributes` and `relationships` names are also the valid values for the `fields` sparse @@ -31,7 +32,7 @@ def self.call(server_context:, resource: nil, **_args) # of THIS resource's shape (and an unknown resource is a clean tool error). descriptor = resolve_descriptor(resource, config) with_authentication(server_context, required_scope: config.registry.required_scope_for(descriptor)) do - McpToolkit::ResourceSchema.call(descriptor) + McpToolkit::ResourceSchema.call(descriptor, registry: config.registry) end rescue McpToolkit::Errors::InvalidParams => e error_response("Invalid request: #{e.message}") diff --git a/lib/mcp_toolkit/transport/controller_methods.rb b/lib/mcp_toolkit/transport/controller_methods.rb index 3fdb958..f5e004f 100644 --- a/lib/mcp_toolkit/transport/controller_methods.rb +++ b/lib/mcp_toolkit/transport/controller_methods.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require "logger" + # The MCP Streamable-HTTP transport, provided as an includable concern. An # app's controller includes this to get the full transport with no per-app code: # @@ -102,6 +104,16 @@ def mcp_extra_tools [] end + # Logger for transport-level diagnostics. Defaults to Rails.logger when running + # inside Rails, and to a $stdout logger otherwise (e.g. the gem's own unit suite) + # so a diagnostic is never silently dropped. Overridable so a host can inject its + # own; never returns nil. + def mcp_logger + return Rails.logger if defined?(Rails) + + @mcp_logger ||= Logger.new($stdout) + end + # ---- per-request context -------------------------------------------- # Per-request context threaded to the tools. The gem merges the request's @@ -190,10 +202,25 @@ def mcp_render_unauthorized(message) end def mcp_render_session_not_found + mcp_log_session_not_found render json: { jsonrpc: "2.0", id: nil, error: { code: -32_001, message: "Session not found or expired" } }, status: :not_found end + + # Warns (greppable, no id/token) when a POST arrives with no matching session. + # The common cause is a session created on one process but looked up on another + # because `cache_store` isn't a shared store — invisible otherwise, since the + # caller just sees a 404. Records only whether a session-id header was PRESENT so + # a header-missing client bug is distinguishable from a cache misconfiguration. + def mcp_log_session_not_found + header_present = request.headers[SESSION_HEADER].present? + mcp_logger.warn( + "[McpToolkit] MCP session not found or expired " \ + "(#{SESSION_HEADER} header present: #{header_present}). If sessions are created but not " \ + "found, cache_store is likely not shared across processes (set it to a shared store, e.g. Rails.cache)." + ) + end end diff --git a/lib/mcp_toolkit/unknown_resource_message.rb b/lib/mcp_toolkit/unknown_resource_message.rb new file mode 100644 index 0000000..1805682 --- /dev/null +++ b/lib/mcp_toolkit/unknown_resource_message.rb @@ -0,0 +1,75 @@ +# frozen_string_literal: true + +begin + require "did_you_mean" +rescue LoadError + nil +end + +# Builds the McpToolkit::Registry::UnknownResource message from a bad name and the +# registered resource names: it states the bad name, then (a) suggests the nearest +# registered name(s) for a near-miss — via Ruby's stdlib DidYouMean spell checker +# when available, else a dependency-free prefix/substring + edit-distance fallback — +# and (b), when the catalog is short, lists them all, so a caller (typically an MCP +# agent that guessed a name) can self-correct without another round-trip to the +# `resources` tool. +class McpToolkit::UnknownResourceMessage + FULL_LIST_MAX = 10 + MAX_SUGGESTIONS = 3 + + def initialize(name, resource_names) + @name = name + @resource_names = resource_names + end + + def build + message = "unknown resource: #{@name.inspect}" + return message if @resource_names.empty? + + suggestions = suggestions_for(@name.to_s, @resource_names) + message += ". Did you mean #{quote_join(suggestions)}?" if suggestions.any? + message += " Registered resources: #{quote_join(@resource_names.sort)}." if @resource_names.size <= FULL_LIST_MAX + message + end + + private + + def suggestions_for(name, names) + if defined?(DidYouMean::SpellChecker) + Array(DidYouMean::SpellChecker.new(dictionary: names).correct(name)).first(MAX_SUGGESTIONS) + else + fallback_suggestions(name, names) + end + end + + def fallback_suggestions(name, names) + target = name.downcase + matches = names.select { |candidate| near_miss?(candidate.downcase, target) } + matches.sort_by { |candidate| levenshtein(candidate.downcase, target) }.first(MAX_SUGGESTIONS) + end + + def near_miss?(candidate, target) + candidate.start_with?(target) || target.start_with?(candidate) || + candidate.include?(target) || levenshtein(candidate, target) <= 2 + end + + def levenshtein(source, target) + return target.length if source.empty? + return source.length if target.empty? + + previous = (0..target.length).to_a + source.each_char.with_index do |source_char, row| + current = [row + 1] + target.each_char.with_index do |target_char, col| + cost = source_char == target_char ? 0 : 1 + current << [previous[col + 1] + 1, current[col] + 1, previous[col] + cost].min + end + previous = current + end + previous.last + end + + def quote_join(names) + names.map { |name| "\"#{name}\"" }.join(", ") + end +end diff --git a/lib/mcp_toolkit/version.rb b/lib/mcp_toolkit/version.rb index e713196..7bb52ea 100644 --- a/lib/mcp_toolkit/version.rb +++ b/lib/mcp_toolkit/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module McpToolkit - VERSION = "0.2.0" + VERSION = "0.3.0" end diff --git a/spec/mcp_toolkit/registry_and_executors_spec.rb b/spec/mcp_toolkit/registry_and_executors_spec.rb index cbcea67..5be4894 100644 --- a/spec/mcp_toolkit/registry_and_executors_spec.rb +++ b/spec/mcp_toolkit/registry_and_executors_spec.rb @@ -331,6 +331,89 @@ def self.serialize_collection(records, scope: nil, total_count: nil, limit: nil, ] ) end + + # The DX gap this closes: a `scheduled_notifications.notification` link is + # discoverably the `notifications` resource (callable via list/get) rather than + # a name the caller has to guess. + context "relationship target resource resolution" do + let(:notification_model) do + Class.new do + def self.columns_hash + { "id" => FakeRelation::Column.new(:integer), "name" => FakeRelation::Column.new(:string) } + end + def self.primary_key = "id" + def self.model_name = FakeModelName.new("notifications") + end + end + + let(:notification_serializer) do + model = notification_model + Class.new(McpToolkit::Serializer::Base) do + attributes :id, :name + self.model_class = model + def self.name = "NotificationSerializer" + end + end + + let(:scheduled_notification_serializer) do + Class.new(McpToolkit::Serializer::Base) do + attributes :id + has_one :notification # singular link -> plural `notifications` resource + has_many :orphans # no registered target + has_one :subject_record, polymorphic: true + self.model_class = Object + def self.name = "ScheduledNotificationSerializer" + end + end + + before do + n_model = notification_model + n_serializer = notification_serializer + sn_serializer = scheduled_notification_serializer + McpToolkit.configure do |c| + c.registry.register(:notifications) do + model n_model + serializer n_serializer + description "Notifications." + scope { |_root| [] } + end + c.registry.register(:scheduled_notifications) do + model Object + serializer sn_serializer + description "Scheduled notifications." + scope { |_root| [] } + end + end + end + + subject(:schema) do + described_class.call(McpToolkit.registry.fetch("scheduled_notifications"), registry: McpToolkit.registry) + end + + it "names the target resource a singular link resolves to" do + relationship = schema[:relationships].find { |r| r[:name] == "notification" } + + expect(relationship).to include( + name: "notification", + kind: "has_one", + polymorphic: false, + target_resource: "notifications" + ) + end + + it "omits target fields (additive/backward-compatible) when no resource matches" do + relationship = schema[:relationships].find { |r| r[:name] == "orphans" } + + expect(relationship.keys).to contain_exactly(:name, :kind, :polymorphic) + end + + it "leaves a polymorphic link unresolved (no single target resource)" do + relationship = schema[:relationships].find { |r| r[:name] == "subject_record" } + + expect(relationship).to include(polymorphic: true) + expect(relationship).not_to have_key(:target_resource) + end + end end describe McpToolkit::Registry do @@ -338,6 +421,25 @@ def self.serialize_collection(records, scope: nil, total_count: nil, limit: nil, expect { McpToolkit.registry.fetch("nope") }.to raise_error(McpToolkit::Registry::UnknownResource) end + it "suggests the closest registered name on a near-miss (did you mean)" do + expect { McpToolkit.registry.fetch("widget") }.to raise_error( + McpToolkit::Registry::UnknownResource, /Did you mean "widgets"\?/ + ) + end + + it "lists the registered resources when the catalog is short" do + expect { McpToolkit.registry.fetch("zzz") }.to raise_error( + McpToolkit::Registry::UnknownResource, /Registered resources: "widgets"\./ + ) + end + + it "suggests a near-miss via UnknownResourceMessage's edit-distance fallback (no did_you_mean)" do + resource_names = %w[notifications scheduled_notifications] + message = McpToolkit::UnknownResourceMessage.new("notification", resource_names) + + expect(message.send(:fallback_suggestions, "notification", resource_names)).to include("notifications") + end + describe "required scope resolution" do subject(:registry) { McpToolkit::Registry.new } diff --git a/spec/mcp_toolkit/transport/controller_methods_spec.rb b/spec/mcp_toolkit/transport/controller_methods_spec.rb new file mode 100644 index 0000000..c001df9 --- /dev/null +++ b/spec/mcp_toolkit/transport/controller_methods_spec.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +require "spec_helper" +require "logger" +require "stringio" + +# The transport concern runs WITHOUT Rails in the gem's suite, so this exercises +# the session-not-found path against a minimal host class that provides only the +# class hooks the concern's `included do` block invokes (`before_action`; CSRF is +# guarded behind respond_to? and thus skipped). `request` / `response` / `render` +# are stubbed to the slice the method touches. +RSpec.describe McpToolkit::Transport::ControllerMethods do + let(:log_output) { StringIO.new } + let(:logger) { Logger.new(log_output) } + + let(:controller_class) do + Class.new do + def self.before_action(*); end + include McpToolkit::Transport::ControllerMethods + + attr_accessor :session_header_value + attr_reader :rendered + + def request + headers = { McpToolkit::Transport::ControllerMethods::SESSION_HEADER => session_header_value } + Struct.new(:headers).new(headers) + end + + def render(*args) + @rendered = args + end + end + end + + let(:controller) { controller_class.new } + + describe "#mcp_render_session_not_found logging (P3)" do + it "warns with a greppable, id-free message when a session is not found" do + controller.session_header_value = "sess-secret-123" + allow(controller).to receive(:mcp_logger).and_return(logger) + + controller.send(:mcp_render_session_not_found) + + expect(log_output.string).to include("[McpToolkit] MCP session not found or expired") + expect(log_output.string).to include("header present: true") + expect(log_output.string).to include("cache_store") + expect(log_output.string).not_to include("sess-secret-123") + end + + it "reports header present: false when no session-id header was sent" do + controller.session_header_value = nil + allow(controller).to receive(:mcp_logger).and_return(logger) + + controller.send(:mcp_render_session_not_found) + + expect(log_output.string).to include("header present: false") + end + + it "still renders the -32001 not-found JSON-RPC error after logging" do + controller.session_header_value = "sess-123" + allow(controller).to receive(:mcp_logger).and_return(logger) + + controller.send(:mcp_render_session_not_found) + + payload = controller.rendered.first + expect(payload).to include(status: :not_found) + expect(payload[:json][:error][:code]).to eq(-32_001) + end + + it "falls back to a $stdout logger (never nil) when Rails is absent, and still renders" do + # A sibling spec loads `rails/version` (partial Rails, no `.logger`) into this + # process, so hide the constant to deterministically exercise the Rails-absent + # branch regardless of load order. + hide_const("Rails") + controller.session_header_value = "sess-123" + + expect { controller.send(:mcp_render_session_not_found) } + .to output(/\[McpToolkit\] MCP session not found or expired/).to_stdout + expect(controller.rendered.first).to include(status: :not_found) + end + end +end diff --git a/spec/mcp_toolkit/unknown_resource_message_spec.rb b/spec/mcp_toolkit/unknown_resource_message_spec.rb new file mode 100644 index 0000000..08b2e5c --- /dev/null +++ b/spec/mcp_toolkit/unknown_resource_message_spec.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe McpToolkit::UnknownResourceMessage do + def build(name, resource_names) + described_class.new(name, resource_names).build + end + + it "states the bad name alone when the registry is empty" do + expect(build("widgets", [])).to eq('unknown resource: "widgets"') + end + + it "suggests the nearest registered name on a near-miss" do + expect(build("widget", %w[widgets gadgets])).to include('Did you mean "widgets"?') + end + + it "lists all registered names, sorted, when the catalog is short" do + expect(build("zzz", %w[widgets gadgets])).to include('Registered resources: "gadgets", "widgets".') + end + + it "neither suggests nor lists for a large catalog with no near-miss" do + resource_names = (1..12).map { |n| "resource_#{n}" } + + message = build("zzzzz", resource_names) + + expect(message).to eq('unknown resource: "zzzzz"') + expect(message).not_to include("Registered resources:") + expect(message).not_to include("Did you mean") + end + + it "finds a near-miss via the dependency-free edit-distance fallback" do + resource_names = %w[notifications scheduled_notifications] + message = described_class.new("notification", resource_names) + + expect(message.send(:fallback_suggestions, "notification", resource_names)).to include("notifications") + end +end