rails-audit-thoughtbot

# Rails Audit Skill (thoughtbot Best Practices)

Perform comprehensive Ruby on Rails application audits based on thoughtbot's Ruby Science and Testing Rails best practices, with emphasis on Plain Old Ruby Objects (POROs) over Service Objects.

## Audit Scope

The audit can be run in two modes:
1. **Full Application Audit**: Analyze entire Rails application
2. **Targeted Audit**: Analyze specific files or directories

## Execution Flow

### Step 1: Determine Audit Scope

Ask user or infer from request:
- Full audit: Analyze all of `app/`, `spec/` or `test/`, `config/`, `db/`, `lib/`
- Targeted audit: Analyze specified paths only

### Step 2: Collect Test Coverage Data (Optional)

**Before doing anything else in this step**, use `AskUserQuestion` to ask the user:
- **Question**: "Would you like to collect actual test coverage data using SimpleCov? This will temporarily set up SimpleCov (if not already present), run the test suite, and capture real coverage metrics."
- **Options**: "Yes, collect coverage (Recommended)" / "No, use estimation"

**If the user declines**: skip the rest of this step entirely. Use estimation mode in Steps 4 and 5. Do NOT spawn the subagent.

**If the user accepts**: use the **Task tool** to spawn a `general-purpose` subagent with this prompt:

> Read the file `agents/simplecov_agent.md` and follow all steps described in it. The audit scope is: {{SCOPE from Step 1}}. Return the coverage data in the output format specified in that file.

**After the agent finishes**, run `rm -rf coverage/` to ensure the coverage directory is removed even if the agent failed to clean up.

**Interpreting the agent's response:**
- If the response starts with `COVERAGE_FAILED`: no coverage data — use estimation mode in Steps 4 and 5. Note the failure reason in the report.
- If the response starts with `COVERAGE_DATA`: parse the structured data and keep it in context for Steps 4 and 5. The data includes overall coverage, per-directory breakdowns, lowest-coverage files, and zero-coverage files.

### Step 2b: Collect Code Quality Metrics (Optional)

**Before doing anything else in this step**, use `AskUserQuestion` to ask the user:
- **Question**: "Would you like to run RubyCritic for code quality metrics (complexity, duplication, code smells)? This will temporarily set up RubyCritic (if not already present) and analyze your codebase."
- **Options**: "Yes, run RubyCritic (Recommended)" / "No, skip static analysis"

**If the user declines**: skip the rest of this step entirely. Do NOT spawn the subagent.

**If the user accepts**: use the **Task tool** to spawn a `general-purpose` subagent with this prompt:

> Read the file `agents/rubycritic_agent.md` and follow all steps described in it. The audit scope is: {{SCOPE from Step 1}}. Return the code quality data in the output format specified in that file.

**After the agent finishes**, run `rm -rf tmp/rubycritic/` to ensure the output directory is removed even if the agent failed to clean up.

**Interpreting the agent's response:**
- If the response starts with `RUBYCRITIC_FAILED`: no code quality data — note the failure reason in the report.
- If the response starts with `RUBYCRITIC_DATA`: parse the structured data and keep it in context for Steps 4 and 5. The data includes overall score, per-directory ratings, worst-rated files, top smells, and most complex files.

### Step 3: Load Reference Materials

Before analyzing, read the relevant reference files:
- `references/code_smells.md` - Code smell patterns to identify
- `references/testing_guidelines.md` - Testing best practices
- `references/poro_patterns.md` - PORO and ActiveModel patterns
- `references/security_checklist.md` - Security vulnerability patterns
- `references/rails_antipatterns.md` - Rails-specific antipatterns (external services, migrations, performance)

### Step 4: Analyze Code by Category

Analyze in this order:

1. **Testing Coverage & Quality**
   - If SimpleCov data was collected in Step 2, use actual coverage percentages instead of estimates
   - Cross-reference per-file SimpleCov data: files with 0% coverage = "missing tests"
   - Check for missing test files
   - Identify untested public methods
   - Review test structure (Four Phase Test)
   - Check for testing antipatterns

2. **Security Vulnerabilities**
   - SQL injection risks
   - Mass assignment vulnerabilities
   - XSS vulnerabilities
   - Authentication/authorization issues
   - Sensitive data exposure

3. **Models & Database**
   - Fat model detection
   - Missing validations
   - N+1 query risks
   - Callback complexity
   - Law of Demeter violations (voyeuristic models)
   - If RubyCritic data was collected, flag models with D/F ratings or high complexity

4. **Controllers**
   - Fat controller detection
   - Business logic in controllers
   - Missing strong parameters
   - Response handling
   - Monolithic controllers (non-RESTful actions, > 7 actions)
   - Bloated sessions (storing objects instead of IDs)
   - If RubyCritic data was collected, flag controllers with D/F ratings or high complexity

5. **Code Design & Architecture**
   - Service Objects → recommend PORO refactoring
   - Large classes
   - Long methods
   - Feature envy
   - Law of Demeter violations
   - Single Responsibility violations
   - If RubyCritic data was collected, cross-reference D/F rated files and high-complexity files with manual code review findings

6. **Views & Presenters**
   - Logic in views (PHPitis)
   - Missing partials for DRY
   - Helper complexity
   - Query logic in views

7. **External Services & Error Handling**
   - Fire and forget (missing exception handling for HTTP calls)
   - Sluggish services (missing timeouts, synchronous calls that should be backgrounded)
   - Bare rescue statements
   - Silent failures (save without checking return value)

8. **Database & Migrations**
   - Messy migrations (model references, missing down methods)
   - Missing indexes on foreign keys, polymorphic associations, uniqueness validations
   - Performance antipatterns (Ruby iteration vs SQL queries)
   - Bulk operations without transactions

### Step 5: Generate Audit Report

Create `RAILS_AUDIT_REPORT.md` in project root with structure defined in `references/report_template.md`.

When SimpleCov coverage data was collected in Step 2, use the **SimpleCov variant** of the Testing section in the report template. When coverage data is not available, use the **estimation variant**.

When RubyCritic data was collected in Step 2b, include the **Code Quality Metrics** section in the report using the RubyCritic variant from the report template. When RubyCritic data is not available, use the **not available variant**.

## Severity Definitions

- **Critical**: Security vulnerabilities, data loss risks, production-breaking issues
- **High**: Performance issues, missing tests for critical paths, major code smells
- **Medium**: Code smells, convention violations, maintainability concerns
- **Low**: Style issues, minor improvements, suggestions

## Key Detection Patterns

### Service Object → PORO Refactoring

When you find classes in `app/services/`:
- Classes named `*Service`, `*Manager`, `*Handler`
- Classes with only `.call` or `.perform` methods
- Recommend: Rename to domain nouns, include `ActiveModel::Model`

### Fat Model Detection

Models with:
- More than 200 lines
- More than 15 public methods
- Multiple unrelated responsibilities
- Recommend: Extract to POROs using composition

### Fat Controller Detection

Controllers with:
- Actions over 15 lines
- Business logic (not request/response handling)
- Multiple instance variable assignments
- Recommend: Extract to form objects or domain models

### Missing Test Detection

For each Ruby file in `app/`:
- Check for corresponding `_spec.rb` or `_test.rb`
- Check for tested public methods
- Report untested files and methods

## Analysis Commands

Use these bash patterns for file discovery:

```bash
# Find all Ruby files by type
find app/models -name "*.rb" -type f
find app/controllers -name "*.rb" -type f
find app/services -name "*.rb" -type f 2>/dev/null

# Find test files
find spec -name "*_spec.rb" -type f 2>/dev/null
find test -name "*_test.rb" -type f 2>/dev/null

# Count lines per file
wc -l app/models/*.rb

# Find long files (over 200 lines)
find app -name "*.rb" -exec wc -l {} + | awk '$1 > 200'
```

## Report Output

Always save the audit report to `/mnt/user-data/outputs/RAILS_AUDIT_REPORT.md` and present it to the user.

# Lines starting with '#' are comments.
# Each line is a file pattern followed by one or more owners.

# More details are here: https://help.github.com/articles/about-codeowners/

# The '*' pattern is global owners.

# Order is important. The last matching pattern has the most precedence.
# The folders are ordered as follows:

# In each subsection folders are ordered first by depth, then alphabetically.
# This should make it easy to add new rules without breaking existing ones.

# Global rule:
* @laicuRoot

# Rails Audit Skill (thoughtbot Best Practices)

A [Claude Code][claude-code] skill that performs comprehensive code audits of
Ruby on Rails applications based on [thoughtbot's][thoughtbot] Ruby Science and
Testing Rails best practices.

[claude-code]: https://docs.anthropic.com/en/docs/claude-code
[thoughtbot]: https://thoughtbot.com

## Quick links

- **[Ruby Science][ruby-science]** - thoughtbot's guide to fixing code smells
- **[Testing Rails][testing-rails]** - thoughtbot's guide to testing Rails applications
- **[Rails Antipatterns][rails-antipatterns]** - Best practices for Ruby on 
  Rails refactoring (Chad Pytel & Tammer Saleh)

[ruby-science]: https://github.com/thoughtbot/ruby-science
[testing-rails]: https://github.com/thoughtbot/testing-rails
[rails-antipatterns]: https://www.informit.com/store/rails-antipatterns-best-practice-ruby-on-rails-refactoring-9780321604811

## Table of contents

- [Overview](#overview)
- [Installation](#installation)
- [Usage](#usage)
  - [Full application audit](#full-application-audit)
  - [Targeted audit](#targeted-audit)
- [Optional data collection](#optional-data-collection)
  - [SimpleCov (test coverage)](#simplecov-test-coverage)
  - [RubyCritic (code quality)](#rubycritic-code-quality)
- [Reference materials](#reference-materials)
- [Contributing](#contributing)
- [License](#license)
- [About thoughtbot](#about-thoughtbot)

## Overview

This skill analyses Rails applications and generates detailed audit reports
covering:

- Testing practices (RSpec)
- Test coverage via [SimpleCov](#optional-data-collection) (optional)
- Code quality metrics via [RubyCritic](#optional-data-collection) (optional)
- Security vulnerabilities
- Code design (skinny controllers, domain models, POROs with ActiveModel)
- Rails conventions
- Database optimisation (missing indexes, migrations hygiene)
- External services (timeouts, error handling, background jobs)
- Performance antipatterns (Ruby vs SQL, silent failures)
- Ruby best practices

## Installation

Copy the skill directory to your Claude Code skills folder:

```bash
cp -r rails-audit-thoughtbot ~/.claude/skills/
```

Or clone directly:

```bash
git clone https://github.com/thoughtbot/rails-audit-thoughtbot ~/.claude/skills/rails-audit-thoughtbot
```

## Usage

If you are in your terminal and not in a Claude session, you can invoke the 
skill directly by using the below. You need to be in the root directory of your 
Rails project.

### Full application audit

```
claude audit
```

If you are in a Claude session, you can reference the skill directly:

```
/rails-audit-thoughtbot
```

### Targeted audit

In a Claude session you can also run targeted audits:

```
/rails-audit-thoughtbot audit controllers
```

This focuses the audit on specific files or directories.

## Optional data collection

During the audit, the skill offers to run optional data-collection steps that
enrich the report with tool-measured metrics. Each step is opt-in — you will be
prompted before anything is installed or executed. If the tool is already in your
Gemfile, the skill uses it directly without modifying your project.

### SimpleCov (test coverage)

Runs your test suite with [SimpleCov](https://github.com/simplecov-ruby/simplecov)
to capture actual line and branch coverage percentages. The report will include
per-directory coverage breakdowns, lowest-coverage files, and zero-coverage files.

- Temporarily adds `simplecov` to the Gemfile (if not already present)
- Runs the full test suite (RSpec or Minitest)
- Restores the original Gemfile after collection
- Cleans up all generated coverage files

### RubyCritic (code quality)

Runs [RubyCritic](https://github.com/whitesmith/rubycritic) to measure code
complexity, duplication, and code smells using Reek, Flay, and Flog. The report
will include per-file ratings (A-F), worst-rated files, most common smells, and
most complex files.

- Temporarily adds `rubycritic` to the Gemfile (if not already present)
- Analyzes `app/` and `lib/` (or targeted paths)
- Restores the original Gemfile after collection
- Cleans up all generated report files

## Reference materials

The skill includes reference documentation based on thoughtbot best practices.
All the materials are compacted information from the books mentioned above.

Recommendations of PORO objects are based on different thoughtbot sources and
[Service objects are poorly-named models][service-objects-poro].

[service-objects-poro]: https://dimiterpetrov.com/blog/service-objects-are-poorly-named-models/

| File | Description |
|------|-------------|
| `references/code_smells.md` | Code smell patterns to identify (Ruby Science) |
| `references/testing_guidelines.md` | Testing best practices (Testing Rails) |
| `references/poro_patterns.md` | PORO and ActiveModel patterns |
| `references/security_checklist.md` | Security vulnerability checklist |
| `references/rails_antipatterns.md` | Rails-specific antipatterns: external services, migrations, performance |
| `references/report_template.md` | Audit report structure template |
| `agents/simplecov_agent.md` | Subagent for SimpleCov test coverage collection |
| `agents/rubycritic_agent.md` | Subagent for RubyCritic code quality metrics |

## Contributing

Contributions are welcome! If you'd like to improve the audit patterns or add
new detection rules:

1. Fork the repository
2. Create your feature branch (`git checkout -b my-new-feature`)
3. Commit your changes (`git commit -am 'Add some feature'`)
4. Push to the branch (`git push origin my-new-feature`)
5. Create a new Pull Request

## License

This skill is open source and available under the [MIT License](LICENSE).

## About thoughtbot

![thoughtbot](https://thoughtbot.com/thoughtbot-logo-for-readmes.svg)

This skill is inspired by and based on thoughtbot's excellent guides:

- [Ruby Science](https://github.com/thoughtbot/ruby-science)
- [Testing Rails](https://github.com/thoughtbot/testing-rails)
- [Rails Antipatterns](https://www.informit.com/store/rails-antipatterns-best-practice-ruby-on-rails-refactoring-9780321604811) by Chad Pytel & Tammer Saleh

The names and logos for thoughtbot are trademarks of thoughtbot, inc.

We love open source software!
See [thoughtbot's other projects][community].

[community]: https://thoughtbot.com/community?utm_source=github

# Code Smells Reference (Ruby Science)

## Detection Checklist

For each code smell, identify the pattern, assess severity, and recommend a solution.

---

## 1. Long Method

**Pattern**: Methods that are difficult to understand at a glance.

**Detection**:
- Methods with more than 10 lines of code
- More than one level of nesting
- More than one level of abstraction
- flog score of 10 or higher

**Severity**: Medium

**Solutions**:
- Extract Method
- Replace Temp with Query

**Example Issue**:
```ruby
# Bad: Long method with multiple responsibilities
def create
  @survey = Survey.find(params[:survey_id])
  @submittable_type = params[:submittable_type_id]
  question_params = params.require(:question).permit(:title, :options)
  @question = @survey.questions.new(question_params)
  @question.submittable_type = @submittable_type
  if @question.save
    redirect_to @survey
  else
    render :new
  end
end
```

---

## 2. Large Class

**Pattern**: Classes with too many responsibilities.

**Detection**:
- Can't describe class in one sentence
- More than 200 lines
- More than 15 public methods
- More than 7 private methods
- flog score of 50 or higher
- Changes for multiple unrelated reasons

**Severity**: High

**Solutions**:
- Extract Class
- Extract Value Object
- Extract Decorator
- Move Method
- Replace Subclasses with Strategies

---

## 3. Feature Envy

**Pattern**: Method that uses another object's data more than its own.

**Detection**:
- Repeated references to same external object
- Parameters used more than instance variables
- Methods with another class name in their name (e.g., `invite_user`)
- Law of Demeter violations

**Severity**: Medium

**Solutions**:
- Move Method
- Extract Method then Move Method

**Example Issue**:
```ruby
# Bad: Feature envy - using answer more than self
def score
  answers.inject(0) do |result, answer|
    question = answer.question
    result + question.score(answer.text)
  end
end
```

---

## 4. Case Statement / Type Code

**Pattern**: Conditional logic based on object type.

**Detection**:
- `case` statements checking class or type code
- Multiple `if/elsif` checking same condition
- `when` clauses that could be polymorphic methods

**Severity**: High (causes shotgun surgery)

**Solutions**:
- Replace Conditional with Polymorphism
- Replace Type Code with Subclasses
- Use Convention over Configuration

**Example Issue**:
```ruby
# Bad: Case statement on type
def summary
  case question_type
  when 'MultipleChoice'
    summarize_multiple_choice_answers
  when 'Open'
    summarize_open_answers
  when 'Scale'
    summarize_scale_answers
  end
end
```

---

## 5. Shotgun Surgery

**Pattern**: Single change requires edits across multiple files.

**Detection**:
- Same small change needed in multiple places
- Duplicated case statements
- Duplicated type checks

**Severity**: High

**Solutions**:
- Replace Conditional with Polymorphism
- Use Convention over Configuration
- Inline Class (if class adds no value)

---

## 6. Divergent Change

**Pattern**: Class changes for multiple unrelated reasons.

**Detection**:
- Class changed more frequently than others
- Different changes aren't related to each other

**Severity**: High

**Solutions**:
- Extract Class
- Move Method
- Extract Validator
- Introduce Form Object

---

## 7. Long Parameter List

**Pattern**: Methods with too many arguments.

**Detection**:
- Methods with more than 3 arguments
- Difficulty changing argument order
- Complex method due to parameter combinations

**Severity**: Medium

**Solutions**:
- Introduce Parameter Object
- Extract Class

**Example Issue**:
```ruby
# Bad: Long parameter list
def completion_notification(first_name, last_name, email, phone, company)
  # ...
end
```

---

## 8. Duplicated Code

**Pattern**: Same or similar code in multiple places.

**Detection**:
- Copy-pasted code blocks
- Similar logic with minor variations
- Parallel class hierarchies

**Severity**: High

**Solutions**:
- Extract Method
- Extract Class
- Extract Partial (views)
- Replace Conditional with Polymorphism

---

## 9. Mixin Abuse

**Pattern**: Using mixins inappropriately.

**Detection**:
- Mixins with methods that accept same parameters repeatedly
- Mixins that don't reference the state of mixed-in class
- Business logic trapped in mixins
- Classes with few public methods except from mixins

**Severity**: Medium

**Solutions**:
- Extract Class
- Replace Mixin with Composition

---

## 10. Callback Complexity

**Pattern**: Complex business logic in ActiveRecord callbacks.

**Detection**:
- `after_create`, `before_save` with business logic
- Callbacks that send emails or process payments
- Attributes to skip callbacks (e.g., `save_without_email`)
- Conditional callbacks

**Severity**: High

**Solutions**:
- Replace Callback with Method
- Extract to PORO

**Example Issue**:
```ruby
# Bad: Business logic in callback
class Invitation < ActiveRecord::Base
  after_create :deliver
  
  def deliver
    Mailer.invitation_notification(self).deliver
  end
end
```

---

## 11. Comments (Code Smell)

**Pattern**: Comments that explain what code does.

**Detection**:
- Comments within method bodies
- Comments restating method name
- TODO comments
- Commented-out code

**Severity**: Low

**Solutions**:
- Introduce Explaining Variable
- Extract Method with descriptive name
- Delete dead code

---

## 12. Single Table Inheritance (STI) Issues

**Pattern**: Problematic use of STI.

**Detection**:
- Need to change from one subclass to another
- Behavior shared among some but not all subclasses
- Subclass is fusion of other subclasses
- Lots of nil columns in database

**Severity**: Medium

**Solutions**:
- Replace Subclasses with Strategies
- Use Polymorphic Associations instead

---

## 13. God Class

**Pattern**: Class that knows too much about the system.

**Detection**:
- References to most other models
- Difficult to answer questions without this class
- Very high number of methods and lines
- Common in `User` model or central domain object

**Severity**: Critical

**Solutions**:
- Extract Class aggressively
- Use composition
- Introduce domain-specific objects

---

## Priority Order for Addressing Smells

1. **Critical**: God Class, Security issues
2. **High**: Duplicated Code, Case Statements, Large Class, Callback Complexity
3. **Medium**: Long Method, Feature Envy, Long Parameter List, Mixin Abuse
4. **Low**: Comments, Naming issues

# PORO Patterns Reference (Plain Old Ruby Objects)

## Philosophy: Service Objects Are Poorly-Named Models

Instead of creating "service objects" with generic names like `*Service`, `*Manager`, `*Handler`, create **domain models** with meaningful names that represent business concepts.

**Key Principle**: A representation of a business domain concept is called a **model**.

---

## Naming Convention

### Bad (Service Object Pattern)
```ruby
# app/services/notification_service.rb
class NotificationService
  def self.call(user, message)
    # sends notification
  end
end
```

### Good (Domain Model Pattern)
```ruby
# app/models/notification.rb
class Notification
  include ActiveModel::Model
  
  attr_accessor :user, :message
  
  def deliver
    # sends notification
  end
end
```

### Naming Guidelines

| Instead of | Use |
|------------|-----|
| `UserSignupService` | `Registration` or `UserSignup` |
| `PaymentProcessor` | `Payment` |
| `NotificationService` | `Notification` or `NotificationDelivery` |
| `EmailSender` | `Email` or `EmailMessage` |
| `OrderCreator` | `Order` or `OrderPlacement` |
| `InvitationManager` | `Invitation` |

**Rule**: Think of the **noun** or **noun group** that describes the domain concept.

---

## ActiveModel::Model

Use `ActiveModel::Model` to give POROs Rails form integration and validation.

### Basic Pattern

```ruby
class Registration
  include ActiveModel::Model
  
  attr_accessor :email, :password, :company_name
  
  validates :email, presence: true, format: { with: URI::MailTo::EMAIL_REGEXP }
  validates :password, presence: true, length: { minimum: 8 }
  validates :company_name, presence: true
  
  def save
    return false unless valid?
    
    create_user
    create_company
    send_welcome_email
    true
  end
  
  private
  
  def create_user
    @user = User.create!(email: email, password: password)
  end
  
  def create_company
    @company = Company.create!(name: company_name, owner: @user)
  end
  
  def send_welcome_email
    RegistrationMailer.welcome(@user).deliver_later
  end
end
```

### Benefits of ActiveModel::Model

1. **Form Integration**: Works with `form_for` / `form_with`
2. **Validations**: Full ActiveRecord validation syntax
3. **Error Messages**: Display validation errors in forms
4. **Attribute Assignment**: Mass assignment from params
5. **Naming Conventions**: Auto-generates form paths

---

## Form Objects

Use when forms don't map 1:1 to a database model.

### Pattern

```ruby
# app/models/contact_form.rb
class ContactForm
  include ActiveModel::Model
  
  attr_accessor :name, :email, :message, :phone
  
  validates :name, presence: true
  validates :email, presence: true, format: { with: URI::MailTo::EMAIL_REGEXP }
  validates :message, presence: true, length: { minimum: 10 }
  
  def submit
    return false unless valid?
    
    ContactMailer.new_inquiry(self).deliver_later
    true
  end
end
```

### Controller Usage

```ruby
class ContactController < ApplicationController
  def new
    @contact = ContactForm.new
  end
  
  def create
    @contact = ContactForm.new(contact_params)
    
    if @contact.submit
      redirect_to thank_you_path, notice: "Message sent!"
    else
      render :new
    end
  end
  
  private
  
  def contact_params
    params.require(:contact_form).permit(:name, :email, :message, :phone)
  end
end
```

### View Usage

```erb
<%= form_with model: @contact, url: contact_path do |f| %>
  <%= f.label :name %>
  <%= f.text_field :name %>
  
  <%= f.label :email %>
  <%= f.email_field :email %>
  
  <%= f.label :message %>
  <%= f.text_area :message %>
  
  <%= f.submit "Send Message" %>
<% end %>
```

---

## Search/Query Objects

Use for complex search forms with filtering and sorting.

### Pattern

```ruby
# app/models/post_search.rb
class PostSearch
  include ActiveModel::Model
  include ActiveModel::Attributes
  
  attribute :query, :string
  attribute :author_id, :integer
  attribute :status, :string
  attribute :sort_column, :string, default: "created_at"
  attribute :sort_direction, :string, default: "desc"
  
  def results
    scope = Post.all
    scope = scope.where("title ILIKE ?", "%#{query}%") if query.present?
    scope = scope.where(author_id: author_id) if author_id.present?
    scope = scope.where(status: status) if status.present?
    scope.order(sort_column => sort_direction)
  end
  
  def sort_options
    [
      ["Newest First", "created_at-desc"],
      ["Oldest First", "created_at-asc"],
      ["Title A-Z", "title-asc"],
      ["Title Z-A", "title-desc"]
    ]
  end
end
```

---

## Value Objects

Use for values with behavior that don't need persistence.

### Pattern

```ruby
# app/models/money.rb
class Money
  include Comparable
  
  attr_reader :cents, :currency
  
  def initialize(cents, currency = "USD")
    @cents = cents.to_i
    @currency = currency
  end
  
  def dollars
    cents / 100.0
  end
  
  def +(other)
    raise ArgumentError, "Currency mismatch" unless currency == other.currency
    Money.new(cents + other.cents, currency)
  end
  
  def <=>(other)
    cents <=> other.cents
  end
  
  def to_s
    format("$%.2f", dollars)
  end
end
```

---

## Decorators/Presenters

Use to add presentation logic without polluting models.

### Pattern

```ruby
# app/models/user_presenter.rb
class UserPresenter
  def initialize(user)
    @user = user
  end
  
  def full_name
    "#{@user.first_name} #{@user.last_name}"
  end
  
  def display_name
    @user.nickname.presence || full_name
  end
  
  def avatar_url
    @user.avatar.attached? ? @user.avatar.url : default_avatar_url
  end
  
  private
  
  def default_avatar_url
    "https://www.gravatar.com/avatar/#{Digest::MD5.hexdigest(@user.email)}"
  end
end
```

---

## Calculator/Query Objects

Use to extract complex calculations or queries.

### Pattern Using Namespaces

```ruby
# app/models/line_item.rb
class LineItem < ApplicationRecord
  def price
    LineItems::Price.new(self).calculate
  end
end

# app/models/line_items/price.rb
module LineItems
  class Price
    def initialize(line_item)
      @line_item = line_item
      @product = line_item.product
    end
    
    def calculate
      base_price + options_price - discount
    end
    
    private
    
    def base_price
      @product.base_price
    end
    
    def options_price
      @line_item.options.sum(&:price)
    end
    
    def discount
      @line_item.coupon&.discount_amount || 0
    end
  end
end
```

---

## Directory Structure

Prefer `app/models/` with namespaces over `app/services/`:

```
app/models/
├── user.rb
├── order.rb
├── registration.rb           # Form object
├── post_search.rb            # Search object
├── user_presenter.rb         # Presenter
├── orders/
│   ├── placement.rb          # Complex operation
│   ├── calculator.rb         # Price calculation
│   └── summary.rb            # Report generation
└── notifications/
    ├── delivery.rb
    └── payload.rb
```

---

## Refactoring Service Objects to POROs

### Step 1: Identify the Domain Concept

```ruby
# Before: What is this service doing?
class UserRegistrationService
  def self.call(params)
    # Creates user, sends email, creates trial subscription
  end
end

# After: It's handling a Registration
class Registration
  include ActiveModel::Model
  # ...
end
```

### Step 2: Add ActiveModel::Model

```ruby
class Registration
  include ActiveModel::Model
  
  attr_accessor :email, :password, :plan
  
  validates :email, presence: true
  validates :password, presence: true
end
```

### Step 3: Replace `.call` with Domain Method

```ruby
class Registration
  include ActiveModel::Model
  
  # Instead of .call, use a meaningful verb
  def complete
    return false unless valid?
    
    ActiveRecord::Base.transaction do
      create_user
      create_subscription
      send_welcome_email
    end
    
    true
  rescue ActiveRecord::RecordInvalid
    false
  end
end
```

### Step 4: Update Controller

```ruby
# Before
def create
  result = UserRegistrationService.call(params)
  if result.success?
    redirect_to dashboard_path
  else
    render :new
  end
end

# After
def create
  @registration = Registration.new(registration_params)
  
  if @registration.complete
    redirect_to dashboard_path
  else
    render :new
  end
end
```

---

## Audit Checklist

When reviewing service objects, check:

1. [ ] Is there a better noun name for this class?
2. [ ] Should it include `ActiveModel::Model`?
3. [ ] Does it have validations that could use Rails validators?
4. [ ] Is `.call` the only public method? (Replace with domain verb)
5. [ ] Is it in `app/services/`? (Consider `app/models/` with namespace)
6. [ ] Does the name end in `Service`, `Manager`, `Handler`? (Remove suffix)

# Rails Antipatterns Reference

> Based on *Rails Antipatterns: Best Practice Ruby on Rails Refactoring* by Chad Pytel & Tammer Saleh (thoughtbot)

This reference covers antipatterns that **extend** the existing audit references. It focuses on areas not fully covered elsewhere: external service handling, database/migration hygiene, performance patterns, and failure handling.

**Cross-references:**
- For code smells (Large Class, Feature Envy, Long Method): see `code_smells.md`
- For security issues (SQL injection, XSS, CSRF): see `security_checklist.md`
- For PORO patterns and Service Object refactoring: see `poro_patterns.md`
- For testing antipatterns: see `testing_guidelines.md`

---

## 1. Voyeuristic Models (Law of Demeter Extensions)

> Extends `code_smells.md` → Feature Envy

**Pattern**: Controllers or views reaching deep into model associations.

**Detection**:
- Chained association access in views: `@invoice.customer.address.street`
- Raw `where`/`order`/`limit` chains in controllers on associations
- Finders defined on wrong model (crossing association boundaries)

**Severity**: Medium

**Solutions**:
```ruby
# Use delegate with prefix
class Invoice < ApplicationRecord
  belongs_to :customer
  delegate :street, :city, :zip, to: :address, prefix: true, allow_nil: true
  # Now: invoice.address_street instead of invoice.customer.address.street
end

# Access finders through association proxy
@memberships = @user.memberships.recently_active  # Good
@memberships = Membership.where(user: @user, active: true).limit(5)  # Bad
```

**Audit Check**: Search views (`app/views/`) for patterns like `@var.association.association.method` (3+ levels). Search controllers for `.where`/`.order`/`.limit` chains on associations.

---

## 2. Monolithic Controllers (REST Violations)

> Extends `code_smells.md` → Large Class

**Pattern**: Controllers that don't embrace REST — custom actions instead of resources.

**Detection**:
- Non-RESTful action names: `search`, `export`, `activate`, `process_payment`
- Routes with excessive `member` or `collection` blocks
- Controller actions > 7 (beyond standard REST actions)
- Conditional logic based on nested resource presence

**Severity**: High

**Solutions**:
```ruby
# Bad: Monolithic controller
class UsersController < ApplicationController
  def activate; end    # Should be: Activations#create
  def deactivate; end  # Should be: Activations#destroy
  def export; end      # Should be: Exports#show
end

# Good: Separate REST controllers
class User::ActivationsController < ApplicationController
  def create; end   # activate
  def destroy; end  # deactivate
end
```

**Audit Check**: Count public actions per controller. Flag controllers with > 7 actions. Check `config/routes.rb` for excessive `member`/`collection` blocks.

---

## 3. Bloated Sessions

> Extends `security_checklist.md` → Session Security

**Pattern**: Storing full objects in session instead of lightweight references.

**Detection**:
- `session[:user] = @user` (full ActiveRecord object)
- `session[:cart] = @cart_items` (collections)
- Session data containing hashes with model attributes

**Severity**: High (causes stale data, serialization issues)

**Solutions**:
```ruby
# Bad: Storing object (stale data, serialization issues)
session[:user] = User.find(params[:id])

# Good: Store only the reference
session[:user_id] = params[:id]

def current_user
  @current_user ||= User.find_by(id: session[:user_id])
end
```

**Audit Check**: Search for `session[` assignments. Flag any storing non-scalar values.

---

## 4. Fire and Forget (External Service Errors)

**Pattern**: Calling external services without proper exception handling.

**Detection**:
- HTTP calls without `rescue` blocks
- Bare `rescue` or `rescue => e` without specific exception classes
- `rescue nil` on external service calls
- `config.action_mailer.raise_delivery_errors = false` without alternative handling

**Severity**: High

**Solutions**:
```ruby
# Bad: Bare rescue hides real problems
def send_to_api(data)
  ApiClient.post("/webhook", data)
rescue
  nil  # Silently swallows ALL errors
end

# Good: Explicit exception handling with reporting
HTTP_ERRORS = [
  Timeout::Error, Errno::ECONNRESET, Net::HTTPBadResponse,
  Net::HTTPHeaderSyntaxError, Net::ProtocolError, EOFError,
  SocketError, Errno::ECONNREFUSED
].freeze

def send_to_api(data)
  ApiClient.post("/webhook", data)
rescue *HTTP_ERRORS => e
  ErrorTracker.notify(e)  # Sentry, Honeybadger, etc.
  nil
end
```

**Audit Check**:
- Search for `rescue\s*$` or `rescue\s*=>` (bare rescue)
- Search for `rescue nil`
- Check `action_mailer` config for suppressed errors

---

## 5. Sluggish Services (Missing Timeouts)

**Pattern**: External service calls blocking requests without timeout configuration.

**Detection**:
- HTTP calls without explicit timeout settings
- Synchronous API calls in request cycle that could be backgrounded
- Default `Net::HTTP` timeout (60 seconds) unchanged

**Severity**: High

**Solutions**:
```ruby
# Bad: No timeout — blocks up to 60 seconds
Net::HTTP.get(URI("https://slow-api.example.com/data"))

# Good: Explicit timeouts
uri = URI("https://slow-api.example.com/data")
http = Net::HTTP.new(uri.host, uri.port)
http.open_timeout = 5
http.read_timeout = 5
http.request(Net::HTTP::Get.new(uri))

# Better: Use Faraday with timeouts
conn = Faraday.new(url: "https://api.example.com") do |f|
  f.options.timeout = 5
  f.options.open_timeout = 2
end

# Best: Background non-critical calls
SendWebhookJob.perform_later(data)
```

**Audit Check**: Search for `Net::HTTP`, `Faraday`, `HTTParty`, `RestClient` usage. Verify timeout configuration. Flag synchronous API calls in controllers that could be backgrounded.

---

## 6. Messy Migrations

**Pattern**: Migrations that reference external model code or lack reversibility.

**Detection**:
- Model class references inside migrations (e.g., `User.all.each`)
- Missing `down` method for non-reversible changes
- Migrations modified after being committed
- Missing `reset_column_information` after schema changes in data migrations

**Severity**: Medium

**Solutions**:
```ruby
# Bad: External model dependency — breaks if User model changes
class AddJobsCountToUser < ActiveRecord::Migration[7.1]
  def up
    add_column :users, :jobs_count, :integer, default: 0
    User.all.each { |u| u.update!(jobs_count: u.jobs.size) }
  end
end

# Good: Pure SQL, no external dependencies
class AddJobsCountToUser < ActiveRecord::Migration[7.1]
  def up
    add_column :users, :jobs_count, :integer, default: 0
    execute <<-SQL
      UPDATE users SET jobs_count = (
        SELECT count(*) FROM jobs WHERE jobs.user_id = users.id
      )
    SQL
  end

  def down
    remove_column :users, :jobs_count
  end
end

# If model needed, define inline
class BackfillData < ActiveRecord::Migration[7.1]
  class User < ApplicationRecord
    self.table_name = "users"
  end

  def up
    User.reset_column_information
    # Safe to use inline User class
  end
end
```

**Audit Check**: Search `db/migrate/` for model class names (e.g., `User.`, `Order.`). Verify each migration has reversible `down` or uses only reversible `change` methods.

---

## 7. Missing Database Indexes

**Pattern**: Tables missing indexes on commonly queried columns.

**Detection**:
- Foreign key columns (`*_id`) without indexes
- Polymorphic type/id pairs without composite indexes
- Columns used in `validates :uniqueness` without unique indexes
- STI `type` columns without indexes
- Columns used in `to_param` (slugs) without indexes
- State/status columns used in `where` without indexes

**Severity**: High (performance)

**Solutions**:
```ruby
# Foreign keys need indexes
add_index :posts, :user_id

# Polymorphic associations need composite indexes
add_index :comments, [:commentable_type, :commentable_id]

# Uniqueness validations need unique indexes
add_index :users, :email, unique: true

# STI needs type index
add_index :vehicles, :type

# Slugs need indexes
add_index :posts, :slug, unique: true
```

**Audit Check**:
```bash
# Find foreign key columns without indexes
# Compare *_id columns in schema.rb against add_index statements
```

Flag: Any `*_id` column without index. Any `validates :uniqueness` without database-level unique index.

---

## 8. Painful Performance (Ruby vs SQL)

**Pattern**: Doing in Ruby what should be done in SQL.

**Detection**:
- `Model.all.select`, `all.map`, `all.reject` (loading entire tables)
- Ruby sorting/filtering after loading records
- `association.length` instead of `association.count`
- Ruby `inject`/`reduce` for sums that could be SQL aggregations

**Severity**: High

**Solutions**:
```ruby
# Bad: Loads ALL orders into Ruby memory
Order.all.select { |o| o.total > 100 }

# Good: Database does the work
Order.where("total > ?", 100)

# Bad: Loads all records to count
user.posts.length

# Good: SQL count
user.posts.count
# Or: user.posts.size (uses counter cache if available)

# Bad: Ruby sum
Order.all.map(&:total).sum

# Good: SQL sum
Order.sum(:total)
```

**Audit Check**: Search for `.all.select`, `.all.map`, `.all.reject`, `.all.each` in models/controllers. Flag `.length` on associations.

---

## 9. Inaudible Failures (Silent Errors)

**Pattern**: Code that fails silently — `save` without checking return value, missing preconditions.

**Detection**:
- `save` without checking return value (in jobs, services, rake tasks)
- `update` without checking return value
- Bulk operations without transactions
- Missing fail-fast precondition checks

**Severity**: High

**Solutions**:
```ruby
# Bad: Silent failure
class Ticket < ApplicationRecord
  def self.bulk_change_owner(user)
    all.each do |ticket|
      ticket.owner = user
      ticket.save  # Returns false silently on failure
    end
  end
end

# Good: Fail loudly with transaction
class Ticket < ApplicationRecord
  def self.bulk_change_owner(user)
    transaction do
      all.find_each do |ticket|
        ticket.update!(owner: user)
      end
    end
  end
end

# In background jobs: Use bang methods
class ProcessOrderJob < ApplicationJob
  def perform(order)
    order.process!  # Raises on failure
    order.save!     # Raises on failure
  end
end

# Fail-fast preconditions
def process_payment(order)
  raise ArgumentError, "Order required" unless order
  raise InvalidStateError, "Order not ready" unless order.ready?
  # ... process
end
```

**Audit Check**: Search for `\.save\b` (without `!`) in models, jobs, service objects. Flag bulk operations without `transaction`. Search for bare `rescue` statements.

---

## 10. Spaghetti SQL (Query Logic in Controllers)

> Extends `code_smells.md` → Duplicated Code

**Pattern**: Raw query logic scattered outside models; not using scopes.

**Detection**:
- `where()`, `order()`, `joins()` chains in controllers
- Repeated query fragments across multiple actions
- Missing `scope` for reusable patterns

**Severity**: Medium

**Solutions**:
```ruby
# Bad: Query logic in controllers
# PostsController
@posts = Post.where(published: true).order(created_at: :desc)
# ApiController
@posts = Post.where(published: true).order(created_at: :desc).limit(10)

# Good: Composable scopes
class Post < ApplicationRecord
  scope :published, -> { where(published: true) }
  scope :newest_first, -> { order(created_at: :desc) }
  scope :recent, ->(n = 10) { newest_first.limit(n) }
end

# Controller
@posts = Post.published.newest_first
```

**Audit Check**: Search controllers for `.where`, `.order`, `.joins`, `.group` chains. Flag query patterns appearing in multiple locations.

---

## 11. PHPitis (Logic in Views)

**Pattern**: Business logic, queries, or complex conditionals in view templates.

**Detection**:
- Model queries in views: `<% User.where(...) %>`
- Nested `if/else` blocks (> 2 levels)
- Calculations or business logic in templates

**Severity**: Medium

**Solutions**:
```erb
<%# Bad: Logic in view %>
<% if @order.status == 'pending' && @order.created_at > 1.hour.ago %>
  <span class="warning">Processing</span>
<% elsif @order.status == 'shipped' && @order.tracking_number.present? %>
  <span class="success">Shipped</span>
<% end %>

<%# Good: Use presenter %>
<%= @order_presenter.status_badge %>
```

**Audit Check**: Search `app/views/` for `Model.find`, `Model.where`, `.order`, `.joins`. Flag views with > 2 levels of conditional nesting.

---

## 12. Gem Hygiene

> Extends `security_checklist.md` → Dependencies

**Pattern**: Unused gems, unvetted choices, or modified vendored code.

**Detection**:
- Gems in `Gemfile` not referenced in codebase
- Gems without recent maintenance (12+ months inactive)
- Modified files in `vendor/` directory
- Multiple gems solving same problem

**Severity**: Low

**TAM Criteria for Gem Selection**:
- **T**ests: Does it have a test suite?
- **A**ctivity: Recent commits and releases?
- **M**aturity: Stable API, good documentation?

**Audit Check**:
- Run `bundle-audit check` for vulnerabilities
- Search for gem names in codebase to find unused gems
- Check `vendor/` for local modifications (should fork instead)

---

## Priority Order for Addressing

1. **Critical/High**: Missing Indexes, Inaudible Failures, Fire and Forget, Sluggish Services
2. **High**: Monolithic Controllers, Bloated Sessions, Painful Performance
3. **Medium**: Voyeuristic Models, Spaghetti SQL, Messy Migrations, PHPitis
4. **Low**: Gem Hygiene

---

## Quick Reference: Detection Patterns

| Antipattern | Search Pattern | Files |
|-------------|----------------|-------|
| Voyeuristic Models | `@\w+\.\w+\.\w+\.` (3+ dots) | views/, controllers/ |
| Monolithic Controllers | Count public methods > 7 | controllers/ |
| Bloated Sessions | `session\[.*\] =` non-scalar | controllers/ |
| Fire and Forget | `rescue\s*$`, `rescue nil` | all .rb |
| Sluggish Services | `Net::HTTP`, `Faraday` without timeout | all .rb |
| Messy Migrations | `[A-Z][a-z]+\.` (model refs) | db/migrate/ |
| Missing Indexes | `*_id` without index | db/schema.rb |
| Painful Performance | `.all.select`, `.all.map` | models/, controllers/ |
| Inaudible Failures | `\.save\b` (without !) | models/, jobs/, services/ |
| Spaghetti SQL | `.where`, `.order` chains | controllers/ |
| PHPitis | `<% .*\.where`, nested `<% if` | views/ |

# Audit Report Template

Use this template when generating the final audit report.

---

```markdown

*Report generated by Rails Audit Skill based on thoughtbot Best Practices*

![thoughtbot](https://thoughtbot.com/thoughtbot-logo-for-readmes.svg)

# Rails Application Audit Report

**Generated**: {{DATE}}
**Application**: {{APP_NAME}}
**Rails Version**: {{RAILS_VERSION}}
**Ruby Version**: {{RUBY_VERSION}}
**Audit Scope**: {{SCOPE: Full Application | Targeted: specific paths}}

---

## Executive Summary

| Category | Critical | High | Medium | Low | Total |
|----------|----------|------|--------|-----|-------|
| Testing | X | X | X | X | X |
| Code Quality | X | X | X | X | X |
| Security | X | X | X | X | X |
| Models | X | X | X | X | X |
| Controllers | X | X | X | X | X |
| Code Design | X | X | X | X | X |
| Views | X | X | X | X | X |
| External Services | X | X | X | X | X |
| Database & Performance | X | X | X | X | X |
| **Total** | **X** | **X** | **X** | **X** | **X** |

### Key Findings

1. **[Most critical finding]**
2. **[Second most critical finding]**
3. **[Third most critical finding]**

---

## 1. Testing Issues

### Overview

**When SimpleCov data is available (actual coverage):**

- **Test Framework**: RSpec / Minitest
- **Coverage Method**: SimpleCov (Actual)
- **Overall Line Coverage**: XX.X%
- **Overall Branch Coverage**: XX.X%
- **Files with Tests**: X / Y (Z%)

| Directory | Files | Line Coverage | Status |
|-----------|-------|---------------|--------|
| app/models/ | X / Y | XX.X% | Below/Meets/Exceeds target |
| app/controllers/ | X / Y | XX.X% | ... |
| app/services/ | X / Y | XX.X% | ... |
| app/helpers/ | X / Y | XX.X% | ... |
| app/mailers/ | X / Y | XX.X% | ... |

**Coverage vs Targets:**

| File Type | Target | Actual | Delta |
|-----------|--------|--------|-------|
| Model | 90% | XX.X% | +/-X% |
| Controller | 80% | XX.X% | +/-X% |
| Service/PORO | 90% | XX.X% | +/-X% |
| Helper | 70% | XX.X% | +/-X% |
| Mailer | 70% | XX.X% | +/-X% |

**Lowest Coverage Files** (bottom 10):

| File | Coverage |
|------|----------|
| path/to/file.rb | XX.X% |

**Zero Coverage Files:**
- `path/to/untested_file.rb`

**When SimpleCov data is NOT available (estimation mode):**

- **Test Framework**: RSpec / Minitest
- **Coverage Method**: Manual Estimation
- **Files with Tests**: X / Y (Z%)
- **Estimated Coverage**: Low / Medium / High

### Critical Issues

> None found (or list issues)

### High Severity

#### [ISSUE_ID] Missing Tests for [ModelName]

**File**: `app/models/model_name.rb`
**Impact**: Untested business logic can introduce regressions
**Details**: No corresponding spec file found at `spec/models/model_name_spec.rb`

**Recommendation**:
Create spec file with tests for:
- Validations
- Public methods: `#method_one`, `#method_two`
- Associations (if complex)

```ruby
# spec/models/model_name_spec.rb
RSpec.describe ModelName do
  describe "validations" do
    it { is_expected.to validate_presence_of(:field) }
  end
  
  describe "#method_one" do
    it "returns expected result" do
      # test implementation
    end
  end
end
```

### Medium Severity

#### [ISSUE_ID] Testing Antipattern: Mystery Guest

**File**: `spec/models/user_spec.rb:45`
**Impact**: Tests are hard to understand
**Details**: Test relies on factory defaults that aren't visible in test

**Current Code**:
```ruby
it "calculates total" do
  user = create(:user) # What attributes matter?
  expect(user.total).to eq 100
end
```

**Recommendation**:
```ruby
it "calculates total" do
  user = create(:user, orders_count: 2, average_order: 50)
  expect(user.total).to eq 100
end
```

### Low Severity

> List low severity testing issues

---

## 2. Code Quality Metrics

### Overview

**When RubyCritic data is available:**

- **Analysis Tool**: RubyCritic (Reek + Flay + Flog)
- **Overall Score**: XX.X / 100
- **Total Files Analyzed**: N

| Rating | Count | Percentage |
|--------|-------|------------|
| A (Excellent) | N | XX% |
| B (Good) | N | XX% |
| C (Needs Improvement) | N | XX% |
| D (Poor) | N | XX% |
| F (Critical) | N | XX% |

**Ratings by Directory:**

| Directory | Avg Score | A | B | C | D | F |
|-----------|-----------|---|---|---|---|---|
| app/models/ | XX.X | N | N | N | N | N |
| app/controllers/ | XX.X | N | N | N | N | N |
| app/services/ | XX.X | N | N | N | N | N |
| app/helpers/ | XX.X | N | N | N | N | N |
| lib/ | XX.X | N | N | N | N | N |

**Worst Rated Files** (D and F):

| File | Rating | Cost | Complexity | Duplication | Smells |
|------|--------|------|------------|-------------|--------|
| path/to/file.rb | F | XX.X | XX.X | N | N |

**Most Common Code Smells:**

| Smell Type | Occurrences | Analyzer |
|------------|-------------|----------|
| TooManyStatements | N | Reek |
| HighComplexity | N | Flog |
| DuplicateCode | N | Flay |

**Most Complex Files** (top 10):

| File | Complexity | Methods | Complexity/Method |
|------|------------|---------|-------------------|
| path/to/file.rb | XX.X | N | XX.X |

**When RubyCritic data is NOT available:**

- **Code Quality Metrics**: Not collected (manual estimation used)

---

## 3. Security Issues

### Critical Issues

#### [ISSUE_ID] SQL Injection Risk

**File**: `app/models/search.rb:23`
**Impact**: Potential data breach, unauthorized data access
**Details**: User input interpolated directly into SQL query

**Current Code**:
```ruby
def search(term)
  where("name LIKE '%#{term}%'")
end
```

**Recommendation**:
```ruby
def search(term)
  where("name LIKE ?", "%#{term}%")
end
```

### High Severity

#### [ISSUE_ID] Mass Assignment Vulnerability

**File**: `app/controllers/users_controller.rb:34`
**Impact**: Users could modify restricted attributes

**Current Code**:
```ruby
def user_params
  params.require(:user).permit!
end
```

**Recommendation**:
```ruby
def user_params
  params.require(:user).permit(:name, :email, :phone)
end
```

### Medium Severity

> List medium severity security issues

### Low Severity

> List low severity security issues

---

## 4. Models Issues

### High Severity

#### [ISSUE_ID] Fat Model: User

**File**: `app/models/user.rb`
**Lines**: 450
**Public Methods**: 32
**Impact**: Hard to maintain, test, and understand

**Details**:
Multiple responsibilities detected:
- Authentication logic
- Profile management
- Notification preferences
- Billing information
- Activity tracking

**Recommendation**:
Extract to domain models:
- `Authentication` (PORO with ActiveModel)
- `UserProfile` (PORO)
- `NotificationSettings` (PORO)
- `BillingInfo` (PORO)

Example extraction:
```ruby
# app/models/user_profile.rb
class UserProfile
  include ActiveModel::Model
  
  attr_accessor :user, :bio, :avatar, :website
  
  def initialize(user)
    @user = user
    @bio = user.bio
    # ...
  end
  
  def update(attributes)
    # ...
  end
end
```

### Medium Severity

#### [ISSUE_ID] Callback with Business Logic

**File**: `app/models/order.rb:15`
**Impact**: Side effects hard to test, can cause unexpected behavior

**Current Code**:
```ruby
after_create :send_confirmation_email
after_create :update_inventory
after_create :notify_warehouse
```

**Recommendation**:
Move to explicit method calls in controller or form object:
```ruby
# app/models/order_placement.rb
class OrderPlacement
  include ActiveModel::Model
  
  def complete
    return false unless order.save
    
    send_confirmation_email
    update_inventory
    notify_warehouse
    true
  end
end
```

---

## 5. Controllers Issues

### High Severity

#### [ISSUE_ID] Fat Controller: OrdersController#create

**File**: `app/controllers/orders_controller.rb:45-120`
**Lines**: 75 lines in single action
**Impact**: Hard to test, violates SRP

**Current Code**:
```ruby
def create
  # 75 lines of business logic
end
```

**Recommendation**:
Extract to form object:
```ruby
def create
  @order_form = OrderForm.new(order_params)
  
  if @order_form.submit
    redirect_to @order_form.order
  else
    render :new
  end
end
```

---

## 6. Code Design Issues

### High Severity

#### [ISSUE_ID] Service Object Should Be Domain Model

**File**: `app/services/user_registration_service.rb`
**Impact**: Poor naming obscures domain concept

**Current Code**:
```ruby
class UserRegistrationService
  def self.call(params)
    # registration logic
  end
end
```

**Recommendation**:
Rename to domain model with ActiveModel:
```ruby
# app/models/registration.rb
class Registration
  include ActiveModel::Model
  
  attr_accessor :email, :password, :company_name
  
  validates :email, presence: true
  validates :password, presence: true, length: { minimum: 8 }
  
  def complete
    return false unless valid?
    
    create_user
    create_company
    send_welcome_email
    true
  end
end
```

### Medium Severity

#### [ISSUE_ID] Feature Envy

**File**: `app/models/report.rb:34`
**Impact**: Logic should be moved to collaborator

**Current Code**:
```ruby
def user_summary
  "#{user.first_name} #{user.last_name} (#{user.email})"
end
```

**Recommendation**:
Move to User model or UserPresenter:
```ruby
# In User model
def full_name_with_email
  "#{full_name} (#{email})"
end
```

---

## 7. Views Issues

### Medium Severity

#### [ISSUE_ID] Logic in View

**File**: `app/views/orders/show.html.erb:23`
**Impact**: Views should be logic-free

**Current Code**:
```erb
<% if @order.status == 'pending' && @order.created_at > 1.hour.ago %>
  <span class="badge warning">Processing</span>
<% elsif @order.status == 'shipped' %>
  <span class="badge success">Shipped</span>
<% end %>
```

**Recommendation**:
Extract to presenter or helper:
```ruby
# app/models/order_presenter.rb
class OrderPresenter
  def status_badge
    case
    when pending_and_recent? then { class: "warning", text: "Processing" }
    when shipped? then { class: "success", text: "Shipped" }
    end
  end
end
```

---

## 8. External Services Issues

### High Severity

#### [ISSUE_ID] Fire and Forget: Missing Exception Handling

**File**: `app/models/webhook_sender.rb:15`
**Impact**: Errors silently swallowed, no visibility into failures
**Details**: External API call without specific exception handling

**Current Code**:
```ruby
def send_webhook(data)
  ApiClient.post("/webhook", data)
rescue
  nil  # Swallows ALL errors including bugs
end
```

**Recommendation**:
```ruby
HTTP_ERRORS = [
  Timeout::Error, Errno::ECONNRESET, Net::HTTPBadResponse,
  SocketError, Errno::ECONNREFUSED
].freeze

def send_webhook(data)
  ApiClient.post("/webhook", data)
rescue *HTTP_ERRORS => e
  ErrorTracker.notify(e)
  nil
end
```

### Medium Severity

#### [ISSUE_ID] Missing Timeout on HTTP Client

**File**: `app/services/api_client.rb:8`
**Impact**: Requests can hang indefinitely, blocking web workers
**Details**: HTTP client uses default 60-second timeout

**Recommendation**:
```ruby
http = Net::HTTP.new(uri.host, uri.port)
http.open_timeout = 5
http.read_timeout = 5
```

---

## 9. Database & Performance Issues

### High Severity

#### [ISSUE_ID] Missing Index on Foreign Key

**File**: `db/schema.rb`
**Impact**: Slow queries on joins and lookups
**Details**: Column `posts.user_id` has no index

**Recommendation**:
```ruby
# db/migrate/XXXXXXXX_add_index_to_posts_user_id.rb
add_index :posts, :user_id
```

#### [ISSUE_ID] Ruby Iteration Instead of SQL

**File**: `app/models/order.rb:45`
**Impact**: Loads all records into memory, very slow on large datasets
**Details**: Using Ruby to filter records that should be SQL query

**Current Code**:
```ruby
def self.high_value
  all.select { |o| o.total > 100 }
end
```

**Recommendation**:
```ruby
def self.high_value
  where("total > ?", 100)
end
```

### Medium Severity

#### [ISSUE_ID] Silent Failure in Background Job

**File**: `app/jobs/process_order_job.rb:12`
**Impact**: Orders may fail processing without any notification
**Details**: Using `save` without checking return value

**Current Code**:
```ruby
def perform(order)
  order.status = "processed"
  order.save
end
```

**Recommendation**:
```ruby
def perform(order)
  order.update!(status: "processed")
end
```

#### [ISSUE_ID] Model Reference in Migration

**File**: `db/migrate/20240115_backfill_counts.rb`
**Impact**: Migration may fail if model changes, breaks future deployments
**Details**: Migration references `User` model directly

**Recommendation**:
Use raw SQL or define model inline:
```ruby
class BackfillCounts < ActiveRecord::Migration[7.1]
  def up
    execute <<-SQL
      UPDATE users SET posts_count = (
        SELECT count(*) FROM posts WHERE posts.user_id = users.id
      )
    SQL
  end
end
```

---

## Recommendations Summary

### Quick Wins (Immediate Action)

1. [ ] Fix SQL injection in `app/models/search.rb:23`
2. [ ] Add strong parameters to `users_controller.rb`
3. [ ] Create spec for `Order` model

### Short-term (This Sprint)

1. [ ] Extract `UserRegistrationService` to `Registration` PORO
2. [ ] Split `User` model into focused domain models
3. [ ] Move callbacks from `Order` to explicit form object

### Long-term (Technical Debt)

1. [ ] Achieve 80% test coverage
2. [ ] Refactor all service objects to domain models
3. [ ] Extract presenters for complex view logic

---

## Files Analyzed

| Directory | Files Analyzed | Issues Found |
|-----------|----------------|--------------|
| app/models/ | X | X |
| app/controllers/ | X | X |
| app/services/ | X | X |
| app/views/ | X | X |
| app/helpers/ | X | X |
| app/jobs/ | X | X |
| db/migrate/ | X | X |
| config/ | X | X |
| spec/ or test/ | X | X |
| **Total** | **X** | **X** |

---

## Appendix: Tools Recommendations

For ongoing code quality:

1. **RuboCop** - Style and lint checking
2. **Brakeman** - Security scanning
3. **SimpleCov** - Test coverage (if SimpleCov was used during this audit: "SimpleCov was used to generate the coverage data in this report. Consider keeping it permanently in your test suite for continuous coverage tracking.")
4. **RubyCritic** - Code quality metrics combining Reek (code smells), Flay (duplication), and Flog (complexity) (if RubyCritic was used during this audit: "RubyCritic was used to generate the code quality metrics in this report. Consider running it periodically to track code quality trends.")
5. **Bullet** - N+1 query detection
6. **bundler-audit** - Gem vulnerability scanning
7. **database_consistency** - Missing indexes and constraints detection
8. **strong_migrations** - Catch unsafe migrations

---
```

# Security Audit Checklist

## Critical Security Issues

### 1. SQL Injection

**Detection Patterns**:
```ruby
# DANGEROUS - String interpolation in queries
where("name = '#{params[:name]}'")
where("name LIKE '%#{term}%'")
find_by_sql("SELECT * FROM users WHERE id = #{id}")
order("#{params[:sort]} #{params[:direction]}")
```

**Safe Alternatives**:
```ruby
# Use parameterized queries
where("name = ?", params[:name])
where("name LIKE ?", "%#{term}%")
where(name: params[:name])
order(Arel.sql(safe_order_string))
```

**Severity**: Critical

---

### 2. Mass Assignment

**Detection Patterns**:
```ruby
# DANGEROUS
params.permit!
User.new(params[:user])  # Without strong params
update_attributes(params)
```

**Safe Alternatives**:
```ruby
# Use strong parameters
def user_params
  params.require(:user).permit(:name, :email)
end

User.new(user_params)
```

**Severity**: Critical

---

### 3. Cross-Site Scripting (XSS)

**Detection Patterns**:
```erb
<%# DANGEROUS - raw output %>
<%= raw user_input %>
<%= user_input.html_safe %>
<%== user_input %>
```

**Safe Alternatives**:
```erb
<%# Safe - auto-escaped %>
<%= user_input %>
<%= sanitize(user_input) %>
```

**Check in JavaScript**:
```javascript
// DANGEROUS
element.innerHTML = userInput;

// Safe
element.textContent = userInput;
```

**Severity**: High

---

### 4. Command Injection

**Detection Patterns**:
```ruby
# DANGEROUS
system("convert #{params[:file]}")
`ls #{user_input}`
exec("command #{args}")
%x(command #{args})
```

**Safe Alternatives**:
```ruby
# Use array form
system("convert", params[:file])
```

**Severity**: Critical

---

### 5. Path Traversal

**Detection Patterns**:
```ruby
# DANGEROUS
send_file(params[:filename])
File.read(params[:path])
render file: params[:template]
```

**Safe Alternatives**:
```ruby
# Validate and sanitize paths
basename = File.basename(params[:filename])
safe_path = Rails.root.join("uploads", basename)
send_file(safe_path) if File.exist?(safe_path)
```

**Severity**: Critical

---

### 6. Insecure Direct Object References (IDOR)

**Detection Patterns**:
```ruby
# DANGEROUS - No authorization check
def show
  @document = Document.find(params[:id])
end
```

**Safe Alternatives**:
```ruby
# Scope to current user
def show
  @document = current_user.documents.find(params[:id])
end

# Or use authorization
def show
  @document = Document.find(params[:id])
  authorize @document
end
```

**Severity**: High

---

### 7. Missing Authentication

**Detection Patterns**:
```ruby
# Check for missing before_action
class AdminController < ApplicationController
  # Missing: before_action :authenticate_admin!
  
  def destroy
    User.find(params[:id]).destroy
  end
end
```

**Audit Check**: Every controller should have authentication unless explicitly public.

**Severity**: Critical

---

### 8. Sensitive Data Exposure

**Detection Patterns**:
```ruby
# DANGEROUS - Logging sensitive data
Rails.logger.info("Password: #{params[:password]}")
Rails.logger.info(params.inspect)

# DANGEROUS - Exposing in JSON
render json: user  # May include password_digest, tokens, etc.
```

**Safe Alternatives**:
```ruby
# Filter sensitive params
config.filter_parameters += [:password, :token, :secret]

# Whitelist JSON attributes
render json: user.as_json(only: [:id, :name, :email])
```

**Severity**: High

---

### 9. Weak Cryptography

**Detection Patterns**:
```ruby
# DANGEROUS
Digest::MD5.hexdigest(password)
Digest::SHA1.hexdigest(password)
Base64.encode64(secret)  # Not encryption!
```

**Safe Alternatives**:
```ruby
# Use bcrypt (via has_secure_password)
BCrypt::Password.create(password)

# For encryption
ActiveSupport::MessageEncryptor
```

**Severity**: High

---

### 10. Session Security

**Detection Patterns**:
```ruby
# Check session configuration
# config/initializers/session_store.rb
Rails.application.config.session_store :cookie_store, 
  key: '_app_session'
  # Missing: secure: true, httponly: true, same_site: :lax
```

**Safe Configuration**:
```ruby
Rails.application.config.session_store :cookie_store,
  key: '_app_session',
  secure: Rails.env.production?,
  httponly: true,
  same_site: :lax,
  expire_after: 30.minutes
```

**Severity**: Medium

---

### 11. Cross-Site Request Forgery (CSRF)

**Detection Patterns**:
```ruby
# DANGEROUS - Skipping CSRF
class ApiController < ApplicationController
  skip_before_action :verify_authenticity_token
end

# DANGEROUS - Not using form helpers
<form action="/posts" method="post">
  # Missing CSRF token
</form>
```

**Safe Alternatives**:
```ruby
# Use proper token handling for APIs
class ApiController < ApplicationController
  protect_from_forgery with: :null_session
end

# Use Rails form helpers
<%= form_with url: posts_path do |f| %>
```

**Severity**: Medium

---

### 12. Redirect Security

**Detection Patterns**:
```ruby
# DANGEROUS - Open redirect
redirect_to params[:return_to]
redirect_to request.referer
```

**Safe Alternatives**:
```ruby
# Validate redirect URLs
redirect_to url_for(params[:return_to]) rescue redirect_to root_path

# Or use allowlist
ALLOWED_REDIRECTS = %w[/dashboard /profile /settings]
redirect_to params[:return_to] if ALLOWED_REDIRECTS.include?(params[:return_to])
```

**Severity**: Medium

---

## Security Audit Checklist

### Authentication
- [ ] All sensitive actions require authentication
- [ ] Password requirements enforced (length, complexity)
- [ ] Account lockout after failed attempts
- [ ] Secure password reset flow
- [ ] Session timeout configured

### Authorization
- [ ] Resources scoped to authorized users
- [ ] Admin actions protected
- [ ] Role-based access control where needed

### Input Validation
- [ ] All user input validated
- [ ] Strong parameters used
- [ ] File upload restrictions (type, size)
- [ ] No SQL interpolation

### Output Encoding
- [ ] No `raw` or `html_safe` with user input
- [ ] JSON responses don't expose sensitive data
- [ ] Logs filtered for sensitive data

### Configuration
- [ ] HTTPS enforced in production
- [ ] Secure session configuration
- [ ] CSRF protection enabled
- [ ] Security headers configured (CSP, X-Frame-Options, etc.)

### Dependencies
- [ ] Gemfile.lock reviewed for vulnerabilities
- [ ] Using `bundler-audit` or similar

---

## Severity Reference

| Severity | Impact | Examples |
|----------|--------|----------|
| Critical | Data breach, full system compromise | SQL injection, RCE, authentication bypass |
| High | Significant data exposure or modification | XSS, IDOR, mass assignment |
| Medium | Limited impact, requires interaction | CSRF, open redirect, session issues |
| Low | Minor issues, defense in depth | Missing headers, verbose errors |

# Testing Guidelines Reference (Testing Rails)

## Test Suite Quality Characteristics

An effective test suite is:
- **Fast**: Run frequently, quick feedback loop
- **Complete**: All public code paths covered
- **Reliable**: No false positives or intermittent failures
- **Isolated**: Tests run independently, clean up after themselves
- **Maintainable**: Easy to add new tests and modify existing ones
- **Expressive**: Tests serve as documentation

---

## Testing Pyramid

Structure your test suite as a pyramid:
- **Base**: Many fast unit/model tests
- **Middle**: Some integration tests
- **Top**: Few slow feature/system tests

---

## Test Types Coverage Requirements

### Feature/System Specs (Integration)
**Required Coverage**:
- All critical user flows
- Happy paths for main features
- Key error handling paths

**Audit Checks**:
- [ ] Login/authentication flow tested
- [ ] Main CRUD operations tested
- [ ] Payment flows tested (if applicable)
- [ ] Critical business workflows tested

### Model Specs
**Required Coverage**:
- All validations
- All public instance methods
- All public class methods
- Associations (if complex logic)

**Audit Checks**:
- [ ] Each model has corresponding spec file
- [ ] All validations tested with shoulda-matchers
- [ ] Business logic methods have unit tests
- [ ] Edge cases covered

### Controller Specs (or Request Specs)
**Required Coverage**:
- Authorization checks
- Error handling paths
- Response formats (especially for APIs)

**Use When**:
- Testing authorization logic
- Multiple sad paths from same happy path
- URL/routing matters

### View Specs
**Required Coverage**:
- Conditional rendering logic
- Complex view logic

**Use When**:
- Significant conditional logic in views
- Avoiding duplicate feature specs

### Helper Specs
**Required Coverage**:
- All public helper methods

### Mailer Specs
**Required Coverage**:
- Email sent to correct recipients
- Correct subject
- Body contains expected content

---

## Four Phase Test Pattern

Every test should follow:

```ruby
it "does something" do
  # Setup - create objects and data
  user = create(:user)
  
  # Exercise - execute the code being tested
  result = user.full_name
  
  # Verify - check expectations
  expect(result).to eq "John Doe"
  
  # Teardown - handled by framework
end
```

**Audit Check**: Tests should have clear separation between phases.

---

## Testing Antipatterns to Flag

### 1. Slow Tests

**Symptoms**:
- Test suite takes more than 5 minutes
- Developers avoid running tests

**Causes**:
- Too many feature specs
- Not using factories efficiently
- Unnecessary database hits

**Audit Check**: Flag if average spec takes > 100ms

### 2. Intermittent Failures

**Symptoms**:
- Tests pass/fail randomly
- "Works on my machine"

**Causes**:
- Shared state between tests
- Time-dependent tests
- Order-dependent tests
- Race conditions in async code

**Audit Check**: Look for `sleep`, time manipulation without proper cleanup

### 3. Brittle Tests

**Symptoms**:
- Tests break when implementation changes
- Tests coupled to HTML structure

**Causes**:
- Testing implementation not behavior
- Over-reliance on specific selectors
- Excessive mocking

**Audit Check**: Flag tests with hardcoded CSS selectors, deep mocking

### 4. Duplication

**Symptoms**:
- Same setup code repeated
- Similar tests with minor variations

**Causes**:
- Missing shared examples
- Missing custom matchers
- Over-extracted test helpers

**Audit Check**: Look for repeated `let` blocks, identical setup

### 5. Mystery Guest

**Symptoms**:
- Test data defined elsewhere
- Hard to understand what test depends on

**Causes**:
- Over-use of fixtures
- Factory defaults that matter

**Audit Check**: Flag fixtures usage, flag factories with too many defaults

### 6. Stubbing System Under Test

**Symptoms**:
- Test stubs the object it's testing

**Causes**:
- Testing implementation details
- Poorly designed code

**Audit Check**: Flag `allow(subject).to receive(...)`

### 7. False Positives

**Symptoms**:
- Test passes but code is broken

**Causes**:
- Not testing the right thing
- Overly broad assertions

**Audit Check**: Look for `expect(page).to have_content("")`

### 8. Using Factories Like Fixtures

**Symptoms**:
- Named factories for every scenario
- `create(:admin_user_with_premium_subscription)`

**Causes**:
- Misunderstanding factory purpose

**Audit Check**: Flag factories with many trait combinations

### 9. Bloated Factories

**Symptoms**:
- Factories create unnecessary associations
- Factory creates too much data

**Causes**:
- Adding defaults "just in case"

**Audit Check**: Flag factories with > 5 attributes, unnecessary associations

### 10. Over-use of `let`, `subject`, `before`

**Symptoms**:
- Tests hard to read
- Must scroll to understand test

**Causes**:
- DRY taken too far in tests

**Audit Check**: Flag tests with > 5 `let` statements

---

## Coverage Requirements by File Type

| File Type | Min Coverage | Test Type |
|-----------|--------------|-----------|
| Model | 90% | Model spec |
| Controller | 80% | Request/Controller spec |
| Service/PORO | 95% | Unit spec |
| Helper | 100% | Helper spec |
| Mailer | 100% | Mailer spec |
| Job | 90% | Job spec |

---

## Missing Test Detection

For each Ruby file in `app/`:

1. Check for corresponding spec:
   - `app/models/user.rb` → `spec/models/user_spec.rb`
   - `app/controllers/users_controller.rb` → `spec/controllers/users_controller_spec.rb` or `spec/requests/users_spec.rb`

2. Check public methods are tested:
   - Extract public method names from source
   - Search for those names in spec file

3. Report:
   - Files without any tests → **High** severity
   - Files with partial coverage → **Medium** severity

---

## FactoryBot Best Practices

**Good Factory**:
```ruby
factory :link do
  title { "Testing Rails" }
  url { "http://example.com" }
  # Only required fields with sensible defaults
end
```

**Bad Factory**:
```ruby
factory :link do
  title { "Testing Rails" }
  url { "http://example.com" }
  upvotes { 10 }  # Not required
  user  # Creates unnecessary association
  created_at { 1.day.ago }  # Unnecessary
end
```

---

## RSpec Best Practices

**Good Test Structure**:
```ruby
RSpec.describe Link, "#score" do
  it "returns upvotes minus downvotes" do
    link = build(:link, upvotes: 5, downvotes: 2)
    
    expect(link.score).to eq 3
  end
end
```

**Avoid**:
- Nested contexts more than 2 levels deep
- `it` blocks without clear descriptions
- Multiple expectations per test (usually)
- Testing private methods directly