Skip to content

Resolve permit_params in controller context, and two form-description fixes - #19

Merged
lloydwatkin merged 1 commit into
mainfrom
fix-permit-params-controller-context
Sep 21, 2026
Merged

lloydwatkin merged 1 commit into
mainfrom
fix-permit-params-controller-context

Conversation

@lloydwatkin

Copy link
Copy Markdown
Member

🤖 Closes #18. Findings 1–3 fixed, finding 4 documented, plus the testing rule that would have caught all three.

1. Block-form permit_params made a resource unwritable

ActiveAdmin instance_execs a block-form permit_params on the controller. RecordWriter#resolve_permitted and FormDescription#permitted_names both resolved it against a bare @config.controller.new, so a block reading controller state raised NameError — and the rescue that exists to recognise a resource with genuinely no permit_params swallowed it.

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

create, update and describe_form all refused such a resource with Resource declares no permit_params, so nothing may be written — not merely unhelpful, but wrong. Both now resolve against a controller carrying the MCP user, which ControllerDispatcher#controller_with_mcp_user already existed to provide.

2. A form block declaring no inputs reported nothing writable

return describe(action, "form", declared) if declared — [] is truthy, so form do |f| f.inputs; f.actions end (the shape of ActiveAdmin's own default_form_config, where Formtastic expands the inputs at render time) described zero attributes with source: "form". There is nothing in such a block to read, so it now falls back to the permitted params, as a resource with no form block already did.

3. inputs for: :association was flattened into the parent

Its fields belong to the associated record, and create/update drop them. has_many already reported a nested: group; both routes into an association's fields now behave the same.

4. Documented, not 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 even though the write tools accept them. Now stated in the README rather than left implicit.

The part that matters more than the fixes

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 default_form_config. The information was not missing. Every fixture used the other branch, so no amount of running or mutating the suite could have failed on them.

CLAUDE.md gains a section, Cover the declaration shapes, not only the behaviours:

  • For any ActiveAdmin construct this gem reads, enumerate the forms ActiveAdmin accepts and keep a fixture for each — permit_params as a list, a block, a block reading controller state, absent, or namespace-level; a form block with inputs, without, or nesting via has_many or inputs for:; a batch_action named by symbol or String title, with a hash or proc form:.
  • When reading ActiveAdmin's source to answer a question, enumerate the branches you did not take.
  • Mutation checking does not cover this and can disguise it. It proves a test is sensitive to a fixture changing; it says nothing about a shape no fixture has. Treat it as evidence about sensitivity, never as evidence of coverage.

That last point is the honest correction to how I reported the earlier PRs: "every new example was mutation-checked" was true, and I let it stand as though it meant the input space was covered.

Testing

  • Unit: 248 examples. The harness gains Placement (block-form permit_params reading current_admin_user) and Roster (a form block leaving its inputs to Formtastic).
  • E2E: 67 examples. The fixture app gains Assignment, carrying both shapes, and Review's form gains an inputs for: :post block.
  • Each fix was verified by reverting it and confirming the new examples fail — the strongest available check, since it tests against the actual defect rather than a stand-in. All four new examples failed; restored and both suites re-run green.

🤖 Generated with Claude Code

… fixes

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>
@lloydwatkin
lloydwatkin merged commit b77907e into main Sep 21, 2026
3 checks passed
@lloydwatkin
lloydwatkin deleted the fix-permit-params-controller-context branch September 21, 2026 07:31
lloydwatkin added a commit that referenced this pull request Sep 21, 2026
Follow-up to #19, which fixed all four findings in #18. This closes two
gaps in the e2e coverage of the block-form `permit_params` shape, and
documents two behaviours #19 changed but did not describe. No library
changes.

## 1. `update` was not covered

`resolve_permitted` gates `update` as well as `create`, and #19 added
only a `create` example against `Assignment`. Reverting the fix in
`RecordWriter` leaves the new update example failing, so the second call
site is now covered by something that can actually see it.

## 2. Nothing proved the block filters

Both branches of `Assignment`'s block — `%i[name notes]` and `%i[name]`
— permit only columns the fixture already writes, so every example
passed whether the block's result was honoured or ignored entirely.

`secret_note` is a new column neither branch permits, and an example
asserts a write never reaches it. Mutating the block to permit
`secret_note` fails that example and no other.

## 3. Two behaviours left undocumented

The README now says that an associated record's fields are reported
under `nested` whether declared with `has_many` or with `inputs for:`,
and that a form block declaring no inputs of its own is described from
`permit_params` — both changed by #19, neither described. It also notes
that a block-form `permit_params` is evaluated as the authenticated MCP
user.

## Testing

`rake e2e` — 69 examples, 0 failures (67 on `main`, plus 2).

Both new examples were verified against two separate mutations:

- reverting `resolve_permitted` to `@config.controller.new` fails both;
- leaving the fix in place but adding `secret_note` to the block's
permitted list fails only the withholding example.

## Note on #20

I had an in-flight branch fixing all four findings independently, opened
as #20 before #19 landed. Its library diff turned out to be
line-for-line equivalent to #19's, so rather than resolve the conflicts
and re-land a duplicate, #20 is closed and this carries only the part
that was not already covered.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

1 participant