Developer docs
Developer docs
Breadcrumbs

Code review checklist

Last Updated: July 20, 2026

This is a checklist of rules and guidance when conducting a code review for a PR on vets-api. For best practices for reviewing code, see the Code Reviews guide.

PR status checks

  • Check for failing CI checks
  • Check for teammate review/approval
  • Request very large PRs be broken up

Critical Focus Areas

  • PII exposure
  • Security issues
  • Feature flag usage
  • Silent failures
  • Code quality & maintainability
  • Database migration compatibility

Rules

Red Flags

  • Large diffs requiring multiple team approvals
  • Lack of context/test details in PR description
  • Untested code behind feature toggles
  • Developer testing in live production environment
  • Long procedural methods
  • Approval with obvious faults still present
  • Migrations affecting existing models
  • disable_ddl_transaction! in non-index migration
  • Large migrations with disable_ddl_transaction!
  • Breaking changes without backward compatibility plan
  • skip_before_action :verify_authenticity_token

Logging

No PII in Rails.logger calls. See this page for a list of PII, PHI. Review all log statements for potential PII exposure:

  • Response bodies that may contain veteran data
  • Request payloads or parameters containing PII
  • File contents, names, or metadata that could identify veterans
  • User identifiers, SSNs, names, addresses, or health information
  • Search Rails.logger in the PR to find usage.
  • Error logging includes only diagnostic info (status codes, error types, timestamps) is acceptable
  • If logging is necessary for debugging, use structured logging with explicitly safe fields

Database and Data Migrations

Consult the vets-api database migrations developer doc before reviewing.

Database Migrations

  • Verify vets-api hot compatibility (multiple versions must work with same schema)
  • Confirm no downtime required
  • Verify performant and won't negatively affect site operation
  • Check for zero_downtime_migrations gem protection
  • Verify migration uses up or change methods only (down methods will not work in production)
  • One change per migration (no multiple table changes)
  • Schema matches the migration exactly
  • No unsafe actions without safety_assured { }
  • Check for disable_ddl_transaction! (should ONLY be in index-only migrations)
  • Verify backward compatibility maintained
  • Check for missing/extra indexes
  • Verify column types match expectations

Strong Migrations-specific

These are checks from the strong_migrations gem. The gem generates warning messages if a migration contains one or more dangerous operations. These include:

  • adding an index non-concurrently
  • adding a foreign key
  • adding a unique constraint
  • adding a json column
  • setting NOT NULL on an existing column
  • adding a column with a volatile default value
  • keeping non-unique indexes to three columns or less

Data Migrations

  • Data migration is rake task (not Rails migration)
  • Located in lib/data_migrations/ with corresponding rakelib/ task
  • TODO comment or follow-up ticket to delete after production deployment

CODEOWNERS

  • File is valid (no errors)
  • Correct team included
  • Don’t add va-platform-backend as the sole owner on directories
  • GitHub team is accurate (ask if skeptical)
  • Directory ownership makes sense (owns all files in dir)
  • Multiple additions in one dir condensed to single line if possible
  • New additions are in alphabetical order

Feature toggles

Consult the Feature Toggles Guide before reviewing.

  • If toggle is being removed: Searched codebase; removed everywhere
  • If toggle is being introduced: Spelling checked; set to false by default; PR description contains: description, requirements, end date
  • If the code in the PR is behind a FT already: Tests cover BOTH disabled AND enabled scenarios; changes make sense behind toggle (not escaping testing)

Code readability and maintainability

  • Don't ignore RuboCop warnings without justification
  • Code is DRY (condensed and clean)
  • Code is well tested
  • Large methods broken into smaller ones
  • Avoid procedural programming (methods doing too much)
  • Should remove unused code
  • Method/variable names should be self-documenting
  • Comments explain WHY, not just WHAT
  • Avoid odd nesting
  • Avoid long object chaining unless justified (doesn't hinder readability/debugging)
  • Avoid bare rescues; specify exception class

Sidekiq

  • Retries explicitly specified (default 25 often inappropriate)
  • Timeouts implemented
  • No PII exposed
  • Passes ID not entire record (optimization)
  • Single purpose (not complex multi-step)
  • Error handling present (not failing silently)
  • Uses IDs not objects as arguments
  • Job tested locally
  • Variables accurately defined
  • If periodic job: scheduled at time when other jobs NOT running
  • Consult Sidekiq Job and Asynchronous Submission Standards before reviewing (requires access)

Errors and potential silent failures

  • Exception handling present and tested
  • Errors appropriately communicated to veteran/third party
  • Follows Zero Silent Failures principles
  • No silent failure modes identified

Test code

  • Test descriptions match actual tests (Copilot is pretty good at catching this)
  • Prefer testing behavior over implementation (ex: testing log outputs vs testing a method’s presence)
  • New code: Spec for each class
  • Modified code: Corresponding spec changes
  • Proper structure: Describe (what) → Context (when) → It (expected)
  • Tests good, bad, AND edge cases
  • Tests optimized (stubbing/mocking used appropriately)
  • Tests actually test something (not everything mocked away)
  • Private methods are tested indirectly through public methods
  • New features not over-mocked
  • For more details about testing best practices, consult Better Specs

VCR cassettes

  • No PII present (see Known Test Data below for exceptions)
  • No obvious hand edits or hard-coded values
  • Re-recorded if old cassette (not manually edited)

Known Test Data

Users are from the internal test-data list (requires access): mvi-staging-users.csv or other mock users. Do not paste SSN/ICN values into public docs or PR descriptions.

Additional checks

  • Don't re-invent solutions
  • Don't use default_scope. Use named scopes: scope :name
  • New gem: Did they go through the third-party library process?
  • Never use expect { }.not_to raise_error(SpecificErrorClass, message). Use expect { }.not_to raise_error
  • Don't use dynamic set_tag for Datadog (VA charged per tag: single tag OK, dynamic generation NOT OK)

Severity classification

Critical: must fix

These issues are blocking:

  • Anything under the Red Flags ("Yikes") section
  • Potential PII exposure
  • Potential security issues
  • Silent failure risk
  • Major code quality & maintainability issues (such as RuboCop warning disables)
  • DB/data migration issues
  • Untested feature toggle states
  • CODEOWNER issues

High: should fix

These issues are probably blocking, but up to reviewer discretion:

  • Minor code quality issues
  • Feature flag issues
  • Improper formatting feature toggle tests
  • Missing Sidekiq standards
  • Missing edge case tests

Medium: could fix

These issues are non-blocking and usually suggestions:

  • Code quality improvements
  • Suboptimal patterns

Low: nitpick

These issues are optional and trivial:

  • Style suggestions
  • Minor optimizations
  • Documentation improvements

Escalation policy

  • skip_before_action :verify_authenticity_token → Identity Team
  • Potential security issues → Platform Security Team

Terminology

This terminology is used to more accurately describe the above rules and what is acceptable during a code review. It’s not required/necessary to be used in code review comments. For example, if a PR contains a default_scope, this is not allowed unless the author has a good reason. If the reason is unclear, ask if default_scope is necessary and explain why it’s not a good practice.

Term

Meaning

Never

Never

Don't

Don't unless you have a great reason

Avoid

Avoid unless you have a good reason

Always

Always

Must

Must unless you have a great reason

Should

Should unless you have a good reason

Prefer

Others are acceptable, but one is better

Help and feedback