Skip to content

Remove rules that can be replaced with a linter.#792

Open
stevepolitodesign wants to merge 2 commits into
mainfrom
sp-ai-rules
Open

Remove rules that can be replaced with a linter.#792
stevepolitodesign wants to merge 2 commits into
mainfrom
sp-ai-rules

Conversation

@stevepolitodesign
Copy link
Copy Markdown
Contributor

In an effort to follow best practices, we remove items that can be
enforced with a linter.

The spirit of this commit is to keep CLAUDE.md as lean as possible, and
relates to this feedback.

File Removed rule Cop / Tool
controllers.md Actions should not exceed 10 lines Metrics/MethodLength (built-in, default Max: 10)
controllers.md Never params.permit! (kept in security.md) No built-in RuboCop cop. Brakeman flags it; custom cop needed for lint coverage. Rails/StrongParametersExpect targets params.require(...).permit(...)params.expect(...), not permit!.
database.md Migrations must be reversible Rails/ReversibleMigration (rubocop-rails)
database.md Add null: false and database-level defaults where appropriate Partial only. Rails/NotNullColumn flags add_column ..., null: false without a default (a related gotcha, not the same rule). Rails/ThreeStateBooleanColumn enforces null: false on boolean columns specifically. No general cop enforces null: false across all columns.
models.md Long parameter lists (max 3 args) Metrics/ParameterLists (built-in, set Max: 3)
models.md Extract models >200 lines / 15 public / 7 private methods Metrics/ClassLength covers the 200-line threshold only. No built-in cop for public/private method counts — custom cop required.
testing.md One expect per it block RSpec/MultipleExpectations (rubocop-rspec)
testing.md Max 2 levels of context nesting RSpec/NestedGroups (rubocop-rspec, set Max: 2)

In an effort to follow [best practices], we remove items that can be
enforced with a linter.

The spirit of this commit is to keep CLAUDE.md as lean as possible, and
relates to this [feedback].

File | Removed rule | Cop / Tool |
|---|---|---|
| controllers.md | Actions should not exceed 10 lines | `Metrics/MethodLength` (built-in, default `Max: 10`) |
| controllers.md | Never `params.permit!` (kept in security.md) | No built-in RuboCop cop. Brakeman flags it; custom cop needed for lint coverage. `Rails/StrongParametersExpect` targets `params.require(...).permit(...)` → `params.expect(...)`, **not** `permit!`. |
| database.md | Migrations must be reversible | `Rails/ReversibleMigration` (rubocop-rails) |
| database.md | Add `null: false` and database-level defaults where appropriate | Partial only. `Rails/NotNullColumn` flags `add_column ..., null: false` *without* a default (a related gotcha, not the same rule). `Rails/ThreeStateBooleanColumn` enforces `null: false` on boolean columns specifically. No general cop enforces `null: false` across all columns. |
| models.md | Long parameter lists (max 3 args) | `Metrics/ParameterLists` (built-in, set `Max: 3`) |
| models.md | Extract models >200 lines / 15 public / 7 private methods | `Metrics/ClassLength` covers the 200-line threshold only. No built-in cop for public/private method counts — custom cop required. |
| testing.md | One `expect` per `it` block | `RSpec/MultipleExpectations` (rubocop-rspec) |
| testing.md | Max 2 levels of context nesting | `RSpec/NestedGroups` (rubocop-rspec, set `Max: 2`)

[best practices]: https://code.claude.com/docs/en/best-practices#write-an-effective-claude-md
[feedback]: thoughtbot/readysetgo#190 (comment)
Copy link
Copy Markdown
Contributor

@JoelQ JoelQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 to the spirit of this change. IMO, most style and a lot of best practices don't belong in CLAUDE.md

Not sure where we should make additional changes so we don't lose this idea? Should these cops be added to suspenders? Should we document some of these things so we don't lose the preferences?

@stevepolitodesign
Copy link
Copy Markdown
Contributor Author

Not sure where we should make additional changes so we don't lose this idea? Should these cops be added to suspenders?

That's my plan 😎. I want to ship this first, since we're already pulling these rules into Suspenders. Then, I plan on introducing some Rubocop tooling.

@cpytel
Copy link
Copy Markdown
Member

cpytel commented May 14, 2026

What if we have a guide itself advice with agents/AI, etc. that way we can use the language of the guides. ie,

  • Don't put things in CLAUDE.md that can be enforced with a linter. [link to pr]

Copy link
Copy Markdown
Contributor

@jaredlt jaredlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very much in favour of "moving things left", and where we can strictly enforce something with linting we should. I left some comments about some of the more "it depends" rules that I feel might still be useful as hints to the LLMs.

# Controllers

- Controllers handle HTTP only: receive request, delegate to model, return response.
- Actions should not exceed 10 lines (excluding strong params). Longer actions often signal business logic that belongs in a model or PORO.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "should not" here is a hint to not make long actions. As opposed to "must not" which is a hard rule.

I worry about enforcing this one with lint rules because there are always exceptions.

I also wonder if we lose some context for the agent when we remove this, especially the second sentence "Longer actions often signal business logic that belongs in a model or PORO."

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worry about enforcing this one with lint rules because there are always exceptions.

That's actually the perspective I'm taking here, but in this case, the author would be responsible for disabling the rule with a comment.

My thinking is there are always exceptions, which is why RuboCop allows rules to be disabled.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you be comfortable with something like :

Avoid long actions, since they often signal business logic that belongs in a model or PORO.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I like that line.

I'm still quite wary of implementing some of the lint rules for the "it depends" scenarios. For the absolutes I love it. For the 90% of the time, I like it. For some of the things that sit more in the middle I do wonder about how this will go. I feel we should explore some of the lint implementations separately before centralising them in the guides/suspenders.

A small part of me worries that a hard lint rule for an "it depends" situation will provide the wrong feedback loop to an agent and it will go a bit nuts 🙃


- Always use the `rails generate migration` command to create migration files.
- Migrations must be reversible.
- Add `null: false` and database-level defaults where appropriate.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a nice hint to the LLMs to prefer null: false where appropriate because as you mention in your description, there is no hard rule for this. It depends.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaredlt we should be covered by Rails/NotNullColumn.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I should elaborate. I like the idea of having the linter bring this to the attention of the author just to be safe. They can always decide to add a comment disabling it in that migration.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of having the linter bring this to the attention of the author just to be safe. They can always decide to add a comment disabling it in that migration

I agree with this. I do think we need to experiment with what works for the linter. We want it to be right most of the time rather than feel like we're fighting it.

That cop doesn't help remind us that ideally we want to use null: false for most columns. It only triggers if an add_column has null: false without a default as well. So if you just add_column without a null: false it will succeed.

Comment thread rails/ai-rules/rules/models.md
Comment thread rails/ai-rules/rules/models.md Outdated
- Factories: only required attributes with sensible defaults. Start in `spec/factories.rb`.
- Shoulda Matchers for validations and associations.
- WebMock blocks all external HTTP in tests — always stub external requests.
- One `expect` per `it` block. Max 2 levels of context nesting.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with removing this one but I do wonder about enforcing the lint rules. Sometimes an extra expect in the same test tells the story better. Sometimes an extra context wrapped around a test tells the story better.

- Migrations must be reversible.
- Add `null: false` and database-level defaults where appropriate.
- Use `text` over `string` if length varies significantly.
- Wrap multi-record operations in transactions. Use `save!` (bang) inside transactions.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the direction to use save! also be removed? Rubocop should flag use of .save or .update anywhere, whether or not in a transaction:

Suggested change
- Wrap multi-record operations in transactions. Use `save!` (bang) inside transactions.
- Wrap multi-record operations in transactions.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know enough about RuboCop here but aren't there valid reasons why you wouldn't want to use the bang version of these methods? If we enforce that via lint then do we need to add exclusions everywhere we don't want them?

Copy link
Copy Markdown
Contributor

@louis-antonopoulos louis-antonopoulos May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are valid reasons why you wouldn't want to use the bang version of a method, and the one I've run across the most is because you're calling some library that does not have a bang method defined for the thing you need to call.

For this PR, removing the guidance from CLAUDE.md simplifies the LLM's task and pushes it to the linter and the human, in which case you add an exception if necessary.

Copy link
Copy Markdown
Contributor

@jaredlt jaredlt May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surfacing errors is a very common case for non-bang save/update methods. I'm worried about us scattering our codebases with exceptions for this very common case. This goes to my point about wanting to ensure we don't fee like we're fighting the linter

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things:

  1. I might be misunderstanding here -- since bang methods raise, shouldn't we encourage those in the linter to surface errors?

  2. Using save! in a transaction in a transaction is something that should be happening across the board, because a save returning false will not cause the transaction to be rolled back, leading to data integrity issues. This should be set at the linting level to make sure humans and LLMs don't do something like forget a bang method, and end up reducing one bank account balance during a funds transfer but failing to increment the recipient's balance because part of the transaction returned false instead of raising.

The goal is to enforce rules at an automatic level and let a human decide about exceptions, not rely on an agent to remember a hint for something that could be catastrophic.

So the change here is don't give hints to the agent where an automatic tool will enforce things. Give hints to the agent where the automatic tool cannot guide it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we have LLMs the cost of creating fancier lint rules has gone down a lot. I could imagine a lint rule that requires save! inside transactions or even one that requires save! if you aren't using the return value.

Copy link
Copy Markdown
Contributor

@louis-antonopoulos louis-antonopoulos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked through all the full files in the PR and only found one tiny additional opportunity for reduction.

Looks good!

@laicuRoot
Copy link
Copy Markdown
Contributor

I share the same comments from @jaredlt. I think some of the rules are very useful for the LLM to generate better code in the very first prompt without having to rectify it later with another prompt or run the linter, especially in "it depends" situations like Longer actions often signal business logic that belongs in a model or PORO..

I think this type of rule or hint is quite useful. Or at least I have found that it enforces this a lot thanks to the rule.

@louis-antonopoulos
Copy link
Copy Markdown
Contributor

@jaredlt and @laicuRoot reading through the comments, I feel like they might be probing the wrong thing for what @stevepolitodesign's PR is working to achieve:

  • Anthropic guides that the more you include in your CLAUDE.md file, the more likely it is to ignore the important things.
  • Telling an agent How to Lint is Waste (😉 lol @jaredlt): you can't possibly tell it everything, and even the smallest suggestion is unnecessary when you can instead tell the agent to run bundle exec rubocop -a before it thinks a commit is ready to ship (or add a pre-commit hook to do so)
  • If your particular linting configuration is confusing to the agent, then you either explain the tricky parts to the agent in CLAUDE.md, or you adjust your linting configuration.

@jaredlt
Copy link
Copy Markdown
Contributor

jaredlt commented May 19, 2026

@louis-antonopoulos I think we're largely in agreement.

Where a lint rule can be applied 100% of the time (no nuance, no "it depends") > defer to lint instead of CLAUDE.md 👍 (on this I think we all agree)

Where a lint rule cannot be applied 100% (is nuance, "it depends") > these are the rules I feel more concerned about. We are attempting to use lint rules here to nudge us towards best practices and better architecture (and in this I generally agree). But it seems we are deferring these to the linter and I am worried about 2 things here:

  • We potentially lose out on giving a hint to the LLM so that they could do the right thing before we even get to the linter (I understand we should limit CLAUDE's rules but they are really not very big yet)
  • We may end up fighting the linter more than it's useful and the linter feedback might give the wrong feedback to the agent

@louis-antonopoulos
Copy link
Copy Markdown
Contributor

@jaredlt Here's how I'm approaching this:

  1. The goal of this file is to improve generated code quality while still requiring human review as the final stopgap. The goal is to reduce developer frustration in what an agent generates. To that end, less is absolutely more.

  2. My experience is that agents don't get stuck when the linter says "do X". Agents get stuck when rules like CLAUDE.md say "do X" and the linter (or some other process) says "do Y".

  3. This file isn't the end game. It's a starting point. On each person's project, they see where the gaps are and add to this file as the project proceeds.

I understand we should limit CLAUDE's rules but they are really not very big yet

This is the crux of the issue, right here. The only way to keep the size manageable is to fight as hard as you can to limit what goes in. Over time, people add, but very few people remove, because everyone thinks "this instruction must have been useful, so I shouldn't take it out."

You can easily see when the absence of an instruction is causing the agent to struggle. But you cannot easily see when the totality of additional instructions are causing other things to get missed or ignored or loop 4 extra times during a prompt request.

The goal should be to automate what can be automated deterministically. The rest is what goes into CLAUDE.md.

@stevepolitodesign
Copy link
Copy Markdown
Contributor Author

@jaredlt @laicuRoot given that you both introduced the original rules, and given the fact that you are actually using these rules in real projects, are there any changes in this PR that you feel are blockers? I'm open to restoring them given that this PR is largely experimental, and is meant to only be a guide.

I can always update Suspenders independently.

@laicuRoot
Copy link
Copy Markdown
Contributor

@jaredlt @laicuRoot given that you both introduced the original rules, and given the fact that you are actually using these rules in real projects, are there any changes in this PR that you feel are blockers? I'm open to restoring them given that this PR is largely experimental, and is meant to only be a guide.

I can always update Suspenders independently.

Hey @stevepolitodesign, I'm on board with the changes you're proposing and support making CLAUDE.md and its rules as lean as possible. My only concern is around the points @jaredlt raised above, specifically the rules that fall more into "it depends" territory.

We can go ahead with the changes, experiment and if we feel that any rule needs to be restored we can open a PR.

Thank you for working on this 👍

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.

6 participants