Ruby on Rails code review standards

# Ruby on Rails code review standards

Follow these standards when writing or changing code in this repository. Code review flags changes that break them.

## Prevent N+1 Queries in Rails

Applies to `**/*.rb`, `**/*.rake`, `**/*.erb`, `**/*.jbuilder`, `**/*.haml`, `**/*.slim`.

Review Active Record access in controllers, views, serializers, mailers, and jobs for queries that run once per record.

Flag changes that:
- Iterate records (`each`, `map`, `find_each`, `render collection:`, `json.array!`) and read a `belongs_to`, `has_one`, or `has_many` association inside the loop when the relation has no `includes`, `preload`, or `eager_load` for it.
- Call `count` or `exists?` on an association for each row. `count` always runs a query, even on a preloaded association; use `size` with a preload, a `counter_cache`, or one grouped count keyed by id.
- Chain `where`, `order`, `limit`, or `find_by` onto an association inside a loop. Each call builds a new query, so preloading the association does not help; preload a scoped association instead.
- Read an association in a serializer, jbuilder view, partial, or helper when the controller that renders it does not preload it.
- Preload only the first level, such as `includes(:author)`, when the loop reads a nested association such as `author.company`.
- Remove an `includes`, `preload`, or `eager_load` call while rendering code still reads that association.

Do not flag:
- Code that handles a single record, such as a show action or a job that loads one row.
- Relations marked `strict_loading` in apps that keep the default `action_on_strict_loading_violation = :raise`, where a missed preload raises instead of querying. Under `:log`, the lazy load still queries, so this exemption does not apply.
- Loops over a small, fixed collection, such as a hard-coded list of a few records.

## Block Mass Assignment and Unscoped Finds in Rails

Applies to `**/app/controllers/**/*.rb`, `**/app/models/**/*.rb`, `**/app/policies/**/*.rb`.

Review Rails controllers and strong parameters for mass assignment and record lookups that skip ownership or tenant checks.

Flag changes that:
- Call `params.permit!` or `to_unsafe_h` and pass the result to `new`, `create`, `update`, or `assign_attributes`.
- Permit attributes that grant privileges or move ownership, such as `role`, `admin`, `account_id`, `user_id`, `owner_id`, or `organization_id`, in `permit` or `expect` calls for controllers ordinary users can reach.
- Load a record by a user-supplied id with `Model.find`, `find_by`, or `where(id: ...)` instead of through `current_user`, the current tenant, or a policy scope.
- Scope through a parent looked up from params, such as `Account.find(params[:account_id]).projects`, without checking that the current user belongs to that parent.
- Accept a foreign key such as `project_id` from params when creating a record without checking that the referenced record belongs to the current user or tenant.
- Remove or skip an authorization call on a changed action, such as Pundit's `authorize` or `policy_scope`, or CanCanCan's `load_and_authorize_resource`.

Do not flag:
- Actions that run an authorization check (`authorize`, `policy_scope`, `accessible_by`, `load_and_authorize_resource`) on the record before using or returning it.
- Admin controllers behind an admin-only `before_action` that intentionally edit roles or ownership.
- Records that are public by design, such as published articles looked up by slug.

## Enforce Zero-Downtime Migrations in Rails

Applies to `**/db/migrate/**/*.rb`, `**/app/models/**/*.rb`.

Review Rails migrations that run on PostgreSQL for changes that lock busy tables or break app servers still running the previous release.

Flag changes that:
- Call `add_index`, `remove_index`, or `add_reference` (which adds an index by default) on an existing table without `algorithm: :concurrently` and `disable_ddl_transaction!`.
- Add a foreign key to an existing table without `validate: false`, followed by `validate_foreign_key` in a later migration.
- Call `remove_column` without first adding the column to the model's `ignored_columns` in an earlier deploy.
- Call `rename_column` or `rename_table` on a table the running release still queries, instead of adding the new column or table, backfilling, and switching reads first.
- Change a column's type with `change_column`, which rewrites the table for most type changes, or call `change_column_null(..., false)` on a large table, which scans it under an exclusive lock.
- Backfill rows (`update_all`, `find_each` with `save`) in the same migration as schema changes, or in one unbatched statement over a large table.
- Call application models from a migration; their validations, callbacks, and default scopes can change after the migration is written.

Do not flag:
- Indexes, references, and constraints on a table created earlier in the same migration.
- `change_column_null(..., false)` after a validated `CHECK (column IS NOT NULL)` constraint, which lets PostgreSQL 12+ skip the table scan.
- Apps on databases other than PostgreSQL, where `algorithm: :concurrently` does not apply.
- Operations wrapped in strong_migrations' `safety_assured` with a comment explaining why they are safe.

## Run Side Effects After Commit in Rails Callbacks

Applies to `**/app/**/*.rb`, `**/lib/**/*.rb`.

Review Active Record callbacks, models, and service objects for side effects that run before the database transaction commits.

Flag changes that:
- Send email (`deliver_now`, `deliver_later`), enqueue jobs, publish messages, or call external APIs from `after_save`, `after_create`, `after_update`, or `after_destroy` instead of `after_commit`, `after_create_commit`, `after_update_commit`, or `after_save_commit`.
- Enqueue a job, send an email, or call an external API inside a `transaction` block. A later failure rolls back the rows but not the side effect, and a fast worker can start before the commit and not find the record.
- Make HTTP requests or other slow I/O in callbacks that run inside the transaction, holding row locks and a database connection for the length of the call.
- Register `after_create_commit` and `after_update_commit` with the same method name. Only the last registration takes effect; use `after_save_commit` or `after_commit on: [:create, :update]`.
- Check `status_changed?`, `changes`, or `status_was` in `after_save` or `after_commit`, which already reflect the saved state; use `saved_change_to_status?`, `saved_changes`, or `status_before_last_save`.
- Rely on a callback for a required side effect while the same change writes through `update_all`, `update_columns`, `insert_all`, or `upsert_all`, which skip callbacks.

Do not flag:
- Jobs and `deliver_later` calls enqueued through Active Job when `enqueue_after_transaction_commit` is enabled for the app or the job, which defers the enqueue until the transaction commits.
- Writes to a transactional outbox table that a separate process publishes after commit.
- Callbacks that only update the record's own attributes or other rows in the same database.

## Use Batch Processing for Large Tables in Rails

Applies to `**/*.rb`, `**/*.rake`.

Review Active Record code in jobs, rake tasks, controllers, and models for work that loads or processes large tables all at once.

Flag changes that:
- Iterate a relation that can grow without bound with `each`, `map`, or `to_a` (`User.all.each`, `Order.where(...).map`) instead of `find_each`, `find_in_batches`, or `in_batches`.
- Load full records to read one column, such as `map(&:id)` or `map(&:email)` on an unloaded relation, instead of `pluck`, `ids`, or a subquery like `where(user_id: User.active.select(:id))`.
- Compute counts or totals in Ruby (`map(&:amount).sum`, `to_a.size`, `length`, `group_by`) instead of `count`, `sum`, or `group` in SQL.
- Call `present?` or `blank?` on an unloaded relation only to test for rows. Both load every matching record; `exists?` runs a `LIMIT 1` query.
- Call `destroy_all`, or update records one by one, over a large relation when callbacks are not needed, instead of `in_batches` with `delete_all` or `update_all`.
- Chain `order` onto a relation before `find_each`, `find_in_batches`, or `in_batches` and rely on that order. Batch methods ignore the relation's order clause and iterate by the cursor column, the primary key by default.
- Return or render an unbounded relation from a controller action or API endpoint without a limit or pagination.

Do not flag:
- Relations bounded by a small limit, a lookup table, or a parent's `has_many` that stays small by design.
- Records already loaded for other work in the same method, where mapping over them adds no query.
- `find_each` loops that save records one at a time because validations or callbacks must run.

## Flag Security Vulnerabilities

Scan for security flaws: hardcoded secrets, SQL injection, XSS, weak auth, exposed data, poor crypto, unsafe deserialization, insecure HTTP calls in production, and more. Ensure all network requests use HTTPS/TLS. Identify attack vectors and security violations to maintain robust defenses.

## Require API Auth, Validation, and Tenant Scoping

Review API handlers and controller endpoints for missing authentication, authorization, tenant scoping, and request validation.

Flag new or modified endpoints that:
- Accept request bodies without runtime validation, such as Zod schemas, DTOs, or framework-native validators.
- Trust user-supplied identifiers for tenant, organization, or account access instead of deriving access from the authenticated session.
- Return sensitive internal errors or use inconsistent HTTP status codes for 400, 401, 403, 404, and 500 cases.
- Mix reads and writes in a way that makes side effects hard to reason about.
- Add public or high-volume endpoints without rate limiting or abuse controls.

## Prevent Database Query Performance Regressions

Flag database access patterns that can degrade production performance, including N+1 queries, unbounded list endpoints, missing pagination, full-table scans, missing indexes on selective filters, per-row writes in loops, and queries that load more columns or rows than needed. Prefer bounded reads, batching, selective indexes, and query shapes that scale with table size.

## Block Unsafe Database Migrations

Review database migrations for operations that can lock large tables, rewrite full tables, run long backfills, create blocking indexes, or require full table scans. Prefer phased rollouts, concurrent indexes, nullable columns followed by backfills, batched data changes, and explicit rollback or recovery plans for risky schema changes.

## Prevent Sensitive Data in Logs

Logging, error reporting, analytics events, and audit trails must not expose sensitive data. Flag code that logs PII, emails, passwords, API keys, auth tokens, session identifiers, payment data, or raw request/response bodies without redaction. Error serialization must preserve enough context to debug while sanitizing secrets and user data.

## Enforce Pragmatic Test Coverage

Be reasonable when evaluating tests. Flag changes when important behavior is untested, but do not require tests for every export, type, config, metadata file, or mechanical change.

Flag PRs that:
- Change core business logic without covering the main success and failure paths.
- Add UI behavior without testing the user-visible interaction or state change.
- Add tests that only assert rendering, existence, snapshots, or that a function returns any value.
- Skip, mute, or weaken existing tests without a clear reason.
- Mock so much of the system that the test no longer validates the behavior the PR claims to change.

## Enforce Rollback Steps in Database Migrations

Each database migration file must include a "down" or "rollback" section that reverses the changes. This ensures all database changes can be safely undone if deployment issues occur. Migration files should clearly separate "up" and "down" migrations using appropriate method names or comments.