From 8ada2264e20e8c228c5ca033de2588da9affadba Mon Sep 17 00:00:00 2001 From: Lloyd Watkin Date: Sat, 19 Sep 2026 11:24:30 +0100 Subject: [PATCH] Expose actions declared in shared concerns with an mcp_action DSL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #16. An action opts in to MCP by carrying an mcp: key on its own declaration, which reaches nothing declared in a shared concern or by another gem. Even where the concern could be edited, one mcp: key there would describe every resource that includes it identically, which is the opposite of what is wanted. Such actions are already registered on the resource config as ordinary ControllerAction and BatchAction objects, so only the metadata was missing. The resource now supplies it by name with mcp_action, resolved when the catalog is built rather than when declared, so a declaration may sit either side of the include and is discarded with the config on a reload. Four things the concern shapes forced out, each of which was a real gap: Two actions of the same name — ActiveAdmin allows a member and a batch action to share one, and a concern declaring both is how that happens — derived the same tool name. tools/list advertised it twice and find returned the member one, leaving the batch action permanently unreachable. Both are now hidden until a tool_name: tells them apart. A member action declared method: [:post, :delete] is one action with two meanings, the second usually undoing the first, but only the first verb was reachable. An annotation may now pick the verb, and an action may carry several annotations, so both meanings become tools. A batch action titled with a String gets a symbol ActiveAdmin derives by mangling that title, punctuation included. Applications generate these in loops from data, so an annotation may name the title instead, and the derived tool name has the punctuation squeezed out. ActiveAdmin's own :if proc on a batch action is now honoured. ActiveAdmin consults it only when rendering, so it will dispatch an action it refuses to display; this is deliberately stricter, because MCP should not be the way round a gate the admin enforces by not offering the button. A proc form: is evaluated in controller context the way ActiveAdmin evaluates it, so those batch actions contribute their param types. That runs application code, so it is lazy: never during catalog construction, only once the caller has authorized the action, which is the posture suggestions: already has. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 39 ++++ README.md | 77 ++++++++ lib/activeadmin_mcp/action_catalog.rb | 117 +++++++++++- lib/activeadmin_mcp/action_definition.rb | 134 ++++++++++++-- lib/activeadmin_mcp/action_runner.rb | 21 +++ lib/activeadmin_mcp/active_admin_ext.rb | 44 +++++ lib/activeadmin_mcp/engine.rb | 8 +- lib/activeadmin_mcp/request_handler.rb | 22 ++- spec/activeadmin_mcp/action_catalog_spec.rb | 167 +++++++++++++++++- .../activeadmin_mcp/action_definition_spec.rb | 91 +++++++++- spec/activeadmin_mcp/action_runner_spec.rb | 32 +++- spec/activeadmin_mcp/active_admin_ext_spec.rb | 34 ++++ .../mcp_action_annotations_spec.rb | 56 ++++++ spec/activeadmin_mcp/request_handler_spec.rb | 44 ++++- spec/e2e/fixture_app/app/admin/posts.rb | 29 +++ spec/e2e/fixture_app/app/admin/reviews.rb | 5 + .../app/models/concerns/flaggable.rb | 70 ++++++++ spec/e2e/mcp_actions_spec.rb | 105 +++++++++++ spec/support/active_admin.rb | 55 ++++++ 19 files changed, 1120 insertions(+), 30 deletions(-) create mode 100644 spec/activeadmin_mcp/mcp_action_annotations_spec.rb create mode 100644 spec/e2e/fixture_app/app/models/concerns/flaggable.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 8605743..e4cf544 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 07ca5f6..a89c52c 100644 --- a/README.md +++ b/README.md @@ -243,6 +243,83 @@ 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. +### 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|