From 4b50648a45d020364bda4da75c72686209d0e057 Mon Sep 17 00:00:00 2001
From: Lloyd Watkin <lloyd@olioex.com>
Date: Mon, 21 Sep 2026 08:24:15 +0100
Subject: [PATCH] Resolve permit_params in controller context, and two
 form-description fixes
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Closes #18.

A resource whose permit_params is a block was refused by create, update and
describe_form as though it had declared no permitted params at all. ActiveAdmin
instance_execs such a block on the controller, and all three resolved it
against a bare instance, so a block reading current_admin_user raised NameError
and the rescue that recognises a resource with genuinely no permit_params
swallowed it. The block form is the usual way to vary the writable set by user,
so this was not an exotic shape.

describe_form also reported no writable attributes for a form block declaring
none of its own — ActiveAdmin's own default form is a bare f.inputs that
Formtastic expands at render time — because an empty result is truthy and short
circuited the fallback. And it reported an `inputs for: :association` block's
fields as attributes of the record itself, which has_many already avoided.

All three were in lines of ActiveAdmin's source quoted in the notes that
justified the original changes: the block branch of

    params.permit(*permitted_params, param_key => block ? instance_exec(&block) : args)

and the bare f.inputs in ActiveAdmin's default_form_config. The information was
not missing. Every fixture simply used the other branch, so no amount of
running or mutating the suite could have failed on them.

CLAUDE.md gains the rule that would have caught all three: enumerate the
declaration forms ActiveAdmin accepts for any construct this gem reads, and
keep a fixture for each. It also records that mutation checking speaks to a
test's sensitivity and never to coverage of shapes no fixture has, which is
what made the gap feel already closed.

The fixture application gains the two missing shapes, and Review's form gains
an inputs for: block. Reverting each fix fails the new examples.

The remaining finding is documented rather than fixed: on the permit_params
fallback path describe_form reports only column-backed fields, because the
names are recovered by offering the controller every column and seeing which
survive, so tag_ids and nested *_attributes keys are missing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 CHANGELOG.md                                  | 24 ++++++++++++
 CLAUDE.md                                     | 32 ++++++++++++++++
 README.md                                     |  7 ++++
 lib/activeadmin_mcp/form_description.rb       | 12 +++++-
 lib/activeadmin_mcp/form_field_collector.rb   | 26 ++++++++++---
 lib/activeadmin_mcp/record_writer.rb          |  8 +++-
 spec/activeadmin_mcp/form_description_spec.rb | 18 +++++++++
 .../form_field_collector_spec.rb              | 29 +++++++++++++++
 spec/activeadmin_mcp/record_writer_spec.rb    | 26 +++++++++++++
 spec/e2e/fixture_app/app/admin/assignments.rb | 20 ++++++++++
 spec/e2e/fixture_app/app/admin/reviews.rb     |  5 +++
 spec/e2e/fixture_app/app/models/assignment.rb |  6 +++
 spec/e2e/fixture_app/app/models/review.rb     |  1 +
 .../20260101000006_create_assignments.rb      | 10 +++++
 spec/e2e/mcp_tools_spec.rb                    | 36 ++++++++++++++++++
 spec/support/active_admin.rb                  | 37 +++++++++++++++++++
 16 files changed, 288 insertions(+), 9 deletions(-)
 create mode 100644 spec/e2e/fixture_app/app/admin/assignments.rb
 create mode 100644 spec/e2e/fixture_app/app/models/assignment.rb
 create mode 100644 spec/e2e/fixture_app/db/migrate/20260101000006_create_assignments.rb

diff --git a/CHANGELOG.md b/CHANGELOG.md
index b19affd..055c00f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -88,6 +88,30 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   `activeadmin-mcp-X.Y.Z.mcpb`, so installing the bundle no longer means
   digging a build artifact out of the Actions tab.
 
+### Fixed
+
+- A resource whose `permit_params` is a block was refused by `create`, `update`
+  and `describe_form` with `Resource declares no permit_params, so nothing may
+  be written`, which was not merely unhelpful but wrong. ActiveAdmin
+  `instance_exec`s such a block on the controller, and the gem resolved it
+  against a bare controller instance, so a block reading `current_admin_user`
+  raised `NameError` — and the rescue that exists to recognise a resource with
+  no `permit_params` at all swallowed it. The block is now resolved against a
+  controller carrying the MCP user, as a dispatched call gets.
+
+- `describe_form` reported no writable attributes at all for a resource whose
+  `form` block declares none of its own — `form do |f| f.inputs; f.actions end`,
+  the shape of ActiveAdmin's own default form, where Formtastic expands the
+  inputs only at render time. There is nothing in such a block to read, so the
+  description now falls back to the resource's permitted params, as it already
+  did for a resource with no `form` block.
+
+- `describe_form` reported the fields of an `inputs for: :association` block as
+  attributes of the record being described, rather than as a nested group.
+  They belong to the associated record and `create` and `update` will drop
+  them. `has_many` was already handled this way; both routes into an
+  association's fields now behave the same.
+
 ### Changed
 
 - A batch action's own ActiveAdmin `:if` proc is now honoured: one the admin UI
diff --git a/CLAUDE.md b/CLAUDE.md
index 23021e4..4d3eebc 100644
--- a/CLAUDE.md
+++ b/CLAUDE.md
@@ -64,6 +64,38 @@ a snapshot taken after migrating and seeding. So examples must not depend on
 what another one left behind, and the suite runs in random order to keep that
 honest. Write each one as though it runs alone, because it might.
 
+### Cover the declaration shapes, not only the behaviours
+
+The rule above asks for an e2e test per MCP action. That is necessary and it is
+not sufficient, because it is organised around what this gem does rather than
+around what the applications it reads look like. Nearly every defect found in
+review so far has been the same thing: a shape of ActiveAdmin declaration that
+no fixture used, so no test could fail on it.
+
+So, for any ActiveAdmin construct this gem reads, enumerate the forms
+ActiveAdmin accepts and make sure a fixture exists for each. For example
+`permit_params` may be a list, a block, a block that reaches for controller
+state, absent entirely, or set on the namespace; a `form` block may declare
+inputs, declare none and leave Formtastic to expand a bare `f.inputs`, or nest
+with `has_many` or `inputs for:`; a `batch_action` may be named with a symbol
+or a String title, and its `form:` may be a hash or a proc.
+
+**When you read ActiveAdmin's source to answer a question, enumerate the
+branches you did not take.** Two of those defects were in lines already quoted
+in the notes justifying the change — `block ? instance_exec(&block) : args` and
+a bare `f.inputs` in ActiveAdmin's own default form. The information was not
+missing; the question "what else does this line permit?" was never asked.
+
+**Mutation checking does not cover this, and can disguise it.** Breaking a
+fixture and watching the example fail proves the test is sensitive to that
+fixture. It says nothing about a shape no fixture has, and no mutation of the
+existing fixtures will ever reveal one. Treat a passing mutation check as
+evidence about test sensitivity, and never describe it as evidence of coverage.
+
+When the shapes are invented rather than observed, they tend to match whatever
+was just built. Prefer shapes taken from real applications or from ActiveAdmin's
+own source and test suite.
+
 ### Writing an e2e example
 
 `E2E::McpClient` speaks the protocol: `tools_list` returns the `tools/list`
diff --git a/README.md b/README.md
index 5f4f26d..8cc4c23 100644
--- a/README.md
+++ b/README.md
@@ -174,6 +174,13 @@ Two limits worth knowing:
 - A field a form block declares but `permit_params` omits is described and then
   silently dropped on write. This cannot arise on the `permit_params` fallback
   path.
+- On the `permit_params` fallback path, only fields backed by a database column
+  are reported. ActiveAdmin keeps no list of the params it permits — only a
+  method that filters against them — so the names are recovered by offering it
+  every column the model has and seeing which survive. Permitted params that
+  are not columns, such as `tag_ids` or a nested `*_attributes` key, are
+  therefore missing from the description even though `create` and `update`
+  will accept them.
 
 ### Running member, collection and batch actions
 
diff --git a/lib/activeadmin_mcp/form_description.rb b/lib/activeadmin_mcp/form_description.rb
index 4ecee09..dece5fc 100644
--- a/lib/activeadmin_mcp/form_description.rb
+++ b/lib/activeadmin_mcp/form_description.rb
@@ -29,8 +29,12 @@ def call(action: "new")
       refusal = write_refusal(write_action)
       return refusal if refusal
 
+      # An empty result is not the same as no form block, but it means the same
+      # thing here: ActiveAdmin's own default form is a bare `f.inputs` that
+      # Formtastic expands only at render time, so a block written that way has
+      # nothing in it to read and the permitted params are all there is.
       declared = declared_inputs
-      return describe(action, "form", declared) if declared
+      return describe(action, "form", declared) if declared&.any?
 
       permitted = permitted_inputs
       return permit_params_refusal unless permitted
@@ -114,9 +118,13 @@ def permitted_inputs
       names.map { |name| { name: name } }
     end
 
+    # The controller carries the MCP user: ActiveAdmin instance_execs a
+    # block-form permit_params on it, so a block reading current_admin_user
+    # raises on a bare instance and the resource looks unwritable.
     def permitted_names
       param_key = @config.param_key.to_sym
-      controller = @config.controller.new
+      controller = ControllerDispatcher.new(config: @config, current_user: @current_user)
+                                       .controller_with_mcp_user
       controller.params = ActionController::Parameters.new(
         param_key => @resource[:model].column_names.index_with { nil }
       )
diff --git a/lib/activeadmin_mcp/form_field_collector.rb b/lib/activeadmin_mcp/form_field_collector.rb
index 8f7567f..177d444 100644
--- a/lib/activeadmin_mcp/form_field_collector.rb
+++ b/lib/activeadmin_mcp/form_field_collector.rb
@@ -32,7 +32,14 @@ def input(name, *_args, **options, &_block)
       self
     end
 
-    def inputs(*_args, **_opts, &block)
+    # `inputs for: :author` scopes its fields to an association, the same way
+    # has_many does; only an unscoped `inputs` groups fields of the record
+    # itself. Descending into a scoped one would advertise the associated
+    # record's fields as attributes of the record being written.
+    def inputs(*_args, **options, &block)
+      association = options[:for]
+      return nest(association, &block) if association
+
       instance_exec(self, &block) if block
       self
     end
@@ -41,11 +48,7 @@ def inputs(*_args, **_opts, &block)
     # can tell an association's fields from the record's own. Flattening them
     # would advertise `body` as an attribute of the parent record.
     def has_many(name, *_args, **_opts, &block)
-      return self unless name.respond_to?(:to_sym)
-      return self if declared?(name.to_sym)
-
-      @inputs << { name: name.to_sym, nested: block ? self.class.new.collect(&block) : [] }
-      self
+      nest(name, &block)
     end
 
     def method_missing(_name, *_args, **_opts, &block)
@@ -59,6 +62,17 @@ def respond_to_missing?(_name, _include_private = false)
 
     private
 
+    # `for:` may name the association or give it as [name, object]; only the
+    # name says anything to a client.
+    def nest(association, &block)
+      name = association.is_a?(Array) ? association.first : association
+      return self unless name.respond_to?(:to_sym)
+      return self if declared?(name.to_sym)
+
+      @inputs << { name: name.to_sym, nested: block ? self.class.new.collect(&block) : [] }
+      self
+    end
+
     def declared?(name)
       @inputs.any? { |input| input[:name] == name }
     end
diff --git a/lib/activeadmin_mcp/record_writer.rb b/lib/activeadmin_mcp/record_writer.rb
index 7ff7ce1..f50b056 100644
--- a/lib/activeadmin_mcp/record_writer.rb
+++ b/lib/activeadmin_mcp/record_writer.rb
@@ -133,9 +133,15 @@ def permit_refusal(permitted)
 
     # Asks the resource's own controller what it would permit, or nil when it
     # has nothing to say because permit_params was never declared.
+    #
+    # The controller carries the MCP user, because ActiveAdmin instance_execs a
+    # block-form permit_params on it: a block reading current_admin_user raises
+    # on a bare instance, and the rescue below would then report a resource
+    # that permits plenty as one that permits nothing.
     def resolve_permitted(attributes)
       param_key = @config.param_key.to_sym
-      controller = @config.controller.new
+      controller = ControllerDispatcher.new(config: @config, current_user: @current_user)
+                                       .controller_with_mcp_user
       controller.params = ActionController::Parameters.new(param_key => attributes)
       permitted = controller.send(:permitted_params)
       scoped = permitted && permitted[param_key]
diff --git a/spec/activeadmin_mcp/form_description_spec.rb b/spec/activeadmin_mcp/form_description_spec.rb
index fbdd800..a78472b 100644
--- a/spec/activeadmin_mcp/form_description_spec.rb
+++ b/spec/activeadmin_mcp/form_description_spec.rb
@@ -80,6 +80,24 @@ def attribute(result, name)
       expect(attribute(result, "name")[:required]).to be(true)
     end
 
+    # ActiveAdmin's own default form is a bare `f.inputs`, which Formtastic
+    # expands only at render time. A resource writing that out by hand has a
+    # form block with nothing in it to read, which is not the same as a form
+    # that permits nothing.
+    it "falls back to permitted params when the form block declares no inputs of its own" do
+      result = describe_form("Roster")
+
+      expect(result[:source]).to eq("permit_params")
+      expect(result[:attributes].map { |attribute| attribute[:name] }).to eq(%w[name notes])
+    end
+
+    it "resolves a block-form permit_params, which ActiveAdmin evaluates on the controller" do
+      result = describe_form("Placement")
+
+      expect(result[:error]).to be_nil
+      expect(result[:attributes].map { |attribute| attribute[:name] }).to eq(%w[name notes])
+    end
+
     it "refuses a resource that declares no permit_params either, because nothing may be written" do
       expect(describe_form("Sighting")[:error]).to match(/permit_params/)
     end
diff --git a/spec/activeadmin_mcp/form_field_collector_spec.rb b/spec/activeadmin_mcp/form_field_collector_spec.rb
index d175e7a..911be72 100644
--- a/spec/activeadmin_mcp/form_field_collector_spec.rb
+++ b/spec/activeadmin_mcp/form_field_collector_spec.rb
@@ -107,6 +107,35 @@ def collect(&block)
     end
   end
 
+  # Formtastic's other route into an association's fields. has_many already
+  # reports one as a nested group; flattening this one would advertise the
+  # associated record's fields as attributes of the record being written.
+  it "records an inputs block scoped to an association as a nested group, as it does has_many" do
+    inputs = collect do |_f|
+      input :title
+      inputs for: :author do |a|
+        a.input :name
+      end
+    end
+
+    expect(inputs).to eq(
+      [
+        { name: :title },
+        { name: :author, nested: [{ name: :name }] },
+      ]
+    )
+  end
+
+  it "still descends into a plain inputs block, which is not scoped to anything" do
+    inputs = collect do |_f|
+      inputs "Details" do
+        input :title
+      end
+    end
+
+    expect(inputs).to eq([{ name: :title }])
+  end
+
   it "records each field once when a form declares the same input twice" do
     inputs = collect do |_f|
       input :title
diff --git a/spec/activeadmin_mcp/record_writer_spec.rb b/spec/activeadmin_mcp/record_writer_spec.rb
index ecb76d9..a6d6db3 100644
--- a/spec/activeadmin_mcp/record_writer_spec.rb
+++ b/spec/activeadmin_mcp/record_writer_spec.rb
@@ -8,6 +8,7 @@
   let(:sightings) { ActiveadminMcp::ResourceRegistry.find("Sighting") }
 
   after do
+    Placement.delete_all
     Volunteer.delete_all
     Sighting.delete_all
     AdminUser.delete_all
@@ -88,6 +89,31 @@ def writer(resource)
     end
   end
 
+  # ActiveAdmin instance_execs a block-form permit_params on the controller, so
+  # resolving it needs the same controller context a dispatched call gets. A
+  # bare controller instance cannot answer current_admin_user, and a resolution
+  # that rescues the resulting NameError reports the resource as declaring no
+  # permitted params at all.
+  describe "a resource whose permit_params is a block reading controller state" do
+    let(:placements) { ActiveadminMcp::ResourceRegistry.find("Placement") }
+
+    it "creates the record, writing every attribute the block permitted" do
+      result = writer(placements).create(attributes: { "name" => "Ann", "notes" => "Weekends" })
+
+      expect(result[:error]).to be_nil
+      expect(Placement.find(result[:id])).to have_attributes(name: "Ann", notes: "Weekends")
+    end
+
+    it "updates the record rather than refusing it for want of permitted params" do
+      placement = Placement.create!(name: "Ann")
+
+      result = writer(placements).update(id: placement.id, attributes: { "notes" => "Weekends" })
+
+      expect(result[:error]).to be_nil
+      expect(placement.reload.notes).to eq("Weekends")
+    end
+  end
+
   describe "updating a record" do
     let!(:volunteer) { Volunteer.create!(name: "Ann", active: true) }
 
diff --git a/spec/e2e/fixture_app/app/admin/assignments.rb b/spec/e2e/fixture_app/app/admin/assignments.rb
new file mode 100644
index 0000000..7b7be33
--- /dev/null
+++ b/spec/e2e/fixture_app/app/admin/assignments.rb
@@ -0,0 +1,20 @@
+# Declared in the two shapes the rest of the fixture application does not use,
+# so the e2e suite covers them rather than only the shapes the gem was written
+# against:
+#
+#   * `permit_params` as a block that reads controller state, which ActiveAdmin
+#     instance_execs on the controller;
+#   * a `form` block declaring no inputs of its own, leaving Formtastic to
+#     expand a bare `f.inputs` at render time, as ActiveAdmin's own default
+#     form does — so there is nothing in the block to read and the description
+#     has to come from the permitted params instead.
+ActiveAdmin.register Assignment do
+  permit_params do
+    current_admin_user ? %i[name notes] : %i[name]
+  end
+
+  form do |f|
+    f.inputs
+    f.actions
+  end
+end
diff --git a/spec/e2e/fixture_app/app/admin/reviews.rb b/spec/e2e/fixture_app/app/admin/reviews.rb
index dd82e68..775106c 100644
--- a/spec/e2e/fixture_app/app/admin/reviews.rb
+++ b/spec/e2e/fixture_app/app/admin/reviews.rb
@@ -15,6 +15,11 @@
       f.input :body, hint: "Shown beneath the post"
       f.input :status, as: :select, collection: %w[pending approved], label: "Moderation status"
     end
+    # Formtastic's other route into an association's fields, alongside
+    # has_many: its inputs belong to the post, not to the review.
+    f.inputs for: :post do |post|
+      post.input :title
+    end
     f.actions
   end
 end
diff --git a/spec/e2e/fixture_app/app/models/assignment.rb b/spec/e2e/fixture_app/app/models/assignment.rb
new file mode 100644
index 0000000..83bf1f1
--- /dev/null
+++ b/spec/e2e/fixture_app/app/models/assignment.rb
@@ -0,0 +1,6 @@
+class Assignment < ApplicationRecord
+  # See the note in post.rb: Ransack 4 requires an explicit allowlist.
+  def self.ransackable_attributes(_auth_object = nil)
+    column_names
+  end
+end
diff --git a/spec/e2e/fixture_app/app/models/review.rb b/spec/e2e/fixture_app/app/models/review.rb
index 677460a..1f9f079 100644
--- a/spec/e2e/fixture_app/app/models/review.rb
+++ b/spec/e2e/fixture_app/app/models/review.rb
@@ -1,5 +1,6 @@
 class Review < ApplicationRecord
   belongs_to :post, optional: true
+  accepts_nested_attributes_for :post
 
   validates :body, presence: true
 
diff --git a/spec/e2e/fixture_app/db/migrate/20260101000006_create_assignments.rb b/spec/e2e/fixture_app/db/migrate/20260101000006_create_assignments.rb
new file mode 100644
index 0000000..6aa7f71
--- /dev/null
+++ b/spec/e2e/fixture_app/db/migrate/20260101000006_create_assignments.rb
@@ -0,0 +1,10 @@
+class CreateAssignments < ActiveRecord::Migration[7.2]
+  def change
+    create_table :assignments do |t|
+      t.string :name
+      t.string :notes
+
+      t.timestamps
+    end
+  end
+end
diff --git a/spec/e2e/mcp_tools_spec.rb b/spec/e2e/mcp_tools_spec.rb
index 23735a0..eeb3360 100644
--- a/spec/e2e/mcp_tools_spec.rb
+++ b/spec/e2e/mcp_tools_spec.rb
@@ -104,6 +104,29 @@ def attribute(result, name)
       end
     end
 
+    # Two declaration shapes the rest of the fixture application does not use.
+    # Both were wrong when first shipped, and neither could fail against a
+    # suite whose every resource declared permit_params as a list.
+    context "for a resource declared in the less common shapes" do
+      let(:result) { client.call_tool("describe_form", resource: "Assignment") }
+
+      it "resolves a permit_params block that reads controller state, rather than reporting the resource as permitting nothing" do
+        expect(result["error"]).to be_nil
+        expect(result["attributes"].map { |attribute| attribute["name"] }).to eq(%w[name notes])
+      end
+
+      it "falls back to permitted params when the form block leaves its inputs to Formtastic" do
+        expect(result["source"]).to eq("permit_params")
+      end
+    end
+
+    it "reports an association's fields as nested rather than as attributes of the record itself" do
+      result = client.call_tool("describe_form", resource: "Review")
+
+      expect(result["attributes"].map { |attribute| attribute["name"] }).not_to include("title")
+      expect(result["nested"]).to include("name" => "post", "attributes" => [{ "name" => "title" }])
+    end
+
     it "describes the edit form when asked for it" do
       result = client.call_tool("describe_form", resource: "Post", action: "edit")
 
@@ -167,6 +190,19 @@ def posts
       expect(posts.length).to eq(3)
     end
 
+    it "creates a record for a resource whose permit_params is a block reading controller state" do
+      result = client.call_tool(
+        "create",
+        resource: "Assignment",
+        attributes: { name: "Saturday sort", notes: "Two volunteers" }
+      )
+
+      expect(result["error"]).to be_nil
+
+      created = client.call_tool("query", resource: "Assignment", q: { id_eq: result["id"] })["records"].first
+      expect(created).to include("name" => "Saturday sort", "notes" => "Two volunteers")
+    end
+
     it "refuses to create a record for a resource registered without the create action" do
       result = client.call_tool("create", resource: "Author", attributes: { name: "Iain" })
 
diff --git a/spec/support/active_admin.rb b/spec/support/active_admin.rb
index fda158c..383ef65 100644
--- a/spec/support/active_admin.rb
+++ b/spec/support/active_admin.rb
@@ -47,6 +47,26 @@
     t.timestamps
   end
 
+  # Registered with a form block that declares no inputs of its own, leaving
+  # Formtastic to expand a bare `f.inputs` at render time — the shape of
+  # ActiveAdmin's own default form. There is nothing there to read, so the
+  # description has to fall back to permitted params.
+  create_table :rosters, force: true do |t|
+    t.string :name
+    t.string :notes
+    t.timestamps
+  end
+
+  # Registered with a block-form permit_params that reads controller state,
+  # which ActiveAdmin instance_execs on the controller. Resolving it against a
+  # controller with no MCP user on it raises, so this is the fixture that keeps
+  # the resolution honest about needing controller context.
+  create_table :placements, force: true do |t|
+    t.string :name
+    t.string :notes
+    t.timestamps
+  end
+
   create_table :admin_users, force: true do |t|
     t.string :email
     t.timestamps
@@ -57,6 +77,8 @@ class Volunteer < ActiveRecord::Base
   validates :name, presence: true
 end
 class Note < ActiveRecord::Base; end
+class Placement < ActiveRecord::Base; end
+class Roster < ActiveRecord::Base; end
 class Shift < ActiveRecord::Base
   has_many :sightings
   accepts_nested_attributes_for :sightings
@@ -204,6 +226,21 @@ def self.included(dsl)
   end
 end
 
+ActiveAdmin.register Placement do
+  permit_params do
+    current_admin_user ? %i[name notes] : %i[name]
+  end
+end
+
+ActiveAdmin.register Roster do
+  permit_params :name, :notes
+
+  form do |f|
+    f.inputs
+    f.actions
+  end
+end
+
 ActiveAdmin.register Note do
   actions :index, :show
 end