Code review findings: permit_params resolved without controller context, and three form-description gaps

[lloydwatkin]

🤖 Findings from a code review of #13, #14 and #17, drafted with Claude Code. Ranked by severity. The first is a real breakage in merged code; the rest are narrower.

1. Block-form permit_params makes a resource unwritable

Where: lib/activeadmin_mcp/record_writer.rb:139 (resolve_permitted) and lib/activeadmin_mcp/form_description.rb:119 (permitted_names). Shipped in #13 and #14.

Both build a bare @config.controller.new to ask the resource what it permits. ActiveAdmin instance_execs a block-form permit_params on the controller, so a block that touches controller state raises on a bare instance. Confirmed against the spec harness:

RAISED: NameError: undefined local variable or method 'current_admin_user'
        for an instance of Admin::BlockPermitsController

Both call sites rescue to nil, and nil is how "this resource never declared permit_params" is signalled. So for a resource like:

ActiveAdmin.register Post do
  permit_params do
    current_admin_user.super_admin? ? [:title, :body, :status] : [:title]
  end
end

• create and update are refused outright, with
  Resource declares no permit_params, so nothing may be written: Post — a
  message that is not merely unhelpful but wrong; and
• describe_form refuses the same way.

Block-form permit_params is a documented ActiveAdmin feature and the usual way to vary the writable set by user, so this is not an exotic shape.

Fix: use ControllerDispatcher#controller_with_mcp_user instead of @config.controller.new. Both callers already hold current_user; they just don't use it. This is the same lesson #10 learned about permission: procs — a proc evaluated outside controller context raises NameError and silently hides the feature — reintroduced at a new call site.

Why it got through: every fixture declares permit_params in the list form. A fix needs an e2e fixture resource using the block form, whose block reads current_admin_user.

2. A form block declaring no inputs reports zero writable attributes

Where: lib/activeadmin_mcp/form_description.rb:31. Shipped in #14.

declared = declared_inputs
return describe(action, "form", declared) if declared

[] is truthy, so a resource whose form block declares no inputs of its own —

form do |f|
  f.inputs          # legal: Formtastic expands this at render time
  f.actions
end

— is described as source: "form" with an empty attributes array, rather than falling back to permit_params. A client reads that as "nothing may be written here".

Fix: fall back to permit_params when the form block yields no inputs, not only when there is no form block at all.

3. inputs for: :association is flattened into the parent's attributes

Where: lib/activeadmin_mcp/form_field_collector.rb:35. Shipped in #14.

inputs ignores its options, so

f.inputs for: :author do |a|
  a.input :name
end

reports name as a top-level writable attribute of the record being described. It belongs to the associated record, and create/update will drop it.

has_many already handles exactly this correctly, reporting a nested: group. inputs for: is the same idea by another route and should produce the same shape.

4. The permit_params probe only offers column names

Where: lib/activeadmin_mcp/form_description.rb:121. Shipped in #14.

ActiveAdmin keeps no list of the params it permits, only a method that filters against them, so describe_form recovers the names by offering the controller every column the model has and seeing which survive. Permitted params that are not columns — tag_ids, *_attributes — therefore never appear, and describe_form under-reports the write surface on the fallback path.

This was a deliberate simplification when #14 was written, but it was never written down. Either widen the probe with association-derived keys (#{association}_ids, nested-attributes keys) or document the limitation in the README; leaving it undocumented is the part that is wrong.

Suggested handling

1–3 are small, self-contained fixes and could go in one PR, each with e2e coverage — the absence of which is why all three got through. 4 is a judgement call between widening the probe and documenting the limit.