From d4ba5fb8c037a37489dded3d9075759ba7937104 Mon Sep 17 00:00:00 2001 From: Lloyd Watkin Date: Fri, 18 Sep 2026 20:21:39 +0100 Subject: [PATCH 01/24] Read MCP metadata from ActiveAdmin action options Co-Authored-By: Claude Sonnet 5 --- lib/activeadmin_mcp.rb | 1 + lib/activeadmin_mcp/active_admin_ext.rb | 31 +++++++ lib/activeadmin_mcp/engine.rb | 6 ++ spec/activeadmin_mcp/active_admin_ext_spec.rb | 40 ++++++++ spec/support/active_admin.rb | 92 +++++++++++++++++++ 5 files changed, 170 insertions(+) create mode 100644 lib/activeadmin_mcp/active_admin_ext.rb create mode 100644 spec/activeadmin_mcp/active_admin_ext_spec.rb create mode 100644 spec/support/active_admin.rb diff --git a/lib/activeadmin_mcp.rb b/lib/activeadmin_mcp.rb index 502a1e4..e81e0a5 100644 --- a/lib/activeadmin_mcp.rb +++ b/lib/activeadmin_mcp.rb @@ -1,6 +1,7 @@ require_relative "activeadmin_mcp/version" require_relative "activeadmin_mcp/configuration" require_relative "activeadmin_mcp/authorization" +require_relative "activeadmin_mcp/active_admin_ext" require_relative "activeadmin_mcp/resource_registry" require_relative "activeadmin_mcp/form_field_collector" require_relative "activeadmin_mcp/record_updater" diff --git a/lib/activeadmin_mcp/active_admin_ext.rb b/lib/activeadmin_mcp/active_admin_ext.rb new file mode 100644 index 0000000..43c4ccb --- /dev/null +++ b/lib/activeadmin_mcp/active_admin_ext.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +module ActiveadminMcp + # Every patch this gem applies to ActiveAdmin lives in this one file, so the + # coupling has a single place to check when ActiveAdmin is upgraded. + # + # ActiveAdmin stores an action's option hash but exposes only the keys it + # uses itself (:method, :title, :form, :if, ...). MCP metadata is declared as + # an extra `mcp:` key on that same hash, which ActiveAdmin carries through + # untouched, so all we need is a reader to get it back out. + module ActiveAdminExt + module ActionOptions + def mcp_options + options = instance_variable_get(:@options) + options.is_a?(Hash) ? options[:mcp] : nil + end + end + + # ActiveAdmin::BatchAction only exists once ActiveAdmin's before_load hooks + # have run, so this is called from ActiveAdmin.after_load rather than at + # require time. + def self.apply! + return false unless defined?(::ActiveAdmin::ControllerAction) + return false unless defined?(::ActiveAdmin::BatchAction) + + ::ActiveAdmin::ControllerAction.include(ActionOptions) + ::ActiveAdmin::BatchAction.include(ActionOptions) + true + end + end +end diff --git a/lib/activeadmin_mcp/engine.rb b/lib/activeadmin_mcp/engine.rb index a771593..cf1b450 100644 --- a/lib/activeadmin_mcp/engine.rb +++ b/lib/activeadmin_mcp/engine.rb @@ -14,5 +14,11 @@ class Engine < ::Rails::Engine end end end + + initializer "activeadmin_mcp.active_admin_ext" do + ActiveSupport.on_load(:after_initialize) do + ActiveAdmin.after_load { ActiveadminMcp::ActiveAdminExt.apply! } if defined?(::ActiveAdmin) + end + end end end diff --git a/spec/activeadmin_mcp/active_admin_ext_spec.rb b/spec/activeadmin_mcp/active_admin_ext_spec.rb new file mode 100644 index 0000000..29a2b77 --- /dev/null +++ b/spec/activeadmin_mcp/active_admin_ext_spec.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +require "spec_helper" +require "support/active_admin" + +RSpec.describe ActiveadminMcp::ActiveAdminExt do + let(:config) { McpSpec::ActiveAdminHarness.volunteer_config } + + def member_action(name) + config.member_actions.find { |action| action.name.to_sym == name } + end + + before { described_class.apply! } + + it "reads the mcp option off a member action" do + expect(member_action(:create_warning).mcp_options).to eq( + description: "Record a warning against a volunteer", + params: { reason: { type: :string, required: true } } + ) + end + + it "returns nil for an action that did not opt in" do + expect(member_action(:undocumented).mcp_options).to be_nil + end + + it "reads the mcp option off a batch action" do + suspend = config.batch_actions.find { |action| action.sym == :suspend } + + expect(suspend.mcp_options).to eq(description: "Suspend the selected volunteers") + end + + # Guard spec. If a future ActiveAdmin starts validating or stripping unknown + # option keys, this fails loudly here rather than silently in a user's admin. + it "confirms ActiveAdmin carries unknown action option keys through untouched" do + options = member_action(:create_warning).instance_variable_get(:@options) + + expect(options).to include(method: :post) + expect(options).to have_key(:mcp) + end +end diff --git a/spec/support/active_admin.rb b/spec/support/active_admin.rb new file mode 100644 index 0000000..9d9fb38 --- /dev/null +++ b/spec/support/active_admin.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +# Boots a minimal Rails + ActiveAdmin application so specs can exercise real +# ActiveAdmin objects (resource configs, controllers, dispatch) rather than +# doubles. Required only by specs that genuinely need it; the rest of the +# suite stays double-based and never loads ActiveAdmin. +require "rails" +require "active_record" +require "action_controller/railtie" +require "active_model/railtie" + +ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:") +ActiveRecord::Schema.verbose = false +ActiveRecord::Schema.define do + create_table :volunteers, force: true do |t| + t.string :name + t.boolean :active, default: true + t.timestamps + end + + create_table :admin_users, force: true do |t| + t.string :email + t.timestamps + end +end + +class Volunteer < ActiveRecord::Base; end +class AdminUser < ActiveRecord::Base; end + +require "active_admin" + +module McpSpec + class HarnessApp < Rails::Application + config.eager_load = false + config.root = File.expand_path("../tmp/harness", __dir__) + config.secret_key_base = "a" * 64 + config.logger = Logger.new(IO::NULL) + config.active_support.to_time_preserves_timezone = :zone + end + + module ActiveAdminHarness + def self.volunteer_config + ActiveAdmin.application.namespaces[:admin].resources.find do |resource| + resource.resource_class == Volunteer + end + end + end +end + +FileUtils.mkdir_p(McpSpec::HarnessApp.config.root) +Rails.application.initialize! + +# InheritedResources, which ActiveAdmin's resource controllers inherit from, +# references ::ApplicationController at load time. +class ApplicationController < ActionController::Base; end + +# Runs ActiveAdmin's before_load hooks, which is what defines +# ActiveAdmin::BatchAction and mixes batch action support into Resource. +ActiveAdmin.application.load! + +# Batch actions default to disabled in a bare boot like this one; a real app +# enables them in its ActiveAdmin initializer. +ActiveAdmin.application.namespaces[:admin].batch_actions = true + +ActiveAdmin.register Volunteer do + actions :index, :show, :edit, :update + + member_action :create_warning, method: :post, mcp: { + description: "Record a warning against a volunteer", + params: { reason: { type: :string, required: true } } + } do + resource.update(name: params[:reason]) + redirect_to resource_path(resource), notice: "Warning recorded" + end + + member_action :undocumented, method: :post do + head :ok + end + + collection_action :export, method: :get, mcp: { description: "Export volunteers" } do + redirect_to collection_path, notice: "Exported" + end + + batch_action :suspend, form: { reason: :text }, + mcp: { description: "Suspend the selected volunteers" } do |ids, inputs| + redirect_to collection_path, notice: "#{ids.size} suspended: #{inputs[:reason]}" + end +end + +Rails.application.routes.draw do + ActiveAdmin.routes(self) +end From 9c089d5c376eb0172e0f236f967c05148da6f360 Mon Sep 17 00:00:00 2001 From: Lloyd Watkin Date: Fri, 18 Sep 2026 20:24:51 +0100 Subject: [PATCH 02/24] Normalise opted-in ActiveAdmin actions into ActionDefinition Co-Authored-By: Claude Haiku 4.5 --- lib/activeadmin_mcp.rb | 1 + lib/activeadmin_mcp/action_definition.rb | 121 ++++++++++++++++++ .../activeadmin_mcp/action_definition_spec.rb | 86 +++++++++++++ 3 files changed, 208 insertions(+) create mode 100644 lib/activeadmin_mcp/action_definition.rb create mode 100644 spec/activeadmin_mcp/action_definition_spec.rb diff --git a/lib/activeadmin_mcp.rb b/lib/activeadmin_mcp.rb index e81e0a5..a74295d 100644 --- a/lib/activeadmin_mcp.rb +++ b/lib/activeadmin_mcp.rb @@ -2,6 +2,7 @@ require_relative "activeadmin_mcp/configuration" require_relative "activeadmin_mcp/authorization" require_relative "activeadmin_mcp/active_admin_ext" +require_relative "activeadmin_mcp/action_definition" require_relative "activeadmin_mcp/resource_registry" require_relative "activeadmin_mcp/form_field_collector" require_relative "activeadmin_mcp/record_updater" diff --git a/lib/activeadmin_mcp/action_definition.rb b/lib/activeadmin_mcp/action_definition.rb new file mode 100644 index 0000000..6aedd38 --- /dev/null +++ b/lib/activeadmin_mcp/action_definition.rb @@ -0,0 +1,121 @@ +# frozen_string_literal: true + +module ActiveadminMcp + # One ActiveAdmin action that an application has opted in to MCP, normalised + # so the rest of the engine does not care whether it came from a + # member_action, a collection_action or a batch_action. + # + # Batch actions are the special case: ActiveAdmin already knows their input + # names and widget types via `form:`, so those become param types and the + # `mcp:` declaration only layers descriptions and hints on top. + class ActionDefinition + KINDS = %i[member collection batch].freeze + + # JSON Schema scalar types an application may declare. + TYPES = %i[string integer number boolean array object].freeze + + # ActiveAdmin batch action form widgets -> JSON Schema types. + FORM_TYPES = { + text: :string, + string: :string, + select: :string, + datepicker: :string, + number: :number, + checkbox: :boolean, + }.freeze + + attr_reader :config, :action, :kind, :errors + + def self.build(config:, action:, kind:) + options = action.mcp_options + return nil unless options.is_a?(Hash) + + new(config: config, action: action, kind: kind, options: options) + end + + def initialize(config:, action:, kind:, options:) + @config = config + @action = action + @kind = kind + @options = options + @errors = [] + validate! + end + + def action_name + (@kind == :batch ? @action.sym : @action.name).to_sym + end + + def resource_name + @config.resource_class.name + end + + def tool_name + "#{resource_name.gsub('::', '_').gsub(/([a-z\d])([A-Z])/, '\1_\2').downcase}_#{action_name}" + end + + def description + @options[:description] + end + + def permission + @options[:permission] + end + + def http_verb + return :post if @kind == :batch + + Array(@action.http_verb).first&.to_sym || :get + end + + # Declared params, with batch actions inheriting their types from the + # ActiveAdmin `form:` hash underneath anything the declaration says. + def params + @params ||= inherited_params.merge(declared_params) do |_key, inherited, declared| + inherited.merge(declared) + end + end + + def valid? + @errors.empty? + end + + private + + def declared_params + raw = @options[:params] + return {} unless raw.is_a?(Hash) + + raw.each_with_object({}) { |(name, spec), acc| acc[name.to_sym] = spec } + end + + def inherited_params + return {} unless @kind == :batch + return {} unless @action.respond_to?(:inputs) + + form = @action.inputs + return {} unless form.is_a?(Hash) + + form.each_with_object({}) do |(name, widget), acc| + acc[name.to_sym] = { type: FORM_TYPES.fetch(widget.to_sym, :string) } + end + end + + def validate! + @errors << "#{tool_name}: mcp declaration needs a description" if description.to_s.strip.empty? + @errors << "#{tool_name}: unknown kind #{@kind}" unless KINDS.include?(@kind) + + params.each do |name, spec| + unless spec.is_a?(Hash) + @errors << "#{tool_name}: param #{name} must be a Hash" + next + end + + type = spec[:type] + @errors << "#{tool_name}: param #{name} has unknown type #{type}" if type && !TYPES.include?(type.to_sym) + end + + @errors << "#{tool_name}: permission must be callable" if permission && !permission.respond_to?(:call) + end + end +end diff --git a/spec/activeadmin_mcp/action_definition_spec.rb b/spec/activeadmin_mcp/action_definition_spec.rb new file mode 100644 index 0000000..76adebc --- /dev/null +++ b/spec/activeadmin_mcp/action_definition_spec.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe ActiveadminMcp::ActionDefinition do + def build_config(name: "Volunteer") + double("config", resource_class: double("model", name: name)) + end + + def member_action(name, mcp:, verb: :post) + double("controller_action", name: name, http_verb: verb, mcp_options: mcp) + end + + def batch_action(name, mcp:, form: nil) + double("batch_action", sym: name, mcp_options: mcp, inputs: form) + end + + it "returns nil when the action did not opt in" do + action = member_action(:undocumented, mcp: nil) + + expect(described_class.build(config: build_config, action: action, kind: :member)).to be_nil + end + + it "exposes the declared metadata and a namespaced tool name" do + action = member_action(:create_warning, mcp: { + description: "Record a warning", + params: { reason: { type: :string, required: true } } + }) + + definition = described_class.build(config: build_config, action: action, kind: :member) + + expect(definition).to have_attributes( + kind: :member, + action_name: :create_warning, + resource_name: "Volunteer", + tool_name: "volunteer_create_warning", + description: "Record a warning", + http_verb: :post + ) + expect(definition.params).to eq(reason: { type: :string, required: true }) + expect(definition).to be_valid + end + + it "inherits batch action param types from the ActiveAdmin form hash" do + action = batch_action(:suspend, mcp: { description: "Suspend" }, form: { reason: :text, notify: :checkbox }) + + definition = described_class.build(config: build_config, action: action, kind: :batch) + + expect(definition.params).to eq( + reason: { type: :string }, + notify: { type: :boolean } + ) + end + + it "lets the mcp declaration layer hints over an inherited batch form type" do + action = batch_action(:suspend, form: { reason: :text }, mcp: { + description: "Suspend", + params: { reason: { hint: "Shown to the volunteer" } } + }) + + definition = described_class.build(config: build_config, action: action, kind: :batch) + + expect(definition.params).to eq(reason: { type: :string, hint: "Shown to the volunteer" }) + end + + it "is invalid when a param declares an unrecognised type" do + action = member_action(:create_warning, mcp: { + description: "Record a warning", + params: { reason: { type: :wibble } } + }) + + definition = described_class.build(config: build_config, action: action, kind: :member) + + expect(definition).not_to be_valid + expect(definition.errors.first).to include("wibble") + end + + it "is invalid without a description" do + action = member_action(:create_warning, mcp: { params: {} }) + + definition = described_class.build(config: build_config, action: action, kind: :member) + + expect(definition).not_to be_valid + expect(definition.errors.first).to include("description") + end +end From 0fdb00e6efcb8b203d8b02a6412cd4140e82b478 Mon Sep 17 00:00:00 2001 From: Lloyd Watkin Date: Fri, 18 Sep 2026 20:28:15 +0100 Subject: [PATCH 03/24] Fix tool_name to handle acronyms and namespaced class names Use ActiveSupport's underscore method instead of hand-rolled snake_casing to correctly convert APIKey and Admin::Volunteer style class names to tool_name identifiers, preventing collisions. Co-Authored-By: Claude Haiku 4.5 --- lib/activeadmin_mcp/action_definition.rb | 2 +- .../activeadmin_mcp/action_definition_spec.rb | 25 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/lib/activeadmin_mcp/action_definition.rb b/lib/activeadmin_mcp/action_definition.rb index 6aedd38..ff82253 100644 --- a/lib/activeadmin_mcp/action_definition.rb +++ b/lib/activeadmin_mcp/action_definition.rb @@ -51,7 +51,7 @@ def resource_name end def tool_name - "#{resource_name.gsub('::', '_').gsub(/([a-z\d])([A-Z])/, '\1_\2').downcase}_#{action_name}" + "#{resource_name.underscore.tr('/', '_')}_#{action_name}" end def description diff --git a/spec/activeadmin_mcp/action_definition_spec.rb b/spec/activeadmin_mcp/action_definition_spec.rb index 76adebc..0c89f1d 100644 --- a/spec/activeadmin_mcp/action_definition_spec.rb +++ b/spec/activeadmin_mcp/action_definition_spec.rb @@ -63,6 +63,31 @@ def batch_action(name, mcp:, form: nil) expect(definition.params).to eq(reason: { type: :string, hint: "Shown to the volunteer" }) end + it "properly handles acronyms and namespaced resource names in tool names" do + action_acronym = member_action(:create_warning, mcp: { + description: "Record a warning", + params: {} + }) + action_namespaced = member_action(:create_warning, mcp: { + description: "Record a warning", + params: {} + }) + + definition_acronym = described_class.build( + config: build_config(name: "APIKey"), + action: action_acronym, + kind: :member + ) + definition_namespaced = described_class.build( + config: build_config(name: "Admin::Volunteer"), + action: action_namespaced, + kind: :member + ) + + expect(definition_acronym.tool_name).to eq("api_key_create_warning") + expect(definition_namespaced.tool_name).to eq("admin_volunteer_create_warning") + end + it "is invalid when a param declares an unrecognised type" do action = member_action(:create_warning, mcp: { description: "Record a warning", From def945cfafb4664b74651a8a1ef57a8313c0552c Mon Sep 17 00:00:00 2001 From: Lloyd Watkin Date: Fri, 18 Sep 2026 20:30:44 +0100 Subject: [PATCH 04/24] Generate JSON Schema for opted-in ActiveAdmin actions Co-Authored-By: Claude Haiku 4.5 --- lib/activeadmin_mcp.rb | 1 + lib/activeadmin_mcp/action_schema.rb | 68 ++++++++++++++++++++++ spec/activeadmin_mcp/action_schema_spec.rb | 67 +++++++++++++++++++++ 3 files changed, 136 insertions(+) create mode 100644 lib/activeadmin_mcp/action_schema.rb create mode 100644 spec/activeadmin_mcp/action_schema_spec.rb diff --git a/lib/activeadmin_mcp.rb b/lib/activeadmin_mcp.rb index a74295d..0b0694a 100644 --- a/lib/activeadmin_mcp.rb +++ b/lib/activeadmin_mcp.rb @@ -3,6 +3,7 @@ require_relative "activeadmin_mcp/authorization" require_relative "activeadmin_mcp/active_admin_ext" require_relative "activeadmin_mcp/action_definition" +require_relative "activeadmin_mcp/action_schema" require_relative "activeadmin_mcp/resource_registry" require_relative "activeadmin_mcp/form_field_collector" require_relative "activeadmin_mcp/record_updater" diff --git a/lib/activeadmin_mcp/action_schema.rb b/lib/activeadmin_mcp/action_schema.rb new file mode 100644 index 0000000..90ffe3a --- /dev/null +++ b/lib/activeadmin_mcp/action_schema.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +module ActiveadminMcp + # Builds the JSON Schema an MCP client sees for one opted-in action. + # + # The binding/advisory split matters: a static `enum:` is enforced before we + # dispatch, while `suggestions:` is only ever a hint to the model. A + # suggestions proc runs application code on every tools/list call, so it is + # wrapped — a raising proc costs its suggestions, never the whole listing. + class ActionSchema + def initialize(definition) + @definition = definition + end + + def to_h + properties = record_properties + required = properties.keys.map(&:to_s) + + @definition.params.each do |name, spec| + properties[name] = property_for(spec) + required << name.to_s if spec[:required] + end + + { type: "object", properties: properties, required: required } + end + + private + + def record_properties + case @definition.kind + when :member + { id: { type: "string", description: "Primary key of the record to act on" } } + when :batch + { ids: { type: "array", items: { type: "string" }, + description: "Primary keys of the records to act on" } } + else + {} + end + end + + def property_for(spec) + property = { type: (spec[:type] || :string).to_s } + + descriptions = [] + descriptions << spec[:hint] if spec[:hint] + + property[:enum] = spec[:enum] if spec[:enum].is_a?(Array) + + suggestions = resolve_suggestions(spec[:suggestions]) + if suggestions&.any? + property[:examples] = suggestions + descriptions << "Suggested values: #{suggestions.join(', ')}" + end + + property[:description] = descriptions.join(". ") unless descriptions.empty? + property + end + + # Advisory only. A proc that blows up must not take the tool listing with it. + def resolve_suggestions(suggestions) + return nil unless suggestions.respond_to?(:call) + + Array(suggestions.call) + rescue StandardError + nil + end + end +end diff --git a/spec/activeadmin_mcp/action_schema_spec.rb b/spec/activeadmin_mcp/action_schema_spec.rb new file mode 100644 index 0000000..b806584 --- /dev/null +++ b/spec/activeadmin_mcp/action_schema_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe ActiveadminMcp::ActionSchema do + def definition(kind: :member, params: {}) + double("definition", kind: kind, params: params) + end + + it "requires an id for member actions" do + schema = described_class.new(definition(kind: :member)).to_h + + expect(schema[:properties]).to include(:id) + expect(schema[:required]).to include("id") + end + + it "requires an ids array for batch actions" do + schema = described_class.new(definition(kind: :batch)).to_h + + expect(schema[:properties][:ids]).to include(type: "array") + expect(schema[:required]).to include("ids") + end + + it "adds no record key for collection actions" do + schema = described_class.new(definition(kind: :collection)).to_h + + expect(schema[:properties]).to be_empty + expect(schema[:required]).to be_empty + end + + it "maps declared params, hints and required-ness" do + schema = described_class.new( + definition(kind: :collection, params: { + reason: { type: :string, required: true, hint: "Shown to the volunteer" } + }) + ).to_h + + expect(schema[:properties][:reason]).to eq(type: "string", description: "Shown to the volunteer") + expect(schema[:required]).to eq(["reason"]) + end + + it "emits a static enum as a binding enum" do + schema = described_class.new( + definition(kind: :collection, params: { severity: { type: :string, enum: %w[low high] } }) + ).to_h + + expect(schema[:properties][:severity][:enum]).to eq(%w[low high]) + end + + it "emits a suggestions proc as advisory examples" do + schema = described_class.new( + definition(kind: :collection, params: { category: { type: :string, suggestions: -> { %w[lateness conduct] } } }) + ).to_h + + expect(schema[:properties][:category][:examples]).to eq(%w[lateness conduct]) + expect(schema[:properties][:category]).not_to have_key(:enum) + expect(schema[:properties][:category][:description]).to include("lateness") + end + + it "keeps the tool usable when a suggestions proc raises" do + schema = described_class.new( + definition(kind: :collection, params: { category: { type: :string, suggestions: -> { raise "boom" } } }) + ).to_h + + expect(schema[:properties][:category]).to eq(type: "string") + end +end From 87c5e400979e4b1ab4bdf6eb7f4aa1c426bd65d8 Mon Sep 17 00:00:00 2001 From: Lloyd Watkin Date: Fri, 18 Sep 2026 20:34:42 +0100 Subject: [PATCH 05/24] Reject reserved param names in ActionDefinition validation A declared param named \`id\` on a member action (or \`ids\` on a batch action) would silently collide with the record key injected by ActionSchema, corrupting the schema and creating confusing semantics. Validate this at declaration time rather than papering over it in the schema. - Member actions reserve \`:id\` - Batch actions reserve \`:ids\` - Collection actions reserve nothing ActionSchema also ensures \`required\` is unique as belt-and-braces. Co-Authored-By: Claude Haiku 4.5 --- lib/activeadmin_mcp/action_definition.rb | 14 ++++++ lib/activeadmin_mcp/action_schema.rb | 2 +- .../activeadmin_mcp/action_definition_spec.rb | 46 +++++++++++++++++++ spec/activeadmin_mcp/action_schema_spec.rb | 10 ++++ 4 files changed, 71 insertions(+), 1 deletion(-) diff --git a/lib/activeadmin_mcp/action_definition.rb b/lib/activeadmin_mcp/action_definition.rb index ff82253..5588f35 100644 --- a/lib/activeadmin_mcp/action_definition.rb +++ b/lib/activeadmin_mcp/action_definition.rb @@ -105,17 +105,31 @@ def validate! @errors << "#{tool_name}: mcp declaration needs a description" if description.to_s.strip.empty? @errors << "#{tool_name}: unknown kind #{@kind}" unless KINDS.include?(@kind) + reserved = reserved_param_name params.each do |name, spec| unless spec.is_a?(Hash) @errors << "#{tool_name}: param #{name} must be a Hash" next end + @errors << "#{tool_name}: param #{name} is reserved" if name == reserved + type = spec[:type] @errors << "#{tool_name}: param #{name} has unknown type #{type}" if type && !TYPES.include?(type.to_sym) end @errors << "#{tool_name}: permission must be callable" if permission && !permission.respond_to?(:call) end + + def reserved_param_name + case @kind + when :member + :id + when :batch + :ids + else + nil + end + end end end diff --git a/lib/activeadmin_mcp/action_schema.rb b/lib/activeadmin_mcp/action_schema.rb index 90ffe3a..01fb553 100644 --- a/lib/activeadmin_mcp/action_schema.rb +++ b/lib/activeadmin_mcp/action_schema.rb @@ -21,7 +21,7 @@ def to_h required << name.to_s if spec[:required] end - { type: "object", properties: properties, required: required } + { type: "object", properties: properties, required: required.uniq } end private diff --git a/spec/activeadmin_mcp/action_definition_spec.rb b/spec/activeadmin_mcp/action_definition_spec.rb index 0c89f1d..a9e6d8e 100644 --- a/spec/activeadmin_mcp/action_definition_spec.rb +++ b/spec/activeadmin_mcp/action_definition_spec.rb @@ -108,4 +108,50 @@ def batch_action(name, mcp:, form: nil) expect(definition).not_to be_valid expect(definition.errors.first).to include("description") end + + it "is invalid when a member action declares a param named id" do + action = member_action(:create_warning, mcp: { + description: "Record a warning", + params: { id: { type: :integer } } + }) + + definition = described_class.build(config: build_config, action: action, kind: :member) + + expect(definition).not_to be_valid + expect(definition.errors).to include(match(/param id is reserved/)) + end + + it "is invalid when a batch action declares a param named ids" do + action = batch_action(:suspend, mcp: { + description: "Suspend", + params: { ids: { type: :array } } + }) + + definition = described_class.build(config: build_config, action: action, kind: :batch) + + expect(definition).not_to be_valid + expect(definition.errors).to include(match(/param ids is reserved/)) + end + + it "is valid when a collection action declares a param named id" do + action = double("controller_action", name: :process, http_verb: :post, mcp_options: { + description: "Process something", + params: { id: { type: :string } } + }) + + definition = described_class.build(config: build_config, action: action, kind: :collection) + + expect(definition).to be_valid + end + + it "is valid when a member action declares a param named ids" do + action = member_action(:create_warning, mcp: { + description: "Record a warning", + params: { ids: { type: :array } } + }) + + definition = described_class.build(config: build_config, action: action, kind: :member) + + expect(definition).to be_valid + end end diff --git a/spec/activeadmin_mcp/action_schema_spec.rb b/spec/activeadmin_mcp/action_schema_spec.rb index b806584..0facc39 100644 --- a/spec/activeadmin_mcp/action_schema_spec.rb +++ b/spec/activeadmin_mcp/action_schema_spec.rb @@ -64,4 +64,14 @@ def definition(kind: :member, params: {}) expect(schema[:properties][:category]).to eq(type: "string") end + + it "ensures no duplicate required entries even if a colliding param somehow reaches it" do + # ActionDefinition rejects this upstream, but to_h should never emit duplicates regardless + schema = described_class.new( + definition(kind: :member, params: { id: { type: :integer, required: true } }) + ).to_h + + expect(schema[:required]).to eq(["id"]) + expect(schema[:required].uniq).to eq(schema[:required]) + end end From cee0bac846e8fdd3a9bfe2654c10af90110cd725 Mon Sep 17 00:00:00 2001 From: Lloyd Watkin Date: Fri, 18 Sep 2026 20:36:43 +0100 Subject: [PATCH 06/24] Validate and coerce MCP action arguments Co-Authored-By: Claude Haiku 4.5 --- lib/activeadmin_mcp.rb | 1 + lib/activeadmin_mcp/action_params.rb | 64 +++++++++++++++++++++ spec/activeadmin_mcp/action_params_spec.rb | 67 ++++++++++++++++++++++ 3 files changed, 132 insertions(+) create mode 100644 lib/activeadmin_mcp/action_params.rb create mode 100644 spec/activeadmin_mcp/action_params_spec.rb diff --git a/lib/activeadmin_mcp.rb b/lib/activeadmin_mcp.rb index 0b0694a..fe95677 100644 --- a/lib/activeadmin_mcp.rb +++ b/lib/activeadmin_mcp.rb @@ -4,6 +4,7 @@ require_relative "activeadmin_mcp/active_admin_ext" require_relative "activeadmin_mcp/action_definition" require_relative "activeadmin_mcp/action_schema" +require_relative "activeadmin_mcp/action_params" require_relative "activeadmin_mcp/resource_registry" require_relative "activeadmin_mcp/form_field_collector" require_relative "activeadmin_mcp/record_updater" diff --git a/lib/activeadmin_mcp/action_params.rb b/lib/activeadmin_mcp/action_params.rb new file mode 100644 index 0000000..03a3014 --- /dev/null +++ b/lib/activeadmin_mcp/action_params.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +module ActiveadminMcp + # Validates and coerces a tool call's arguments against its ActionDefinition + # before anything reaches the controller. + # + # Undeclared arguments are dropped rather than passed through: the declaration + # is the contract, and forwarding unknown keys into a controller action would + # let a client reach parameters the application never opted in to. + class ActionParams + def initialize(definition) + @definition = definition + end + + def call(arguments) + arguments ||= {} + result = { params: {} } + + case @definition.kind + when :member + id = arguments["id"] + return { error: "id is required" } if id.nil? || id.to_s.empty? + + result[:record_id] = id.to_s + when :batch + ids = Array(arguments["ids"]).reject { |id| id.to_s.empty? } + return { error: "ids is required" } if ids.empty? + + result[:record_ids] = ids.map(&:to_s) + end + + @definition.params.each do |name, spec| + value = arguments[name.to_s] + + if value.nil? || value.to_s.empty? + return { error: "#{name} is required" } if spec[:required] + + next + end + + if spec[:enum].is_a?(Array) && !spec[:enum].include?(value) + return { error: "#{name} must be one of: #{spec[:enum].join(', ')}" } + end + + result[:params][name] = coerce(value, spec[:type]) + end + + result + end + + private + + def coerce(value, type) + case type&.to_sym + when :integer then Integer(value) + when :number then Float(value) + when :boolean then [true, "true", "1", 1].include?(value) + else value + end + rescue ArgumentError, TypeError + value + end + end +end diff --git a/spec/activeadmin_mcp/action_params_spec.rb b/spec/activeadmin_mcp/action_params_spec.rb new file mode 100644 index 0000000..f7d8ce2 --- /dev/null +++ b/spec/activeadmin_mcp/action_params_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe ActiveadminMcp::ActionParams do + def definition(kind: :member, params: {}) + double("definition", kind: kind, params: params) + end + + def call(definition, arguments) + described_class.new(definition).call(arguments) + end + + it "extracts the record id for member actions" do + result = call(definition(kind: :member), { "id" => "42" }) + + expect(result[:record_id]).to eq("42") + expect(result).not_to have_key(:error) + end + + it "refuses a member action with no id" do + expect(call(definition(kind: :member), {})).to eq(error: "id is required") + end + + it "refuses a batch action with an empty ids list" do + expect(call(definition(kind: :batch), { "ids" => [] })).to eq(error: "ids is required") + end + + it "refuses a missing required param" do + result = call(definition(kind: :collection, params: { reason: { type: :string, required: true } }), {}) + + expect(result).to eq(error: "reason is required") + end + + it "refuses a value outside a binding enum" do + definition = definition(kind: :collection, params: { severity: { type: :string, enum: %w[low high] } }) + + result = call(definition, { "severity" => "urgent" }) + + expect(result[:error]).to eq("severity must be one of: low, high") + end + + it "does not enforce suggestions" do + definition = definition(kind: :collection, params: { category: { type: :string, suggestions: -> { %w[conduct] } } }) + + result = call(definition, { "category" => "something else" }) + + expect(result[:params]).to eq(category: "something else") + end + + it "coerces declared types" do + definition = definition(kind: :collection, params: { + count: { type: :integer }, notify: { type: :boolean } + }) + + result = call(definition, { "count" => "3", "notify" => "true" }) + + expect(result[:params]).to eq(count: 3, notify: true) + end + + it "drops arguments that were never declared" do + result = call(definition(kind: :collection, params: { reason: { type: :string } }), + { "reason" => "Late", "admin_override" => "true" }) + + expect(result[:params]).to eq(reason: "Late") + end +end From 90334a480505f5f389df8336022dac4aa9398cf4 Mon Sep 17 00:00:00 2001 From: Lloyd Watkin Date: Fri, 18 Sep 2026 20:40:38 +0100 Subject: [PATCH 07/24] Fix ActionParams: coerce before enum check, refuse coercion failures The brief's code had two critical defects: 1. Enum checking occurred before type coercion. MCP arguments arrive as JSON (usually strings), but enums are declared in native Ruby types. A client sending "1" for enum: [1, 2, 3] was rejected because "1" != 1. 2. Coercion failures were caught and returned as raw values. So "abc" for type: :integer silently reached the controller as a String. Fixes: - Add CoercionError, raised by coerce on failure - Coerce first, catching CoercionError and returning error - Check enum against the coerced value, not the raw one - For :boolean, properly handle "false"/"true"/"0"/"1" and raise on invalid Add 6 covering tests: - Integer enum coercion (Finding 1) - Integer coercion failure (Finding 2) - Boolean string coercion ("false" -> false, not silent degradation) - Boolean coercion failure - Required boolean param sent as false (not treated as missing) Co-Authored-By: Claude Haiku 4.5 --- lib/activeadmin_mcp/action_params.rb | 32 ++++++++++---- spec/activeadmin_mcp/action_params_spec.rb | 49 ++++++++++++++++++++++ 2 files changed, 73 insertions(+), 8 deletions(-) diff --git a/lib/activeadmin_mcp/action_params.rb b/lib/activeadmin_mcp/action_params.rb index 03a3014..a1b82dc 100644 --- a/lib/activeadmin_mcp/action_params.rb +++ b/lib/activeadmin_mcp/action_params.rb @@ -8,6 +8,8 @@ module ActiveadminMcp # is the contract, and forwarding unknown keys into a controller action would # let a client reach parameters the application never opted in to. class ActionParams + class CoercionError < StandardError; end + def initialize(definition) @definition = definition end @@ -38,11 +40,17 @@ def call(arguments) next end - if spec[:enum].is_a?(Array) && !spec[:enum].include?(value) + begin + coerced_value = coerce(value, spec[:type]) + rescue CoercionError + return { error: "#{name} must be a #{spec[:type]}" } + end + + if spec[:enum].is_a?(Array) && !spec[:enum].include?(coerced_value) return { error: "#{name} must be one of: #{spec[:enum].join(', ')}" } end - result[:params][name] = coerce(value, spec[:type]) + result[:params][name] = coerced_value end result @@ -52,13 +60,21 @@ def call(arguments) def coerce(value, type) case type&.to_sym - when :integer then Integer(value) - when :number then Float(value) - when :boolean then [true, "true", "1", 1].include?(value) - else value + when :integer + Integer(value) + when :number + Float(value) + when :boolean + case value + when true, "true", "1", 1 then true + when false, "false", "0", 0 then false + else raise CoercionError, "invalid boolean value" + end + else + value end - rescue ArgumentError, TypeError - value + rescue ArgumentError, TypeError => e + raise CoercionError, e.message end end end diff --git a/spec/activeadmin_mcp/action_params_spec.rb b/spec/activeadmin_mcp/action_params_spec.rb index f7d8ce2..3b1b1a3 100644 --- a/spec/activeadmin_mcp/action_params_spec.rb +++ b/spec/activeadmin_mcp/action_params_spec.rb @@ -64,4 +64,53 @@ def call(definition, arguments) expect(result[:params]).to eq(reason: "Late") end + + it "coerces a value before checking enum (Finding 1: string '1' should match integer enum [1, 2, 3])" do + definition = definition(kind: :collection, params: { count: { type: :integer, enum: [1, 2, 3] } }) + + result = call(definition, { "count" => "1" }) + + expect(result[:params]).to eq(count: 1) + end + + it "refuses a coerced value outside enum" do + definition = definition(kind: :collection, params: { count: { type: :integer, enum: [1, 2, 3] } }) + + result = call(definition, { "count" => "9" }) + + expect(result[:error]).to eq("count must be one of: 1, 2, 3") + end + + it "refuses a value that cannot be coerced to integer (Finding 2: 'abc' for :integer)" do + definition = definition(kind: :collection, params: { count: { type: :integer } }) + + result = call(definition, { "count" => "abc" }) + + expect(result[:error]).to eq("count must be a integer") + end + + it "coerces the string 'false' to boolean false" do + definition = definition(kind: :collection, params: { active: { type: :boolean } }) + + result = call(definition, { "active" => "false" }) + + expect(result[:params]).to eq(active: false) + end + + it "refuses a value that cannot be coerced to boolean" do + definition = definition(kind: :collection, params: { active: { type: :boolean } }) + + result = call(definition, { "active" => "wibble" }) + + expect(result[:error]).to eq("active must be a boolean") + end + + it "accepts a required boolean param sent as false (not treated as missing)" do + definition = definition(kind: :collection, params: { active: { type: :boolean, required: true } }) + + result = call(definition, { "active" => false }) + + expect(result[:params]).to eq(active: false) + expect(result).not_to have_key(:error) + end end From 2f7dcbf7618434678a06071ff597862316ff8ee1 Mon Sep 17 00:00:00 2001 From: Lloyd Watkin Date: Fri, 18 Sep 2026 20:43:49 +0100 Subject: [PATCH 08/24] Discover opted-in ActiveAdmin actions across the admin namespace Co-Authored-By: Claude Haiku 4.5 --- lib/activeadmin_mcp.rb | 1 + lib/activeadmin_mcp/action_catalog.rb | 66 +++++++++++++++++++ spec/activeadmin_mcp/action_catalog_spec.rb | 73 +++++++++++++++++++++ 3 files changed, 140 insertions(+) create mode 100644 lib/activeadmin_mcp/action_catalog.rb create mode 100644 spec/activeadmin_mcp/action_catalog_spec.rb diff --git a/lib/activeadmin_mcp.rb b/lib/activeadmin_mcp.rb index fe95677..1db5899 100644 --- a/lib/activeadmin_mcp.rb +++ b/lib/activeadmin_mcp.rb @@ -5,6 +5,7 @@ require_relative "activeadmin_mcp/action_definition" require_relative "activeadmin_mcp/action_schema" require_relative "activeadmin_mcp/action_params" +require_relative "activeadmin_mcp/action_catalog" require_relative "activeadmin_mcp/resource_registry" require_relative "activeadmin_mcp/form_field_collector" require_relative "activeadmin_mcp/record_updater" diff --git a/lib/activeadmin_mcp/action_catalog.rb b/lib/activeadmin_mcp/action_catalog.rb new file mode 100644 index 0000000..7fcabb4 --- /dev/null +++ b/lib/activeadmin_mcp/action_catalog.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +module ActiveadminMcp + # Finds every ActiveAdmin action an application has opted in to MCP. + # + # Nothing is cached: ActiveAdmin reloads resources in development, and a + # stale catalog would advertise tools that no longer exist. + module ActionCatalog + # Tool names the engine reserves for itself, including names Phase 2 and 3 + # will take, so an application cannot silently shadow one later. + RESERVED = %w[list_resources query update create describe_form].freeze + + class << self + def all + ResourceRegistry.resources.flat_map { |entry| definitions_for(entry[:config]) } + end + + def find(tool_name) + all.find { |definition| definition.tool_name == tool_name } + end + + private + + def definitions_for(config) + candidates(config).filter_map do |action, kind| + definition = ActionDefinition.build(config: config, action: action, kind: kind) + next unless definition + + next warn_and_skip(definition.errors.join("; ")) unless definition.valid? + next warn_and_skip("#{definition.tool_name} collides with a built-in tool") if reserved?(definition) + + definition + end + end + + def candidates(config) + pairs = [] + pairs.concat(safe_actions(config, :member_actions).map { |a| [a, :member] }) + pairs.concat(safe_actions(config, :collection_actions).map { |a| [a, :collection] }) + pairs.concat(safe_actions(config, :batch_actions).map { |a| [a, :batch] }) if batch_enabled?(config) + pairs.select { |action, _kind| action.respond_to?(:mcp_options) } + end + + def safe_actions(config, reader) + config.respond_to?(reader) ? Array(config.public_send(reader)) : [] + end + + # ActiveAdmin keeps registered batch actions even when the namespace has + # batch actions switched off, and hides them in the UI. Match that. + def batch_enabled?(config) + return false unless config.respond_to?(:batch_actions_enabled?) + + config.batch_actions_enabled? + end + + def reserved?(definition) + RESERVED.include?(definition.tool_name) + end + + def warn_and_skip(message) + warn("[activeadmin_mcp] ignoring action: #{message}") + nil + end + end + end +end diff --git a/spec/activeadmin_mcp/action_catalog_spec.rb b/spec/activeadmin_mcp/action_catalog_spec.rb new file mode 100644 index 0000000..9b8996a --- /dev/null +++ b/spec/activeadmin_mcp/action_catalog_spec.rb @@ -0,0 +1,73 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe ActiveadminMcp::ActionCatalog do + def action(name, mcp:, verb: :post) + double("controller_action", name: name, http_verb: verb, mcp_options: mcp) + end + + def batch(name, mcp:, form: nil) + double("batch_action", sym: name, mcp_options: mcp, inputs: form) + end + + def config(member: [], collection: [], batch_actions: [], batch_enabled: true, name: "Volunteer") + double( + "config", + resource_class: double("model", name: name), + member_actions: member, + collection_actions: collection, + batch_actions: batch_actions, + batch_actions_enabled?: batch_enabled + ) + end + + def stub_resources(*configs) + entries = configs.map { |c| { name: c.resource_class.name, model: c.resource_class, config: c } } + allow(ActiveadminMcp::ResourceRegistry).to receive(:resources).and_return(entries) + end + + it "collects opted-in actions and ignores the rest" do + stub_resources(config(member: [ + action(:create_warning, mcp: { description: "Record a warning" }), + action(:undocumented, mcp: nil) + ])) + + expect(described_class.all.map(&:tool_name)).to eq(["volunteer_create_warning"]) + end + + # Verified against ActiveAdmin 3.5.2: batch_actions_enabled? can be false + # while batch actions are still registered. Exposing them would surface an + # action the admin UI itself hides. + it "skips batch actions when the namespace has them disabled" do + stub_resources(config(batch_actions: [batch(:suspend, mcp: { description: "Suspend" })], + batch_enabled: false)) + + expect(described_class.all).to be_empty + end + + it "includes batch actions when the namespace has them enabled" do + stub_resources(config(batch_actions: [batch(:suspend, mcp: { description: "Suspend" })])) + + expect(described_class.all.map(&:tool_name)).to eq(["volunteer_suspend"]) + end + + it "refuses a tool name that collides with a built-in tool" do + stub_resources(config(collection: [action(:resources, mcp: { description: "Clash" })], name: "List")) + + expect(described_class.all).to be_empty + end + + it "drops invalid declarations rather than exposing them" do + stub_resources(config(member: [action(:create_warning, mcp: { params: {} })])) + + expect(described_class.all).to be_empty + end + + it "finds a definition by tool name" do + stub_resources(config(member: [action(:create_warning, mcp: { description: "Record a warning" })])) + + expect(described_class.find("volunteer_create_warning").action_name).to eq(:create_warning) + expect(described_class.find("nope")).to be_nil + end +end From 5dfb2f1afc400d923ed832ca30004c5f46042302 Mon Sep 17 00:00:00 2001 From: Lloyd Watkin Date: Fri, 18 Sep 2026 20:49:46 +0100 Subject: [PATCH 09/24] Dispatch ActiveAdmin controller actions with the MCP user injected Also fix a latent test-harness bug the new spec exposed: support/active_admin.rb and support/active_record.rb each pointed ActiveRecord::Base at a separate anonymous :memory: SQLite database, so whichever loaded last silently stranded the other's tables. Point both at the same shared-cache database and keep a raw connection open for the process lifetime so the shared cache survives ActiveRecord::Base.establish_connection being called more than once. Co-Authored-By: Claude Sonnet 5 --- lib/activeadmin_mcp.rb | 1 + lib/activeadmin_mcp/controller_dispatcher.rb | 92 +++++++++++++++++++ .../controller_dispatcher_spec.rb | 76 +++++++++++++++ spec/spec_helper.rb | 13 +++ spec/support/active_admin.rb | 8 +- spec/support/active_record.rb | 12 ++- 6 files changed, 200 insertions(+), 2 deletions(-) create mode 100644 lib/activeadmin_mcp/controller_dispatcher.rb create mode 100644 spec/activeadmin_mcp/controller_dispatcher_spec.rb diff --git a/lib/activeadmin_mcp.rb b/lib/activeadmin_mcp.rb index 1db5899..02a7ef6 100644 --- a/lib/activeadmin_mcp.rb +++ b/lib/activeadmin_mcp.rb @@ -6,6 +6,7 @@ require_relative "activeadmin_mcp/action_schema" require_relative "activeadmin_mcp/action_params" require_relative "activeadmin_mcp/action_catalog" +require_relative "activeadmin_mcp/controller_dispatcher" require_relative "activeadmin_mcp/resource_registry" require_relative "activeadmin_mcp/form_field_collector" require_relative "activeadmin_mcp/record_updater" diff --git a/lib/activeadmin_mcp/controller_dispatcher.rb b/lib/activeadmin_mcp/controller_dispatcher.rb new file mode 100644 index 0000000..1136814 --- /dev/null +++ b/lib/activeadmin_mcp/controller_dispatcher.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +module ActiveadminMcp + # Runs a real ActiveAdmin controller action out of band, so an MCP tool call + # goes through the same before_action chain, authorization and callbacks as a + # click in the admin UI. + # + # We synthesize the request rather than route one. Routing would mean issuing + # a second HTTP request against the app, which would have to authenticate as + # the MCP user by forging an admin session — a back door this gem should not + # have. Building the request by hand lets us inject the already-authenticated + # MCP user directly onto the controller instead. + class ControllerDispatcher + def initialize(config:, current_user:) + @config = config + @current_user = current_user + end + + def call(action:, path:, verb: :get, params: {}, path_params: {}) + controller = build_controller + request = build_request(path: path, verb: verb, params: params, action: action, path_params: path_params) + response = ActionDispatch::Response.new + + controller.set_request!(request) + controller.set_response!(response) + controller.process(action) + + capture(request, response) + rescue StandardError => e + { error: "#{@config.resource_class.name}##{action} failed: #{e.message}" } + end + + private + + def build_controller + user = @current_user + controller = @config.controller.new + + controller.define_singleton_method(ActiveadminMcp.config.current_user_method) { user } + controller.define_singleton_method(:current_active_admin_user) { user } + + # The namespace's authentication_method (typically Devise's + # authenticate_admin_user!) would redirect us to a login page. The MCP + # request has already authenticated by bearer token, so it is a no-op + # here — this does NOT skip authorization, which still runs in full. + auth_method = @config.namespace.authentication_method + controller.define_singleton_method(auth_method) { true } if auth_method + + controller + end + + def build_request(path:, verb:, params:, action:, path_params:) + env = Rack::MockRequest.env_for( + path, + method: verb.to_s.upcase, + params: params.transform_keys(&:to_s) + ) + # Flash needs somewhere to live; without a session the action raises. + env["rack.session"] = {} + env["action_dispatch.request.path_parameters"] = + { controller: controller_path, action: action.to_s }.merge(path_params) + + ActionDispatch::Request.new(env) + end + + def controller_path + @config.controller.name.underscore.sub(/_controller\z/, "") + end + + def capture(request, response) + result = { status: response.status } + + if response.redirect? + result[:redirect_to] = response.location + else + # Deliberately not the body: admin HTML is large and almost entirely + # chrome, and would swamp the client's context for no benefit. + result[:rendered] = true + end + + flash = extract_flash(request) + result[:flash] = flash if flash&.any? + result + end + + def extract_flash(request) + request.flash.to_hash + rescue StandardError + nil + end + end +end diff --git a/spec/activeadmin_mcp/controller_dispatcher_spec.rb b/spec/activeadmin_mcp/controller_dispatcher_spec.rb new file mode 100644 index 0000000..5222543 --- /dev/null +++ b/spec/activeadmin_mcp/controller_dispatcher_spec.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +require "spec_helper" +require "support/active_admin" + +RSpec.describe ActiveadminMcp::ControllerDispatcher do + let(:config) { McpSpec::ActiveAdminHarness.volunteer_config } + let(:admin) { AdminUser.create!(email: "admin@example.com") } + let(:volunteer) { Volunteer.create!(name: "Ann") } + + def dispatch(action:, params: {}, path: nil, path_params: {}) + described_class.new(config: config, current_user: admin).call( + action: action, + path: path || config.route_member_action_path(action, volunteer), + verb: :post, + params: params, + path_params: { id: volunteer.id.to_s }.merge(path_params) + ) + end + + it "runs the action and captures the redirect and flash" do + result = dispatch(action: :create_warning, params: { reason: "Late again" }) + + expect(result[:status]).to eq(302) + expect(result[:redirect_to]).to include("/admin/volunteers/#{volunteer.id}") + expect(result[:flash]).to eq("notice" => "Warning recorded") + expect(result).not_to have_key(:error) + end + + it "actually performs the action's side effect" do + dispatch(action: :create_warning, params: { reason: "Late again" }) + + expect(volunteer.reload.name).to eq("Late again") + end + + it "exposes the MCP user to the controller as the current admin user" do + dispatcher = described_class.new(config: config, current_user: admin) + controller = dispatcher.send(:build_controller) + + expect(controller.send(ActiveadminMcp.config.current_user_method)).to eq(admin) + expect(controller.send(:current_active_admin_user)).to eq(admin) + end + + it "returns an error hash rather than raising when the action blows up" do + allow_any_instance_of(config.controller).to receive(:create_warning).and_raise("kaboom") + + result = dispatch(action: :create_warning, params: { reason: "Late" }) + + expect(result[:error]).to include("kaboom") + end + + it "reports a rendered response without returning the body" do + result = dispatch(action: :undocumented) + + expect(result[:status]).to eq(200) + expect(result[:rendered]).to be(true) + expect(result).not_to have_key(:body) + end + + context "when the namespace has an authentication method" do + around do |example| + namespace = ActiveAdmin.application.namespaces[:admin] + previous = namespace.authentication_method + namespace.authentication_method = :authenticate_admin_user! + example.run + namespace.authentication_method = previous + end + + it "neutralises it rather than redirecting to a login page" do + result = dispatch(action: :create_warning, params: { reason: "Late again" }) + + expect(result[:redirect_to]).to include("/admin/volunteers/#{volunteer.id}") + expect(volunteer.reload.name).to eq("Late again") + end + end +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 195e9f6..7c1b285 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -2,6 +2,19 @@ require "active_record" require "action_controller" require "activeadmin_mcp" +require "sqlite3" + +# spec/support/active_admin.rb and spec/support/active_record.rb each define +# their own tables against a shared in-memory SQLite database (see the +# comments in those files for why it must be shared rather than two separate +# ":memory:" databases). A SQLite shared-cache database is destroyed the +# moment its last connection closes, and ActiveRecord::Base.establish_connection +# closes whatever pool it replaces — so without a connection held open +# independently of ActiveRecord for the life of the process, the database +# (and every table in it) would vanish the first time either support file's +# pool got replaced. This constant just keeps one connection open so that +# never happens. +KEEPALIVE_SQLITE_CONNECTION = SQLite3::Database.new("file:activeadmin_mcp_test?mode=memory&cache=shared") RSpec.configure do |config| config.expect_with :rspec do |expectations| diff --git a/spec/support/active_admin.rb b/spec/support/active_admin.rb index 9d9fb38..f10b48c 100644 --- a/spec/support/active_admin.rb +++ b/spec/support/active_admin.rb @@ -9,7 +9,13 @@ require "action_controller/railtie" require "active_model/railtie" -ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:") +# Shared-cache URI, not a bare ":memory:" database — see the comment in +# spec/support/active_record.rb for why: two anonymous in-memory databases +# would fight over ActiveRecord::Base's single connection pool depending on +# spec load order, stranding whichever support file's tables loaded first. +ActiveRecord::Base.establish_connection( + adapter: "sqlite3", database: "file:activeadmin_mcp_test?mode=memory&cache=shared" +) ActiveRecord::Schema.verbose = false ActiveRecord::Schema.define do create_table :volunteers, force: true do |t| diff --git a/spec/support/active_record.rb b/spec/support/active_record.rb index bcd52b2..0a9a3e6 100644 --- a/spec/support/active_record.rb +++ b/spec/support/active_record.rb @@ -2,7 +2,17 @@ # Spin up an in-memory SQLite database with just the tables the ApiToken # model needs. Loaded only by specs that exercise the ActiveRecord model. -ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:") +# +# Uses a shared-cache URI rather than a bare ":memory:" database. Both this +# file and spec/support/active_admin.rb call establish_connection, which +# replaces ActiveRecord::Base's connection pool wholesale; with two distinct +# anonymous ":memory:" databases, whichever support file loads last would +# silently strand the other's tables for the rest of the suite. A shared +# in-memory database lets both support files add their tables to the same +# underlying store regardless of load order. +ActiveRecord::Base.establish_connection( + adapter: "sqlite3", database: "file:activeadmin_mcp_test?mode=memory&cache=shared" +) ActiveRecord::Schema.verbose = false ActiveRecord::Schema.define do From 4e43ba76eb676427475cc4d2c5f9fad65d41a28c Mon Sep 17 00:00:00 2001 From: Lloyd Watkin Date: Fri, 18 Sep 2026 20:56:08 +0100 Subject: [PATCH 10/24] Cover authorization enforcement and clean up test rows in ControllerDispatcher spec Adds a denying ActiveAdmin::AuthorizationAdapter context proving that neutralising the namespace authentication_method does not also bypass authorization: the write is blocked and no error hash masks a real regression. Also stops leaking Volunteer/AdminUser rows across the shared in-memory test database between examples. Co-Authored-By: Claude Sonnet 5 --- .../controller_dispatcher_spec.rb | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/spec/activeadmin_mcp/controller_dispatcher_spec.rb b/spec/activeadmin_mcp/controller_dispatcher_spec.rb index 5222543..e1069de 100644 --- a/spec/activeadmin_mcp/controller_dispatcher_spec.rb +++ b/spec/activeadmin_mcp/controller_dispatcher_spec.rb @@ -3,11 +3,25 @@ require "spec_helper" require "support/active_admin" +# An authorization adapter that denies everything, used to prove that +# neutralising the namespace's authentication_method (see build_controller) +# does not also neutralise authorization, which must keep running in full. +class DenyingAuthorizationAdapter < ActiveAdmin::AuthorizationAdapter + def authorized?(_action, _subject = nil) + false + end +end + RSpec.describe ActiveadminMcp::ControllerDispatcher do let(:config) { McpSpec::ActiveAdminHarness.volunteer_config } let(:admin) { AdminUser.create!(email: "admin@example.com") } let(:volunteer) { Volunteer.create!(name: "Ann") } + after do + Volunteer.delete_all + AdminUser.delete_all + end + def dispatch(action:, params: {}, path: nil, path_params: {}) described_class.new(config: config, current_user: admin).call( action: action, @@ -73,4 +87,29 @@ def dispatch(action:, params: {}, path: nil, path_params: {}) expect(volunteer.reload.name).to eq("Late again") end end + + context "when the authorization adapter denies the action" do + around do |example| + namespace = ActiveAdmin.application.namespaces[:admin] + previous = namespace.authorization_adapter + namespace.authorization_adapter = DenyingAuthorizationAdapter + example.run + namespace.authorization_adapter = previous + end + + it "blocks the action instead of running it" do + result = dispatch(action: :create_warning, params: { reason: "Late again" }) + + # ActiveAdmin's default on_unauthorized_access handler rescues + # ActiveAdmin::AccessDenied internally and turns it into a redirect + # with a flash message, rather than letting the exception reach + # ControllerDispatcher's own rescue — so the denial surfaces as a + # flash entry, not a top-level :error key. What actually matters is + # proven below: the write never happened. + expect(result).not_to have_key(:error) + expect(result[:flash]&.values&.join).to match(/not authorized/i) + + expect(volunteer.reload.name).to eq("Ann") + end + end end From d097bd761ac46856130fb2d3c137aaf99403deca Mon Sep 17 00:00:00 2001 From: Lloyd Watkin Date: Fri, 18 Sep 2026 21:01:04 +0100 Subject: [PATCH 11/24] Gate and run opted-in ActiveAdmin actions for MCP clients Co-Authored-By: Claude Sonnet 5 --- lib/activeadmin_mcp.rb | 1 + lib/activeadmin_mcp/action_runner.rb | 129 +++++++++++++++++++++ spec/activeadmin_mcp/action_runner_spec.rb | 98 ++++++++++++++++ 3 files changed, 228 insertions(+) create mode 100644 lib/activeadmin_mcp/action_runner.rb create mode 100644 spec/activeadmin_mcp/action_runner_spec.rb diff --git a/lib/activeadmin_mcp.rb b/lib/activeadmin_mcp.rb index 02a7ef6..6ba5345 100644 --- a/lib/activeadmin_mcp.rb +++ b/lib/activeadmin_mcp.rb @@ -7,6 +7,7 @@ require_relative "activeadmin_mcp/action_params" require_relative "activeadmin_mcp/action_catalog" require_relative "activeadmin_mcp/controller_dispatcher" +require_relative "activeadmin_mcp/action_runner" require_relative "activeadmin_mcp/resource_registry" require_relative "activeadmin_mcp/form_field_collector" require_relative "activeadmin_mcp/record_updater" diff --git a/lib/activeadmin_mcp/action_runner.rb b/lib/activeadmin_mcp/action_runner.rb new file mode 100644 index 0000000..7e164c2 --- /dev/null +++ b/lib/activeadmin_mcp/action_runner.rb @@ -0,0 +1,129 @@ +# frozen_string_literal: true + +module ActiveadminMcp + # Runs one opted-in ActiveAdmin action for an MCP client. + # + # Two gates gate every call, in this order, both before anything is + # dispatched: + # + # 1. ActiveAdmin's own authorization adapter - the same check `query` and + # `update` make today. + # 2. The action's optional `permission:` proc. + # + # The proc can only ever narrow access. A record the MCP user cannot touch + # stays untouchable whether or not a proc is declared, and the controller's + # own before_action chain runs again during dispatch regardless. + class ActionRunner + def initialize(definition:, current_user:) + @definition = definition + @current_user = current_user + end + + def call(arguments) + parsed = ActionParams.new(@definition).call(arguments) + return parsed if parsed[:error] + + record = find_record(parsed[:record_id]) + return { error: "Record not found: #{@definition.resource_name}##{parsed[:record_id]}" } if record_missing?(record, parsed) + + subject = record || @definition.config.resource_class + return { error: "Not authorized to run #{@definition.tool_name}" } unless authorized?(subject) + + dispatcher = ControllerDispatcher.new(config: @definition.config, current_user: @current_user) + + refusal = permission_refusal(dispatcher, record, parsed) + return { error: refusal } if refusal + + dispatcher.call( + action: dispatch_action, + path: path_for(record, parsed), + verb: @definition.http_verb, + params: dispatch_params(parsed), + path_params: path_params(parsed) + ) + end + + private + + def find_record(id) + return nil unless id + + @definition.config.resource_class.find_by(id: id) + end + + def record_missing?(record, parsed) + parsed.key?(:record_id) && record.nil? + end + + def authorized?(subject) + Authorization.for(@definition.config, @current_user) + .authorized?(@definition.action_name, subject) + end + + # Evaluated the way ActiveAdmin evaluates batch action `:if` procs, so + # current_admin_user, can? and the usual admin helpers are in scope. + # Returns a refusal message, or nil when the proc allows the call. + def permission_refusal(dispatcher, record, parsed) + permission = @definition.permission + return nil unless permission + + controller = dispatcher.send(:build_controller) + outcome = ::MethodOrProcHelper.render_in_context(controller, permission, *permission_args(permission, record, parsed)) + + return outcome if outcome.is_a?(String) + return nil if outcome + + "Not permitted to run #{@definition.tool_name}" + rescue StandardError => e + "Permission check failed for #{@definition.tool_name}: #{e.message}" + end + + # render_in_context instance_execs the proc AND passes args along, so a + # zero-arity lambda would raise ArgumentError if we always handed it a + # record. Match what the proc actually accepts. + def permission_args(permission, record, parsed) + return [] if permission.respond_to?(:arity) && permission.arity.zero? + + case @definition.kind + when :member then [record] + when :batch then [parsed[:record_ids]] + else [] + end + end + + # Batch actions enter through ActiveAdmin's own batch_action controller + # method, so its slicing of inputs to the declared form: keys applies + # exactly as it does in the admin UI. + def dispatch_action + @definition.kind == :batch ? :batch_action : @definition.action_name + end + + def dispatch_params(parsed) + return parsed[:params] unless @definition.kind == :batch + + { + batch_action: @definition.action_name.to_s, + collection_selection: parsed[:record_ids], + batch_action_inputs: JSON.generate(parsed[:params]), + } + end + + def path_params(parsed) + parsed[:record_id] ? { id: parsed[:record_id] } : {} + end + + def path_for(record, parsed) + config = @definition.config + + case @definition.kind + when :member then config.route_member_action_path(@definition.action_name, record) + # RouteBuilder#batch_action_path calls `.permit!` on the params it is + # given, which a plain Hash (its own default argument) does not + # respond to. Pass ActionController::Parameters explicitly to avoid + # tripping over ActiveAdmin's own default. + when :batch then config.route_batch_action_path(ActionController::Parameters.new) + else config.route_collection_path + end + end + end +end diff --git a/spec/activeadmin_mcp/action_runner_spec.rb b/spec/activeadmin_mcp/action_runner_spec.rb new file mode 100644 index 0000000..7d6551f --- /dev/null +++ b/spec/activeadmin_mcp/action_runner_spec.rb @@ -0,0 +1,98 @@ +# frozen_string_literal: true + +require "spec_helper" +require "support/active_admin" + +RSpec.describe ActiveadminMcp::ActionRunner do + let(:config) { McpSpec::ActiveAdminHarness.volunteer_config } + let(:admin) { AdminUser.create!(email: "admin@example.com") } + let!(:volunteer) { Volunteer.create!(name: "Ann") } + + after do + Volunteer.delete_all + AdminUser.delete_all + end + + # Memoised deliberately: ActionCatalog.all rebuilds definitions on every + # call, so re-deriving one per example would stub a different object than the + # one under test. + def find_definition(name, kind) + ActiveadminMcp::ActionCatalog.all.find do |d| + d.action_name == name && d.kind == kind + end + end + + let(:definition) { find_definition(:create_warning, :member) } + let(:batch_definition) { find_definition(:suspend, :batch) } + + def run(definition, arguments, current_user: admin) + described_class.new(definition: definition, current_user: current_user).call(arguments) + end + + it "runs an authorized member action" do + result = run(definition, { "id" => volunteer.id.to_s, "reason" => "Late again" }) + + expect(result[:status]).to eq(302) + expect(volunteer.reload.name).to eq("Late again") + end + + it "refuses before dispatching when ActiveAdmin authorization says no" do + deny = double("authorization") + allow(deny).to receive(:authorized?).and_return(false) + allow(ActiveadminMcp::Authorization).to receive(:for).and_return(deny) + + result = run(definition, { "id" => volunteer.id.to_s, "reason" => "Late again" }) + + expect(result[:error]).to include("Not authorized") + expect(volunteer.reload.name).to eq("Ann") + end + + it "refuses when the record does not exist" do + result = run(definition, { "id" => "999999", "reason" => "Late" }) + + expect(result[:error]).to include("not found") + end + + it "surfaces a validation error from ActionParams without dispatching" do + result = run(definition, { "id" => volunteer.id.to_s }) + + expect(result).to eq(error: "reason is required") + expect(volunteer.reload.name).to eq("Ann") + end + + it "refuses when the permission proc returns false" do + allow(definition).to receive(:permission).and_return(->(_record) { false }) + + result = run(definition, { "id" => volunteer.id.to_s, "reason" => "Late" }) + + expect(result[:error]).to include("Not permitted") + expect(volunteer.reload.name).to eq("Ann") + end + + it "uses a string returned by the permission proc as the refusal reason" do + allow(definition).to receive(:permission).and_return(->(_record) { "Volunteer is suspended" }) + + result = run(definition, { "id" => volunteer.id.to_s, "reason" => "Late" }) + + expect(result[:error]).to eq("Volunteer is suspended") + end + + it "runs the permission proc in controller context" do + seen = nil + allow(definition).to receive(:permission).and_return(proc { |record| seen = [record, current_active_admin_user]; true }) + + run(definition, { "id" => volunteer.id.to_s, "reason" => "Late" }) + + expect(seen).to eq([volunteer, admin]) + end + + it "runs a batch action over the selected records" do + other = Volunteer.create!(name: "Bea") + + result = run(batch_definition, + { "ids" => [volunteer.id.to_s, other.id.to_s], "reason" => "No shows" }) + + expect(result[:status]).to eq(302) + expect(result[:flash]["notice"]).to eq("2 suspended: No shows") + end +end From d8e7dc01a18013a6ad0ea978d75dc22ee450c12b Mon Sep 17 00:00:00 2001 From: Lloyd Watkin Date: Fri, 18 Sep 2026 21:08:20 +0100 Subject: [PATCH 12/24] Cover collection permission procs and batch action record identity Adds action_runner_spec coverage for the untested :collection permission-proc path (allow/refuse/string-reason), and strengthens the batch action spec to prove the right records were mutated rather than just the right count, by having the harness's suspend batch action actually update selected records. Co-Authored-By: Claude Sonnet 5 --- spec/activeadmin_mcp/action_runner_spec.rb | 34 ++++++++++++++++++++++ spec/support/active_admin.rb | 1 + 2 files changed, 35 insertions(+) diff --git a/spec/activeadmin_mcp/action_runner_spec.rb b/spec/activeadmin_mcp/action_runner_spec.rb index 7d6551f..baed840 100644 --- a/spec/activeadmin_mcp/action_runner_spec.rb +++ b/spec/activeadmin_mcp/action_runner_spec.rb @@ -88,11 +88,45 @@ def run(definition, arguments, current_user: admin) it "runs a batch action over the selected records" do other = Volunteer.create!(name: "Bea") + unselected = Volunteer.create!(name: "Cee") result = run(batch_definition, { "ids" => [volunteer.id.to_s, other.id.to_s], "reason" => "No shows" }) expect(result[:status]).to eq(302) expect(result[:flash]["notice"]).to eq("2 suspended: No shows") + expect(volunteer.reload.name).to eq("Suspended: No shows") + expect(other.reload.name).to eq("Suspended: No shows") + expect(unselected.reload.name).to eq("Cee") + end + + describe "collection-kind permission proc" do + let(:collection_definition) { find_definition(:export, :collection) } + + it "dispatches when a zero-arity permission proc returns true" do + allow(collection_definition).to receive(:permission).and_return(-> { true }) + + result = run(collection_definition, {}) + + expect(result[:status]).to eq(302) + expect(result[:flash]["notice"]).to eq("Exported") + end + + it "refuses generically when a zero-arity permission proc returns false" do + allow(collection_definition).to receive(:permission).and_return(-> { false }) + + result = run(collection_definition, {}) + + expect(result[:error]).to include("Not permitted") + expect(result[:flash]).to be_nil + end + + it "uses a string returned by the permission proc as the refusal reason" do + allow(collection_definition).to receive(:permission).and_return(-> { "Exports are disabled" }) + + result = run(collection_definition, {}) + + expect(result[:error]).to eq("Exports are disabled") + end end end diff --git a/spec/support/active_admin.rb b/spec/support/active_admin.rb index f10b48c..931d86e 100644 --- a/spec/support/active_admin.rb +++ b/spec/support/active_admin.rb @@ -89,6 +89,7 @@ class ApplicationController < ActionController::Base; end batch_action :suspend, form: { reason: :text }, mcp: { description: "Suspend the selected volunteers" } do |ids, inputs| + Volunteer.where(id: ids).update_all(name: "Suspended: #{inputs[:reason]}") redirect_to collection_path, notice: "#{ids.size} suspended: #{inputs[:reason]}" end end From be9068daeb0215472cdcec0b9ef62561c4356946 Mon Sep 17 00:00:00 2001 From: Lloyd Watkin Date: Fri, 18 Sep 2026 21:12:22 +0100 Subject: [PATCH 13/24] Expose opted-in ActiveAdmin actions as MCP tools Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 9 ++ README.md | 62 +++++++++++ lib/activeadmin_mcp/request_handler.rb | 108 ++++++++++++------- spec/activeadmin_mcp/request_handler_spec.rb | 63 +++++++++++ 4 files changed, 206 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e04ce5..d1fd7f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- ActiveAdmin `member_action`, `collection_action` and `batch_action` + definitions can be exposed as MCP tools by adding an `mcp:` option to them. + Actions are opt-in: nothing is exposed without that option. Execution runs + through the real ActiveAdmin controller, so `before_action` chains, + authorization and callbacks all apply, and an optional `permission:` proc can + narrow access further. + ### Changed - **Breaking:** the minimum supported Ruby is now 4.0 and the minimum Rails is diff --git a/README.md b/README.md index 94a02e9..568d9e6 100644 --- a/README.md +++ b/README.md @@ -70,6 +70,7 @@ read/query setup without authentication. | `list_resources` | List the ActiveAdmin resources the current user may read, along with their attributes. | | `query` | Query a resource the current user may read, using Ransack syntax, scoped to the records they may access (`limit` defaults to 25, capped at 100). | | `update` | Update an existing record, honouring ActiveAdmin's permitted params and authorization. | +| *(per action)* | Any ActiveAdmin member, collection or batch action the application has opted in with an `mcp:` option, exposed as its own tool. | ### Query examples @@ -98,6 +99,67 @@ The `update` tool applies the same rules as the ActiveAdmin UI: - **Permitted fields only** — attributes are filtered through the resource's `permit_params`; fields the admin form doesn't accept are silently dropped. +### Running member, collection and batch actions + +ActiveAdmin actions are **not** exposed by default. An action becomes an MCP +tool only when you add an `mcp:` option to it: + +```ruby +ActiveAdmin.register Volunteer do + member_action :create_warning, method: :post, mcp: { + description: "Record a warning against a volunteer", + permission: ->(volunteer) { volunteer.active? && can?(:warn, volunteer) }, + params: { + reason: { type: :string, required: true, + hint: "Short free-text summary shown to the volunteer" }, + severity: { type: :string, enum: %w[low medium high] }, + category: { type: :string, + suggestions: -> { WarningCategory.pluck(:name) } } + } + } do + # your existing action body, unchanged + end +end +``` + +That registers a `volunteer_create_warning` tool. Batch actions opt in the same +way, and inherit their param types from the `form:` hash you already declare: + +```ruby +batch_action :suspend, form: { reason: :text }, + mcp: { description: "Suspend the selected volunteers" } do |ids, inputs| + # ... +end +``` + +**Declaring params** + +- `type:` — one of `:string`, `:integer`, `:number`, `:boolean`, `:array`, `:object`. +- `required:` — refuses the call when the value is missing. +- `hint:` — static guidance shown to the agent. Always a plain string. +- `enum:` — **binding**. A value outside the list is refused before dispatch. +- `suggestions:` — a proc evaluated when tools are listed. **Advisory only**, + never enforced, so use it for live values from the database. If it raises, + the tool is still listed without suggestions. + +**Authorization** + +`permission:` is an *additional* gate, never a replacement. Every call first +passes your ActiveAdmin authorization adapter exactly as `query` and `update` +do; the proc can only narrow access further, never widen it. It is evaluated in +controller context, so `current_admin_user`, `can?` and the usual admin helpers +are available. Return `false` to refuse, or a `String` to refuse with a reason +the agent can act on. + +**What you get back** + +Actions are executed through your real ActiveAdmin controller, so the action's +`before_action` chain, authorization and callbacks all run. The tool returns the +response status, the redirect target and any flash messages — not the rendered +HTML. Redirect-style (submit-side) actions are the supported case; a `GET` +action that renders a full admin view is best-effort and may fail for want of a +view context. + ## Connecting a client `activeadmin_mcp` has been tested with **Claude Code** (Anthropic) over the diff --git a/lib/activeadmin_mcp/request_handler.rb b/lib/activeadmin_mcp/request_handler.rb index a6fb0ca..688e74d 100644 --- a/lib/activeadmin_mcp/request_handler.rb +++ b/lib/activeadmin_mcp/request_handler.rb @@ -38,45 +38,74 @@ def initialize_result end def tools_list - { - tools: [ - { - name: "list_resources", - description: "List the ActiveAdmin resources the authenticated user is authorized " \ - "to read, with their attributes", - inputSchema: { type: "object", properties: {} }, - }, - { - name: "query", - description: "Query an ActiveAdmin resource using Ransack syntax. Respects ActiveAdmin " \ - "authorization: the resource must be readable by the authenticated user, " \ - "and results are scoped to the records they may access.", - inputSchema: { - type: "object", - properties: { - resource: { type: "string", description: "Resource name (e.g., 'User', 'Post')" }, - q: { type: "object", description: "Ransack query (e.g., {name_cont: 'john'})" }, - limit: { type: "integer", description: "Max records (default: 25)" }, - }, - required: ["resource"], + { tools: built_in_tools + action_tools } + end + + def action_tools + ActionCatalog.all.filter_map do |definition| + next unless authorized_to_list?(definition) + + { + name: definition.tool_name, + description: definition.description, + inputSchema: ActionSchema.new(definition).to_h, + } + end + end + + # Collection and batch actions whose permission proc takes no record can be + # resolved now, so the tool is simply hidden. A member action's proc needs a + # record, so its tool stays listed and refusal happens at call time. + def authorized_to_list?(definition) + permission = definition.permission + return true unless permission + return true unless permission.respond_to?(:arity) && permission.arity.zero? + + begin + !!permission.call + rescue StandardError + false + end + end + + def built_in_tools + [ + { + name: "list_resources", + description: "List the ActiveAdmin resources the authenticated user is authorized " \ + "to read, with their attributes", + inputSchema: { type: "object", properties: {} }, + }, + { + name: "query", + description: "Query an ActiveAdmin resource using Ransack syntax. Respects ActiveAdmin " \ + "authorization: the resource must be readable by the authenticated user, " \ + "and results are scoped to the records they may access.", + inputSchema: { + type: "object", + properties: { + resource: { type: "string", description: "Resource name (e.g., 'User', 'Post')" }, + q: { type: "object", description: "Ransack query (e.g., {name_cont: 'john'})" }, + limit: { type: "integer", description: "Max records (default: 25)" }, }, + required: ["resource"], }, - { - name: "update", - description: "Update an existing record. Only fields the resource's ActiveAdmin " \ - "form permits are written, and the update respects ActiveAdmin authorization.", - inputSchema: { - type: "object", - properties: { - resource: { type: "string", description: "Resource name (e.g., 'User', 'Post')" }, - id: { type: ["integer", "string"], description: "Primary key of the record to update" }, - attributes: { type: "object", description: "Attributes to update (e.g., {name: 'New name'})" }, - }, - required: %w[resource id attributes], + }, + { + name: "update", + description: "Update an existing record. Only fields the resource's ActiveAdmin " \ + "form permits are written, and the update respects ActiveAdmin authorization.", + inputSchema: { + type: "object", + properties: { + resource: { type: "string", description: "Resource name (e.g., 'User', 'Post')" }, + id: { type: ["integer", "string"], description: "Primary key of the record to update" }, + attributes: { type: "object", description: "Attributes to update (e.g., {name: 'New name'})" }, }, + required: %w[resource id attributes], }, - ], - } + }, + ] end def call_tool(params) @@ -87,12 +116,19 @@ def call_tool(params) when "list_resources" then tool_list_resources when "query" then tool_query(args) when "update" then tool_update(args) - else { error: "Unknown tool: #{name}" } + else tool_action(name, args) end { content: [{ type: "text", text: JSON.pretty_generate(result) }] } end + def tool_action(name, args) + definition = ActionCatalog.find(name) + return { error: "Unknown tool: #{name}" } unless definition + + ActionRunner.new(definition: definition, current_user: @current_user).call(args) + end + def tool_list_resources entries = ResourceRegistry.resources.select { |entry| authorized_to_read?(entry) } { resources: entries.map { |entry| ResourceRegistry.resource_info(entry) } } diff --git a/spec/activeadmin_mcp/request_handler_spec.rb b/spec/activeadmin_mcp/request_handler_spec.rb index 116e439..295fab5 100644 --- a/spec/activeadmin_mcp/request_handler_spec.rb +++ b/spec/activeadmin_mcp/request_handler_spec.rb @@ -40,6 +40,8 @@ def handle(method, params = nil, id: 1) end describe "tools/list" do + before { allow(ActiveadminMcp::ActionCatalog).to receive(:all).and_return([]) } + it "advertises the list_resources, query and update tools" do tools = handle("tools/list")[:result][:tools] @@ -235,4 +237,65 @@ def stub_resource(authorized: true) end end end + + describe "action tools" do + def definition(tool_name: "volunteer_create_warning", permission: nil) + double( + "definition", + tool_name: tool_name, + description: "Record a warning", + kind: :member, + params: { reason: { type: :string, required: true } }, + permission: permission, + resource_name: "Volunteer", + config: double("config") + ) + end + + def handle(request, current_user: :admin) + ActiveadminMcp::RequestHandler.new(current_user: current_user).handle(request) + end + + it "lists opted-in actions alongside the built-in tools" do + allow(ActiveadminMcp::ActionCatalog).to receive(:all).and_return([definition]) + allow(ActiveadminMcp::ResourceRegistry).to receive(:resources).and_return([]) + + response = handle({ "id" => 1, "method" => "tools/list" }) + names = response[:result][:tools].map { |tool| tool[:name] } + + expect(names).to include("volunteer_create_warning") + tool = response[:result][:tools].find { |t| t[:name] == "volunteer_create_warning" } + expect(tool[:description]).to eq("Record a warning") + expect(tool[:inputSchema][:required]).to include("id", "reason") + end + + it "routes a call to the action runner" do + target = definition + allow(ActiveadminMcp::ActionCatalog).to receive(:find).with("volunteer_create_warning").and_return(target) + + runner = instance_double(ActiveadminMcp::ActionRunner, call: { status: 302 }) + allow(ActiveadminMcp::ActionRunner).to receive(:new) + .with(definition: target, current_user: :admin).and_return(runner) + + response = handle({ + "id" => 2, "method" => "tools/call", + "params" => { "name" => "volunteer_create_warning", + "arguments" => { "id" => "1", "reason" => "Late" } } + }) + + expect(runner).to have_received(:call).with({ "id" => "1", "reason" => "Late" }) + expect(response[:result][:content].first[:text]).to include("302") + end + + it "reports an unknown tool" do + allow(ActiveadminMcp::ActionCatalog).to receive(:find).and_return(nil) + + response = handle({ + "id" => 3, "method" => "tools/call", + "params" => { "name" => "nope", "arguments" => {} } + }) + + expect(response[:result][:content].first[:text]).to include("Unknown tool") + end + end end From 0190e2d7ffa6e24efbcc347388d89c30c5c5723a Mon Sep 17 00:00:00 2001 From: Lloyd Watkin Date: Fri, 18 Sep 2026 21:32:14 +0100 Subject: [PATCH 14/24] Scope batch ids, stop leaking exception text, publish the controller builder ControllerDispatcher#build_controller was private and reached through send(:...). It is now the public controller_with_mcp_user, since listing-time permission checks need the same context a dispatched call gets. It also stubs the ActiveAdmin namespace's own current_user_method alongside the configured one, so an action body calling either gets the MCP user rather than nil when the two settings diverge. Batch actions could only be authorized at the class level, because there is no single record, which left the submitted ids unchecked: a client could name records the adapter's scope_collection excludes and ActiveAdmin would mutate them. ActionRunner now runs the ids back through scope_collection and refuses the whole call if any falls outside it. Refusing rather than narrowing is deliberate - a client must never believe it acted on N records when it acted on fewer. Both rescue sites interpolated the exception message into the error returned to the client, which could carry raw SQL, table names or file paths. The detail now goes to the log and the client gets a generic failure naming the resource and action. Co-Authored-By: Claude Opus 5 (1M context) --- lib/activeadmin_mcp/action_runner.rb | 40 ++++++++++- lib/activeadmin_mcp/controller_dispatcher.rb | 37 ++++++++-- spec/activeadmin_mcp/action_runner_spec.rb | 67 +++++++++++++++++++ .../controller_dispatcher_spec.rb | 52 +++++++++++++- 4 files changed, 184 insertions(+), 12 deletions(-) diff --git a/lib/activeadmin_mcp/action_runner.rb b/lib/activeadmin_mcp/action_runner.rb index 7e164c2..b7f697e 100644 --- a/lib/activeadmin_mcp/action_runner.rb +++ b/lib/activeadmin_mcp/action_runner.rb @@ -9,6 +9,9 @@ module ActiveadminMcp # 1. ActiveAdmin's own authorization adapter - the same check `query` and # `update` make today. # 2. The action's optional `permission:` proc. + # 3. For batch actions only, the submitted ids are run back through the + # adapter's scope_collection, since gate 1 could only authorize the + # resource class. # # The proc can only ever narrow access. A record the MCP user cannot touch # stays untouchable whether or not a proc is declared, and the controller's @@ -34,6 +37,9 @@ def call(arguments) refusal = permission_refusal(dispatcher, record, parsed) return { error: refusal } if refusal + out_of_scope = unauthorized_batch_ids(parsed) + return { error: out_of_scope } if out_of_scope + dispatcher.call( action: dispatch_action, path: path_for(record, parsed), @@ -67,7 +73,7 @@ def permission_refusal(dispatcher, record, parsed) permission = @definition.permission return nil unless permission - controller = dispatcher.send(:build_controller) + controller = dispatcher.controller_with_mcp_user outcome = ::MethodOrProcHelper.render_in_context(controller, permission, *permission_args(permission, record, parsed)) return outcome if outcome.is_a?(String) @@ -75,7 +81,37 @@ def permission_refusal(dispatcher, record, parsed) "Not permitted to run #{@definition.tool_name}" rescue StandardError => e - "Permission check failed for #{@definition.tool_name}: #{e.message}" + # Keep the detail server-side: the message can carry SQL, paths and other + # internals the MCP client has no business seeing. + warn("[activeadmin_mcp] permission proc for #{@definition.tool_name} raised #{e.class}: #{e.message}") + "Permission check failed for #{@definition.tool_name}" + end + + # Gate 1 can only authorize the resource class for a batch action, because + # there is no single record. That leaves the submitted ids unchecked, so a + # client could name records the adapter's scope_collection excludes. Run the + # ids back through the scope and refuse the whole call if any falls outside + # it — narrowing silently would let a client believe it acted on records it + # never touched. + def unauthorized_batch_ids(parsed) + return nil unless @definition.kind == :batch + + ids = Array(parsed[:record_ids]) + return nil if ids.empty? + + klass = @definition.config.resource_class + key = klass.primary_key + scoped = Authorization.for(@definition.config, @current_user) + .scope_collection(klass.where(key => ids), @definition.action_name) + permitted = scoped.pluck(key).map(&:to_s) + refused = ids - permitted + return nil if refused.empty? + + "Not authorized to run #{@definition.tool_name} on #{@definition.resource_name} " \ + "#{refused.join(', ')} (not found, or outside your permitted scope)" + rescue StandardError => e + warn("[activeadmin_mcp] scoping batch ids for #{@definition.tool_name} raised #{e.class}: #{e.message}") + "Not authorized to run #{@definition.tool_name}" end # render_in_context instance_execs the proc AND passes args along, so a diff --git a/lib/activeadmin_mcp/controller_dispatcher.rb b/lib/activeadmin_mcp/controller_dispatcher.rb index 1136814..69dd01a 100644 --- a/lib/activeadmin_mcp/controller_dispatcher.rb +++ b/lib/activeadmin_mcp/controller_dispatcher.rb @@ -17,7 +17,7 @@ def initialize(config:, current_user:) end def call(action:, path:, verb: :get, params: {}, path_params: {}) - controller = build_controller + controller = controller_with_mcp_user request = build_request(path: path, verb: verb, params: params, action: action, path_params: path_params) response = ActionDispatch::Response.new @@ -27,17 +27,24 @@ def call(action:, path:, verb: :get, params: {}, path_params: {}) capture(request, response) rescue StandardError => e - { error: "#{@config.resource_class.name}##{action} failed: #{e.message}" } + # The exception text can carry internals — SQL fragments, table names, + # file paths. It belongs in the application's log, not in a tool result + # that goes to an MCP client. + warn("[activeadmin_mcp] #{@config.resource_class.name}##{action} raised #{e.class}: #{e.message}") + { error: "#{@config.resource_class.name}##{action} failed" } end - private - - def build_controller + # A controller instance for this resource with the MCP user injected, ready + # either to process a request or to serve as the evaluation context for an + # action's `permission:` proc. Public because listing-time permission checks + # need exactly the same context a dispatched call gets. + def controller_with_mcp_user user = @current_user controller = @config.controller.new - controller.define_singleton_method(ActiveadminMcp.config.current_user_method) { user } - controller.define_singleton_method(:current_active_admin_user) { user } + current_user_methods.each do |method_name| + controller.define_singleton_method(method_name) { user } + end # The namespace's authentication_method (typically Devise's # authenticate_admin_user!) would redirect us to a login page. The MCP @@ -49,6 +56,22 @@ def build_controller controller end + private + + # An application can point ActiveadminMcp at one current-user method and the + # ActiveAdmin namespace at another. Stub both, so an action body calling + # either gets the MCP user rather than nil. uniq keeps us from defining the + # same singleton method twice when they agree. + def current_user_methods + namespace_method = @config.namespace.current_user_method if @config.namespace.respond_to?(:current_user_method) + + [ + ActiveadminMcp.config.current_user_method, + namespace_method, + :current_active_admin_user, + ].select { |name| name.respond_to?(:to_sym) }.map(&:to_sym).uniq + end + def build_request(path:, verb:, params:, action:, path_params:) env = Rack::MockRequest.env_for( path, diff --git a/spec/activeadmin_mcp/action_runner_spec.rb b/spec/activeadmin_mcp/action_runner_spec.rb index baed840..7cd64f0 100644 --- a/spec/activeadmin_mcp/action_runner_spec.rb +++ b/spec/activeadmin_mcp/action_runner_spec.rb @@ -3,6 +3,20 @@ require "spec_helper" require "support/active_admin" +# Authorizes every action at the class level but scopes collections down to +# active records, the shape a real CanCanCan or Pundit adapter has: gate 1 +# passes for a batch action (its subject is the resource class), and only +# scope_collection knows which records are actually reachable. +class ActiveOnlyAuthorizationAdapter < ActiveAdmin::AuthorizationAdapter + def authorized?(_action, _subject = nil) + true + end + + def scope_collection(collection, _action = :read) + collection.where(active: true) + end +end + RSpec.describe ActiveadminMcp::ActionRunner do let(:config) { McpSpec::ActiveAdminHarness.volunteer_config } let(:admin) { AdminUser.create!(email: "admin@example.com") } @@ -77,6 +91,19 @@ def run(definition, arguments, current_user: admin) expect(result[:error]).to eq("Volunteer is suspended") end + it "keeps a raising permission proc's message out of the refusal" do + allow(definition).to receive(:permission).and_return( + ->(_record) { raise "SQLite3::SQLException: no such table: nope_secret" } + ) + messages = [] + allow_any_instance_of(described_class).to receive(:warn) { |_, message| messages << message } + + result = run(definition, { "id" => volunteer.id.to_s, "reason" => "Late" }) + + expect(result[:error]).to eq("Permission check failed for volunteer_create_warning") + expect(messages.join).to include("nope_secret") + end + it "runs the permission proc in controller context" do seen = nil allow(definition).to receive(:permission).and_return(proc { |record| seen = [record, current_active_admin_user]; true }) @@ -129,4 +156,44 @@ def run(definition, arguments, current_user: admin) expect(result[:error]).to eq("Exports are disabled") end end + + # Gate 1 can only authorize the resource class for a batch action, so without + # an explicit id-scope check a client could name records the adapter excludes + # and ActiveAdmin would happily mutate them. + describe "batch ids outside the authorized scope" do + around do |example| + namespace = ActiveAdmin.application.namespaces[:admin] + previous = namespace.authorization_adapter + namespace.authorization_adapter = ActiveOnlyAuthorizationAdapter + example.run + namespace.authorization_adapter = previous + end + + it "refuses the whole call and mutates nothing when one id is out of scope" do + hidden = Volunteer.create!(name: "Hidden", active: false) + + result = run(batch_definition, + { "ids" => [volunteer.id.to_s, hidden.id.to_s], "reason" => "No shows" }) + + expect(result[:error]).to include("Not authorized", hidden.id.to_s) + expect(volunteer.reload.name).to eq("Ann") + expect(hidden.reload.name).to eq("Hidden") + end + + it "refuses ids that do not exist at all" do + result = run(batch_definition, { "ids" => ["999999"], "reason" => "No shows" }) + + expect(result[:error]).to include("999999") + end + + it "still runs when every id is inside the scope" do + other = Volunteer.create!(name: "Bea") + + result = run(batch_definition, + { "ids" => [volunteer.id.to_s, other.id.to_s], "reason" => "No shows" }) + + expect(result[:status]).to eq(302) + expect(volunteer.reload.name).to eq("Suspended: No shows") + end + end end diff --git a/spec/activeadmin_mcp/controller_dispatcher_spec.rb b/spec/activeadmin_mcp/controller_dispatcher_spec.rb index e1069de..eb5d8ec 100644 --- a/spec/activeadmin_mcp/controller_dispatcher_spec.rb +++ b/spec/activeadmin_mcp/controller_dispatcher_spec.rb @@ -4,7 +4,7 @@ require "support/active_admin" # An authorization adapter that denies everything, used to prove that -# neutralising the namespace's authentication_method (see build_controller) +# neutralising the namespace's authentication_method (see controller_with_mcp_user) # does not also neutralise authorization, which must keep running in full. class DenyingAuthorizationAdapter < ActiveAdmin::AuthorizationAdapter def authorized?(_action, _subject = nil) @@ -49,7 +49,7 @@ def dispatch(action:, params: {}, path: nil, path_params: {}) it "exposes the MCP user to the controller as the current admin user" do dispatcher = described_class.new(config: config, current_user: admin) - controller = dispatcher.send(:build_controller) + controller = dispatcher.controller_with_mcp_user expect(controller.send(ActiveadminMcp.config.current_user_method)).to eq(admin) expect(controller.send(:current_active_admin_user)).to eq(admin) @@ -57,10 +57,56 @@ def dispatch(action:, params: {}, path: nil, path_params: {}) it "returns an error hash rather than raising when the action blows up" do allow_any_instance_of(config.controller).to receive(:create_warning).and_raise("kaboom") + allow_any_instance_of(described_class).to receive(:warn) result = dispatch(action: :create_warning, params: { reason: "Late" }) - expect(result[:error]).to include("kaboom") + expect(result[:error]).to eq("Volunteer#create_warning failed") + end + + # An exception message can carry SQL, table names and file paths. The client + # gets a generic failure; the detail goes to the log. + it "keeps the exception message out of the client's result and logs it instead" do + allow_any_instance_of(config.controller).to receive(:create_warning) + .and_raise("SQLite3::SQLException: no such table: nope_secret: SELECT * FROM nope_secret") + messages = [] + allow_any_instance_of(described_class).to receive(:warn) { |_, message| messages << message } + + result = dispatch(action: :create_warning, params: { reason: "Late" }) + + expect(result[:error]).not_to include("nope_secret") + expect(messages.join).to include("nope_secret") + end + + # ActiveadminMcp.config.current_user_method and the ActiveAdmin namespace's + # own current_user_method can disagree; an action body calling either one must + # get the MCP user, not nil. + context "when the namespace names a different current user method" do + around do |example| + namespace = ActiveAdmin.application.namespaces[:admin] + previous = namespace.current_user_method + namespace.current_user_method = :current_namespace_admin + example.run + namespace.current_user_method = previous + end + + it "stubs the namespace's method as well as the configured one" do + controller = described_class.new(config: config, current_user: admin).controller_with_mcp_user + + expect(controller.send(:current_namespace_admin)).to eq(admin) + expect(controller.send(ActiveadminMcp.config.current_user_method)).to eq(admin) + expect(controller.send(:current_active_admin_user)).to eq(admin) + end + end + + it "defines each current user method once when the two settings agree" do + namespace = ActiveAdmin.application.namespaces[:admin] + allow(namespace).to receive(:current_user_method).and_return(ActiveadminMcp.config.current_user_method) + + controller = described_class.new(config: config, current_user: admin).controller_with_mcp_user + + defined_names = controller.singleton_methods.map(&:to_s) + expect(defined_names.count(ActiveadminMcp.config.current_user_method.to_s)).to eq(1) end it "reports a rendered response without returning the body" do From f912b32deda9c1328eed67b796ac9c5d4add21ff Mon Sep 17 00:00:00 2001 From: Lloyd Watkin Date: Fri, 18 Sep 2026 21:32:14 +0100 Subject: [PATCH 15/24] Authorize action tools at listing time, in controller context tools/list filtered action tools only on the permission: proc and never applied the ActiveAdmin authorization adapter, so a user authorized for nothing was still shown every opted-in action tool. It now runs the same adapter check list_resources makes, against the resource class - there is no record at listing time. That gate runs before the schema is built, not after the list is assembled: ActionSchema calls the application's suggestions: procs, which read the database, so filtering afterwards would hide the tool and still hand its rows to a fully denied user. Listing-time permission: procs were called bare, so self was the RequestHandler and current_admin_user and can? were undefined. The README's own example raised NameError, was swallowed, and hid the tool from every user including superusers with no diagnostic. They are now evaluated through MethodOrProcHelper.render_in_context against a real controller, exactly as ActionRunner evaluates them at call time, and a proc that raises hides its own tool and warns rather than doing so silently. Co-Authored-By: Claude Opus 5 (1M context) --- lib/activeadmin_mcp/request_handler.rb | 33 +++- spec/activeadmin_mcp/request_handler_spec.rb | 157 +++++++++++++++++-- 2 files changed, 171 insertions(+), 19 deletions(-) diff --git a/lib/activeadmin_mcp/request_handler.rb b/lib/activeadmin_mcp/request_handler.rb index 688e74d..c2993b0 100644 --- a/lib/activeadmin_mcp/request_handler.rb +++ b/lib/activeadmin_mcp/request_handler.rb @@ -41,8 +41,13 @@ def tools_list { tools: built_in_tools + action_tools } end + # Both gates run before the schema is built, never after: ActionSchema calls + # the application's `suggestions:` procs, which read the database. Filtering + # an already-assembled list would mean those procs had already run — and + # their values already been read — for a user authorized for none of it. def action_tools ActionCatalog.all.filter_map do |definition| + next unless authorized_to_run?(definition) next unless authorized_to_list?(definition) { @@ -53,19 +58,37 @@ def action_tools end end + # The same authorization adapter check ActionRunner makes before dispatch, + # and the one list_resources makes for reads. There is no record at listing + # time, so the subject is the resource class. + def authorized_to_run?(definition) + Authorization.for(definition.config, @current_user) + .authorized?(definition.action_name, definition.config.resource_class) + rescue StandardError => e + warn("[activeadmin_mcp] hiding #{definition.tool_name}: authorization check raised #{e.class}: #{e.message}") + false + end + # Collection and batch actions whose permission proc takes no record can be # resolved now, so the tool is simply hidden. A member action's proc needs a # record, so its tool stays listed and refusal happens at call time. + # + # The proc is evaluated in controller context, exactly as ActionRunner + # evaluates it at call time, so `current_admin_user`, `can?` and the rest of + # the admin helpers are in scope. Evaluating it bare would make every such + # proc raise NameError and hide its tool from everybody. def authorized_to_list?(definition) permission = definition.permission return true unless permission return true unless permission.respond_to?(:arity) && permission.arity.zero? - begin - !!permission.call - rescue StandardError - false - end + controller = ControllerDispatcher.new(config: definition.config, current_user: @current_user) + .controller_with_mcp_user + !!::MethodOrProcHelper.render_in_context(controller, permission) + rescue StandardError => e + # One broken proc hides its own tool and nothing else, but it says so. + warn("[activeadmin_mcp] hiding #{definition.tool_name}: permission proc raised #{e.class}: #{e.message}") + false end def built_in_tools diff --git a/spec/activeadmin_mcp/request_handler_spec.rb b/spec/activeadmin_mcp/request_handler_spec.rb index 295fab5..a18a1cd 100644 --- a/spec/activeadmin_mcp/request_handler_spec.rb +++ b/spec/activeadmin_mcp/request_handler_spec.rb @@ -1,8 +1,24 @@ require "spec_helper" +require "support/active_admin" RSpec.describe ActiveadminMcp::RequestHandler do subject(:handler) { described_class.new } + # A stand-in ActiveAdmin authorization adapter. `scope_collection` mirrors + # the real adapters by returning the collection it is handed, so tests can + # assert on the relation the handler builds. `calls` records every + # authorized? call, so a test can assert on the subject that was checked. + def adapter_class(authorized:, calls: []) + Class.new do + define_method(:initialize) { |*| } + define_method(:authorized?) do |action, subject = nil| + calls << [action, subject] + authorized + end + def scope_collection(collection, *) = collection + end + end + def handle(method, params = nil, id: 1) request = { "id" => id, "method" => method } request["params"] = params if params @@ -80,17 +96,6 @@ def call_tool(name, arguments = {}) JSON.parse(text) end - # A stand-in ActiveAdmin authorization adapter. `scope_collection` mirrors - # the real adapters by returning the collection it is handed, so tests can - # assert on the relation the handler builds. - def adapter_class(authorized:) - Class.new do - define_method(:initialize) { |*| } - define_method(:authorized?) { |*| authorized } - def scope_collection(collection, *) = collection - end - end - def resource_config(authorized: true) namespace = double("namespace", authorization_adapter: adapter_class(authorized: authorized)) double("config", namespace: namespace) @@ -239,16 +244,22 @@ def stub_resource(authorized: true) end describe "action tools" do - def definition(tool_name: "volunteer_create_warning", permission: nil) + let(:resource_class) { Class.new } + + def definition(tool_name: "volunteer_create_warning", permission: nil, + params: { reason: { type: :string, required: true } }, + adapter: adapter_class(authorized: true)) + namespace = double("namespace", authorization_adapter: adapter) double( "definition", tool_name: tool_name, description: "Record a warning", kind: :member, - params: { reason: { type: :string, required: true } }, + params: params, permission: permission, + action_name: :create_warning, resource_name: "Volunteer", - config: double("config") + config: double("config", namespace: namespace, resource_class: resource_class) ) end @@ -256,6 +267,14 @@ def handle(request, current_user: :admin) ActiveadminMcp::RequestHandler.new(current_user: current_user).handle(request) end + def tool_names(definitions, current_user: :admin) + allow(ActiveadminMcp::ActionCatalog).to receive(:all).and_return(Array(definitions)) + allow(ActiveadminMcp::ResourceRegistry).to receive(:resources).and_return([]) + + handle({ "id" => 1, "method" => "tools/list" }, current_user: current_user)[:result][:tools] + .map { |tool| tool[:name] } + end + it "lists opted-in actions alongside the built-in tools" do allow(ActiveadminMcp::ActionCatalog).to receive(:all).and_return([definition]) allow(ActiveadminMcp::ResourceRegistry).to receive(:resources).and_return([]) @@ -297,5 +316,115 @@ def handle(request, current_user: :admin) expect(response[:result][:content].first[:text]).to include("Unknown tool") end + + describe "authorization at listing time" do + it "hides an action tool the authorization adapter refuses" do + denied = definition(adapter: adapter_class(authorized: false)) + + expect(tool_names(denied)).not_to include("volunteer_create_warning") + end + + it "checks the resource class, since there is no record at listing time" do + calls = [] + listed = definition(adapter: adapter_class(authorized: true, calls: calls)) + + tool_names(listed) + + expect(calls).to eq([[:create_warning, resource_class]]) + end + + it "hides only the offending tool when an adapter raises" do + exploding = Class.new do + define_method(:initialize) { |*| } + define_method(:authorized?) { |*| raise "adapter exploded" } + end + boom = definition(tool_name: "volunteer_boom", adapter: exploding) + allow_any_instance_of(described_class).to receive(:warn) + + names = tool_names([boom, definition]) + + expect(names).not_to include("volunteer_boom") + expect(names).to include("volunteer_create_warning") + end + + # The schema is what runs the application's `suggestions:` procs, so it + # must never be built for a tool the user is not authorized for. Filtering + # an assembled list would be too late: the proc would already have read + # the database and handed its rows over. + it "never runs a suggestions proc for a user the adapter refuses" do + ran = false + suggestions = -> { ran = true; %w[secret-category] } + denied = definition( + adapter: adapter_class(authorized: false), + params: { category: { type: :string, suggestions: suggestions } } + ) + + names = tool_names(denied) + + # Asserted first, deliberately: filtering the assembled list would hide + # the tool and still leak, so the proc not running is the real property. + expect(ran).to be(false) + expect(names).not_to include("volunteer_create_warning") + end + + it "does run a suggestions proc for an authorized user" do + ran = false + suggestions = -> { ran = true; %w[visible-category] } + allowed = definition(params: { category: { type: :string, suggestions: suggestions } }) + + expect(tool_names(allowed)).to include("volunteer_create_warning") + expect(ran).to be(true) + end + end + + # These need the real ActiveAdmin harness: the point of the fix is that the + # proc is instance_exec'd against a real controller, which no double can + # stand in for. + describe "a permission proc at listing time" do + let(:admin) { AdminUser.create!(email: "admin@example.com") } + + after { AdminUser.delete_all } + + def catalog_definition(action_name, kind) + ActiveadminMcp::ActionCatalog.all.find do |d| + d.action_name == action_name && d.kind == kind + end + end + + let(:export) { catalog_definition(:export, :collection) } + + it "evaluates a zero-arity proc in controller context" do + seen = nil + allow(export).to receive(:permission).and_return(-> { seen = current_active_admin_user; true }) + + names = tool_names(export, current_user: admin) + + expect(seen).to eq(admin) + expect(names).to include("volunteer_export") + end + + it "hides the tool when a controller-context proc refuses" do + allow(export).to receive(:permission).and_return(-> { current_active_admin_user.nil? }) + + expect(tool_names(export, current_user: admin)).not_to include("volunteer_export") + end + + it "hides only the raising tool and keeps the rest of the listing" do + allow(export).to receive(:permission).and_return(-> { raise "proc exploded" }) + allow_any_instance_of(described_class).to receive(:warn) + + names = tool_names([export, definition], current_user: admin) + + expect(names).not_to include("volunteer_export") + expect(names).to include("volunteer_create_warning", "query") + end + + it "leaves a member action's proc for call time" do + warning = catalog_definition(:create_warning, :member) + allow(warning).to receive(:permission).and_return(->(_record) { false }) + + expect(tool_names(warning, current_user: admin)).to include("volunteer_create_warning") + end + end end end From 63bb87af43ce3c5bd364f67641e765d17d26a16c Mon Sep 17 00:00:00 2001 From: Lloyd Watkin Date: Fri, 18 Sep 2026 21:32:14 +0100 Subject: [PATCH 16/24] Refuse undroppable batch params, and say so when the readers cannot install ActiveAdmin's batch_action controller method slices submitted inputs down to the declared form: keys, so a param declared only under mcp: was advertised to the client, possibly as required, validated, JSON-encoded - and then dropped before the block ever saw it. ActionDefinition#validate! now rejects it at declaration time, alongside the existing reserved-name rejection. Only batch actions that actually declare a form: hash are affected; without one ActiveAdmin slices nothing. ActiveAdminExt.apply! returning false makes every opted-in tool vanish from tools/list, because ActionCatalog can no longer find an mcp: option anywhere. It now warns rather than turning the whole feature off in silence. Co-Authored-By: Claude Opus 5 (1M context) --- lib/activeadmin_mcp/action_definition.rb | 23 +++++++++ lib/activeadmin_mcp/active_admin_ext.rb | 14 +++++- .../activeadmin_mcp/action_definition_spec.rb | 50 +++++++++++++++++++ spec/activeadmin_mcp/active_admin_ext_spec.rb | 14 ++++++ 4 files changed, 99 insertions(+), 2 deletions(-) diff --git a/lib/activeadmin_mcp/action_definition.rb b/lib/activeadmin_mcp/action_definition.rb index 5588f35..b8ac659 100644 --- a/lib/activeadmin_mcp/action_definition.rb +++ b/lib/activeadmin_mcp/action_definition.rb @@ -106,6 +106,7 @@ def validate! @errors << "#{tool_name}: unknown kind #{@kind}" unless KINDS.include?(@kind) reserved = reserved_param_name + form_keys = batch_form_keys params.each do |name, spec| unless spec.is_a?(Hash) @errors << "#{tool_name}: param #{name} must be a Hash" @@ -114,6 +115,15 @@ def validate! @errors << "#{tool_name}: param #{name} is reserved" if name == reserved + # ActiveAdmin's own batch_action controller method slices the submitted + # inputs down to the declared form: keys before calling the block, so a + # param declared only under mcp: would be advertised, validated, sent — + # and then silently dropped. Refuse it at declaration time instead. + if form_keys && !form_keys.include?(name) + @errors << "#{tool_name}: param #{name} is not in the batch action's form: hash, " \ + "so ActiveAdmin would drop it before the action runs" + end + type = spec[:type] @errors << "#{tool_name}: param #{name} has unknown type #{type}" if type && !TYPES.include?(type.to_sym) end @@ -121,6 +131,19 @@ def validate! @errors << "#{tool_name}: permission must be callable" if permission && !permission.respond_to?(:call) end + # The declared form: keys of a batch action, or nil when this is not a batch + # action or the batch action declares no form at all (in which case + # ActiveAdmin passes inputs through unsliced). + def batch_form_keys + return nil unless @kind == :batch + return nil unless @action.respond_to?(:inputs) + + form = @action.inputs + return nil unless form.is_a?(Hash) + + form.keys.map(&:to_sym) + end + def reserved_param_name case @kind when :member diff --git a/lib/activeadmin_mcp/active_admin_ext.rb b/lib/activeadmin_mcp/active_admin_ext.rb index 43c4ccb..65f9b1d 100644 --- a/lib/activeadmin_mcp/active_admin_ext.rb +++ b/lib/activeadmin_mcp/active_admin_ext.rb @@ -20,12 +20,22 @@ def mcp_options # have run, so this is called from ActiveAdmin.after_load rather than at # require time. def self.apply! - return false unless defined?(::ActiveAdmin::ControllerAction) - return false unless defined?(::ActiveAdmin::BatchAction) + unless applicable? + # Without these readers every opted-in action silently vanishes from + # tools/list, because ActionCatalog can no longer see an mcp: option + # anywhere. Say so rather than shipping a feature that is quietly off. + warn("[activeadmin_mcp] ActiveAdmin::ControllerAction / ActiveAdmin::BatchAction not found: " \ + "MCP action options cannot be read, so no opted-in actions will be exposed as tools.") + return false + end ::ActiveAdmin::ControllerAction.include(ActionOptions) ::ActiveAdmin::BatchAction.include(ActionOptions) true end + + def self.applicable? + defined?(::ActiveAdmin::ControllerAction) && defined?(::ActiveAdmin::BatchAction) ? true : false + end end end diff --git a/spec/activeadmin_mcp/action_definition_spec.rb b/spec/activeadmin_mcp/action_definition_spec.rb index a9e6d8e..ae9e9e1 100644 --- a/spec/activeadmin_mcp/action_definition_spec.rb +++ b/spec/activeadmin_mcp/action_definition_spec.rb @@ -133,6 +133,56 @@ def batch_action(name, mcp:, form: nil) expect(definition.errors).to include(match(/param ids is reserved/)) end + # ActiveAdmin's batch_action controller method slices submitted inputs down + # to the declared form: keys, so a param declared only under mcp: would be + # advertised, required, validated, encoded — and then dropped before the + # block ever saw it. + it "is invalid when a batch param is missing from the ActiveAdmin form hash" do + action = batch_action(:suspend, form: { reason: :text }, mcp: { + description: "Suspend", + params: { reason: { hint: "Why" }, notify_manager: { type: :boolean, required: true } } + }) + + definition = described_class.build(config: build_config, action: action, kind: :batch) + + expect(definition).not_to be_valid + expect(definition.errors).to include(match(/param notify_manager is not in the batch action's form/)) + end + + it "is valid when every batch param is in the form hash" do + action = batch_action(:suspend, form: { reason: :text, notify: :checkbox }, mcp: { + description: "Suspend", + params: { reason: { hint: "Why" } } + }) + + definition = described_class.build(config: build_config, action: action, kind: :batch) + + expect(definition).to be_valid + end + + # No form: hash means ActiveAdmin slices nothing, so there is nothing to drop. + it "allows declared params on a batch action with no form hash" do + action = batch_action(:suspend, form: nil, mcp: { + description: "Suspend", + params: { reason: { type: :string } } + }) + + definition = described_class.build(config: build_config, action: action, kind: :batch) + + expect(definition).to be_valid + end + + it "does not apply the batch form rule to member actions" do + action = member_action(:create_warning, mcp: { + description: "Record a warning", + params: { reason: { type: :string } } + }) + + definition = described_class.build(config: build_config, action: action, kind: :member) + + expect(definition).to be_valid + end + it "is valid when a collection action declares a param named id" do action = double("controller_action", name: :process, http_verb: :post, mcp_options: { description: "Process something", diff --git a/spec/activeadmin_mcp/active_admin_ext_spec.rb b/spec/activeadmin_mcp/active_admin_ext_spec.rb index 29a2b77..349067a 100644 --- a/spec/activeadmin_mcp/active_admin_ext_spec.rb +++ b/spec/activeadmin_mcp/active_admin_ext_spec.rb @@ -37,4 +37,18 @@ def member_action(name) expect(options).to include(method: :post) expect(options).to have_key(:mcp) end + + # If ActiveAdmin ever renames these classes, ActionCatalog's respond_to? + # guard makes every opted-in tool vanish from tools/list with nothing said + # anywhere. A warning is the only signal an operator would get. + describe "when ActiveAdmin does not provide the expected classes" do + it "warns and returns false rather than failing silently" do + allow(described_class).to receive(:applicable?).and_return(false) + messages = [] + allow(described_class).to receive(:warn) { |message| messages << message } + + expect(described_class.apply!).to be(false) + expect(messages.join).to include("no opted-in actions will be exposed") + end + end end From 4793ab6923648003794b51f4d2f5548f6236c3f7 Mon Sep 17 00:00:00 2001 From: Lloyd Watkin Date: Fri, 18 Sep 2026 21:32:14 +0100 Subject: [PATCH 17/24] Document per-user tool listing, batch authorization and the auth caveat The README claimed every call first passes the ActiveAdmin authorization adapter. That is now true for batch actions too, but the shape differs enough to state: batch authorization is resource-level plus an id-scope check through scope_collection, not a per-record policy evaluation. Also notes that neutralising the namespace's authentication_method disables its authorization half too when an application uses a combined authentication-and-authorization method, and documents the new batch form: declaration rule. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 20 ++++++++++++++++++++ README.md | 27 +++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d1fd7f0..6c18156 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 authorization and callbacks all apply, and an optional `permission:` proc can narrow access further. + `tools/list` is user-specific: an action is advertised only when the + authenticated MCP user passes the resource's authorization adapter, and a + zero-argument `permission:` proc is evaluated at listing time in controller + context (so `current_admin_user` and `can?` work there as they do at call + time). A param's `suggestions:` proc, which runs application code against the + database, is never evaluated for a user who is not authorized for the action. + + Batch action calls additionally run the submitted ids through the adapter's + `scope_collection` and refuse the entire call if any id falls outside it, + rather than silently acting on fewer records than the client asked for. + + A batch action param declared under `mcp:` but missing from the action's + ActiveAdmin `form:` hash is now a declaration error. ActiveAdmin slices + submitted inputs to the `form:` keys, so such a param was advertised, + required and validated, and then silently dropped before the block ran. + + Action failures return a generic error naming the resource and action; the + underlying exception message is written to the log instead of being sent to + the MCP client, where it could disclose SQL, table names or file paths. + ### Changed - **Breaking:** the minimum supported Ruby is now 4.0 and the minimum Rails is diff --git a/README.md b/README.md index 568d9e6..7eb1a6d 100644 --- a/README.md +++ b/README.md @@ -142,6 +142,11 @@ end never enforced, so use it for live values from the database. If it raises, the tool is still listed without suggestions. +On a batch action, every declared param must also appear in the action's `form:` +hash. ActiveAdmin slices submitted inputs down to the declared `form:` keys +before calling the block, so a param declared only under `mcp:` would be +advertised to the client and then dropped; the declaration is refused instead. + **Authorization** `permission:` is an *additional* gate, never a replacement. Every call first @@ -151,6 +156,28 @@ controller context, so `current_admin_user`, `can?` and the usual admin helpers are available. Return `false` to refuse, or a `String` to refuse with a reason the agent can act on. +Tools are also listed per user: an action whose resource the adapter refuses is +left out of `tools/list` entirely, and a `permission:` proc that takes no +arguments is evaluated at listing time (in the same controller context) so the +tool is hidden rather than offered and then refused. + +For **batch actions** the adapter check is necessarily resource-level — there is +no single record to authorize — so it is `authorized?(:, YourModel)` +rather than a per-record policy evaluation. To stop that being a hole, the ids +the client submits are run back through the adapter's `scope_collection`, and +the whole call is refused if any of them falls outside the scope. Nothing is +narrowed silently: the call either acts on every id you asked for or on none. + +One caveat on authentication. Dispatch neutralises the namespace's +`authentication_method` callback, because the MCP request has already +authenticated by bearer token and that callback would otherwise redirect to a +login page. If your `authentication_method` is a *combined* authentication-and- +authorization method — one that also, say, rejects non-superusers — then +neutralising it disables that authorization half too. Resource-level +authorization still runs through the adapter, but the "we only skip +authentication" framing is not universal; keep authorization in the adapter, +not in the authentication callback. + **What you get back** Actions are executed through your real ActiveAdmin controller, so the action's From 050ad416d7615ce95db7a0a0a1bd8e1616998c7f Mon Sep 17 00:00:00 2001 From: Lloyd Watkin Date: Sat, 19 Sep 2026 08:36:32 +0100 Subject: [PATCH 18/24] Fix form-less batch actions dropping every param, not none batch_form_keys conflated "not a batch action" and "batch action with no form: hash" under a single nil return, so validate! skipped its check entirely for form-less batch actions. But ActiveAdmin's batch_action controller does inputs.slice(*valid_keys), and with no form: valid_keys is nil, so slice(*nil) == slice() drops every input, not none. A required: true param declared only under mcp: would be silently discarded before the block ran. Restructure batch_form_keys into three distinct outcomes (not a batch action, form: Hash with real keys, no-form with an empty permitted set) plus an explicit skip for Proc forms that ActiveAdmin evaluates in controller context. Invert the test that pinned the wrong behaviour and add coverage for the no-form and Proc-form cases. Co-Authored-By: Claude Sonnet 5 --- lib/activeadmin_mcp/action_definition.rb | 49 ++++++++++++++----- .../activeadmin_mcp/action_definition_spec.rb | 25 ++++++++-- 2 files changed, 60 insertions(+), 14 deletions(-) diff --git a/lib/activeadmin_mcp/action_definition.rb b/lib/activeadmin_mcp/action_definition.rb index b8ac659..ff21434 100644 --- a/lib/activeadmin_mcp/action_definition.rb +++ b/lib/activeadmin_mcp/action_definition.rb @@ -116,12 +116,27 @@ def validate! @errors << "#{tool_name}: param #{name} is reserved" if name == reserved # ActiveAdmin's own batch_action controller method slices the submitted - # inputs down to the declared form: keys before calling the block, so a - # param declared only under mcp: would be advertised, validated, sent — - # and then silently dropped. Refuse it at declaration time instead. - if form_keys && !form_keys.include?(name) - @errors << "#{tool_name}: param #{name} is not in the batch action's form: hash, " \ - "so ActiveAdmin would drop it before the action runs" + # inputs down to the keys of the batch action's `inputs` (its `form:` + # hash) before calling the block: `inputs.slice(*valid_keys)`. When + # there is no `form:` at all, `inputs` is nil, so `valid_keys` is nil, + # and `slice(*nil)` is `slice()` — which drops EVERY input, not none. + # So a form-less batch action permits nothing, and any declared param + # is a declaration error. A Proc form is evaluated by ActiveAdmin in + # controller context (MethodOrProcHelper.render_in_context), so we + # cannot know its keys here and skip the check rather than guess. + case form_keys + when :unknown_proc_form + nil + when :no_form + @errors << "#{tool_name}: param #{name} cannot be declared because this batch action " \ + "has no form: hash, so ActiveAdmin drops every input before the action runs" + when nil + nil # not a batch action, or no `inputs` method at all: no rule applies + else + unless form_keys.include?(name) + @errors << "#{tool_name}: param #{name} is not among the batch action's declared " \ + "form: keys, so ActiveAdmin would drop it before the action runs" + end end type = spec[:type] @@ -131,17 +146,29 @@ def validate! @errors << "#{tool_name}: permission must be callable" if permission && !permission.respond_to?(:call) end - # The declared form: keys of a batch action, or nil when this is not a batch - # action or the batch action declares no form at all (in which case - # ActiveAdmin passes inputs through unsliced). + # The permitted param keys for a batch action's `inputs` (its `form:` + # hash), distinguishing three outcomes the caller must treat differently: + # + # * not a batch action, or the action has no `inputs` method at all -> + # nil, the batch form: rule does not apply. + # * `inputs` is a Hash -> its keys, the permitted set ActiveAdmin will + # slice submitted params down to. + # * `inputs` is nil (no `form:` declared) -> :no_form. ActiveAdmin still + # slices, against a nil key list, which yields an EMPTY permitted set + # (`hash.slice(*nil)` is `hash.slice()` == `{}`), so every declared + # param here is a declaration error. + # * `inputs` is a Proc -> :unknown_proc_form. ActiveAdmin evaluates it in + # controller context via `render_in_context`, so we cannot know its + # keys at declaration time. We skip the check rather than guess. def batch_form_keys return nil unless @kind == :batch return nil unless @action.respond_to?(:inputs) form = @action.inputs - return nil unless form.is_a?(Hash) + return form.keys.map(&:to_sym) if form.is_a?(Hash) + return :unknown_proc_form if form.is_a?(Proc) - form.keys.map(&:to_sym) + :no_form end def reserved_param_name diff --git a/spec/activeadmin_mcp/action_definition_spec.rb b/spec/activeadmin_mcp/action_definition_spec.rb index ae9e9e1..dfc3744 100644 --- a/spec/activeadmin_mcp/action_definition_spec.rb +++ b/spec/activeadmin_mcp/action_definition_spec.rb @@ -146,7 +146,7 @@ def batch_action(name, mcp:, form: nil) definition = described_class.build(config: build_config, action: action, kind: :batch) expect(definition).not_to be_valid - expect(definition.errors).to include(match(/param notify_manager is not in the batch action's form/)) + expect(definition.errors).to include(match(/param notify_manager is not among the batch action's declared form/)) end it "is valid when every batch param is in the form hash" do @@ -160,8 +160,11 @@ def batch_action(name, mcp:, form: nil) expect(definition).to be_valid end - # No form: hash means ActiveAdmin slices nothing, so there is nothing to drop. - it "allows declared params on a batch action with no form hash" do + # With no form: hash, ActiveAdmin's batch_action controller calls + # `inputs.slice(*nil.try(:keys))`, i.e. `slice()`, which drops every + # submitted input. So a form-less batch action has an empty permitted set, + # and declaring any param is a declaration error, not a free pass. + it "is invalid when a form-less batch action declares any param" do action = batch_action(:suspend, form: nil, mcp: { description: "Suspend", params: { reason: { type: :string } } @@ -169,6 +172,22 @@ def batch_action(name, mcp:, form: nil) definition = described_class.build(config: build_config, action: action, kind: :batch) + expect(definition).not_to be_valid + expect(definition.errors).to include(match(/param reason/)) + end + + # A Proc form is evaluated by ActiveAdmin in controller context via + # MethodOrProcHelper.render_in_context, so we cannot know its keys at + # declaration time. We skip the check rather than guess, so this must not + # be falsely rejected. + it "does not reject a batch action whose form is a Proc" do + action = batch_action(:suspend, form: -> { { reason: :text } }, mcp: { + description: "Suspend", + params: { reason: { type: :string } } + }) + + definition = described_class.build(config: build_config, action: action, kind: :batch) + expect(definition).to be_valid end From d4098bee93db9292aaba282b232718f6b6aeebf1 Mon Sep 17 00:00:00 2001 From: Lloyd Watkin Date: Sat, 19 Sep 2026 08:45:35 +0100 Subject: [PATCH 19/24] Ignore docs/ Design specs and implementation plans for work in progress are kept on disk but out of the repository. Co-Authored-By: Claude Opus 5 (1M context) --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 596f30e..d1685f3 100644 --- a/.gitignore +++ b/.gitignore @@ -3,6 +3,7 @@ /_yardoc/ /coverage/ /doc/ +/docs/ /pkg/ /spec/reports/ /tmp/ From 83c7992b00804bb007b2b1eb55aa30b6f5c56a75 Mon Sep 17 00:00:00 2001 From: Lloyd Watkin Date: Sat, 19 Sep 2026 09:19:16 +0100 Subject: [PATCH 20/24] Drop the frozen_string_literal magic comments from the action files Matches the convention adopted on main for the rest of the codebase. Co-Authored-By: Claude Opus 5 (1M context) --- lib/activeadmin_mcp/action_catalog.rb | 2 -- lib/activeadmin_mcp/action_definition.rb | 2 -- lib/activeadmin_mcp/action_params.rb | 2 -- lib/activeadmin_mcp/action_runner.rb | 2 -- lib/activeadmin_mcp/action_schema.rb | 2 -- lib/activeadmin_mcp/active_admin_ext.rb | 2 -- lib/activeadmin_mcp/controller_dispatcher.rb | 2 -- spec/activeadmin_mcp/action_catalog_spec.rb | 2 -- spec/activeadmin_mcp/action_definition_spec.rb | 2 -- spec/activeadmin_mcp/action_params_spec.rb | 2 -- spec/activeadmin_mcp/action_runner_spec.rb | 2 -- spec/activeadmin_mcp/action_schema_spec.rb | 2 -- spec/activeadmin_mcp/active_admin_ext_spec.rb | 2 -- spec/activeadmin_mcp/controller_dispatcher_spec.rb | 2 -- spec/support/active_admin.rb | 2 -- 15 files changed, 30 deletions(-) diff --git a/lib/activeadmin_mcp/action_catalog.rb b/lib/activeadmin_mcp/action_catalog.rb index 7fcabb4..55355f1 100644 --- a/lib/activeadmin_mcp/action_catalog.rb +++ b/lib/activeadmin_mcp/action_catalog.rb @@ -1,5 +1,3 @@ -# frozen_string_literal: true - module ActiveadminMcp # Finds every ActiveAdmin action an application has opted in to MCP. # diff --git a/lib/activeadmin_mcp/action_definition.rb b/lib/activeadmin_mcp/action_definition.rb index ff21434..21e1779 100644 --- a/lib/activeadmin_mcp/action_definition.rb +++ b/lib/activeadmin_mcp/action_definition.rb @@ -1,5 +1,3 @@ -# frozen_string_literal: true - module ActiveadminMcp # One ActiveAdmin action that an application has opted in to MCP, normalised # so the rest of the engine does not care whether it came from a diff --git a/lib/activeadmin_mcp/action_params.rb b/lib/activeadmin_mcp/action_params.rb index a1b82dc..8eba4bb 100644 --- a/lib/activeadmin_mcp/action_params.rb +++ b/lib/activeadmin_mcp/action_params.rb @@ -1,5 +1,3 @@ -# frozen_string_literal: true - module ActiveadminMcp # Validates and coerces a tool call's arguments against its ActionDefinition # before anything reaches the controller. diff --git a/lib/activeadmin_mcp/action_runner.rb b/lib/activeadmin_mcp/action_runner.rb index b7f697e..cbc7c83 100644 --- a/lib/activeadmin_mcp/action_runner.rb +++ b/lib/activeadmin_mcp/action_runner.rb @@ -1,5 +1,3 @@ -# frozen_string_literal: true - module ActiveadminMcp # Runs one opted-in ActiveAdmin action for an MCP client. # diff --git a/lib/activeadmin_mcp/action_schema.rb b/lib/activeadmin_mcp/action_schema.rb index 01fb553..352fabf 100644 --- a/lib/activeadmin_mcp/action_schema.rb +++ b/lib/activeadmin_mcp/action_schema.rb @@ -1,5 +1,3 @@ -# frozen_string_literal: true - module ActiveadminMcp # Builds the JSON Schema an MCP client sees for one opted-in action. # diff --git a/lib/activeadmin_mcp/active_admin_ext.rb b/lib/activeadmin_mcp/active_admin_ext.rb index 65f9b1d..ec21330 100644 --- a/lib/activeadmin_mcp/active_admin_ext.rb +++ b/lib/activeadmin_mcp/active_admin_ext.rb @@ -1,5 +1,3 @@ -# frozen_string_literal: true - module ActiveadminMcp # Every patch this gem applies to ActiveAdmin lives in this one file, so the # coupling has a single place to check when ActiveAdmin is upgraded. diff --git a/lib/activeadmin_mcp/controller_dispatcher.rb b/lib/activeadmin_mcp/controller_dispatcher.rb index 69dd01a..743a30e 100644 --- a/lib/activeadmin_mcp/controller_dispatcher.rb +++ b/lib/activeadmin_mcp/controller_dispatcher.rb @@ -1,5 +1,3 @@ -# frozen_string_literal: true - module ActiveadminMcp # Runs a real ActiveAdmin controller action out of band, so an MCP tool call # goes through the same before_action chain, authorization and callbacks as a diff --git a/spec/activeadmin_mcp/action_catalog_spec.rb b/spec/activeadmin_mcp/action_catalog_spec.rb index 9b8996a..d8511ff 100644 --- a/spec/activeadmin_mcp/action_catalog_spec.rb +++ b/spec/activeadmin_mcp/action_catalog_spec.rb @@ -1,5 +1,3 @@ -# frozen_string_literal: true - require "spec_helper" RSpec.describe ActiveadminMcp::ActionCatalog do diff --git a/spec/activeadmin_mcp/action_definition_spec.rb b/spec/activeadmin_mcp/action_definition_spec.rb index dfc3744..1c9ddef 100644 --- a/spec/activeadmin_mcp/action_definition_spec.rb +++ b/spec/activeadmin_mcp/action_definition_spec.rb @@ -1,5 +1,3 @@ -# frozen_string_literal: true - require "spec_helper" RSpec.describe ActiveadminMcp::ActionDefinition do diff --git a/spec/activeadmin_mcp/action_params_spec.rb b/spec/activeadmin_mcp/action_params_spec.rb index 3b1b1a3..1d828ba 100644 --- a/spec/activeadmin_mcp/action_params_spec.rb +++ b/spec/activeadmin_mcp/action_params_spec.rb @@ -1,5 +1,3 @@ -# frozen_string_literal: true - require "spec_helper" RSpec.describe ActiveadminMcp::ActionParams do diff --git a/spec/activeadmin_mcp/action_runner_spec.rb b/spec/activeadmin_mcp/action_runner_spec.rb index 7cd64f0..a7e0b87 100644 --- a/spec/activeadmin_mcp/action_runner_spec.rb +++ b/spec/activeadmin_mcp/action_runner_spec.rb @@ -1,5 +1,3 @@ -# frozen_string_literal: true - require "spec_helper" require "support/active_admin" diff --git a/spec/activeadmin_mcp/action_schema_spec.rb b/spec/activeadmin_mcp/action_schema_spec.rb index 0facc39..c33a5b2 100644 --- a/spec/activeadmin_mcp/action_schema_spec.rb +++ b/spec/activeadmin_mcp/action_schema_spec.rb @@ -1,5 +1,3 @@ -# frozen_string_literal: true - require "spec_helper" RSpec.describe ActiveadminMcp::ActionSchema do diff --git a/spec/activeadmin_mcp/active_admin_ext_spec.rb b/spec/activeadmin_mcp/active_admin_ext_spec.rb index 349067a..5ec10bd 100644 --- a/spec/activeadmin_mcp/active_admin_ext_spec.rb +++ b/spec/activeadmin_mcp/active_admin_ext_spec.rb @@ -1,5 +1,3 @@ -# frozen_string_literal: true - require "spec_helper" require "support/active_admin" diff --git a/spec/activeadmin_mcp/controller_dispatcher_spec.rb b/spec/activeadmin_mcp/controller_dispatcher_spec.rb index eb5d8ec..13ad460 100644 --- a/spec/activeadmin_mcp/controller_dispatcher_spec.rb +++ b/spec/activeadmin_mcp/controller_dispatcher_spec.rb @@ -1,5 +1,3 @@ -# frozen_string_literal: true - require "spec_helper" require "support/active_admin" diff --git a/spec/support/active_admin.rb b/spec/support/active_admin.rb index 931d86e..e6fb49e 100644 --- a/spec/support/active_admin.rb +++ b/spec/support/active_admin.rb @@ -1,5 +1,3 @@ -# frozen_string_literal: true - # Boots a minimal Rails + ActiveAdmin application so specs can exercise real # ActiveAdmin objects (resource configs, controllers, dispatch) rather than # doubles. Required only by specs that genuinely need it; the rest of the From 773731e24d6831517eb61ed45e57602369a94455 Mon Sep 17 00:00:00 2001 From: Lloyd Watkin Date: Sat, 19 Sep 2026 09:27:52 +0100 Subject: [PATCH 21/24] Add e2e coverage for the mcp: action opt-in, and fix CSRF on dispatch Covers member_action/collection_action/batch_action opt-in via mcp:, binding enum: enforcement, required param refusal, controller-context evaluation of a zero-argument permission: proc at tools/list time, and batch action scoping to selected records only. Along the way, found and fixed a real defect the new suite exposed: ControllerDispatcher's synthesized request carried no CSRF token, so Rails forgery protection refused every non-GET member action and every batch action. Unit specs never caught this because they mock around real ActiveAdmin/Rails dispatch. Co-Authored-By: Claude Sonnet 5 --- lib/activeadmin_mcp/controller_dispatcher.rb | 8 ++ spec/e2e/fixture_app/README.md | 31 ++++++-- spec/e2e/fixture_app/app/admin/posts.rb | 52 +++++++++++++ .../20260101000003_add_status_to_posts.rb | 5 ++ spec/e2e/fixture_app/db/seeds.rb | 9 ++- spec/e2e/mcp_actions_spec.rb | 77 +++++++++++++++++++ spec/e2e/mcp_server_spec.rb | 4 +- 7 files changed, 175 insertions(+), 11 deletions(-) create mode 100644 spec/e2e/fixture_app/db/migrate/20260101000003_add_status_to_posts.rb create mode 100644 spec/e2e/mcp_actions_spec.rb diff --git a/lib/activeadmin_mcp/controller_dispatcher.rb b/lib/activeadmin_mcp/controller_dispatcher.rb index 743a30e..d143c17 100644 --- a/lib/activeadmin_mcp/controller_dispatcher.rb +++ b/lib/activeadmin_mcp/controller_dispatcher.rb @@ -51,6 +51,14 @@ def controller_with_mcp_user auth_method = @config.namespace.authentication_method controller.define_singleton_method(auth_method) { true } if auth_method + # The synthesized request carries no session-bound CSRF token, so + # Rails' own forgery protection would refuse every non-GET action + # (member actions declared `method: :post`, and every batch action, + # which always dispatches as one). The MCP request has already + # authenticated by bearer token; this does NOT skip authorization, + # which still runs in full. + controller.define_singleton_method(:verified_request?) { true } + controller end diff --git a/spec/e2e/fixture_app/README.md b/spec/e2e/fixture_app/README.md index 6e9f87b..e529035 100644 --- a/spec/e2e/fixture_app/README.md +++ b/spec/e2e/fixture_app/README.md @@ -12,19 +12,38 @@ path there. Each one exists to give a claim in the README something to bite on: - `app/admin/posts.rb` permits `title` and `body` but **not** `slug`, so the - suite can prove an unpermitted attribute is dropped rather than written. + suite can prove an unpermitted attribute is dropped rather than written. It + also carries the MCP action fixtures: + - `member_action :publish` is opted in via `mcp:` with a required + `visibility` param bound to a static `enum:`, so the suite can prove a + value outside the enum is refused before dispatch and a permitted value + runs against the real controller. + - `member_action :archive` has no `mcp:` key at all, so the suite can prove + the opt-in guarantee: an action that exists in the admin UI is not + automatically exposed as a tool. + - `batch_action :set_status` is opted in via `mcp:` with only a + description — its param type is inherited from `form:` — so the suite can + prove a batch action applies to exactly the selected records and leaves + the rest untouched. + - `collection_action :purge_drafts` is opted in via `mcp:` with a + zero-argument `permission:` proc that calls `current_admin_user`, so the + suite can prove the proc is evaluated in controller context at + `tools/list` time rather than raising `NameError` and silently hiding the + tool. - `app/admin/authors.rb` registers `actions :index, :show`, so the suite can prove `update` refuses a resource the admin UI would not let you edit. - `app/models/*.rb` allowlist `ransackable_attributes`, which Ransack 4 requires before it will filter on an attribute at all. -- `db/migrate/*.rb` create the `authors` and `posts` tables. They are checked +- `db/migrate/*.rb` create the `authors` and `posts` tables, and add the + `status` column `posts` needs for the MCP action fixtures. They are checked in with fixed version numbers rather than produced by `rails generate model`, so the schema under test is visible and does not change from run to run. - `db/seeds.rb` is restorative: it resets existing rows rather than only - creating missing ones, because the suite's `update` examples mutate a post - and the generated application is cached between runs. It reads the admin - credentials from the environment so that they are defined in exactly one - place, `AppBuilder`. + creating missing ones, because the suite's `update` examples mutate a post, + the MCP action examples mutate a post's `status` (directly and via a batch + action), and the generated application is cached between runs. It reads the + admin credentials from the environment so that they are defined in exactly + one place, `AppBuilder`. Two files here are not copied into the application: diff --git a/spec/e2e/fixture_app/app/admin/posts.rb b/spec/e2e/fixture_app/app/admin/posts.rb index cd6c28b..bd01f4d 100644 --- a/spec/e2e/fixture_app/app/admin/posts.rb +++ b/spec/e2e/fixture_app/app/admin/posts.rb @@ -2,4 +2,56 @@ # the MCP `update` tool drops attributes the admin form does not accept. ActiveAdmin.register Post do permit_params :title, :body + + # Opted in via `mcp:`, with a required `visibility` param bound to a static + # enum, so the e2e suite can prove an out-of-enum value is refused before + # dispatch, and that a permitted value actually runs against the real + # controller. + member_action :publish, method: :post, mcp: { + description: "Publish a post with the given visibility", + params: { + visibility: { + type: :string, + required: true, + enum: %w[public unlisted], + hint: "Who can see the post once published", + }, + }, + } do + resource.update!(status: params[:visibility]) + redirect_to resource_path(resource), notice: "Published" + end + + # No `mcp:` key at all, so the e2e suite can prove the opt-in guarantee: an + # action that exists in the admin UI is not automatically exposed as a tool. + member_action :archive, method: :post do + resource.update!(status: "archived") + redirect_to resource_path(resource), notice: "Archived" + end + + # Opted in via `mcp:` with only a description: its param type is inherited + # from `form:`, so the e2e suite can prove a batch action applies to exactly + # the selected records and leaves the rest untouched. + batch_action :set_status, form: { status: :text }, mcp: { + description: "Set the status on the selected posts", + } do |ids, inputs| + Post.where(id: ids).update_all(status: inputs["status"]) + redirect_to collection_path, notice: "Status updated" + end + + # Opted in via `mcp:` with a ZERO-ARGUMENT `permission:` proc that calls + # `current_admin_user`, so the e2e suite can prove the proc is evaluated in + # controller context at tools/list time. `current_admin_user` is only + # defined on the controller: if listing-time evaluation ever regressed to a + # bare `proc.call`, this would raise NameError, the tool would be hidden by + # the rescue, and the example asserting it IS listed would fail. + # E2E_ADMIN_EMAIL is only set for the process that seeds the database, not + # for the running server, so the seeded admin's email is fixed here rather + # than read from the environment: it has to match AppBuilder::ADMIN_EMAIL. + collection_action :purge_drafts, method: :post, mcp: { + description: "Purge draft posts, restricted to the seeded admin", + permission: -> { current_admin_user&.email == "admin@example.com" }, + } do + redirect_to collection_path, notice: "Purged" + end end diff --git a/spec/e2e/fixture_app/db/migrate/20260101000003_add_status_to_posts.rb b/spec/e2e/fixture_app/db/migrate/20260101000003_add_status_to_posts.rb new file mode 100644 index 0000000..cb5235d --- /dev/null +++ b/spec/e2e/fixture_app/db/migrate/20260101000003_add_status_to_posts.rb @@ -0,0 +1,5 @@ +class AddStatusToPosts < ActiveRecord::Migration[7.2] + def change + add_column :posts, :status, :string, default: "draft", null: false + end +end diff --git a/spec/e2e/fixture_app/db/seeds.rb b/spec/e2e/fixture_app/db/seeds.rb index 29bf6e9..b5620f1 100644 --- a/spec/e2e/fixture_app/db/seeds.rb +++ b/spec/e2e/fixture_app/db/seeds.rb @@ -1,7 +1,9 @@ # Restorative by design: the e2e suite's `update` examples rewrite a post's -# title, and the generated application is cached between runs, so seeding has -# to reset existing rows rather than only create missing ones. Records are -# keyed on stable natural keys (email, slug) that no example mutates. +# title, and the MCP action examples rewrite a post's status (directly and via +# a batch action), and the generated application is cached between runs, so +# seeding has to reset existing rows rather than only create missing ones. +# Records are keyed on stable natural keys (email, slug) that no example +# mutates. admin_email = ENV.fetch("E2E_ADMIN_EMAIL") admin_password = ENV.fetch("E2E_ADMIN_PASSWORD") @@ -30,6 +32,7 @@ Post.find_or_initialize_by(slug: slug).tap do |post| post.title = title post.body = body + post.status = "draft" post.save! end end diff --git a/spec/e2e/mcp_actions_spec.rb b/spec/e2e/mcp_actions_spec.rb new file mode 100644 index 0000000..772504b --- /dev/null +++ b/spec/e2e/mcp_actions_spec.rb @@ -0,0 +1,77 @@ +RSpec.describe "MCP actions declared with an mcp: option" do + let(:client) { E2E::McpClient.new(url: E2E::AppServer.instance.mcp_url, token: E2E.token) } + + def post_status(slug) + client.call_tool("query", resource: "Post", q: { slug_eq: slug })["records"].first["status"] + end + + def post_id(slug) + client.call_tool("query", resource: "Post", q: { slug_eq: slug })["records"].first["id"] + end + + describe "tools/list" do + it "advertises a member_action opted in with an mcp: key as its own tool, with its declared params in the input schema" do + tools = client.tools_list["tools"] + publish = tools.find { |tool| tool["name"] == "post_publish" } + + expect(publish).not_to be_nil + expect(publish["inputSchema"]["properties"]).to include("visibility") + expect(publish["inputSchema"]["properties"]["visibility"]["enum"]).to eq(%w[public unlisted]) + expect(publish["inputSchema"]["required"]).to include("visibility") + end + + it "does not advertise a member_action registered without an mcp: key, proving MCP exposure is opt-in" do + tools = client.tools_list["tools"] + + expect(tools.map { |tool| tool["name"] }).not_to include("post_archive") + end + + it "advertises a collection_action whose zero-argument permission: proc calls current_admin_user, proving the proc is evaluated in controller context at listing time rather than raised as a bare NameError that would hide the tool" do + tools = client.tools_list["tools"] + + expect(tools.map { |tool| tool["name"] }).to include("post_purge_drafts") + end + end + + describe "calling an opted-in member action" do + it "runs the action against the real controller, so the change is visible on a subsequent query" do + id = post_id("small-gods") + + result = client.call_tool("post_publish", id: id, visibility: "public") + + expect(result["error"]).to be_nil + expect(post_status("small-gods")).to eq("public") + end + + it "refuses a value outside a static enum: before dispatch, and leaves the record's status unchanged" do + id = post_id("small-gods") + + result = client.call_tool("post_publish", id: id, visibility: "top-secret") + + expect(result["error"]).to match(/visibility/) + expect(post_status("small-gods")).to eq("draft") + end + + it "refuses a call omitting a required param before dispatch, and leaves the record's status unchanged" do + id = post_id("small-gods") + + result = client.call_tool("post_publish", id: id) + + expect(result["error"]).to match(/visibility/) + expect(post_status("small-gods")).to eq("draft") + end + end + + describe "calling an opted-in batch action" do + it "applies to exactly the selected records, leaving an unselected record untouched" do + selected_id = post_id("a-wizard-of-earthsea") + other_id = post_id("the-tombs-of-atuan") + + result = client.call_tool("post_set_status", ids: [selected_id], status: "archived") + + expect(result["error"]).to be_nil + expect(post_status("a-wizard-of-earthsea")).to eq("archived") + expect(post_status("the-tombs-of-atuan")).to eq("draft") + end + end +end diff --git a/spec/e2e/mcp_server_spec.rb b/spec/e2e/mcp_server_spec.rb index 18377ec..9798e70 100644 --- a/spec/e2e/mcp_server_spec.rb +++ b/spec/e2e/mcp_server_spec.rb @@ -12,10 +12,10 @@ expect(result["capabilities"]).to have_key("tools") end - it "advertises the three tools" do + it "advertises the three built-in tools, alongside whatever the fixture app has opted in to MCP" do names = client.tools_list["tools"].map { |tool| tool["name"] } - expect(names).to contain_exactly("list_resources", "query", "update") + expect(names).to include("list_resources", "query", "update") end end From 45ce5f2fb9042d5f29e92c48de6e0613047e52a1 Mon Sep 17 00:00:00 2001 From: Lloyd Watkin Date: Sat, 19 Sep 2026 09:30:05 +0100 Subject: [PATCH 22/24] Prove the opt-in guarantee on the call side, and write down what e2e taught us MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The listing example showed an action without an `mcp:` key is not advertised. Hidden and not-callable are different claims, though, and a tool that were merely hidden while still dispatchable by name would be a hole rather than an untidiness — so assert the refusal too. Breaking `ActionDefinition.build` to ignore the opt-in shows what the guarantee holds back: ActiveAdmin's default `destroy` batch actions appear as `post_destroy`, `author_destroy` and `admin_user_destroy`. CLAUDE.md gains the Ruby 4.0 invocation (there is no .ruby-version, and the failure when you miss it does not name the cause), and the lessons this work paid for: assert the side effect rather than the response, assert an untouched control record when an example is about which records were affected, prefer `include` over exact tool lists, and remember the seed environment is not the server's environment. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 46 ++++++++++++++++++++++++++++++++++++ spec/e2e/mcp_actions_spec.rb | 12 ++++++++++ 2 files changed, 58 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index 41b8b34..cbd7761 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -10,6 +10,16 @@ over HTTP and is mounted at `/mcp` by default. The tools it offers — `list_resources`, `query`, `update` — are defined in `lib/activeadmin_mcp/request_handler.rb`. +## Ruby version + +This gem requires Ruby 4.0 and CI runs 4.0.7. There is no `.ruby-version`, so +if your shell defaults to an older Ruby every `bundle` command fails with a +resolution error that does not mention the Ruby version as the cause. Prefix +commands with the version rather than debugging the symptom: + + RBENV_VERSION=4.0.7 bundle exec rake spec + RBENV_VERSION=4.0.7 bundle exec rake e2e + ## Testing MCP actions **Every MCP action must be covered by an end-to-end test, not only by unit @@ -54,6 +64,42 @@ a snapshot taken after migrating and seeding. So examples must not depend on what another one left behind, and the suite runs in random order to keep that honest. Write each one as though it runs alone, because it might. +### Writing an e2e example + +`E2E::McpClient` speaks the protocol: `tools_list` returns the `tools/list` +result, and `call_tool(name, arguments)` unwraps both layers of a tool result +— the JSON-RPC envelope and the pretty-printed JSON inside the text content +block — and hands back the payload. A tool that refuses returns a hash with an +`"error"` key rather than raising, so assert on that key. + +**Assert the side effect, not only the response.** An example that checks a +refusal came back has not shown the action was prevented: the same assertion +passes whether the call was refused before dispatch or ran and then reported +an error. Read the record back with `query` and assert it is unchanged. The +same applies in reverse for a successful call — assert the change landed, not +merely that no error came back. + +**When an example is about which records were affected, assert an untouched +control record.** A batch example that only checks the selected rows changed +cannot tell "acted on the ones I asked for" from "acted on everything". Seed +or pick a record that must not change, and assert it did not. + +**Prefer `include` to exact lists when asserting on the tool listing.** The +fixture application opts actions in to MCP, so the listing legitimately grows +when someone adds one; `contain_exactly` there turns an unrelated addition +into a failure in a file that has nothing to do with it. + +**Environment variables set for seeding are not set for the running server.** +The seeds read credentials from the environment, but the application boots as +a separate process without them. A fixture that needs the seeded admin's +identity at request time has to hard-code it to match `AppBuilder`, not read +`ENV`. + +Things this suite has caught that the unit specs structurally could not: that +Rails' forgery protection refuses the synthesized request every non-GET action +depends on, and that a `permission:` proc evaluated outside controller context +raises `NameError` and silently hides a tool. Both looked fine against mocks. + ### Write e2e descriptions out in full Give e2e examples and their enclosing blocks descriptions verbose enough that diff --git a/spec/e2e/mcp_actions_spec.rb b/spec/e2e/mcp_actions_spec.rb index 772504b..8461558 100644 --- a/spec/e2e/mcp_actions_spec.rb +++ b/spec/e2e/mcp_actions_spec.rb @@ -62,6 +62,18 @@ def post_id(slug) end end + describe "calling an action that was never opted in" do + # Absence from tools/list is not by itself a guarantee: a tool that were + # merely hidden but still dispatchable by name would be a hole, not a + # nicety. This asserts the call side of the opt-in promise. + it "refuses a member_action registered without an mcp: key as an unknown tool, and leaves the record untouched" do + result = client.call_tool("post_archive", id: post_id("small-gods")) + + expect(result["error"]).to eq("Unknown tool: post_archive") + expect(post_status("small-gods")).to eq("draft") + end + end + describe "calling an opted-in batch action" do it "applies to exactly the selected records, leaving an unselected record untouched" do selected_id = post_id("a-wizard-of-earthsea") From 9dd330899b04d3a5e57317554570c432c460471c Mon Sep 17 00:00:00 2001 From: Lloyd Watkin Date: Sat, 19 Sep 2026 09:39:03 +0100 Subject: [PATCH 23/24] Cover the raising and refusing action paths, and group the e2e suite by action type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fixture actions and the examples they exist for: `member_action :explode` raises, proving a failing action comes back as a generic error naming the resource and action while the exception's own message — which can carry SQL, table names and file paths — stays out of the MCP client's reach. `member_action :feature` carries a `permission:` proc taking the record that returns a refusal string for a draft post. That covers three things nothing did before: a record-aware proc keeps its tool advertised, because it cannot be resolved at listing time; the refusal reaches the client in the proc's own wording; and the proc is consulted per record rather than refusing outright, which the accompanying allow case is there to pin. The examples are now grouped by action type, so `--format documentation` reads as a specification of what each kind of ActiveAdmin action does over MCP. Grouping surfaced a gap: nothing asserted a batch action's param types come from its ActiveAdmin `form:` hash rather than its `mcp:` declaration, so that example is added too. Each new example was verified able to fail — by leaking `e.message` from the dispatcher, by ignoring a string returned from a permission proc, and by not inheriting `form:` types. Co-Authored-By: Claude Opus 5 (1M context) --- spec/e2e/fixture_app/README.md | 9 ++ spec/e2e/fixture_app/app/admin/posts.rb | 26 ++++ spec/e2e/mcp_actions_spec.rb | 154 ++++++++++++++++-------- 3 files changed, 138 insertions(+), 51 deletions(-) diff --git a/spec/e2e/fixture_app/README.md b/spec/e2e/fixture_app/README.md index e529035..8816f93 100644 --- a/spec/e2e/fixture_app/README.md +++ b/spec/e2e/fixture_app/README.md @@ -25,6 +25,15 @@ Each one exists to give a claim in the README something to bite on: description — its param type is inherited from `form:` — so the suite can prove a batch action applies to exactly the selected records and leaves the rest untouched. + - `member_action :explode` raises from its body, so the suite can prove a + failing action comes back as a generic error naming the resource and + action, with the exception's own message — which can carry SQL, table + names and file paths — kept away from the MCP client. + - `member_action :feature` carries a `permission:` proc that takes the + record and returns a refusal *string* for a draft post, so the suite can + prove a record-aware proc keeps its tool advertised (it cannot be resolved + at listing time), refuses at call time with the proc's own wording, and + allows the call once the record satisfies it. - `collection_action :purge_drafts` is opted in via `mcp:` with a zero-argument `permission:` proc that calls `current_admin_user`, so the suite can prove the proc is evaluated in controller context at diff --git a/spec/e2e/fixture_app/app/admin/posts.rb b/spec/e2e/fixture_app/app/admin/posts.rb index bd01f4d..ae47914 100644 --- a/spec/e2e/fixture_app/app/admin/posts.rb +++ b/spec/e2e/fixture_app/app/admin/posts.rb @@ -39,6 +39,32 @@ redirect_to collection_path, notice: "Status updated" end + # The body raises deliberately, so the e2e suite can prove an action that + # blows up comes back as a generic error naming the resource and action, + # with the exception's own message kept away from the MCP client — it can + # carry SQL, table names and file paths. + member_action :explode, method: :post, mcp: { + description: "Always raises, so the error path has something to bite on", + } do + raise ActiveRecord::StatementInvalid, "SQLite3::SQLException: no such table: classified_dossier" + end + + # Opted in with a `permission:` proc that takes the record. A proc needing a + # record cannot be resolved at tools/list time, so the tool stays advertised + # and the proc runs at call time instead. It returns a String for a draft + # post, which the client should see as the refusal reason, and true once the + # post has been published — so the suite can prove the proc is consulted + # per record rather than simply always refusing. + member_action :feature, method: :post, mcp: { + description: "Feature a published post on the front page", + permission: lambda { |post| + post.status == "draft" ? "Only a published post can be featured" : true + }, + } do + resource.update!(status: "featured") + redirect_to resource_path(resource), notice: "Featured" + end + # Opted in via `mcp:` with a ZERO-ARGUMENT `permission:` proc that calls # `current_admin_user`, so the e2e suite can prove the proc is evaluated in # controller context at tools/list time. `current_admin_user` is only diff --git a/spec/e2e/mcp_actions_spec.rb b/spec/e2e/mcp_actions_spec.rb index 8461558..2a9ceec 100644 --- a/spec/e2e/mcp_actions_spec.rb +++ b/spec/e2e/mcp_actions_spec.rb @@ -1,6 +1,14 @@ RSpec.describe "MCP actions declared with an mcp: option" do let(:client) { E2E::McpClient.new(url: E2E::AppServer.instance.mcp_url, token: E2E.token) } + def tool_names + client.tools_list["tools"].map { |tool| tool["name"] } + end + + def tool(name) + client.tools_list["tools"].find { |candidate| candidate["name"] == name } + end + def post_status(slug) client.call_tool("query", resource: "Post", q: { slug_eq: slug })["records"].first["status"] end @@ -9,81 +17,125 @@ def post_id(slug) client.call_tool("query", resource: "Post", q: { slug_eq: slug })["records"].first["id"] end - describe "tools/list" do - it "advertises a member_action opted in with an mcp: key as its own tool, with its declared params in the input schema" do - tools = client.tools_list["tools"] - publish = tools.find { |tool| tool["name"] == "post_publish" } + describe "a member_action" do + describe "in tools/list" do + it "is advertised as its own tool, with its declared params in the input schema" do + publish = tool("post_publish") + + expect(publish).not_to be_nil + expect(publish["inputSchema"]["properties"]).to include("visibility") + expect(publish["inputSchema"]["properties"]["visibility"]["enum"]).to eq(%w[public unlisted]) + expect(publish["inputSchema"]["required"]).to include("visibility") + end - expect(publish).not_to be_nil - expect(publish["inputSchema"]["properties"]).to include("visibility") - expect(publish["inputSchema"]["properties"]["visibility"]["enum"]).to eq(%w[public unlisted]) - expect(publish["inputSchema"]["required"]).to include("visibility") + it "is not advertised when it carries no mcp: key, proving MCP exposure is opt-in" do + expect(tool_names).not_to include("post_archive") + end end - it "does not advertise a member_action registered without an mcp: key, proving MCP exposure is opt-in" do - tools = client.tools_list["tools"] + describe "when called" do + it "runs against the real ActiveAdmin controller, so the change is visible on a subsequent query" do + id = post_id("small-gods") - expect(tools.map { |tool| tool["name"] }).not_to include("post_archive") - end + result = client.call_tool("post_publish", id: id, visibility: "public") - it "advertises a collection_action whose zero-argument permission: proc calls current_admin_user, proving the proc is evaluated in controller context at listing time rather than raised as a bare NameError that would hide the tool" do - tools = client.tools_list["tools"] + expect(result["error"]).to be_nil + expect(post_status("small-gods")).to eq("public") + end - expect(tools.map { |tool| tool["name"] }).to include("post_purge_drafts") - end - end + it "refuses a value outside a param's static enum: before dispatch, and leaves the record's status unchanged" do + id = post_id("small-gods") - describe "calling an opted-in member action" do - it "runs the action against the real controller, so the change is visible on a subsequent query" do - id = post_id("small-gods") + result = client.call_tool("post_publish", id: id, visibility: "top-secret") - result = client.call_tool("post_publish", id: id, visibility: "public") + expect(result["error"]).to match(/visibility/) + expect(post_status("small-gods")).to eq("draft") + end - expect(result["error"]).to be_nil - expect(post_status("small-gods")).to eq("public") - end + it "refuses a call omitting a required param before dispatch, and leaves the record's status unchanged" do + id = post_id("small-gods") + + result = client.call_tool("post_publish", id: id) - it "refuses a value outside a static enum: before dispatch, and leaves the record's status unchanged" do - id = post_id("small-gods") + expect(result["error"]).to match(/visibility/) + expect(post_status("small-gods")).to eq("draft") + end - result = client.call_tool("post_publish", id: id, visibility: "top-secret") + # Absence from tools/list is not by itself a guarantee: a tool that were + # merely hidden but still dispatchable by name would be a hole, not an + # untidiness. This asserts the call side of the opt-in promise. + it "is refused as an unknown tool when it carries no mcp: key, and leaves the record untouched" do + result = client.call_tool("post_archive", id: post_id("small-gods")) - expect(result["error"]).to match(/visibility/) - expect(post_status("small-gods")).to eq("draft") + expect(result["error"]).to eq("Unknown tool: post_archive") + expect(post_status("small-gods")).to eq("draft") + end + + it "reports a generic failure naming the resource and action when its body raises, keeping the exception's own message away from the client where it could disclose SQL, table names or file paths" do + result = client.call_tool("post_explode", id: post_id("small-gods")) + + expect(result["error"]).to eq("Post#explode failed") + expect(result["error"]).not_to include("classified_dossier") + expect(result["error"]).not_to match(/SQLite3|no such table|StatementInvalid/) + end end - it "refuses a call omitting a required param before dispatch, and leaves the record's status unchanged" do - id = post_id("small-gods") + describe "guarded by a permission: proc that takes the record" do + it "is still advertised, because a proc needing a record cannot be resolved at tools/list time and must refuse at call time instead" do + expect(tool_names).to include("post_feature") + end + + it "refuses the call with the string the proc returned as the reason, and leaves the record unchanged" do + result = client.call_tool("post_feature", id: post_id("small-gods")) + + expect(result["error"]).to eq("Only a published post can be featured") + expect(post_status("small-gods")).to eq("draft") + end + + # Without this, a proc hardcoded to refuse everything would satisfy the + # example above: the refusal has to be shown to depend on the record. + it "allows the call once the record satisfies the proc, proving the proc is consulted per record rather than refusing unconditionally" do + id = post_id("small-gods") + client.call_tool("post_publish", id: id, visibility: "public") - result = client.call_tool("post_publish", id: id) + result = client.call_tool("post_feature", id: id) - expect(result["error"]).to match(/visibility/) - expect(post_status("small-gods")).to eq("draft") + expect(result["error"]).to be_nil + expect(post_status("small-gods")).to eq("featured") + end end end - describe "calling an action that was never opted in" do - # Absence from tools/list is not by itself a guarantee: a tool that were - # merely hidden but still dispatchable by name would be a hole, not a - # nicety. This asserts the call side of the opt-in promise. - it "refuses a member_action registered without an mcp: key as an unknown tool, and leaves the record untouched" do - result = client.call_tool("post_archive", id: post_id("small-gods")) - - expect(result["error"]).to eq("Unknown tool: post_archive") - expect(post_status("small-gods")).to eq("draft") + describe "a collection_action" do + describe "in tools/list" do + it "is advertised when its zero-argument permission: proc calls current_admin_user, proving the proc is evaluated in controller context at listing time rather than raising NameError and silently hiding the tool" do + expect(tool_names).to include("post_purge_drafts") + end end end - describe "calling an opted-in batch action" do - it "applies to exactly the selected records, leaving an unselected record untouched" do - selected_id = post_id("a-wizard-of-earthsea") - other_id = post_id("the-tombs-of-atuan") + describe "a batch_action" do + describe "in tools/list" do + it "is advertised with an ids array, and with its param types taken from the action's ActiveAdmin form: hash rather than from the mcp: declaration" do + set_status = tool("post_set_status") + + expect(set_status).not_to be_nil + expect(set_status["inputSchema"]["properties"]["ids"]["type"]).to eq("array") + expect(set_status["inputSchema"]["required"]).to include("ids") + expect(set_status["inputSchema"]["properties"]["status"]["type"]).to eq("string") + end + end + + describe "when called" do + it "applies to exactly the selected records, leaving an unselected record untouched" do + selected_id = post_id("a-wizard-of-earthsea") - result = client.call_tool("post_set_status", ids: [selected_id], status: "archived") + result = client.call_tool("post_set_status", ids: [selected_id], status: "archived") - expect(result["error"]).to be_nil - expect(post_status("a-wizard-of-earthsea")).to eq("archived") - expect(post_status("the-tombs-of-atuan")).to eq("draft") + expect(result["error"]).to be_nil + expect(post_status("a-wizard-of-earthsea")).to eq("archived") + expect(post_status("the-tombs-of-atuan")).to eq("draft") + end end end end From 4e23ee19ab683f4d7a665e69ec084c1b014fa96c Mon Sep 17 00:00:00 2001 From: Lloyd Watkin Date: Sat, 19 Sep 2026 09:43:42 +0100 Subject: [PATCH 24/24] Fold the e2e comments into the descriptions they were explaining CLAUDE.md asks for a long description over a comment explaining a short one. The comment on the unknown-tool example carried the reason that example exists, which belongs in the output someone reads when it fails; the one on the permission allow case only repeated what its description already said. Co-Authored-By: Claude Opus 5 (1M context) --- spec/e2e/mcp_actions_spec.rb | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/spec/e2e/mcp_actions_spec.rb b/spec/e2e/mcp_actions_spec.rb index 2a9ceec..7c57389 100644 --- a/spec/e2e/mcp_actions_spec.rb +++ b/spec/e2e/mcp_actions_spec.rb @@ -61,10 +61,7 @@ def post_id(slug) expect(post_status("small-gods")).to eq("draft") end - # Absence from tools/list is not by itself a guarantee: a tool that were - # merely hidden but still dispatchable by name would be a hole, not an - # untidiness. This asserts the call side of the opt-in promise. - it "is refused as an unknown tool when it carries no mcp: key, and leaves the record untouched" do + it "is refused as an unknown tool when it carries no mcp: key, and leaves the record untouched, since being absent from tools/list would not on its own stop it being dispatched by name" do result = client.call_tool("post_archive", id: post_id("small-gods")) expect(result["error"]).to eq("Unknown tool: post_archive") @@ -92,9 +89,7 @@ def post_id(slug) expect(post_status("small-gods")).to eq("draft") end - # Without this, a proc hardcoded to refuse everything would satisfy the - # example above: the refusal has to be shown to depend on the record. - it "allows the call once the record satisfies the proc, proving the proc is consulted per record rather than refusing unconditionally" do + it "allows the call once the record satisfies the proc, proving the proc is consulted per record rather than refusing unconditionally as a proc hardcoded to refuse would also do" do id = post_id("small-gods") client.call_tool("post_publish", id: id, visibility: "public")