diff --git a/CHANGELOG.md b/CHANGELOG.md index 01f3634..b19affd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,25 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- An `mcp_action` DSL, for exposing actions declared somewhere an `mcp:` key + cannot be added — a shared concern, or another gem. The resource annotates + the action by name from its own registration, so resources sharing an action + can describe it differently, and whichever does not annotate exposes nothing. + Annotations are resolved when the tool list is built rather than when they + are declared, so they may appear either side of the `include`. + + Alongside `mcp:`'s own options it takes `kind:` (needed only to disambiguate + a name belonging to more than one action, which is refused rather than + guessed), `tool_name:`, and `http_verb:` (which verb to dispatch for an + action declared with several, such as `method: [:post, :delete]`). An action + may carry more than one annotation, each producing its own tool. An + annotation replaces an inline `mcp:` declaration wholesale rather than + merging into it. + + A batch action declared with a String title — the ones applications generate + in loops from data — may be annotated by that title, rather than by the + symbol ActiveAdmin derives from it, which can carry punctuation. + - A `describe_form` tool, which describes the fields behind a resource's create or update form so a client need not guess them from column names. It reads the resource's own `form do ... end` block when it declares one — reporting @@ -71,6 +90,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- A batch action's own ActiveAdmin `:if` proc is now honoured: one the admin UI + hides because `:if` refuses is neither listed nor runnable over MCP. This is + stricter than ActiveAdmin, which consults `:if` only when rendering and will + dispatch such an action regardless — deliberately so, since MCP should not be + the way round a gate the admin enforces by not offering the button. A proc + that raises, typically because it reads request state a listing cannot + supply, hides the tool and says so in the log. + +- A batch action whose `form:` is a proc rather than a hash now contributes its + param types, by evaluating the proc in controller context exactly as + ActiveAdmin does. Previously proc forms were skipped and inherited nothing. + Like `suggestions:`, this runs application code, and is never evaluated for a + user the resource's authorization adapter refuses. + +- Two actions that would be exposed under the same tool name are now both + hidden, with a declaration error naming the clash, rather than one silently + shadowing the other. A tool name carries no kind, so a `member_action` and a + `batch_action` of the same name derived the same one, and only the member one + was ever reachable. Give all but one an explicit `tool_name:`. + - **Behaviour change:** `update` now dispatches the resource's real ActiveAdmin `update` action instead of calling `record.update` directly. Everything the admin UI runs on a save now runs on an MCP update too: the controller's diff --git a/README.md b/README.md index 2908305..5f4f26d 100644 --- a/README.md +++ b/README.md @@ -297,6 +297,83 @@ 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. +### Actions declared somewhere you can't add `mcp:` + +An action declared by a shared concern, or by another gem, has no declaration +you can hang an `mcp:` key on — and if it did, every resource including it +would get the same description, params and `permission:` proc. Annotate it by +name from the registration instead, with `mcp_action`: + +```ruby +module Flaggable + def self.included(dsl) + dsl.send(:member_action, :flag, method: [:post, :delete]) { ... } + dsl.send(:batch_action, :flag, form: proc { { reason: :text } }) { |ids, inputs| ... } + end +end + +ActiveAdmin.register Volunteer do + include Flaggable + + mcp_action :flag, kind: :batch, tool_name: "volunteer_bulk_flag", + description: "Flag the selected volunteers", + params: { reason: { type: :string, required: true } } + + mcp_action :flag, kind: :member, tool_name: "volunteer_flag", + description: "Flag a volunteer", + params: { reason: { type: :string, required: true } } + + mcp_action :flag, kind: :member, http_verb: :delete, tool_name: "volunteer_unflag", + description: "Remove a volunteer's flag" +end +``` + +`mcp_action` takes everything `mcp:` takes, plus: + +| Option | Meaning | +|--------|---------| +| `kind:` | `:member`, `:collection` or `:batch`. Optional; needed only when one name belongs to more than one action, which is refused rather than guessed. | +| `tool_name:` | The MCP tool name, in place of the derived `_`. | +| `http_verb:` | Which verb to dispatch, for an action declared with several (`method: [:post, :delete]`). A verb the action does not answer to is a declaration error. | + +It **annotates**; it never declares. Naming an action the resource does not +have warns and skips. It is resolved when the tool list is built, not when it +is called, so it may appear above or below the `include`. + +An annotation **replaces** an inline `mcp:` declaration rather than merging +into it, so a shared generic declaration and a per-resource one cannot +half-combine into something neither author wrote. + +An action may carry more than one annotation, each producing its own tool — +which is how an action answering to two verbs, one undoing the other, becomes +two tools. + +**Opting in is still per resource.** Two resources including the same concern +share the actions, not the exposure: whichever does not annotate exposes +nothing. + +**Names must not collide.** A tool name carries no kind, so a `member_action` +and a `batch_action` of the same name derive the same one. Rather than let one +silently shadow the other, both are hidden until a `tool_name:` tells them +apart. + +**Batch actions declared with a String title** — the ones applications generate +in loops from data — may be annotated by that title, rather than by the symbol +ActiveAdmin derives from it by titleizing and underscoring, which can carry +punctuation. The derived tool name has that punctuation squeezed out. + +**ActiveAdmin's `:if` proc is honoured.** A batch action the admin UI hides +because its `:if` refuses is neither listed nor runnable over MCP. ActiveAdmin +itself consults `:if` only when rendering, so this is stricter than ActiveAdmin +is — deliberately: MCP should not be the way round a gate the admin enforces by +not offering the button. A proc that raises, typically because it reads request +state a tool listing cannot supply, hides the tool and says so in the log. + +**A proc `form:` is evaluated** in controller context, the way ActiveAdmin +evaluates it, so a batch action whose form varies by resource still contributes +its param types. Like `suggestions:`, this runs application code, and is never +evaluated for a user the resource's authorization adapter refuses. + ## Connecting a client `activeadmin_mcp` has been tested with **Claude Code** (Anthropic) over the diff --git a/lib/activeadmin_mcp/action_catalog.rb b/lib/activeadmin_mcp/action_catalog.rb index 55355f1..067bb80 100644 --- a/lib/activeadmin_mcp/action_catalog.rb +++ b/lib/activeadmin_mcp/action_catalog.rb @@ -9,20 +9,47 @@ module ActionCatalog RESERVED = %w[list_resources query update create describe_form].freeze class << self - def all - ResourceRegistry.resources.flat_map { |entry| definitions_for(entry[:config]) } + # The authenticated user is carried through so a batch action's `form:` + # proc can be evaluated in controller context, the way ActiveAdmin + # evaluates it. Nothing here evaluates it — see ActionDefinition#params, + # which does, lazily, once the caller has authorized the action. + def all(current_user: nil) + without_colliding_names( + ResourceRegistry.resources.flat_map do |entry| + definitions_for(entry[:config], current_user) + end + ) end - def find(tool_name) - all.find { |definition| definition.tool_name == tool_name } + def find(tool_name, current_user: nil) + all(current_user: current_user).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 + # A tool name carries no kind, so a member and a batch action of the same + # name derive the same one — which ActiveAdmin allows, and a shared + # concern declaring both is how it happens in practice. Advertising a + # duplicate would leave whichever the catalog found second permanently + # unreachable, since find returns the first match. Refuse both instead, + # and say which name, so the declaration can choose a tool_name. + def without_colliding_names(definitions) + definitions.group_by(&:tool_name).flat_map do |tool_name, sharing| + next sharing if sharing.one? + + warn_and_skip("#{sharing.length} actions would both be called #{tool_name}; " \ + "give all but one an explicit tool_name:") + [] + end + end + + def definitions_for(config, current_user = nil) + annotated, inline = partition_declarations(config) + + (annotated + inline).filter_map do |action, kind, options| + definition = ActionDefinition.new( + config: config, action: action, kind: kind, options: options, current_user: current_user + ) 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) @@ -31,6 +58,80 @@ def definitions_for(config) end end + # Resolves each mcp_action annotation to the action it names, and leaves + # every unannotated action to its own inline mcp: declaration. An + # annotation replaces an inline declaration wholesale rather than merging + # into it: one declaration wins, and it is visible which. + def partition_declarations(config) + actions = candidates(config) + annotated = [] + claimed = [] + + safe_annotations(config).each do |annotation| + matches = matching_actions(actions, annotation) + + next warn_and_skip(annotation_missing(config, annotation)) if matches.empty? + next warn_and_skip(annotation_ambiguous(config, annotation)) unless matches.one? + + action, kind = matches.first + claimed << action + annotated << [action, kind, annotation[:options]] + end + + inline = actions.filter_map do |action, kind| + # By identity, not equality: ActiveAdmin::ControllerAction is + # Comparable through a `priority` its instances do not all have, so + # asking an Array whether it includes one can raise. + next if claimed.any? { |claimed_action| claimed_action.equal?(action) } + + options = action.mcp_options + [action, kind, options] if options.is_a?(Hash) + end + + [annotated, inline] + end + + def matching_actions(actions, annotation) + wanted = annotation[:action_name].to_s + + actions.select do |action, kind| + names_of(action, kind).include?(wanted) && + (annotation[:kind].nil? || annotation[:kind] == kind) + end + end + + # A batch action may be declared with a String title, from which + # ActiveAdmin derives the symbol by titleizing, stripping spaces and + # underscoring — which can leave punctuation in it. Applications generate + # those in loops from data, so an annotation may name either the title it + # wrote or the symbol ActiveAdmin made of it. + def names_of(action, kind) + names = [declared_name(action, kind).to_s] + names << action.title.to_s if kind == :batch && action.respond_to?(:title) + names + end + + def declared_name(action, kind) + (kind == :batch ? action.sym : action.name).to_sym + end + + def annotation_missing(config, annotation) + "#{resource_name(config)} annotates #{annotation[:action_name]}, which it does not declare" + end + + def annotation_ambiguous(config, annotation) + "#{resource_name(config)} annotates #{annotation[:action_name]}, which names more than one " \ + "action; say which with kind:" + end + + def resource_name(config) + config.resource_class.name + end + + def safe_annotations(config) + config.respond_to?(:mcp_annotations) ? Array(config.mcp_annotations) : [] + end + def candidates(config) pairs = [] pairs.concat(safe_actions(config, :member_actions).map { |a| [a, :member] }) diff --git a/lib/activeadmin_mcp/action_definition.rb b/lib/activeadmin_mcp/action_definition.rb index 21e1779..900f6ff 100644 --- a/lib/activeadmin_mcp/action_definition.rb +++ b/lib/activeadmin_mcp/action_definition.rb @@ -9,6 +9,10 @@ module ActiveadminMcp class ActionDefinition KINDS = %i[member collection batch].freeze + # MCP tool names are referenced by clients as identifiers, so keep them to + # what every client can quote without escaping. + TOOL_NAME = /\A[a-z0-9][a-z0-9_-]*\z/ + # JSON Schema scalar types an application may declare. TYPES = %i[string integer number boolean array object].freeze @@ -24,18 +28,19 @@ class ActionDefinition attr_reader :config, :action, :kind, :errors - def self.build(config:, action:, kind:) + def self.build(config:, action:, kind:, current_user: nil) options = action.mcp_options return nil unless options.is_a?(Hash) - new(config: config, action: action, kind: kind, options: options) + new(config: config, action: action, kind: kind, options: options, current_user: current_user) end - def initialize(config:, action:, kind:, options:) + def initialize(config:, action:, kind:, options:, current_user: nil) @config = config @action = action @kind = kind @options = options + @current_user = current_user @errors = [] validate! end @@ -48,8 +53,28 @@ def resource_name @config.resource_class.name end + # A declaration may choose its own name: an action shared by a concern can + # need a different one on each resource, and an action exposed once per + # verb needs a distinct name per tool. def tool_name + declared = @options[:tool_name] + return declared.to_s if declared + + derived_tool_name + end + + # Squeezed down to the characters a tool name may carry, because an action + # name is not always tame: ActiveAdmin derives a batch action's symbol from + # a String title and can leave punctuation in it. A resource may still + # choose its own name with tool_name:, which is validated rather than + # squeezed, since a name someone typed deliberately should not be silently + # rewritten. + def derived_tool_name "#{resource_name.underscore.tr('/', '_')}_#{action_name}" + .downcase + .gsub(/[^a-z0-9_-]+/, "_") + .squeeze("_") + .delete_suffix("_") end def description @@ -60,10 +85,32 @@ def permission @options[:permission] end + # ActiveAdmin's own `:if` proc on a batch action, which decides whether the + # admin UI offers it at all. Returned rather than evaluated: like a + # `permission:` proc it belongs in controller context, which only the + # caller can build. + def display_if + return nil unless @kind == :batch + return nil unless @action.respond_to?(:display_if_block) + + @action.display_if_block + end + + # ActiveAdmin always posts a batch action, whatever a declaration says. + # Otherwise a declaration may pick among the verbs the action answers to — + # `method: [:post, :delete]` is one action with two meanings, and without + # this only the first would ever be reachable. def http_verb return :post if @kind == :batch - Array(@action.http_verb).first&.to_sym || :get + declared = @options[:http_verb]&.to_sym + return declared if declared + + action_verbs.first || :get + end + + def action_verbs + Array(@action.http_verb).compact.map(&:to_sym) end # Declared params, with batch actions inheriting their types from the @@ -89,14 +136,57 @@ def declared_params def inherited_params return {} unless @kind == :batch - return {} unless @action.respond_to?(:inputs) + + resolved_form.each_with_object({}) do |(name, widget), acc| + acc[name.to_sym] = { type: form_type(widget) } + end + end + + # A form: entry is usually a widget name, but ActiveAdmin also accepts an + # array of options, which it renders as a select. There is no type to read + # off that, and its values are not treated as a binding enum: they were + # resolved once, at listing time, and a declaration wanting to offer them + # should say so with suggestions:, which is advisory by design. + def form_type(widget) + return :string unless widget.respond_to?(:to_sym) + + FORM_TYPES.fetch(widget.to_sym, :string) + end + + # ActiveAdmin lets `form:` be a proc and evaluates it in controller context + # at render time, which is the only way a concern shared across resources + # can vary its options. Evaluated here the same way, so a proc form still + # contributes its param types. + # + # Deliberately lazy: this runs application code, so it must not happen + # while the catalog is merely being built, before the caller has checked + # the user is authorized for the action at all. Same posture as + # `suggestions:`. + def resolved_form + return @resolved_form if defined?(@resolved_form) + + @resolved_form = resolve_form || {} + end + + def resolve_form + return nil unless @action.respond_to?(:inputs) form = @action.inputs - return {} unless form.is_a?(Hash) + return form if form.is_a?(Hash) + return nil unless form.is_a?(Proc) - form.each_with_object({}) do |(name, widget), acc| - acc[name.to_sym] = { type: FORM_TYPES.fetch(widget.to_sym, :string) } - end + evaluate_form(form) + end + + def evaluate_form(form) + controller = ControllerDispatcher.new(config: @config, current_user: @current_user) + .controller_with_mcp_user + evaluated = ::MethodOrProcHelper.render_in_context(controller, form) + evaluated.is_a?(Hash) ? evaluated : nil + rescue StandardError => e + # The tool keeps whatever the declaration said; it just inherits nothing. + warn("[activeadmin_mcp] evaluating the #{tool_name} form: proc raised #{e.class}: #{e.message}") + nil end def validate! @@ -105,7 +195,10 @@ def validate! reserved = reserved_param_name form_keys = batch_form_keys - params.each do |name, spec| + # Declared params only. Inherited ones come from ActiveAdmin's own form: + # hash and are always well formed, and reading them would mean evaluating + # a proc form here — the one place it must not happen. + declared_params.each do |name, spec| unless spec.is_a?(Hash) @errors << "#{tool_name}: param #{name} must be a Hash" next @@ -142,6 +235,27 @@ def validate! end @errors << "#{tool_name}: permission must be callable" if permission && !permission.respond_to?(:call) + + validate_tool_name! + validate_http_verb! + end + + def validate_tool_name! + return if tool_name.match?(TOOL_NAME) + + @errors << "#{tool_name}: tool_name must match #{TOOL_NAME.source}" + end + + # Dispatching a verb the action never declared would reach nothing, or + # worse, the wrong branch of the action's own body. + def validate_http_verb! + declared = @options[:http_verb]&.to_sym + return if declared.nil? || @kind == :batch + + verbs = action_verbs + return if verbs.empty? || verbs.include?(declared) + + @errors << "#{tool_name}: http_verb #{declared} is not one the action answers to (#{verbs.join(', ')})" end # The permitted param keys for a batch action's `inputs` (its `form:` diff --git a/lib/activeadmin_mcp/action_runner.rb b/lib/activeadmin_mcp/action_runner.rb index cbc7c83..0a0de4e 100644 --- a/lib/activeadmin_mcp/action_runner.rb +++ b/lib/activeadmin_mcp/action_runner.rb @@ -32,6 +32,9 @@ def call(arguments) dispatcher = ControllerDispatcher.new(config: @definition.config, current_user: @current_user) + unavailable = display_refusal(dispatcher) + return { error: unavailable } if unavailable + refusal = permission_refusal(dispatcher, record, parsed) return { error: refusal } if refusal @@ -64,6 +67,24 @@ def authorized?(subject) .authorized?(@definition.action_name, subject) end + # ActiveAdmin's `:if` proc decides whether the admin UI offers a batch + # action at all, but ActiveAdmin only consults it when rendering — a + # dispatched request reaches the action regardless. Consulting it here too + # keeps MCP from becoming the way round a gate the admin enforces by not + # offering the button. + def display_refusal(dispatcher) + block = @definition.display_if + return nil unless block + + controller = dispatcher.controller_with_mcp_user + return nil if ::MethodOrProcHelper.render_in_context(controller, block) + + "#{@definition.tool_name} is not available to you" + rescue StandardError => e + warn("[activeadmin_mcp] if: proc for #{@definition.tool_name} raised #{e.class}: #{e.message}") + "#{@definition.tool_name} is not available to you" + 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. diff --git a/lib/activeadmin_mcp/active_admin_ext.rb b/lib/activeadmin_mcp/active_admin_ext.rb index ec21330..2b33800 100644 --- a/lib/activeadmin_mcp/active_admin_ext.rb +++ b/lib/activeadmin_mcp/active_admin_ext.rb @@ -14,6 +14,50 @@ def mcp_options end end + # Records MCP annotations declared against a resource for actions it does + # not declare inline — typically because a shared concern declares them, + # and one description could not suit every resource that includes it. + # + # They live on the resource config rather than on the action, so a + # declaration may appear above or below the `include` that brings the + # action in, and so they are discarded with the config when ActiveAdmin + # reloads in development. + module ResourceAnnotations + def mcp_annotations + @mcp_annotations ||= [] + end + end + + module McpActionDsl + # Annotates an already-declared action so it is exposed as an MCP tool. + # Never declares the action itself: naming one that does not exist is + # reported when the catalog is built, not here, because the action may + # legitimately be declared after this call. + def mcp_action(name, kind: nil, **options) + config.mcp_annotations << { + action_name: name.to_sym, + kind: kind&.to_sym, + options: options, + } + end + end + + # Installed separately from, and earlier than, apply!. A registration block + # calls mcp_action while ActiveAdmin loads its resources, which is before + # ActiveAdmin.after_load fires — so waiting for that hook would mean the + # method did not exist at the only moment anybody calls it. + # + # ActiveAdmin::Resource and ActiveAdmin::ResourceDSL both exist as soon as + # ActiveAdmin is required, so there is nothing to wait for. Idempotent: + # including a module twice is a no-op. + def self.apply_dsl! + return false unless defined?(::ActiveAdmin::Resource) && defined?(::ActiveAdmin::ResourceDSL) + + ::ActiveAdmin::Resource.include(ResourceAnnotations) + ::ActiveAdmin::ResourceDSL.include(McpActionDsl) + true + 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. diff --git a/lib/activeadmin_mcp/engine.rb b/lib/activeadmin_mcp/engine.rb index cf1b450..ff1e0c8 100644 --- a/lib/activeadmin_mcp/engine.rb +++ b/lib/activeadmin_mcp/engine.rb @@ -17,7 +17,13 @@ class Engine < ::Rails::Engine initializer "activeadmin_mcp.active_admin_ext" do ActiveSupport.on_load(:after_initialize) do - ActiveAdmin.after_load { ActiveadminMcp::ActiveAdminExt.apply! } if defined?(::ActiveAdmin) + next unless defined?(::ActiveAdmin) + + # The DSL has to exist before ActiveAdmin loads the registrations that + # call it; the option readers only have to exist before the catalog is + # read, and ActiveAdmin::BatchAction does not exist until load time. + ActiveadminMcp::ActiveAdminExt.apply_dsl! + ActiveAdmin.after_load { ActiveadminMcp::ActiveAdminExt.apply! } end end end diff --git a/lib/activeadmin_mcp/request_handler.rb b/lib/activeadmin_mcp/request_handler.rb index d44a6db..b1db8e0 100644 --- a/lib/activeadmin_mcp/request_handler.rb +++ b/lib/activeadmin_mcp/request_handler.rb @@ -46,8 +46,9 @@ def tools_list # 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| + ActionCatalog.all(current_user: @current_user).filter_map do |definition| next unless authorized_to_run?(definition) + next unless offered_by_active_admin?(definition) next unless authorized_to_list?(definition) { @@ -69,6 +70,23 @@ def authorized_to_run?(definition) false end + # ActiveAdmin's own `:if` proc on a batch action decides whether the admin + # UI offers it. Evaluated in controller context, as ActiveAdmin evaluates + # it, so `authorized?` and `current_admin_user` are in scope. A proc + # reaching for request state it cannot have here raises, and the tool is + # hidden rather than offered past a gate we could not read. + def offered_by_active_admin?(definition) + block = definition.display_if + return true unless block + + controller = ControllerDispatcher.new(config: definition.config, current_user: @current_user) + .controller_with_mcp_user + !!::MethodOrProcHelper.render_in_context(controller, block) + rescue StandardError => e + warn("[activeadmin_mcp] hiding #{definition.tool_name}: if: proc 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. @@ -182,7 +200,7 @@ def call_tool(params) end def tool_action(name, args) - definition = ActionCatalog.find(name) + definition = ActionCatalog.find(name, current_user: @current_user) return { error: "Unknown tool: #{name}" } unless definition ActionRunner.new(definition: definition, current_user: @current_user).call(args) diff --git a/spec/activeadmin_mcp/action_catalog_spec.rb b/spec/activeadmin_mcp/action_catalog_spec.rb index d8511ff..c1a6e74 100644 --- a/spec/activeadmin_mcp/action_catalog_spec.rb +++ b/spec/activeadmin_mcp/action_catalog_spec.rb @@ -5,21 +5,34 @@ 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) + def batch(name, mcp:, form: nil, title: nil, display_if: nil) + double( + "batch_action", + sym: name, + mcp_options: mcp, + inputs: form, + title: title || name.to_s.titleize, + display_if_block: display_if + ) end - def config(member: [], collection: [], batch_actions: [], batch_enabled: true, name: "Volunteer") + def config(member: [], collection: [], batch_actions: [], batch_enabled: true, name: "Volunteer", + annotations: []) double( "config", resource_class: double("model", name: name), member_actions: member, collection_actions: collection, batch_actions: batch_actions, - batch_actions_enabled?: batch_enabled + batch_actions_enabled?: batch_enabled, + mcp_annotations: annotations ) end + def annotation(name, kind: nil, **options) + { action_name: name, kind: kind, options: options } + 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) @@ -68,4 +81,150 @@ def stub_resources(*configs) expect(described_class.find("volunteer_create_warning").action_name).to eq(:create_warning) expect(described_class.find("nope")).to be_nil end + + # Actions declared by a shared concern cannot carry an mcp: key of their own, + # so the resource annotates them by name. + describe "actions annotated with mcp_action" do + it "exposes an action that carries no mcp: key of its own" do + stub_resources(config( + member: [action(:tag, mcp: nil)], + annotations: [annotation(:tag, description: "Tag a volunteer")] + )) + + definitions = described_class.all + expect(definitions.map(&:tool_name)).to eq(["volunteer_tag"]) + expect(definitions.first.description).to eq("Tag a volunteer") + end + + it "replaces an inline mcp: declaration rather than merging into it" do + stub_resources(config( + member: [action(:tag, mcp: { description: "Generic", params: { a: { type: :string } } })], + annotations: [annotation(:tag, description: "This resource's own wording")] + )) + + definition = described_class.all.first + expect(definition.description).to eq("This resource's own wording") + expect(definition.params).to eq({}) + end + + it "builds one tool per annotation, so one action can be exposed more than once" do + stub_resources(config( + member: [action(:tag, mcp: nil, verb: %i[post delete])], + annotations: [ + annotation(:tag, tool_name: "volunteer_tag", description: "Tag"), + annotation(:tag, tool_name: "volunteer_untag", http_verb: :delete, description: "Untag"), + ] + )) + + expect(described_class.all.map { |d| [d.tool_name, d.http_verb] }) + .to contain_exactly(["volunteer_tag", :post], ["volunteer_untag", :delete]) + end + + it "matches an annotation to the kind it names when one name is both a member and a batch action" do + stub_resources(config( + member: [action(:tag, mcp: nil)], + batch_actions: [batch(:tag, mcp: nil, form: { label: :text })], + annotations: [annotation(:tag, kind: :batch, description: "Tag the selected volunteers")] + )) + + definitions = described_class.all + expect(definitions.map(&:kind)).to eq([:batch]) + end + + it "refuses an annotation whose kind is ambiguous rather than guessing which action was meant" do + allow(described_class).to receive(:warn) + stub_resources(config( + member: [action(:tag, mcp: nil)], + batch_actions: [batch(:tag, mcp: nil, form: { label: :text })], + annotations: [annotation(:tag, description: "Tag")] + )) + + expect(described_class.all).to be_empty + expect(described_class).to have_received(:warn).with(/kind/) + end + + it "warns and skips an annotation naming an action the resource does not declare" do + allow(described_class).to receive(:warn) + stub_resources(config( + member: [action(:create_warning, mcp: { description: "Record a warning" })], + annotations: [annotation(:typo, description: "Nothing declares this")] + )) + + expect(described_class.all.map(&:tool_name)).to eq(["volunteer_create_warning"]) + expect(described_class).to have_received(:warn).with(/typo/) + end + end + + # tool_name carries no kind, so a member and a batch action of the same name + # derive the same one. Advertising it twice would leave whichever the catalog + # happened to find second permanently unreachable. + describe "two tools that would share a name" do + it "hides both rather than silently shadowing one" do + allow(described_class).to receive(:warn) + stub_resources(config( + member: [action(:tag, mcp: { description: "Tag one" })], + batch_actions: [batch(:tag, mcp: { description: "Tag the selected" })] + )) + + expect(described_class.all).to be_empty + expect(described_class).to have_received(:warn).with(/volunteer_tag/) + end + + it "leaves the other tools of the same resource alone" do + allow(described_class).to receive(:warn) + stub_resources(config( + member: [action(:tag, mcp: { description: "Tag one" }), + action(:create_warning, mcp: { description: "Record a warning" })], + batch_actions: [batch(:tag, mcp: { description: "Tag the selected" })] + )) + + expect(described_class.all.map(&:tool_name)).to eq(["volunteer_create_warning"]) + end + + it "catches a collision between two different resources, since tool names are global" do + allow(described_class).to receive(:warn) + stub_resources( + config(member: [action(:tag, mcp: { description: "A", tool_name: "shared_name" })]), + config(name: "Shift", member: [action(:tag, mcp: { description: "B", tool_name: "shared_name" })]) + ) + + expect(described_class.all).to be_empty + end + end + + # ActiveAdmin derives a batch action's sym from a String title by mangling it + # (titleize, strip spaces, underscore), which can leave punctuation in the + # symbol. Applications generate these in loops from data, so requiring the + # annotation to name the mangled symbol would be unusable. + describe "a batch action declared with a String title" do + it "matches an annotation that names the action by its title" do + stub_resources(config( + batch_actions: [batch(:"warning:_fridge_left_open", mcp: nil, + title: "Warning: Fridge left open", form: { note: :text })], + annotations: [annotation("Warning: Fridge left open", kind: :batch, description: "Send a warning")] + )) + + expect(described_class.all.map(&:description)).to eq(["Send a warning"]) + end + + it "still matches an annotation that names the symbol ActiveAdmin derived" do + stub_resources(config( + batch_actions: [batch(:"warning:_fridge_left_open", mcp: nil, + title: "Warning: Fridge left open")], + annotations: [annotation(:"warning:_fridge_left_open", kind: :batch, description: "Send a warning")] + )) + + expect(described_class.all.map(&:description)).to eq(["Send a warning"]) + end + + it "derives a usable tool name from a symbol carrying punctuation, rather than refusing it" do + stub_resources(config( + batch_actions: [batch(:"warning:_fridge_left_open", mcp: nil, + title: "Warning: Fridge left open")], + annotations: [annotation("Warning: Fridge left open", kind: :batch, description: "Send a warning")] + )) + + expect(described_class.all.map(&:tool_name)).to eq(["volunteer_warning_fridge_left_open"]) + end + end end diff --git a/spec/activeadmin_mcp/action_definition_spec.rb b/spec/activeadmin_mcp/action_definition_spec.rb index 1c9ddef..b354917 100644 --- a/spec/activeadmin_mcp/action_definition_spec.rb +++ b/spec/activeadmin_mcp/action_definition_spec.rb @@ -9,8 +9,9 @@ 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) + def batch_action(name, mcp:, form: nil, display_if: nil) + double("batch_action", sym: name, mcp_options: mcp, inputs: form, + title: name.to_s.titleize, display_if_block: display_if) end it "returns nil when the action did not opt in" do @@ -221,4 +222,90 @@ def batch_action(name, mcp:, form: nil) expect(definition).to be_valid end + + # An action shared by a concern may need a different tool name on each + # resource, and one answering to several verbs needs one tool per verb. + describe "naming and verb selection" do + def build(mcp, verb: :post) + described_class.new( + config: build_config, + action: member_action(:tag, mcp: mcp, verb: verb), + kind: :member, + options: mcp + ) + end + + it "uses the tool name the declaration chose, in place of the derived one" do + definition = build({ description: "Remove a tag", tool_name: "volunteer_untag" }) + + expect(definition.tool_name).to eq("volunteer_untag") + expect(definition).to be_valid + end + + it "still derives a tool name when the declaration does not choose one" do + expect(build({ description: "Tag" }).tool_name).to eq("volunteer_tag") + end + + it "rejects a tool name that is not a usable MCP tool name" do + definition = build({ description: "Tag", tool_name: "volunteer tag!" }) + + expect(definition).not_to be_valid + expect(definition.errors.join).to match(/tool_name/) + end + + it "dispatches with the verb the declaration chose, when the action answers to several" do + definition = build({ description: "Remove a tag", http_verb: :delete }, verb: %i[post delete]) + + expect(definition.http_verb).to eq(:delete) + expect(definition).to be_valid + end + + it "still takes the action's first verb when the declaration does not choose one" do + definition = build({ description: "Tag" }, verb: %i[post delete]) + + expect(definition.http_verb).to eq(:post) + end + + # Dispatching a verb the action never declared would route to nothing, or + # worse, to a different branch of the action's own body. + it "refuses a verb the action does not answer to" do + definition = build({ description: "Tag", http_verb: :put }, verb: %i[post delete]) + + expect(definition).not_to be_valid + expect(definition.errors.join).to match(/put/) + end + + it "ignores a declared verb on a batch action, which ActiveAdmin always posts" do + definition = described_class.new( + config: build_config, + action: batch_action(:tag, mcp: nil), + kind: :batch, + options: { description: "Tag", http_verb: :delete } + ) + + expect(definition.http_verb).to eq(:post) + end + end + # ActiveAdmin hides a batch action whose :if proc refuses. Exposing it over + # MCP regardless would offer an action the admin UI itself will not show. + describe "a batch action guarded by an :if proc" do + it "exposes the proc so the caller can evaluate it in controller context" do + guard = proc { false } + definition = described_class.new( + config: build_config, action: batch_action(:purge, mcp: nil, display_if: guard), + kind: :batch, options: { description: "Purge" } + ) + + expect(definition.display_if).to be(guard) + end + + it "has nothing to evaluate for a member action, which ActiveAdmin does not gate this way" do + definition = described_class.new( + config: build_config, action: member_action(:tag, mcp: nil), + kind: :member, options: { description: "Tag" } + ) + + expect(definition.display_if).to be_nil + end + end end diff --git a/spec/activeadmin_mcp/action_runner_spec.rb b/spec/activeadmin_mcp/action_runner_spec.rb index a7e0b87..7344e0a 100644 --- a/spec/activeadmin_mcp/action_runner_spec.rb +++ b/spec/activeadmin_mcp/action_runner_spec.rb @@ -29,7 +29,7 @@ def scope_collection(collection, _action = :read) # 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| + ActiveadminMcp::ActionCatalog.all(current_user: admin).find do |d| d.action_name == name && d.kind == kind end end @@ -194,4 +194,34 @@ def run(definition, arguments, current_user: admin) expect(volunteer.reload.name).to eq("Suspended: No shows") end end + + # ActiveAdmin hides a batch action whose :if proc refuses, but does not stop + # a dispatched request reaching it. Refusing the call as well keeps MCP from + # being the way round a gate the admin UI enforces by not offering it. + describe "a batch action guarded by an :if proc" do + let!(:shift) { Shift.create!(name: "Saturday") } + + after { Shift.delete_all } + + def purge + run(find_definition(:purge, :batch), { "ids" => [shift.id.to_s] }) + end + + it "refuses the call when the proc refuses, and leaves the records alone" do + expect(purge[:error]).to match(/not available/i) + expect(Shift.exists?(shift.id)).to be(true) + end + + it "runs the action for a user the proc admits" do + superuser = AdminUser.create!(email: "superuser@example.com") + definition = ActiveadminMcp::ActionCatalog.all(current_user: superuser) + .find { |d| d.action_name == :purge && d.kind == :batch } + + result = described_class.new(definition: definition, current_user: superuser) + .call({ "ids" => [shift.id.to_s] }) + + expect(result[:error]).to be_nil + expect(Shift.exists?(shift.id)).to be(false) + end + end end diff --git a/spec/activeadmin_mcp/active_admin_ext_spec.rb b/spec/activeadmin_mcp/active_admin_ext_spec.rb index 5ec10bd..60f56ee 100644 --- a/spec/activeadmin_mcp/active_admin_ext_spec.rb +++ b/spec/activeadmin_mcp/active_admin_ext_spec.rb @@ -27,6 +27,40 @@ def member_action(name) expect(suspend.mcp_options).to eq(description: "Suspend the selected volunteers") end + # Actions declared in a shared concern cannot carry an mcp: key of their own, + # so the resource annotates them by name instead. + describe "the mcp_action DSL" do + let(:shift_config) do + ActiveAdmin.application.namespaces[:admin].resources.find { |r| r.resource_class == Shift } + end + + it "records an annotation against the resource for an action declared elsewhere" do + expect(shift_config.mcp_annotations).to include( + hash_including( + action_name: :flag, + kind: :batch, + options: hash_including(description: "Flag the selected shifts") + ) + ) + end + + it "records one annotation per declaration, so an action can become more than one tool" do + member = shift_config.mcp_annotations.select { |a| a[:kind] == :member } + + expect(member.map { |a| a[:options][:tool_name] }).to eq(%w[shift_flag shift_unflag]) + end + + it "carries the tool_name and http_verb a declaration chose" do + unflag = shift_config.mcp_annotations.find { |a| a[:options][:tool_name] == "shift_unflag" } + + expect(unflag[:options][:http_verb]).to eq(:delete) + end + + it "leaves a resource that annotates nothing with no annotations" do + expect(McpSpec::ActiveAdminHarness.volunteer_config.mcp_annotations).to eq([]) + end + 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 diff --git a/spec/activeadmin_mcp/mcp_action_annotations_spec.rb b/spec/activeadmin_mcp/mcp_action_annotations_spec.rb new file mode 100644 index 0000000..e53fe9a --- /dev/null +++ b/spec/activeadmin_mcp/mcp_action_annotations_spec.rb @@ -0,0 +1,56 @@ +require "spec_helper" +require "support/active_admin" + +# The whole annotation path against real ActiveAdmin objects: a concern in the +# shape applications actually use, declaring its actions in self.included so +# none of them can carry an mcp: key, and the resource annotating them by name. +RSpec.describe "an ActiveAdmin action annotated with mcp_action" do + let(:admin) { AdminUser.create!(email: "admin@example.com") } + + before { ActiveadminMcp::ActiveAdminExt.apply! } + + after { AdminUser.delete_all } + + def definitions + ActiveadminMcp::ActionCatalog.all(current_user: admin) + end + + def definition(tool_name) + definitions.find { |candidate| candidate.tool_name == tool_name } + end + + it "exposes an action a shared concern declared, which could carry no mcp: key of its own" do + expect(definitions.map(&:tool_name)).to include("shift_bulk_flag", "shift_flag", "shift_unflag") + end + + it "leaves actions the resource did not annotate unexposed, so the opt-in still holds" do + expect(definitions.map(&:tool_name)).not_to include("volunteer_undocumented") + end + + it "exposes one tool per declaration when a member action answers to more than one verb" do + expect(definition("shift_flag").http_verb).to eq(:post) + expect(definition("shift_unflag").http_verb).to eq(:delete) + end + + it "tells a member and a batch action of the same name apart by the kind the declaration named" do + expect(definition("shift_bulk_flag").kind).to eq(:batch) + expect(definition("shift_flag").kind).to eq(:member) + end + + # ActiveAdmin allows form: to be a proc and evaluates it in controller + # context; a concern shared across resources needs one, because its options + # depend on the resource. Reading it statically is not possible, so the gem + # evaluates it the same way ActiveAdmin does. + describe "a batch action whose form: is a proc rather than a hash" do + it "inherits the param types the proc declares" do + params = definition("shift_bulk_flag").params + + expect(params[:reason]).to include(type: :string) + expect(params[:notify]).to eq(type: :boolean) + end + + it "keeps what the declaration says about a param it also inherits" do + expect(definition("shift_bulk_flag").params[:reason]).to include(required: true) + end + end +end diff --git a/spec/activeadmin_mcp/request_handler_spec.rb b/spec/activeadmin_mcp/request_handler_spec.rb index 9ca612c..95998b5 100644 --- a/spec/activeadmin_mcp/request_handler_spec.rb +++ b/spec/activeadmin_mcp/request_handler_spec.rb @@ -327,7 +327,7 @@ def stub_resource(authorized: true) describe "action tools" do let(:resource_class) { Class.new } - def definition(tool_name: "volunteer_create_warning", permission: nil, + def definition(tool_name: "volunteer_create_warning", permission: nil, display_if: nil, params: { reason: { type: :string, required: true } }, adapter: adapter_class(authorized: true)) namespace = double("namespace", authorization_adapter: adapter) @@ -338,6 +338,7 @@ def definition(tool_name: "volunteer_create_warning", permission: nil, kind: :member, params: params, permission: permission, + display_if: display_if, action_name: :create_warning, resource_name: "Volunteer", config: double("config", namespace: namespace, resource_class: resource_class) @@ -371,7 +372,8 @@ def tool_names(definitions, current_user: :admin) it "routes a call to the action runner" do target = definition - allow(ActiveadminMcp::ActionCatalog).to receive(:find).with("volunteer_create_warning").and_return(target) + allow(ActiveadminMcp::ActionCatalog).to receive(:find) + .with("volunteer_create_warning", current_user: :admin).and_return(target) runner = instance_double(ActiveadminMcp::ActionRunner, call: { status: 302 }) allow(ActiveadminMcp::ActionRunner).to receive(:new) @@ -507,5 +509,43 @@ def catalog_definition(action_name, kind) expect(tool_names(warning, current_user: admin)).to include("volunteer_create_warning") end end + + # ActiveAdmin consults a batch action's :if proc only when rendering the + # UI, so listing one it refuses would offer a tool the admin itself will + # not show. + describe "a batch action guarded by ActiveAdmin's own :if proc" do + let(:admin) { AdminUser.create!(email: "admin@example.com") } + let(:superuser) { AdminUser.create!(email: "superuser@example.com") } + + after { AdminUser.delete_all } + + def purge_for(current_user) + ActiveadminMcp::ActionCatalog.all(current_user: current_user) + .find { |d| d.action_name == :purge && d.kind == :batch } + end + + let(:purge) { purge_for(admin) } + + it "hides the tool from a user the proc refuses" do + expect(tool_names(purge, current_user: admin)).not_to include("shift_purge") + end + + it "lists the tool for a user the proc admits" do + expect(tool_names(purge, current_user: superuser)).to include("shift_purge") + end + + # Such a proc commonly reads request state — a filter from params — which + # a listing has no way to supply. + it "hides the tool, and says why, when the proc raises for want of a request it cannot have" do + messages = [] + allow_any_instance_of(described_class).to receive(:warn) { |_, message| messages << message } + allow(purge).to receive(:display_if).and_return(proc { params[:q][:type] == "x" }) + + names = tool_names(purge, current_user: admin) + + expect(names).not_to include("shift_purge") + expect(messages.join).to match(/if: proc/) + end + end end end diff --git a/spec/e2e/fixture_app/app/admin/posts.rb b/spec/e2e/fixture_app/app/admin/posts.rb index fff3c01..fc76cff 100644 --- a/spec/e2e/fixture_app/app/admin/posts.rb +++ b/spec/e2e/fixture_app/app/admin/posts.rb @@ -3,6 +3,35 @@ ActiveAdmin.register Post do permit_params :title, :body + # Neither action below is declared here, so neither can carry an `mcp:` key. + # The mcp_action declarations that follow annotate them by name. + include Flaggable + + mcp_action :flag, kind: :batch, tool_name: "post_bulk_flag", + description: "Flag the selected posts", + params: { reason: { type: :string, required: true } } + + mcp_action :flag, kind: :member, tool_name: "post_flag", + description: "Flag a post", + params: { reason: { type: :string, required: true } } + + mcp_action :flag, kind: :member, http_verb: :delete, tool_name: "post_unflag", + description: "Remove a post's flag" + + mcp_action :clear_flags, kind: :collection, description: "Clear the flag on every flagged post" + + # Named by the title the concern gave them, because the symbols ActiveAdmin + # derived from those titles carry punctuation. No tool_name: either, so the + # derived one has to be usable on its own. + Flaggable::WARNING_REASONS.each do |reason| + mcp_action "Warning: #{reason}", kind: :batch, + description: "Warn the selected posts: #{reason.downcase}" + end + + mcp_action :purge, kind: :batch, description: "Delete the selected posts" + + mcp_action :guarded_flag, kind: :batch, description: "Flag the selected posts, if the controller allows it" + # These ActiveAdmin callbacks fire only when the create or update action runs # through the real controller, so the e2e suite can read their effects to # prove an MCP write is dispatched rather than written straight to the model. diff --git a/spec/e2e/fixture_app/app/admin/reviews.rb b/spec/e2e/fixture_app/app/admin/reviews.rb index 5244b88..dd82e68 100644 --- a/spec/e2e/fixture_app/app/admin/reviews.rb +++ b/spec/e2e/fixture_app/app/admin/reviews.rb @@ -5,6 +5,11 @@ ActiveAdmin.register Review do permit_params :body, :status + # Includes the same concern as Post but annotates none of it, so the e2e + # suite can prove sharing an action does not share its MCP exposure: the + # opt-in is still per resource. + include Flaggable + form do |f| f.inputs "Review" do f.input :body, hint: "Shown beneath the post" diff --git a/spec/e2e/fixture_app/app/models/concerns/flaggable.rb b/spec/e2e/fixture_app/app/models/concerns/flaggable.rb new file mode 100644 index 0000000..4e05392 --- /dev/null +++ b/spec/e2e/fixture_app/app/models/concerns/flaggable.rb @@ -0,0 +1,70 @@ +# A shared concern in the shape applications use when several resources need +# the same actions: the DSL calls live in self.included and are shared verbatim +# by every resource that includes it, so none of them can carry an `mcp:` key +# without describing every resource identically. A resource annotates them with +# `mcp_action` instead. +# +# Between them these declarations cover every way such a concern is written: +# +# * the same name used for both a member and a batch action +# * a member action answering to two verbs, one undoing the other +# * a collection action +# * a batch action whose `form:` is a proc rather than a hash +# * batch actions generated in a loop from data, titled with a String, whose +# symbols ActiveAdmin derives by mangling that title +# * a batch action ActiveAdmin hides behind an `:if` proc +# * a controller `before_action` guarding the batch dispatch +# +# It lives under app/models/concerns rather than app/admin only because +# ActiveAdmin excludes app/admin from the autoload paths; nothing about the +# concern depends on where it sits. +module Flaggable + WARNING_REASONS = ["Fridge left open", "Past use by date"].freeze + + def self.included(dsl) + dsl.send(:member_action, :flag, method: [:post, :delete]) do + if request.delete? + resource.update!(status: "unflagged") + redirect_to resource_path(resource), notice: "Flag removed" + else + resource.update!(status: "flagged:#{params[:reason]}") + redirect_to resource_path(resource), notice: "Flagged" + end + end + + dsl.send(:collection_action, :clear_flags, method: :post) do + active_admin_config.resource_class.where("status LIKE 'flagged:%'").update_all(status: "draft") + redirect_to collection_path, notice: "Flags cleared" + end + + dsl.send(:batch_action, :flag, form: proc { { reason: :text, notify: :checkbox } }) do |ids, inputs| + active_admin_config.resource_class.where(id: ids).update_all(status: "flagged:#{inputs['reason']}") + redirect_to collection_path, notice: "Flagged #{ids.size}" + end + + WARNING_REASONS.each do |reason| + dsl.send(:batch_action, "Warning: #{reason}", confirm: "Send a warning?") do |ids| + active_admin_config.resource_class.where(id: ids).update_all(status: "warned:#{reason}") + redirect_to collection_path, notice: "Warned #{ids.size}" + end + end + + dsl.send(:batch_action, :purge, if: proc { current_admin_user&.email == "superuser@example.com" }) do |ids| + active_admin_config.resource_class.where(id: ids).delete_all + redirect_to collection_path, notice: "Purged" + end + + dsl.send(:batch_action, :guarded_flag) do |ids| + active_admin_config.resource_class.where(id: ids).update_all(status: "guarded") + redirect_to collection_path, notice: "Guarded" + end + + dsl.controller do + before_action(only: :batch_action) do + if params[:batch_action] == "guarded_flag" + redirect_to collection_path, alert: "Refused by the concern's before_action" + end + end + end + end +end diff --git a/spec/e2e/mcp_actions_spec.rb b/spec/e2e/mcp_actions_spec.rb index 7c57389..f842d0d 100644 --- a/spec/e2e/mcp_actions_spec.rb +++ b/spec/e2e/mcp_actions_spec.rb @@ -133,4 +133,109 @@ def post_id(slug) end end end + # The driving case for mcp_action: actions a shared concern declares, which + # cannot carry an mcp: key of their own without describing every resource + # that includes the concern identically. + describe "an action declared in a shared concern and annotated with mcp_action" do + describe "in tools/list" do + it "is advertised under the tool name the annotation chose" do + expect(tool_names).to include("post_flag", "post_unflag", "post_bulk_flag") + end + + it "is not advertised for a resource that includes the same concern but annotates nothing, so exposure stays per resource" do + expect(tool_names.grep(/\Areview_/)).to be_empty + end + + it "inherits the param types of a batch action whose form: is a proc, by evaluating it as ActiveAdmin does" do + properties = tool("post_bulk_flag")["inputSchema"]["properties"] + + expect(properties["reason"]["type"]).to eq("string") + expect(properties["notify"]["type"]).to eq("boolean") + end + end + + describe "when called" do + it "runs the member action against the real controller" do + client.call_tool("post_flag", id: post_id("small-gods"), reason: "discworld") + + expect(post_status("small-gods")).to eq("flagged:discworld") + end + + # Both tools dispatch the same action; only the verb differs, and the + # action's own body branches on it. Without the annotation choosing one, + # only the first verb ActiveAdmin recorded would ever be reachable. + it "dispatches the verb the annotation chose, reaching the other branch of the same action" do + client.call_tool("post_unflag", id: post_id("small-gods")) + + expect(post_status("small-gods")).to eq("unflagged") + end + + it "runs a collection action the concern declared" do + client.call_tool("post_flag", id: post_id("small-gods"), reason: "discworld") + + result = client.call_tool("post_clear_flags") + + expect(result["error"]).to be_nil + expect(post_status("small-gods")).to eq("draft") + end + + it "tells the batch action of the same name apart from the member one, and applies it to exactly the selected records" do + result = client.call_tool( + "post_bulk_flag", + ids: [post_id("a-wizard-of-earthsea"), post_id("the-tombs-of-atuan")], + reason: "earthsea" + ) + + expect(result["error"]).to be_nil + expect(post_status("a-wizard-of-earthsea")).to eq("flagged:earthsea") + expect(post_status("the-tombs-of-atuan")).to eq("flagged:earthsea") + expect(post_status("small-gods")).to eq("draft") + end + end + + # ActiveAdmin lets a batch action be titled with a String and derives its + # symbol by mangling that title, which leaves punctuation in the symbol. + # Applications generate these in loops from data, so the annotation names + # the title it wrote rather than the symbol ActiveAdmin made of it. + describe "a batch action generated in a loop and titled with a String" do + it "is advertised under a tool name derived from the mangled symbol, with the punctuation removed" do + expect(tool_names).to include("post_warning_fridge_left_open", "post_warning_past_use_by_date") + end + + it "runs the action the title named, and no other of the same family" do + result = client.call_tool("post_warning_fridge_left_open", ids: [post_id("small-gods")]) + + expect(result["error"]).to be_nil + expect(post_status("small-gods")).to eq("warned:Fridge left open") + expect(post_status("a-wizard-of-earthsea")).to eq("draft") + end + end + + # ActiveAdmin hides a batch action whose :if proc refuses, but consults the + # proc only when rendering — a dispatched request reaches the action + # regardless. The seeded admin is not the superuser the proc asks for. + describe "a batch action ActiveAdmin hides behind an :if proc" do + it "is not advertised, because the admin UI would not offer it either" do + expect(tool_names).not_to include("post_purge") + end + + it "refuses the call as well, so MCP is not the way round the gate, and deletes nothing" do + before_count = client.call_tool("query", resource: "Post")["count"] + + result = client.call_tool("post_purge", ids: [post_id("small-gods")]) + + expect(result["error"]).to match(/not available/i) + expect(client.call_tool("query", resource: "Post")["count"]).to eq(before_count) + end + end + + # The concern guards the batch dispatch with a controller before_action. + # Dispatching through the real controller is what makes that still apply. + it "is stopped by a before_action the concern declared, and leaves the records unchanged" do + result = client.call_tool("post_guarded_flag", ids: [post_id("small-gods")]) + + expect(result["flash"]&.values&.join).to match(/before_action/i) + expect(post_status("small-gods")).to eq("draft") + end + end end diff --git a/spec/support/active_admin.rb b/spec/support/active_admin.rb index 3937084..fda158c 100644 --- a/spec/support/active_admin.rb +++ b/spec/support/active_admin.rb @@ -133,7 +133,62 @@ class ApplicationController < ActionController::Base; end end end +# Installs the mcp_action DSL. In an application the engine does this before +# ActiveAdmin loads its registrations; here the registrations below are plain +# top-level code, so it has to happen before them. +ActiveadminMcp::ActiveAdminExt.apply_dsl! + +module McpSpec + # In the shape of the application concerns this gem has to support: the DSL + # calls live in self.included and are shared verbatim across resources, so + # nothing here can carry an mcp: key of its own without describing every + # resource that includes it identically. + # + # Deliberately awkward in the same three ways a real one is: the same name + # used for both a member and a batch action, a member action answering to two + # verbs, and a batch action whose form: is a proc rather than a hash. + module SharedFlagActions + def self.included(dsl) + dsl.send(:member_action, :flag, method: [:post, :delete]) do + if request.delete? + resource.update(location: nil) + redirect_to resource_path(resource), notice: "Flag removed" + else + resource.update(location: params[:label]) + redirect_to resource_path(resource), notice: "Flagged" + end + end + + # ActiveAdmin hides a batch action from the UI when its :if proc refuses. + dsl.send(:batch_action, :purge, if: proc { current_admin_user&.email == "superuser@example.com" }) do |ids| + Shift.where(id: ids).delete_all + redirect_to collection_path, notice: "Purged" + end + + dsl.send(:batch_action, :flag, form: proc { { reason: :text, notify: :checkbox } }) do |ids, inputs| + Shift.where(id: ids).update_all(location: inputs["reason"]) + redirect_to collection_path, notice: "Flagged #{ids.size}" + end + end + end +end + ActiveAdmin.register Shift do + include McpSpec::SharedFlagActions + + mcp_action :flag, kind: :batch, tool_name: "shift_bulk_flag", + description: "Flag the selected shifts", + params: { reason: { type: :string, required: true } } + + mcp_action :flag, kind: :member, tool_name: "shift_flag", + description: "Flag a shift", + params: { reason: { type: :string, required: true } } + + mcp_action :purge, kind: :batch, description: "Purge the selected shifts" + + mcp_action :flag, kind: :member, http_verb: :delete, tool_name: "shift_unflag", + description: "Remove a shift's flag" + permit_params :name, :location, :starts_at form do |f|