Code Review Standards

Follow REST conventions for predictable APIs: - GET /resources - List all - GET /resources/:id - Show one - POST /resources - Create - PUT/PATCH /resources/:id - Update - DELETE /resources/:id - Destroy Use nested routes for relationships: - GET /users/:user_id/orders - User's orders - POST /orders/:order_id/items - Add item to order Return appropriate HTTP status codes: - 200 OK, 201 Created, 204 No Content - 400 Bad Request, 401 Unauthorized, 404 Not Found - 500 Internal Server Error

Use a consistent error response format across all endpoints: ```json { "error": { "code": "validation_error", "message": "Validation failed", "details": [ { "field": "email", "message": "is invalid" }, { "field": "name", "message": "can't be blank" } ] } } ``` Include: - Machine-readable error code - Human-readable message - Field-level details for validation errors

Include version in your API URLs or headers: - URL versioning: `/api/v1/users` - Header versioning: `Accept: application/vnd.api+json; version=1` This allows you to: - Make breaking changes without affecting existing clients - Gradually migrate clients to new versions - Deprecate old versions with advance notice ```ruby # Rails routes namespace :api do namespace :v1 do resources :users end namespace :v2 do resources :users end end ```

Follow 12-factor app principles for configuration: - Store config in environment variables, not in code - Use .env files for local development (never commit .env) - Commit .env.example with dummy values to document required variables - Never hardcode secrets, API keys, or connection strings This enables the same codebase to run in any environment.

Encrypt or hash sensitive data: - Passwords: Use bcrypt (has_secure_password in Rails) - API keys: Encrypt with application-level encryption - PII: Consider field-level encryption - Never log sensitive data ```ruby class User < ApplicationRecord has_secure_password # Handles bcrypt hashing encrypts :ssn # Rails 7+ encryption end ```

Always add database indexes for: - Foreign keys (Rails doesn't add these automatically) - Columns used in WHERE clauses - Columns used in ORDER BY - Columns used in JOINs ```ruby # Migration add_index :orders, :user_id add_index :orders, :status add_index :orders, [:user_id, :created_at] # Composite for user's recent orders ``` Use EXPLAIN ANALYZE to verify indexes are being used.

Wrap multi-step database operations in transactions: - Ensures all-or-nothing execution - Prevents partial updates on failure - Maintains data consistency ```ruby ActiveRecord::Base.transaction do order.update!(status: 'paid') Payment.create!(order: order, amount: order.total) InventoryService.decrement(order.items) end # If any step fails, all changes are rolled back ```

When adding or updating dependencies, always use the latest stable version: - Check the official registry before adding (rubygems.org, npmjs.com, pypi.org) - Security vulnerabilities are fixed in newer versions - Performance improvements accumulate over time Do NOT use older versions. If the latest version has breaking changes, fix the code to work with the new API. Never assume migration is "too hard" without actually attempting the upgrade first.

Use FastAPI's Depends() for dependency injection: - Database sessions - Authentication - Configuration - External services ```python from fastapi import Depends from sqlalchemy.ext.asyncio import AsyncSession async def get_db() -> AsyncGenerator[AsyncSession, None]: async with async_session() as session: yield session async def get_current_user( token: str = Depends(oauth2_scheme), db: AsyncSession = Depends(get_db) ) -> User: ... @app.get("/me") async def get_me(user: User = Depends(get_current_user)): return user ```

Use async database drivers with FastAPI: - asyncpg for PostgreSQL - SQLAlchemy 2.0 async mode - databases package for simple queries ```python from sqlalchemy.ext.asyncio import create_async_engine, AsyncSession engine = create_async_engine( "postgresql+asyncpg://user:pass@localhost/db", echo=True, ) async def get_user(db: AsyncSession, user_id: int) -> User | None: result = await db.execute( select(User).where(User.id == user_id) ) return result.scalar_one_or_none() ```

Define explicit Pydantic models for all API inputs and outputs: - Automatic validation and documentation - Type safety with IDE support - Clear API contract ```python from pydantic import BaseModel, Field class UserCreate(BaseModel): email: str = Field(..., description="User email address") name: str = Field(..., min_length=1, max_length=100) class UserResponse(BaseModel): id: int email: str name: str created_at: datetime class Config: from_attributes = True @app.post("/users", response_model=UserResponse) async def create_user(user: UserCreate) -> UserResponse: ... ```

Make small, focused commits that do one thing well. Use Conventional Commits: - feat: new feature - fix: bug fix - refactor: code restructuring without behavior change - test: adding or updating tests - docs: documentation only - chore: maintenance tasks Example: `feat: add user authentication endpoint` Each commit should be independently deployable and revertible.

Always use `const` by default, `let` when reassignment is needed: - `const`: Block-scoped, cannot be reassigned - `let`: Block-scoped, can be reassigned - `var`: Function-scoped, hoisted - NEVER USE ```typescript // Good const users = await fetchUsers(); let count = 0; for (const user of users) { count++; } // Bad var users = await fetchUsers(); ```

Prefer async/await for asynchronous code: - More readable than callbacks or .then() chains - Easier error handling with try/catch - Works naturally with loops and conditionals ```typescript // Good async function fetchUserData(id: string): Promise<User> { try { const response = await fetch(`/api/users/${id}`); if (!response.ok) throw new Error('Failed to fetch'); return await response.json(); } catch (error) { logger.error('Failed to fetch user', { id, error }); throw error; } } // Avoid function fetchUserData(id) { return fetch(`/api/users/${id}`) .then(res => res.json()) .catch(err => console.log(err)); } ```

Always use TypeScript instead of plain JavaScript: - Catch errors at compile time - Better IDE support and refactoring - Self-documenting code - Safer dependency updates Use strict mode for maximum benefit: ```json { "compilerOptions": { "strict": true, "noImplicitAny": true, "strictNullChecks": true } } ```

Configure ESLint with recommended rules: ```json { "extends": [ "eslint:recommended", "plugin:@typescript-eslint/recommended", "plugin:@typescript-eslint/recommended-requiring-type-checking" ], "rules": { "@typescript-eslint/no-unused-vars": "error", "@typescript-eslint/no-explicit-any": "error", "no-console": "warn" } } ``` Run ESLint in CI and pre-commit hooks.

Prohibited excuses: "pre-existing", "unrelated errors", "would require refactoring", "tedious", "for now", "at this point", "tired", "frustrating", "time-consuming". These indicate you're making excuses. Recognize them and continue working.

You do not define success. The user and the test suite do. Do not redefine scope, declare victory on your terms, or summarize partial progress as completion. Done means: tests pass, linter passes, formatter run.

You are the only coder. Every line was written by you (this or previous sessions). There are no "pre-existing issues" - only issues you created and haven't fixed. Do not attribute problems to external sources or wait for someone else.

Use `uv` as the default Python package manager - it's the preferred choice: - 10-100x faster than pip/poetry - Built-in virtual environment management - Lockfile support (uv.lock) for reproducible builds - Compatible with pyproject.toml and requirements.txt - Rust-based, single binary, no Python dependency ```bash # Install uv curl -LsSf https://astral.sh/uv/install.sh | sh # Create new project uv init myproject cd myproject # Add dependencies uv add fastapi sqlalchemy pydantic # Add dev dependencies uv add --dev pytest ruff mypy # Run commands uv run python main.py uv run pytest ``` NEVER use pip, poetry, or pipenv for new projects - always use uv.

Use async/await for I/O-bound operations: - HTTP requests (use httpx or aiohttp) - Database queries (use asyncpg, databases) - File operations (use aiofiles) ```python import httpx async def fetch_users() -> list[User]: async with httpx.AsyncClient() as client: response = await client.get("https://api.example.com/users") return [User(**u) for u in response.json()] ``` Don't mix sync and async code - choose one paradigm per service.

Standard uv project structure: ``` myproject/ ├── pyproject.toml # Project config and dependencies ├── uv.lock # Locked dependencies (commit this!) ├── .python-version # Python version (e.g., "3.12") ├── src/ │ └── myproject/ │ ├── __init__.py │ └── main.py └── tests/ └── test_main.py ``` Key files: - `pyproject.toml`: Define dependencies here, not requirements.txt - `uv.lock`: Always commit - ensures reproducible builds - `.python-version`: uv auto-installs the correct Python version

Add type hints to all function signatures and class attributes: - Enables static analysis with mypy/pyright - Improves IDE autocompletion - Documents expected types - Catches type errors before runtime ```python from typing import Optional def process_user(user_id: int, name: str | None = None) -> dict[str, Any]: ... class UserService: def __init__(self, repo: UserRepository) -> None: self.repo = repo ```

Use Black for formatting and Ruff for linting: - Black: Uncompromising code formatter (no configuration debates) - Ruff: Extremely fast linter (replaces flake8, isort, pylint) ```toml # pyproject.toml [tool.black] line-length = 88 [tool.ruff] line-length = 88 select = ["E", "F", "I", "N", "W", "UP"] ``` Run both in CI and pre-commit hooks.

Use dataclasses for simple data containers, Pydantic for validation: ```python from dataclasses import dataclass from pydantic import BaseModel # Simple data container @dataclass class Point: x: float y: float # With validation (API models, config) class UserCreate(BaseModel): email: str name: str age: int = Field(ge=0, le=150) ``` Avoid plain dicts for structured data - they lack type safety and validation.

Define scripts in pyproject.toml for common commands: ```toml [project.scripts] myapp = "myproject.main:main" [tool.uv.scripts] dev = "fastapi dev src/myproject/main.py" test = "pytest tests/ -v" lint = "ruff check src/" format = "ruff format src/" typecheck = "mypy src/" ``` Run with: `uv run dev`, `uv run test`, etc. For CI/production: ```bash # Install dependencies only (no dev) uv sync --no-dev # Export to requirements.txt if needed uv export > requirements.txt ```

Use uv workspaces for multi-package Python projects: ```toml # Root pyproject.toml [tool.uv.workspace] members = ["packages/*"] [tool.uv.sources] mylib = { workspace = true } ``` Structure: ``` monorepo/ ├── pyproject.toml # Workspace root ├── uv.lock # Single lockfile for all packages └── packages/ ├── mylib/ │ ├── pyproject.toml │ └── src/mylib/ └── myapp/ ├── pyproject.toml # depends on mylib └── src/myapp/ ``` Benefits: - Single lockfile across all packages - Local package editable installs - Atomic dependency updates

Use pytest as your testing framework: - Simple assert statements - Powerful fixtures for setup/teardown - Parametrized tests for multiple inputs - Rich plugin ecosystem ```python import pytest @pytest.fixture def user(): return User(name="Test", email="[email protected]") def test_user_full_name(user): assert user.full_name == "Test" @pytest.mark.parametrize("input,expected", [ (1, 1), (2, 4), (3, 9) ]) def test_square(input, expected): assert square(input) == expected ```

Always use Strong Parameters to whitelist attributes: - Define permitted params in a private method - Never use `.permit!` which allows all attributes - Be explicit about nested attributes - Use `require` for the root key ```ruby private def user_params params.require(:user).permit(:name, :email, address_attributes: [:street, :city]) end ```

Use Rails built-in authentication (has_secure_password, authenticate_by) for all authentication logic. Never use Devise, Sorcery, Clearance, or similar authentication gems. Rails 8 provides everything needed: - has_secure_password for password hashing - authenticate_by for secure credential lookup - generates_token_for for password resets and email verification ```ruby class User < ApplicationRecord has_secure_password generates_token_for :password_reset, expires_in: 15.minutes end ```

Use eager loading to prevent N+1 queries: - `includes`: For associations you'll access - `preload`: Force separate queries (useful for complex conditions) - `eager_load`: Force LEFT OUTER JOIN Use the Bullet gem in development to detect N+1 queries automatically. ```ruby # Bad: N+1 queries @posts = Post.all @posts.each { |p| p.author.name } # Queries author for each post # Good: Eager loading @posts = Post.includes(:author) @posts.each { |p| p.author.name } # Single query for authors ```

Keep controllers thin - they should only: - Authenticate and authorize - Parse params and set instance variables - Call model/service methods - Render response Business logic belongs in: - Models (for single-model operations) - Service objects (for multi-model operations) - Form objects (for complex form handling) - Query objects (for complex queries) ```ruby # Good: Thin controller def create @order = OrderCreator.new(order_params, current_user).call respond_with @order end ```

For Rails 7+ applications, prefer Hotwire (Turbo + Stimulus) over React/Vue: - Turbo Drive: Automatic AJAX page transitions - Turbo Frames: Partial page updates without JavaScript - Turbo Streams: Real-time updates over WebSocket - Stimulus: Lightweight JavaScript for sprinkles of behavior Only use a JavaScript framework when you need complex client-side state management (e.g., collaborative editing, complex forms, offline support).

Don't rely solely on ActiveRecord validations - add database constraints: - NOT NULL for required fields - UNIQUE indexes for unique fields - Foreign key constraints for associations - Check constraints for business rules ```ruby # Migration add_column :users, :email, :string, null: false add_index :users, :email, unique: true add_foreign_key :orders, :users ``` Database constraints are your last line of defense against bad data.

Move slow operations to background jobs: - Email sending - File processing - External API calls - Report generation - Data imports/exports Use SolidQueue (Rails 8) or Sidekiq. Keep web requests under 200ms. ```ruby # Good: Background job SendWelcomeEmailJob.perform_later(user) # Bad: Synchronous in controller UserMailer.welcome(user).deliver_now ```

Use Minitest for all tests. Do not add RSpec to the project. Minitest is: - Rails default, zero configuration - Faster boot time - Simpler syntax, less magic - Already integrated with fixtures ```ruby class UserTest < ActiveSupport::TestCase test "validates email presence" do user = User.new(name: "Test") assert_not user.valid? assert_includes user.errors[:email], "can't be blank" end end ``` Use fixtures for test data - they're fast and simple.

Always use `has_many :through` instead of `has_and_belongs_to_many`: - Allows adding attributes to the join model - Provides a model for the join table (validations, callbacks) - More flexible for future requirements - Easier to query and understand ```ruby # Good class User < ApplicationRecord has_many :memberships has_many :teams, through: :memberships end # Avoid class User < ApplicationRecord has_and_belongs_to_many :teams end ```

Define scopes for frequently used query conditions: - Makes code more readable - Enables method chaining - Centralizes query logic - Easier to test ```ruby class Order < ApplicationRecord scope :recent, -> { where('created_at > ?', 1.week.ago) } scope :completed, -> { where(status: 'completed') } scope :for_user, ->(user) { where(user: user) } end # Usage Order.recent.completed.for_user(current_user) ```

Use a data fetching library instead of useEffect + fetch: - Automatic caching and revalidation - Loading and error states - Optimistic updates - Request deduplication ```tsx import { useQuery } from '@tanstack/react-query'; function UserList() { const { data: users, isLoading, error } = useQuery({ queryKey: ['users'], queryFn: () => fetch('/api/users').then(r => r.json()), }); if (isLoading) return <Loading />; if (error) return <Error message={error.message} />; return <List items={users} />; } ```

Always use functional components with hooks, not class components: - Simpler, less boilerplate - Better TypeScript support - Easier to test - Hooks enable code reuse without HOCs ```tsx // Good function UserProfile({ userId }: { userId: string }) { const [user, setUser] = useState<User | null>(null); useEffect(() => { fetchUser(userId).then(setUser); }, [userId]); if (!user) return <Loading />; return <Profile user={user} />; } // Avoid class components class UserProfile extends React.Component { ... } ```

When props need to pass through multiple levels, use Context or state management: - React Context for static/rarely changing data - Zustand/Jotai for simple state - Redux Toolkit for complex state with dev tools ```tsx // Context for auth const AuthContext = createContext<AuthState | null>(null); export function AuthProvider({ children }) { const [user, setUser] = useState<User | null>(null); return ( <AuthContext.Provider value={{ user, setUser }}> {children} </AuthContext.Provider> ); } export const useAuth = () => useContext(AuthContext); ```

In Ruby, favor composition and modules over deep inheritance hierarchies: - Use modules for shared behavior (concerns in Rails) - Inject dependencies rather than inheriting from base classes - Keep inheritance depth to 2-3 levels maximum - Use duck typing - if it quacks like a duck, treat it like a duck ```ruby # Good: Composition class OrderProcessor def initialize(payment_gateway:, notifier:) @payment_gateway = payment_gateway @notifier = notifier end end # Avoid: Deep inheritance class SpecialOrderProcessor < OrderProcessor < BaseProcessor < ApplicationService ```

Add `# frozen_string_literal: true` to the top of Ruby files: - Prevents accidental string mutation - Improves memory usage and performance - Catches bugs where strings are mutated unexpectedly Configure RuboCop to enforce this automatically.

Be intentional about method return values: - Methods that perform actions should return meaningful results or self - Query methods should return the queried value - Predicate methods (ending in ?) should return true/false - Bang methods (ending in !) should modify in place or raise ```ruby # Good: Clear intent def process_order validate! charge_payment send_confirmation self # Allow chaining end # Good: Predicate returns boolean def valid? = errors.empty? ```

Leverage modern Ruby features for cleaner code: - Pattern matching: `case obj in { name:, age: } then ...` - Endless methods: `def square(x) = x * x` - Hash shorthand: `{ x:, y: }` instead of `{ x: x, y: y }` - Numbered block parameters: `array.map { _1 * 2 }` - Data classes for simple value objects Keep Ruby version at 3.2+ for best performance and features.

Separate authentication (who are you?) from authorization (what can you do?): Authentication: - Use secure password hashing (bcrypt) - Implement rate limiting on login - Use secure session management - Consider MFA for sensitive applications Authorization: - Check permissions on every request - Use policy objects or authorization gems (Pundit, CanCanCan) - Never rely on client-side checks alone ```ruby # Pundit example def update @post = Post.find(params[:id]) authorize @post # Raises unless user can update @post.update!(post_params) end ```

Force HTTPS for all traffic: - Configure SSL/TLS in production - Redirect HTTP to HTTPS - Use secure cookies (Secure, HttpOnly, SameSite) - Set HSTS headers ```ruby # Rails config/environments/production.rb config.force_ssl = true config.ssl_options = { hsts: { expires: 1.year, subdomains: true } } ```

Never trust user input - validate and sanitize everything: - Use allowlists, not denylists - Validate type, length, format, and range - Sanitize HTML to prevent XSS - Use parameterized queries to prevent SQL injection ```ruby # Rails automatically escapes in views, but be explicit with user HTML: sanitize(user_content, tags: %w[p br strong em]) # Always use parameterized queries (ActiveRecord does this by default): User.where(email: params[:email]) # Safe User.where("email = '#{params[:email]}'") # DANGEROUS - SQL injection ```

When writing unit tests, never mock the class you are testing. You should test the real instance of the class to ensure it behaves correctly. Mocking the class under test defeats the purpose of the test and can hide bugs.

If you encounter a test failure, do not ignore it or mark it as skipped. - Investigate the root cause immediately - If it's a real bug, fix it before proceeding - If it's a flaky test, fix the flakiness - Never wave off failures as "pre-existing" - own the codebase state A test suite with skipped or ignored tests is a test suite you can't trust.

Write tests that verify behavior, not internal implementation: - Test public interfaces, not private methods - Focus on inputs and outputs - Tests should survive refactoring - If implementation changes but behavior doesn't, tests should pass ```ruby # Good: Test behavior test "order total includes tax" do order = Order.new(items: [Item.new(price: 100)]) assert_equal 108, order.total # 8% tax end # Bad: Test implementation test "order calls calculate_tax method" do order = Order.new(items: [Item.new(price: 100)]) assert order.instance_variable_get(:@tax_calculated) end ```

Follow Test-Driven Development for new features and bug fixes: 1. RED: Write a failing test that defines the expected behavior 2. GREEN: Write the minimum code to make the test pass 3. REFACTOR: Clean up the code while keeping tests green This ensures every line of code has a reason to exist and is tested.

Avoid mocking internal code or private methods. Mocks should be used only at system boundaries: - External HTTP APIs (use WebMock, VCR, or similar) - Time-dependent code (freeze time with travel_to) - File system operations that would persist outside tmp/ or create side effects - Third-party services with rate limits or costs Internal services, database operations, and business logic should use real implementations to ensure proper integration.

Each test should verify one logical concept: - Multiple assertions are fine if they test the same thing - Separate tests for separate behaviors - Clear test names that describe the behavior ```ruby # Good: One concept test "user is invalid without email" do user = User.new(name: "Test") assert_not user.valid? assert_includes user.errors[:email], "can't be blank" end # Bad: Multiple concepts test "user validation" do user = User.new assert_not user.valid? # Missing everything user.email = "[email protected]" assert_not user.valid? # Missing name user.name = "Test" assert user.valid? # Now valid end ```

For web applications, prefer integration/request tests over isolated unit tests: - Test the full request/response cycle - Catch issues with middleware, routing, authentication - More confidence that the feature actually works ```ruby # Good: Integration test test "user can create a post" do sign_in users(:alice) post posts_path, params: { post: { title: "Hello", body: "World" } } assert_redirected_to post_path(Post.last) assert_equal "Hello", Post.last.title end # Unit tests are still valuable for: # - Complex business logic # - Edge cases # - Performance-critical code ```

Use fixtures for consistent test data. Fixtures are: - Fast: loaded at database level, not through ActiveRecord - Simple: YAML files, no DSL to learn - Stable: good for reference data that rarely changes ```ruby # test/fixtures/users.yml admin: name: Admin User email: [email protected] role: admin regular: name: Regular User email: [email protected] role: member ``` Reference in tests with `users(:admin)`.

Ensure your changes work on your local machine before pushing: - Run the application and manually verify changes - Run the full test suite - Check for console errors or warnings CI is for catching environment-specific issues, not basic functionality.

Before every commit, run the full quality pipeline: 1. Format code (prettier, black, rubocop --autocorrect, etc.) 2. Run linter (eslint, pylint, rubocop, etc.) 3. Run full test suite Never rely on CI to catch issues you could have caught locally. Configure pre-commit hooks to enforce this automatically.