-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: define thoughtbot rules for AI-enabled IDEs #764
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
rails/ai-rules/rails-rules.md
Outdated
Key Conventions | ||
- Follow RESTful routing conventions. | ||
- Use concerns for shared behavior across models or controllers. | ||
- Implement service objects for complex business logic. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure about this for example. Lately I've been more leaning towards Service objects are poorly-named models.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥 This is the controversial one! My personal opinion is that service objects are an anti (or at least overused) pattern and should only be a fallback when other patterns don't fit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also have veered towards treating service objects as an anti-pattern in many situations. I even have a note with giant list of what other design patterns or common constructs a service object frequently is trying to become.
@laicuRoot thanks for starting this! I'm curious if you added each line as a consequence of the AI performing poorly on something? Or did you add things from our existing guides that seemed useful? Some of these things eg. naming conventions I haven't had any trouble with (I'm using Claude Code). I think it would be good if we can avoid adding things that don't improve the situation so we have a list that is maintainable. Based on my Claude Code usage and observing when it doesn't perform so well I've added the following to my
Some things here, mostly around the commit message guidance is too personal / prescriptive and don't think we'd want that in shared rules. |
rails/ai-rules/rails-rules.md
Outdated
- Use ActiveRecord effectively for database operations. | ||
|
||
Syntax and Formatting | ||
- Follow the Ruby Style Guide (https://rubystyle.guide/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would opt to solve this with a hook (or similar) that runs Standard at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know about hooks 🤔 thank you for sharing this. I guess we can include hooks within the guides?
rails/ai-rules/rails-rules.md
Outdated
Syntax and Formatting | ||
- Follow the Ruby Style Guide (https://rubystyle.guide/) | ||
- Use Ruby's expressive syntax (e.g., unless, ||=, &.) | ||
- Prefer double quotes for strings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've definitely seen this. Again I wonder about something that runs Standard at the end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Syntax feels like a really low-level thing to include in a rules file. 👍 to having it respond to a linter rather than codifying here. We can either deterministically run the linter via something like a hook, or we can include an instruction like "ALWAYS run standard
after making code changes".
AI agents are really good at running a tool, seeing some failures, and then fixing those failures.
rails/ai-rules/rails-rules.md
Outdated
|
||
Testing | ||
- Write comprehensive tests using RSpec. | ||
- Follow TDD/BDD practices. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I haven't experimented with yet is instructing it to follow TDD, but it's on my list. Does it follow this process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not follow the process in terms of suggesting the test first and then the code but, at least, it includes tests for every single class or change in the logic that is adding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have gotten LLMs to do true red/green/refactor TDD. This has been with one-off agent sessions in my editor (both Claude and GPT) with an initial prompt rather than extracting to a rule though.
Good question @jaredlt. I’ve included items from existing guides that I thought would be helpful for the AI to take into account. I also added sections like the one on testing after noticing that the AI wasn’t suggesting tests. I’ve been using this setup since I subscribed to Cursor about two months ago. |
Yes, I completely agree. I like yours actually 👍 short and to the point. |
@rdnewmanbot we were just talking about this yesterday! |
rails/ai-rules/rails-rules.md
Outdated
- Optimize database queries using includes, joins, or select. | ||
|
||
Key Conventions | ||
- Follow RESTful routing conventions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we specify that this means sticking to the 7 restful actions: index, show, new, create, edit, update, delete? I'm not sure what the ideal level of detail is in a rules file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is a good idea to be detailed about this. I also found that if you add a link to a blog post or source that explains the rule, it considers the content of the blog post too.
rails/ai-rules/rails-rules.md
Outdated
Testing | ||
- Write comprehensive tests using RSpec. | ||
- Follow TDD/BDD practices. | ||
- Use factories (FactoryBot) for test data generation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth suggesting that test setup should be local? Tell the AI to optimize for readability and changeability? Otherwise I find that LLMs love to create deeply nested contexts with let
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be worth addressing let
statements and preferring an approach of keeping test cases self-contained rather than keeping the spec file DRY.
@jaredlt if you're using Claude Code, there's a setting to have it not add the co-authored by line to your commit:
|
This may be a bit controversial. There is evidence that AI models tend to perform better if threatened.
|
I’ve pushed an update that keeps only the suggestions and rules based on my experience using Cursor. I like @jaredlt approach of keeping it concise, focusing just on instructions to correct behaviours we don’t want when using the AI as a pair programming partner 🙂 |
3f6a363
to
b5ef9b8
Compare
I saw this recently shared in an official vscode update. https://gist.github.com/burkeholland/a232b706994aa2f4b2ddd3d97b11f9a7 I don't think this is what we want but it is interesting to see how people are iterating and creating versions of these instructions. Maybe we can version this script and experiment with it. |
Do we feel this is good enough to merge? |
@laicuRoot I will experiment using these rules for a bit. One thing I noticed is that I don't know how well it works to have links in there and assume the model will look them up. When I asked the model if it will look up those links it said:
|
I love the idea of defining thoughtbot rules! It would be neat if we could somehow just generate rules based on our existing guides that auto-update when those change, maybe we could use AI to generate rules based off of them 😆 |
@samithoughtbot Claude Code claims it will use links:
This has me wondering: how different are best practices in instructions for different models/agents? 🤔 |
@JoelQ that's a good point, I wonder if it will be less brittle to not include links in the instructions but then you have the downside of having to update the instructions. I believe this concern is less about the model itself and more about the function calling it is given access to. These guides will only work for models that have access to internet browsing functionality. Another small, but valid downside, is they require the user to be connected to the internet. |
How is this mean to be used? Are you expected to copy-paste these into your rule file of choice? |
rails/ai-rules/rails-rules.md
Outdated
|
||
- Use factories (FactoryBot) for test data generation (https://thoughtbot.github.io/factory_bot/) | ||
- Always write tests to cover new code generated | ||
- Prefer RSpec for Rails applications but use the existing test framework if there is one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this is too generic? My understanding is that AI instruction files benefit from being super specific to the project it's running. Have we been using this instruction successfully on a variety of projects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree these are generic but I think the intention is for them to be a starting point for getting up and running with instructions. These rules make sense across all our rails projects and then you can add to them in your own project with more specific details. Do you think that is useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've started saying explicitly Use RSpec
for projects that use RSpec. Given that's a thoughtbot default maybe we say that here as well. And then folks can change it if the application uses something else eg. minitest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that makes sense. I've modified the file if you want to have a look 🙂
I believe so. Is there another way? |
I was experimenting with claude code today for the first time and it does have a handy I see this guide as being useful when setting up a project for the first time or using as project-agnostic instructions. For example in
This simply references that file stored on your local machine. You could then add that line to the instructions of each rails project. Helpful tip: To see what files are included in a claude prompt you can type |
b5ef9b8
to
1cd2593
Compare
Hey folks! I've made some changes in this PR. Following some of the comments, I've kept it simple and short so we can have this as initial point for anyone who is starting using Cursor/Claude or any other tool that accepts rules files. If you feel strongly against any of the points, please comment and we can change it. Otherwise, I will merge this next week. Thank you so much 🙏 |
Add thoughtbot Rails AI development rules Create AI rules for AI-assisted Rails development.
1cd2593
to
437f52c
Compare
Yes, that's the idea. Just as starting point / template for Rails applications to get things rolling. |
rails/ai-rules/rails-rules.md
Outdated
|
||
- Always check for N+1 queries when rendering collections | ||
- Prefer includes for eager loading | ||
- Scope queries to only the fields needed with select |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that it will add .select
to all queries? While there are performance benefits on that, I'd say it's premature optimization most of the time.
|
||
## Testing | ||
|
||
- Always write tests to cover new code generated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we tell it to write tests first? I think @JoelQ had more success with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have struggled with this one personally when working with Cursor. Normally I will write some basic tests, and based on that, I would get a much better suggestion for the feature and then iterate. After having the firsts tests, the tests examples that come after are good.
@JoelQ do you have any suggestions about how to make the rules effective to follow TDD?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've usually done that with an initial prompt. Zed's agent (using Claude) does this well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a good start.
I wonder what would it happen if we feed an LLM with the whole guides repo and ask it to generate the rails-rules.
@MatheusRich good question! LLM Coding Guide: Ruby/Rails Best PracticesThis guide provides coding standards and best practices for Ruby, Rails, RSpec testing, and object-oriented design. Follow these guidelines when generating or reviewing code. Object-Oriented Design PrinciplesCore Principles
Ruby Style GuidelinesGeneral Style
Method Definitions# Use def with parentheses when there are arguments
def method_with_arguments(arg_one, arg_two)
# method body
end
# Use def self.method for class methods
def self.class_method
# method body
end
# Avoid bang (!) method names - prefer descriptive names
def process_data # instead of process_data!
# method body
end
# Use ? suffix for predicate methods
def active?
# predicate logic
end Class Structureclass SomeClass
# Order: constants, macros, public methods, private methods
SOME_CONSTANT = "value"
# Class methods above instance methods
def self.class_method
# body
end
def initialize(attributes)
@attribute = attributes[:attribute]
end
def public_method
# body
end
private
def private_method
# body
end
end Conditionals# Limit conditional modifiers to short, simple cases
do_later if async?
# Use multi-line form for complex conditions
if signed_in? && !current_user.active?
block_access!
end
# Avoid ternary operators - use multi-line if instead
if condition
positive_result
else
negative_result
end Collections and Blocks# Prefer specific enumerable methods
items.detect { |item| item.condition? } # not find
items.select { |item| item.valid? } # not find_all
items.map(&:attribute) # not collect
items.reduce(:+) # not inject
# Use &:method_name for simple method calls
items.map(&:name)
# Empty lines around multi-line blocks
some_method_before
items.each do |item|
process_item(item)
perform_action(item)
end
some_method_after Variables and Naming# Prefix unused variables with underscore
def method(_unused_param, used_param)
process(used_param)
end
# Factory naming conventions
user_factory = build(:user) # factory creates user
user = user_factory.create # variable from factory
# Memoization with leading underscore
def expensive_calculation
@_expensive_calculation ||= complex_computation
end Rails Specific GuidelinesApplication Structure
Routes# Use :only option to explicitly state exposed routes
resources :posts, only: [:index, :show, :create]
# Avoid :except option and member/collection routes
# Order resourceful routes alphabetically
resources :articles
resources :comments
resources :posts
# Use _url for mailer views and redirects, _path everywhere else
redirect_to post_url(post) # in controller redirects
link_to "Post", post_path(post) # in views Controllersclass PostsController < ApplicationController
# Order: filters, public methods, private methods
before_action :authenticate_user!
def show
# Avoid instantiating more than one object
@post = Post.find(params[:id])
end
private # Use private, not protected
def post_params
params.require(:post).permit(:title, :content)
end
end Modelsclass Post < ApplicationRecord
# Order: constants, associations (alphabetically), validations (alphabetically), methods
# Associations by type, then alphabetically
belongs_to :author, class_name: "User"
belongs_to :category
has_many :comments
has_many :tags
# Validations alphabetically by attribute
validates :content, presence: true
validates :title, presence: true, uniqueness: true
# Use def self.method, not scope
def self.published
where(published: true)
end
# Use touch: true for belongs_to relationships
belongs_to :user, touch: true
end Database and Migrationsclass CreatePosts < ActiveRecord::Migration[7.0]
def change
create_table :posts do |t|
t.timestamps null: false
t.string :title, null: false
t.text :content, null: false, default: "" # Empty string default for non-required text
t.references :user, null: false
end
add_index :posts, :title
add_foreign_key :posts, :users, on_delete: :restrict # Explicit on_delete behavior
end
end Naming Conventions
Temporal Code# Prefer zone-aware methods
Time.current # not Time.now
Date.current # not Date.today
Time.zone.parse("2024-07-04 16:05:37") # not Time.parse RSpec Testing GuidelinesGeneral Testing Principles
Spec Structuredescribe PostPolicy do
describe "#allowed?" do
context "when user has access" do
it "returns true" do
# Setup
policy = build_policy(user_id: 1, post_owner_id: 1)
# Exercise & Verify
expect(policy).to be_allowed
end
end
context "when user lacks access" do
it "returns false" do
policy = build_policy(user_id: 1, post_owner_id: 2)
expect(policy).not_to be_allowed
end
end
end
private
def build_policy(user_id:, post_owner_id:)
user = instance_double("User", id: user_id)
post = instance_double("Post", user_id: post_owner_id)
PostPolicy.new(user, post)
end
end System/Acceptance Tests# spec/system/user_signs_up_spec.rb
describe "User signs up" do
it "creates account with valid details" do
visit sign_up_path
within_fieldset "Sign up" do
fill_in "Email", with: "[email protected]"
fill_in "Password", with: "SecurePass123"
click_button "Sign up"
end
expect(page).to have_button("Sign out")
end
end Test Organization
FactoriesFactoryBot.define do
factory :post do
title { "Sample Post" }
content { "Post content here" }
published_at { 1.week.from_now } # Use blocks for date/time attributes
trait :published do
published_at { 1.day.ago }
end
end
end Code Review Focus AreasRails Specific
General Quality
Key Tools and ConfigurationRubocop Configuration
Bundler Best Practices# Gemfile
ruby "3.2.0" # Specify Ruby version
gem "rails", "7.0.4" # Exact version for fragile gems
gem "rspec-rails", "~> 6.0" # Pessimistic for semantic versioning
gem "pg" # Versionless for safe-to-update gems This guide represents battle-tested practices for maintainable, readable, and robust Ruby/Rails applications. Apply these principles consistently to produce high-quality code that follows community standards. I wonder if perhaps that is the recommendation here? We recommend feeding these guides into an LLM? @laicuRoot what do you think? |
One other thing I noticed is that instructions that are longer use more tokens! |
I think we could start small as this PR proposes and then iterate considering the information you have shared of our guides digested by LLMs. How do you feel about that @samithoughtbot ? Do you think is a good starting point for this? |
@laicuRoot ye I think that is a good idea! I also noticed that my LLM used |
Has anyone tested these? It feels like we're just throwing random stuff in a PR. Normally things go into the guides after after we've experimented with them and come up with an agreed-upon best practice. I'd love to get some actual experience reports for rules. Some things we should probably capture along with the experience:
|
FWIW, I've been using Zed's agent (backed by Claude) on an existing Rails codebase and it does most of the things in this file as of cf84b18 out of the box:
If I prompt for it, I can even get it to TDD. The prompt is pretty minimal, usually just one or two lines like "We are going to build a feature with TDD", "We are going to build a feature using a test-first approach", or "We are going to be using a TDD workflow, red-green-refactor" |
cf84b18
to
e86908e
Compare
Some I have on my system prompts is to write tests without |
I’ve been using this set of rules with Cursor and Claude for a while now in an existing project. One place where they’re particularly useful is when generating tests: if the rules aren’t attached, or if I don’t explicitly state in the prompt how the tests should be written, the output often includes let or before blocks by default. Similar case when creating new routes, based in my experience, unless rules are attached, it does not follow the RESTful Rails way. This I've seen it in existing and new projects. In personal projects I've found very helpful to indicate that while dealing with views always consider using helpers and partials, otherwise I've seen that sometimes it would throw everything in the view. This morning for example, in a breakable toy, it prepared data within the view, instead of the Controller. Since I set the rules to It would be good to hear other people experience. I agree that it would be good to have more reports using this set of rules. |
During a Fusion AI task force meeting, we discussed the tools we use and how we enforce rules. As part of that conversation, we decided it would be valuable to define official thoughtbot guidelines for editors that integrate AI, such as Cursor.
This PR opens the discussion by proposing an initial set of rules. This guide is a starting point for how we write and structure Rails apps at thoughtbot. It’s intended as a starting point to ensure consistency, readability, and shared best practices across our codebases. The guide is a living document it.