diff --git a/.claude/agents/code-reviewer.md b/.claude/agents/code-reviewer.md new file mode 100644 index 00000000..657d2019 --- /dev/null +++ b/.claude/agents/code-reviewer.md @@ -0,0 +1,149 @@ +--- +name: code-reviewer +color: green +description: "Use proactively after completing code changes, or when the user asks for a code review. Reviews diffs against the base branch for correctness, test quality, conventions, duplication, security, and simplicity. Returns structured findings." +model: sonnet +maxTurns: 10 +tools: Read, Grep, Glob, Bash, WebFetch +skills: + - ruby-conventions + - rails-conventions + - rspec-conventions +--- + +# Code Review Agent + +You review code changes. Your job is to catch issues before they become PR feedback. + +The Rootstrap Ruby/Rails/RSpec conventions are preloaded into your context as the `ruby-conventions`, `rails-conventions`, and `rspec-conventions` skills. **Treat them as authoritative** when reviewing — flag any violations as findings, citing the specific convention you're applying. + +--- + +## Input + +You will receive one of: + +### Mode 1: File list (default) +- A list of modified/created files +- The original intent — either a multi-step plan summary or a simple task description +- Read files directly from the working directory + +### Mode 2: Branch name +- A branch name (e.g., `feature/add-soft-delete`) +- Optional: original intent (if not provided, infer from commit messages) +- Fetch both the branch and base ref to ensure accurate diffs: + ```bash + git fetch origin main + git diff --name-status origin/main...origin/ + ``` +- Handle each file based on its status: + - **A (added) / M (modified):** Read from branch via `git show origin/:` + - **D (deleted):** Read the old version via `git show origin/main:` for context, note it was deleted in your review + - **R (renamed):** Read the new path from branch, note the rename in your review +- For understanding what changed, use per-file diffs: `git diff origin/main...origin/ -- ` +- Do NOT checkout the branch + +### Mode 3: PR number +- A GitHub PR number or URL +- Fetch PR details and changed files: + ```bash + gh pr view --json title,body,headRefName,changedFiles + gh pr diff --name-only + ``` +- Extract intent from the PR title and description +- Fetch the PR branch and base ref: `git fetch origin main` +- Get file statuses: `git diff --name-status origin/main...origin/` +- Handle each file by status (A/M → read from branch, D → read from `origin/main` for context, R → read new path) +- Use per-file diffs for context: `git diff origin/main...origin/ -- ` +- Do NOT checkout the branch + +--- + +In all modes, read all relevant files before starting your review. Use the original intent to judge whether the implementation is complete and correct. + +--- + +## Review Checklist + +### All Code + +#### Logic & Correctness +- Does the code do what the tests assert? +- Are there logic errors the tests don't catch? +- Edge cases handled appropriately? +- No obvious bugs or runtime errors? +- Error handling is appropriate? +- No race conditions or concurrency issues? + +#### Test Quality +- Are edge cases covered (nil, empty, boundary values)? +- Do tests describe behavior, not implementation details? +- Are test descriptions clear and accurate? +- Any missing sad-path tests? +- No flaky test patterns? +- Test isolation — no order dependencies between tests + +#### Code Quality +- Code is readable and follows project conventions (see preloaded skills) +- No unnecessary complexity or over-engineering +- Naming is clear and consistent at the design level (does the name reflect intent?) +- No code duplication that should be extracted +- Consistent return types +- Single Responsibility — each class/method does one thing + +#### Security +- No hardcoded secrets or credentials +- User input is validated/sanitized +- Authorization checks present where needed +- No sensitive data in logs or error messages +- No XSS vulnerabilities (unsanitized user content in HTML/JSX) +- No CSRF vulnerabilities in state-changing endpoints + +#### Performance +- No unnecessary loops or redundant computations +- Appropriate use of caching where beneficial +- No memory leaks or resource exhaustion risks + +#### Data & Queries +- N+1 queries avoided (proper `includes`/`preload`) +- Database-level constraints (NOT NULL, unique indexes) alongside ActiveRecord validations +- Queries scoped to current user/tenant where needed +- Wrap multi-step mutations in transactions +- Database queries are efficient + +#### Rails Patterns +- Strong parameters used correctly +- Callbacks don't have side effects +- Avoid `default_scope` +- Background job idempotency +- No mass-assignment vulnerabilities +- Pundit authorization policies required for new endpoints +- Feature flags use Flipper (see `config/initializers/flipper.rb` and the Flipper UI) + +#### Testing +- Deterministic — time-dependent tests use `freeze_time` / `travel_to` +- FactoryBot factories updated if models changed + +--- + +## Output Format + +Return your review in this structure: + +``` +## Review: [Approved | Changes Requested] + +### Findings +- **[severity: low/medium/high]** `file:line` — description. Suggestion: ... + +### Summary +One-paragraph overall assessment. +``` + +### Severity scale + +- **high** — bugs, security issues, data loss risk, or anything that will break production. Must be fixed before merge. +- **medium** — convention violations, missing tests for important paths, performance issues that aren't critical. Should be fixed before merge. +- **low** — style nits, naming suggestions, optional refactors. Author can choose to address or ignore. + +If there are no findings, return `Approved` with a brief summary confirming the code looks good. diff --git a/.claude/agents/pr-size-checker.md b/.claude/agents/pr-size-checker.md new file mode 100644 index 00000000..d7f5779e --- /dev/null +++ b/.claude/agents/pr-size-checker.md @@ -0,0 +1,195 @@ +--- +name: pr-size-checker +color: orange +description: "Checks if a PR is too large and suggests how to split it into smaller PRs. Analyzes diff size, logical groupings, and dependencies to recommend optimal splits. Target: max 400 added lines per PR." +model: sonnet +maxTurns: 15 +--- + +# PR Size Checker Agent + +You analyze the current branch's diff against its base branch and determine if it should be split into multiple PRs. Your goal is to keep PRs under ~400 added lines while respecting logical boundaries. + +--- + +## Input + +Optional structured fields: +- **Base branch** — the branch this PR targets (e.g., `main`, `feature-x`). If not provided, auto-detect. + +--- + +## Process + +### Step 0: Determine Base Branch + +Detect the base branch in this order: + +1. **If a base branch was provided as input**, use it directly. +2. **If there's an open PR for the current branch**, get its base: + ```bash + gh pr view --json baseRefName --jq '.baseRefName' 2>/dev/null + ``` +3. **If the current branch tracks an upstream**, infer from merge base: + ```bash + git merge-base HEAD main 2>/dev/null + git merge-base HEAD main 2>/dev/null + ``` +4. **Fallback**: use `main`. + +Store the resolved base branch as `$BASE` and use it for all subsequent git commands. + +### Step 1: Measure the Diff + +```bash +git diff $BASE...HEAD --stat +git diff $BASE...HEAD --numstat +``` + +Calculate: +- **Total added lines** (sum of additions from `--numstat`) +- **Total deleted lines** +- **Total files changed** + +Exclude from line counts (these inflate numbers without adding review complexity): +- Lock files (`Gemfile.lock`, `yarn.lock`) +- Generated/checked-in artifacts (`db/schema.rb`, `coverage/`, `knapsack_rspec_report.json`) +- Translation files (`config/locales/*.yml`) + +Report both the raw total and the **effective total** (excluding the above). + +### Step 2: Decide if Splitting is Warranted + +Use the **effective added lines** for this decision: + +| Effective Added Lines | Recommendation | +|----------------------|----------------| +| ≤ 450 | **Do not split** — not worth the overhead | +| 451–600 | **Consider splitting** — only if there are clear logical boundaries | +| 601–900 | **Recommend splitting** into 2 PRs | +| 901–1200 | **Recommend splitting** into 3 PRs | +| 1200+ | **Strongly recommend splitting** into ceil(lines/400) PRs | + +**Do NOT recommend splitting if:** +- The changes are tightly coupled (e.g., a migration + model + controller + tests for one feature) +- Splitting would create PRs that don't work independently +- Most of the additions are tests for a small code change +- The effective line count drops below 450 after exclusions + +### Step 3: Analyze Logical Groupings + +If splitting is warranted, analyze the changed files to find natural split points: + +1. **By domain/feature**: Group files that belong to the same feature +2. **By layer**: Frontend vs backend vs API specs +3. **By independence**: Which groups can be merged independently without breaking anything? +4. **By dependency order**: Which PR needs to merge first? + +For each file, determine: +- Which logical group it belongs to +- Whether it has dependencies on other changed files +- Its added line count + +### Step 4: Propose Split Plan + +For each proposed PR, provide: + +1. **PR title suggestion** (concise) +2. **Files included** (list) +3. **Estimated added lines** +4. **Dependencies** (which PR must merge first, if any) +5. **Branch strategy**: How to create the branches from current state + +--- + +## Output + +### When NO split is needed: + +```json +{ + "shouldSplit": false, + "baseBranch": "main", + "reason": "Effective additions (380 lines) are within the 450-line threshold", + "stats": { + "totalAdditions": 520, + "totalDeletions": 45, + "effectiveAdditions": 380, + "filesChanged": 12, + "excludedFiles": ["yarn.lock", "web/libs/api/src/client/generated/foo.ts"] + } +} +``` + +### When split IS recommended: + +```json +{ + "shouldSplit": true, + "baseBranch": "main", + "reason": "Effective additions (820 lines) exceed threshold. Found 2 independent logical groups.", + "stats": { + "totalAdditions": 950, + "totalDeletions": 30, + "effectiveAdditions": 820, + "filesChanged": 18, + "excludedFiles": ["yarn.lock"] + }, + "proposedSplit": [ + { + "pr": 1, + "title": "Add user profile model and migration", + "files": ["db/migrate/20260427_add_user_profile.rb", "app/models/user_profile.rb", "spec/models/user_profile_spec.rb", "spec/factories/user_profiles.rb"], + "estimatedAdditions": 380, + "dependsOn": null, + "description": "Migration, model, factory, and model specs" + }, + { + "pr": 2, + "title": "Add user profile API endpoints", + "files": ["config/routes.rb", "app/controllers/api/v1/user_profiles_controller.rb", "app/views/api/v1/user_profiles/index.json.jbuilder", "spec/requests/api/v1/user_profiles_spec.rb"], + "estimatedAdditions": 440, + "dependsOn": "PR 1", + "description": "Routes, controller, jbuilder views, and request specs" + } + ], + "splitInstructions": "1. Create branch `feature-part-1` from current branch with only PR 1 files\n2. After PR 1 merges, rebase current branch on main for PR 2" +} +``` + +--- + +## Important Guidelines + +- **Be pragmatic, not rigid.** A 460-line PR that's all one cohesive feature is better as one PR than split awkwardly. +- **Tests stay with their code.** Never suggest putting implementation in one PR and its tests in another. +- **Generated/checked-in artifacts don't count** but must go with whatever PR modifies their source (e.g., `db/schema.rb` ships with the migration that produced it). +- **Migrations + model changes** must be in the same PR. +- **Routes + controller** must be in the same PR +- **Factories stay with the model they target** when that model is being introduced. +- When in doubt, lean toward NOT splitting. Bad splits create more review overhead than a slightly large PR. + +--- + +## Error Handling + +### No Diff from Base Branch + +```json +{ + "shouldSplit": false, + "error": "no_changes", + "message": "Branch has no changes compared to {base_branch}" +} +``` + +### Cannot Determine Base Branch + +```json +{ + "shouldSplit": false, + "error": "no_base_branch", + "message": "Could not determine base branch. Falling back to main.", + "suggestion": "Ensure you are on a feature branch, or provide the base branch explicitly" +} +``` diff --git a/.claude/commands/create-pr.md b/.claude/commands/create-pr.md new file mode 100644 index 00000000..83fa1aaf --- /dev/null +++ b/.claude/commands/create-pr.md @@ -0,0 +1,47 @@ +# Create PR + +Create a pull request following the official template. + +**For PR content guidance, see `.claude/rules/github-pr-formatting.md`** + +## Workflow + +### 1. Check PR Size + +Before anything else, run the `pr-size-checker` agent to analyze the diff size. + +- If the agent recommends splitting, present the split plan to the user and ask how they'd like to proceed +- Only continue with PR creation after the user confirms (either to proceed as-is or after splitting) + +### 2. Prepare Branch + +- Check for uncommitted changes or unpushed commits +- If found, ask user if they want to proceed (advise pushing manually due to long pre-push hooks) + +### 3. Gather Information + +```bash +# Commit log on this branch +git log main..HEAD --oneline + +# Files changed +git diff main...HEAD --name-only +``` + +### 4. Generate PR Description + +Follow `.claude/rules/github-pr-formatting.md` to fill each section of the template (Board, Description, Notes, Tasks, Risk, Preview). + +### 5. Create PR + +```bash +gh pr create --title "[{TICKET-ID}] {Concise description}" --body "{BODY}" --base main +``` + +If there is no associated ticket, drop the `[TICKET-ID]` prefix and use just the description. + +### 6. Amend the Body if Needed + +```bash +gh pr edit {PR_NUMBER} --body "{UPDATED_BODY}" +``` diff --git a/.claude/rules/github-pr-formatting.md b/.claude/rules/github-pr-formatting.md new file mode 100644 index 00000000..126091bb --- /dev/null +++ b/.claude/rules/github-pr-formatting.md @@ -0,0 +1,152 @@ +# GitHub PR Creation Guide + +This is the single source of truth for creating pull requests. `.claude/commands/create-pr.md` references this file. + +## Step 0: Check PR Size + +**ALWAYS run the `pr-size-checker` agent before creating a PR.** This agent analyzes the diff and recommends whether the PR should be split into smaller ones (target: ~400 added lines max per PR). + +- If the agent recommends splitting, present the split plan to the user and ask how to proceed +- Only continue with PR creation after the user confirms (either as-is or after splitting) +- Skip this step only if the user explicitly asks to skip size checking + +## Step 1: Read the Official Template + +**ALWAYS start by reading `.github/PULL_REQUEST_TEMPLATE.md`** + +```bash +cat .github/PULL_REQUEST_TEMPLATE.md +``` + +Every PR must include ALL sections from the template. Do not skip sections — fill with `* N/A` if not applicable. + +## Step 2: Fill Each Section + +The template has six sections: **Board**, **Description**, **Notes**, **Tasks**, **Risk**, **Preview**. + +### Board + +- A link to the ticket this PR closes, in the form `* [Ticket #123](https://link-to-ticket)` +- If there is no ticket, write `* N/A` and explain why in the Description (e.g., dependency bump, hotfix, internal cleanup) + +### Description + +- Concise summary of what changed and **why** +- For dependency bumps, mention the from/to versions and any notable upstream changes (e.g., breaking changes, CVE fixes) +- For Rails upgrades, link the release notes or upgrade guide +- Reference any related PRs by number (e.g., "depends on #1234", "follow-up to #1230") + +### Notes + +- Anything reviewers should know that doesn't fit elsewhere: caveats, deferred work, follow-ups, manual steps required after merge +- If none, write `* N/A` + +### Tasks + +Use checkboxes (`- [ ]` / `- [x]`) to enumerate the work this PR does. Tick the boxes for items already completed in the PR; leave any deferred items unchecked and mention them in **Notes**. + +```markdown +- [x] Add `UserProfile` model + migration +- [x] Add `Api::V1::UserProfilesController` with `index`/`show` +- [x] Add request specs covering happy path and 404 +- [ ] Wire up `update`/`destroy` (deferred to follow-up) +``` + +### Risk + +- One short line about the blast radius of the change +- For trivial changes: `* Low — isolated to ` +- For migrations / data changes: state the rollback plan explicitly +- For dependency bumps: note whether the upstream changelog flagged breaking changes + +### Preview + +- Sample `curl` requests + responses for new/changed endpoints +- Console output for changed jobs / rake tasks +- Screenshots or GIFs (rare in an API repo, but useful for any rendered output) +- `* N/A` for changes with no observable surface (refactors, internal cleanup) + +## Step 3: Create the PR + +```bash +gh pr create --title "[{TICKET-ID}] {Concise description}" --body "{BODY}" --base main +``` + +**Title format:** + +- **If there is a ticket**, prefix the title with `[TICKET-ID]` (e.g., `[RS-123] Add user profile endpoints`). The same ticket should also be linked in the **Board** section. +- **If there is no ticket** (dependency bumps, hotfixes, internal cleanup), drop the prefix and use just the description (e.g., `Upgrade Rails to 8.1.3`, `Security: Update erb gem to patch CVE-2026-41316`). + +Keep the title under 70 characters; details belong in the body. + + +--- + +## Formatting Rules + +### Markdown List Spacing Bug + +GitHub collapses the spacing between two lists separated only by a blank line, making the boundary invisible. Always insert an HTML comment `` (with its own blank lines) between adjacent list groups. + +**Insert a separator whenever any of these transitions occur — even across headings or bold lines:** + +| Transition | Example trigger | +|---|---| +| Plain bullets → task-list checkboxes | `- item` followed by `- [ ] checkbox` | +| Task-list checkboxes → plain bullets | reverse of above | +| Plain bullets → plain bullets (two logical groups) | two bullet groups split by a blank line | +| Task-list → task-list (different logical groups) | e.g., separate verification groups | +| Heading / bold paragraph → list | e.g., `#### Tasks` followed by `- [ ] …` | +| Standalone link/paragraph → list | anything non-list followed by list inside the same section | + +**Rule of thumb:** if two list-shaped blocks end up in the same section with only blank lines between them, separate them. + +```markdown +- Item 1 +- Item 2 + + + +- [ ] Checkbox 1 +- [ ] Checkbox 2 +``` + +```markdown +**Migration:** +- [ ] Step A +- [ ] Step B + + + +**Endpoint:** +- [ ] Step C +- [ ] Step D +``` + +**Mandatory pre-flight before `gh pr create` / `gh pr edit --body`:** re-read the body end-to-end with this table in mind. Missing separators is a recurring mistake — never skip this check. + +### Code Blocks + +Don't unnecessarily escape backticks in code blocks. + +### Tasks Format + +Always use checkboxes for task lists, with sub-bullets when grouping related items: + +```markdown +- [x] Run the migration on a fresh DB +- Verify the new endpoint: + - [x] `GET /api/v1/foo` returns 200 + - [x] `POST /api/v1/foo` validates required params +``` + +--- + +## Making Changes to Existing PRs + +Use `gh` CLI: + +```bash +gh pr edit {PR_NUMBER} --body "{NEW_BODY}" +gh pr edit {PR_NUMBER} --add-label "label-name" +``` diff --git a/.claude/skills/rails-conventions/SKILL.md b/.claude/skills/rails-conventions/SKILL.md new file mode 100644 index 00000000..2b602161 --- /dev/null +++ b/.claude/skills/rails-conventions/SKILL.md @@ -0,0 +1,154 @@ +--- +name: rails-conventions +description: Rootstrap Rails conventions. Use when writing, reviewing, or editing any Rails code — controllers, models, migrations, routes, views, mailers, initializers, locale files, or Rails config. Covers routing, ActiveRecord, migrations, i18n, time zones, mailers, assets, Bundler groups, and logging. +--- + +# Rails Conventions (Rootstrap) + +Apply these whenever producing or modifying Rails-specific code. Full guide: https://github.com/rootstrap/tech-guides/blob/master/ruby/rails.md + +Complements `ruby-conventions` (language-level style still applies). + +## Configuration +- Custom initialization in `config/initializers`; one file per gem, named after the gem (e.g. `carrierwave.rb`). +- Environment-specific settings in `config/environments/`; shared settings in `config/application.rb`. +- Create a `staging` environment that mirrors production. +- Extra YAML config under `config/`, loaded via `Rails::Application.config_for(:yaml_file)`. +- Append non-default assets to `config.assets.precompile` in `production.rb` (e.g. admin CSS/JS). `application.*` and non-JS/CSS assets are already included. + +## Routing +- Prefer `resources` over custom routes; use `:only`/`:except` to limit routes. + ```ruby + # bad + get 'topics/:id', to: 'topics#show' + # good + resources :topics, only: :show + ``` +- `member`/`collection` for extra RESTful actions; use block form when many. +- Express associations with nested routes; use `shallow: true` beyond 1 level deep. +- `namespace` to group related actions (e.g. `admin`). +- Never use the wildcard `match ':controller(/:action(/:id(.:format)))'` route. +- Avoid `match` unless mapping multiple HTTP verbs via `:via`. + +## Controllers +- Keep controllers skinny — no business logic (belongs in models/services). +- Each action should ideally call only one method beyond an initial `find`/`new`. +- Share at most two instance variables between controller and view. + +## Rendering +- Prefer templates/partials over `render inline:`. +- `render plain:` over `render text:`. +- Use HTTP status symbols, not numbers. + ```ruby + # bad + render status: 500 + # good + render status: :forbidden + ``` + +## Models +- Introduce non-ActiveRecord model classes freely; short, meaningful names. +- Use ActiveAttr gem for non-persisted models needing AR-like behavior. +- Keep models for business logic/persistence; move formatting/HTML concerns to decorators. + +## ActiveRecord +- Don't alter AR defaults (table names, primary keys) without strong reason. +- Group macros at top in order: `default_scope`, constants, `attr_*`, `enum`, associations, validations, callbacks, other macros (e.g. devise). +- Prefer `has_many :through` over `has_and_belongs_to_many`. +- Prefer `self[:attr]` / `self[:attr] = value` over `read_attribute`/`write_attribute`. +- Use "sexy" validation syntax: + ```ruby + validates :email, presence: true, length: { maximum: 100 } + ``` +- Extract reused/regex validators into `app/validators` as `EachValidator` subclasses. If the validator is generic and used across multiple apps, extract it to a shared gem instead. +- Use named scopes; convert complex parameterized scopes into class methods returning a relation. +- Beware validation-skipping methods: `update_attribute`, `update_columns`, `update_all`, `increment!`, `toggle`, `touch`, counter methods. +- User-friendly URLs: override `to_param` or use `friendly_id`. + ```ruby + def to_param + "#{id} #{name}".parameterize + end + ``` +- Use `find_each` for iterating AR collections, not `all.each`. +- Always add `prepend: true` on `before_destroy` callbacks that perform validation. +- Always set `dependent:` on `has_many`/`has_one`. +- When persisting, use bang methods (`save!`, `create!`, `update!`) or handle the returned status. + +## ActiveRecord Queries +- **Never interpolate params into SQL strings.** Use `?` placeholders or named placeholders (prefer named when >1). +- `find(id)` over `where(id: id).take`; `find_by(attrs)` for attribute-based single lookups. +- `where.not(id: id)` over `where("id != ?", id)`. +- Heredocs with `.squish` for explicit SQL in `find_by_sql`: + ```ruby + User.find_by_sql(<<-SQL.squish) + SELECT ... + SQL + ``` + +## Migrations +- Keep `schema.rb` (or `structure.sql`) in version control; use `rake db:schema:load` for new DBs. +- Enforce defaults at the DB level, not the application. +- Enforce foreign-key constraints (Rails 4.2+). +- Use `change` for constructive migrations; use `up`/`down` for non-reversible ones like `drop_table` (or pass a block to `drop_table` inside `change`). `change` only works for commands listed in `ActiveRecord::Migration::CommandRecorder`. +- If using a model inside a migration, redefine it with an explicit `table_name`: + ```ruby + class MigrationProduct < ActiveRecord::Base + self.table_name = :products + end + ``` +- Name foreign keys explicitly (e.g. `name: :articles_author_id_fk`). +- Avoid `FLOAT` for rational numbers; use `DECIMAL` or a base-unit integer (money in cents). + +## Views +- Never call models directly from views. +- Complex formatting → decorators. +- Use partials and layouts to deduplicate. + +## Internationalization +- No user-facing strings in views/models/controllers; move to `config/locales`. +- `activerecord` scope for model/attribute translations (`User.model_name.human`, `human_attribute_name`). +- Organize locales into `locales/models` and `locales/views`; load extra dirs via `config.i18n.load_path`. +- Short forms `I18n.t` / `I18n.l`. +- Lazy lookup (`t '.title'`) in views; dot-separated keys elsewhere instead of `:scope`. + +## Assets +- `app/assets` → app-specific. +- `lib/assets` → in-house libs. +- `vendor/assets` → third-party. +- Prefer gemified assets (e.g. `jquery-rails`, `bootstrap-sass`). + +## Mailers +- Name classes `SomethingMailer`; provide both HTML and plain-text templates. +- `config.action_mailer.raise_delivery_errors = true` in development. +- Local SMTP (Mailcatcher/Letter Opener) in development. +- Set `default_url_options[:host]` per environment. +- Always use `_url` helpers (not `_path`) in email bodies. +- Format `default from:` as `'Your Name '`. +- Test env: `delivery_method = :test`; dev/prod: `:smtp`. +- Inline CSS for HTML emails (use `premailer-rails` or `roadie`). +- Send emails in background jobs (e.g. Sidekiq); never inline during request. + +## Active Support Core Extensions +- Prefer `&.` over `try!`. +- Prefer stdlib (`start_with?`, `end_with?`, `include?`) over AS aliases (`starts_with?`, `in?`). +- Prefer plain comparisons over `inquiry`, `Numeric#positive?`/`negative?`, etc. + +## Time +- Set `config.time_zone` in `application.rb`. +- **Never use `Time.parse` or `Time.now`.** Use `Time.zone.parse`, `Time.zone.now`, or `Time.current`. + +## Bundler +- Dev/test gems in proper Gemfile groups. +- Prefer well-established gems; review source of obscure ones. +- Group OS-specific gems under `darwin` / `linux` and use `Bundler.require(platform)`. +- **Never remove `Gemfile.lock` from version control.** + +## Managing Processes +- Use `foreman` to manage multiple external processes (see `Procfile` / `Procfile.dev`). + +## Logging +- Pass a block to `Rails.logger.debug` when interpolating to avoid string-building at suppressed levels. + ```ruby + # good + Rails.logger.debug { "attrs: #{@person.attributes.inspect}" } + ``` diff --git a/.claude/skills/rspec-conventions/SKILL.md b/.claude/skills/rspec-conventions/SKILL.md new file mode 100644 index 00000000..38887ca2 --- /dev/null +++ b/.claude/skills/rspec-conventions/SKILL.md @@ -0,0 +1,85 @@ +--- +name: rspec-conventions +description: Rootstrap RSpec conventions. Use when writing, reviewing, or editing RSpec test files (spec/**/*_spec.rb, spec/rails_helper.rb, spec/spec_helper.rb, spec/support/**/*.rb) or factories. Covers describe/context structure, let/subject, matchers, factories, mocking/stubbing, shared examples, and spec types (model, request, feature, controller). +--- + +# RSpec Conventions (Rootstrap) + +Apply these whenever producing or modifying RSpec test code. Full guide: https://github.com/rootstrap/tech-guides/blob/master/ruby/rspec/style_guide.md + +Complements `ruby-conventions` — language-level Ruby style still applies. + +## Structure & Formatting +- No blank line directly after `describe`/`context`/`feature` opening. +- Leave one blank line after `let`/`subject` groups and after `before`/`after` blocks. +- Group `let` and `subject` together; separate them from `before`/`after` with a blank line. +- One blank line around each `it` block. +- Use `before` (not `before(:each)`); `before(:all)` should be rare and explicit. + +## describe & context +- `describe '#method'` for instance methods, `describe '.method'` for class methods. +- `context` descriptions must start with `when` or `with`, forming a grammatical sentence. + ```ruby + context 'when the display name is not present' + ``` +- Every `context` should have a matching opposite/negative case — a lone context is a smell. +- Don't end `it` descriptions with a conditional ("...if X") — wrap in a `context` instead. + +## it / Examples +- **Never start `it` with "should" / "should not".** State behavior directly. + ```ruby + it 'returns the summary' # not: 'should return the summary' + ``` +- One expectation per `it` block (split multi-assertion examples). +- Avoid generating examples via iterators; use shared examples instead. + ```ruby + shared_examples 'responds successfully' do + it 'returns 200' do ... end + end + it_behaves_like 'responds successfully' + ``` + +## let & subject +- Prefer `let` over `before { @x = ... }` for test data (lazy, no instance vars). +- `let` is **lazy** (evaluated on first reference) and **cached within a single example, not across examples**. Use `let!` when you need eager evaluation before each example. +- Use `let` for values shared across several (not necessarily all) `it`s in a context; avoid overuse — it hurts readability. +- Use `subject` when describing a single primary object. + ```ruby + subject { create(:article) } + ``` + +## Matchers +- Use RSpec magic matchers for predicate methods: `expect(subject).to be_published` (calls `published?`). +- Prefer `change` matchers over counting state before/after. + ```ruby + expect { article.publish }.to change(Article, :count).by(1) + ``` +- Avoid asserting incidental state (e.g. `Article.count == 2`) — test the delta or direct effect. + +## Factories & Fixtures +- Use **FactoryBot** (`create(:article)`); **never** Rails fixtures. +- Avoid `ModelName.create` in integration specs — reach for the factory. +- Use **Faker** for fake data; **Database Cleaner** keeps state isolated. + +## Mocking / Stubbing / Doubles +- Use mocks/stubs sparingly — favor them in isolated/behavioral specs, not integration specs. +- Only stub against small, stable, well-known APIs. +- **Never stub a method whose return you're asserting on** — produces false positives. + ```ruby + # Bad: stubs nil? itself + allow(summary).to receive(:nil?).and_return(true) + # Good: stub the upstream method, let the code run + allow(subject).to receive(:summary).and_return(nil) + ``` +- Use **Webmock/VCR** for HTTP; never hit real APIs in tests. + +## Shared Examples +- Extract shared examples when duplication is real and clarifies intent — don't DRY prematurely. +- Duplication inside specs is acceptable, even preferred, if it aids readability. + +## Spec Types (quick guidance) +- **Model/service/job/mailer specs** — unit tests of public methods and callbacks; don't test private methods. +- **Feature specs** — full UI flow via Capybara; only enable JS driver when required. +- **Controller specs** — for non-API controllers and edge cases faster than feature specs. +- **Request specs** (`type: :request`) — preferred over controller specs for APIs; exercises routes and real responses. +- Prefer `describe ... do` style over Capybara's `feature`/`scenario` DSL for consistency. diff --git a/.claude/skills/ruby-conventions/SKILL.md b/.claude/skills/ruby-conventions/SKILL.md new file mode 100644 index 00000000..2accf5d5 --- /dev/null +++ b/.claude/skills/ruby-conventions/SKILL.md @@ -0,0 +1,190 @@ +--- +name: ruby-conventions +description: Rootstrap Ruby style conventions. Use when writing, reviewing, or editing any Ruby source file (*.rb, *.rake, Gemfile, Rakefile, *.gemspec, config.ru) to ensure code follows the Rootstrap Ruby style guide — covers layout, syntax, naming, classes/modules, exceptions, collections, strings, regexes, metaprogramming, and general best practices. +--- + +# Ruby Conventions (Rootstrap) + +Apply these conventions whenever producing or modifying Ruby code. Full guide: https://github.com/rootstrap/tech-guides/blob/master/ruby/README.md + +Most of these are enforced by RuboCop (see `.rubocop.yml`). When in doubt, run RuboCop. + +## Source Code Layout +- UTF-8, Unix line endings, 2-space indentation (no tabs), one expression per line (no `;`). +- Max 100 chars per line; end files with a newline; no trailing whitespace. +- Prefer `FooError = Class.new(StandardError)` over empty `class ... end`. +- Avoid single-line methods (exception: `def no_op; end`). +- Spaces around operators, after commas/colons; no spaces inside `(`, `[`, around `!`, or in range literals (`1..3`). +- Exception: no spaces around exponent: `c**2`. +- Indent `when` the same as `case`. +- Blank lines between methods and around access modifiers; no blank lines inside class/method bodies. +- No trailing comma after last arg/element. +- Spaces around `=` in default params: `def f(a = 1)`. +- Avoid line continuation `\` except for string concatenation. +- In multi-line chains, keep `.` on the next line. +- Use underscores in big numbers: `1_000_000`. Lowercase prefixes `0x`, `0o`, `0b`. +- No block comments (`=begin`/`=end`). +- Never more than one consecutive blank line. +- Align multi-line method args after `(`, OR single-indent with `)` on its own line (avoid "double indent"). +- For `case`/`if` result assignment, either align branches under the keyword or break with `kind =` on its own line. + +## Syntax +- Use `::` only for constants/constructors, not method calls. +- `def` with parens when params exist, omit when empty. +- Parens around method arguments, except: no-arg calls, DSL methods (`validates :name, ...`), and keyword-like (`attr_reader`, `puts`). +- Optional args at end of parameter list. +- Prefix unused block vars with `_`: `|_k, v|`. +- Avoid parallel assignment (`a, b, c = 1, 2, 3`) except for swap, method-return destructuring, or splat. +- Prefer trailing-underscore form: `a, = foo.split(',')` over `a, _, _ = ...`. Use named underscore vars (`_second`) when position conveys meaning. +- Don't use `for`; use iterators (`each`). +- No `then` in multi-line `if`; no `if x; ...`. Always put the condition on the same line as `if`/`unless`. +- Avoid multi-line ternary — use `if`/`unless` instead. +- No `while cond do` / `until cond do` for multi-line loops (drop the `do`). +- Favor ternary over `if/then/else` one-liners; don't nest ternaries. +- Leverage `if`/`case` as expressions. +- Use `!` not `not`; avoid `!!` boolean coercion. +- **`and`/`or` keywords are banned** — use `&&`/`||`. +- Favor modifier `if`/`unless`/`while`/`until` for single-line bodies; avoid on multi-line blocks; don't nest modifiers. +- Favor `unless` for negatives; favor `until` over `while` for negative loop conditions. **Never `unless` with `else`**. +- No parens around control-expression conditions (except safe assignment `if (v = ...)`). +- Use `Kernel#loop` for infinite loops; prefer `loop { ... break unless ... }` over `begin ... end while` for post-condition loops. +- Omit outer `{}` AND parens for internal DSL methods: `validates :name, presence: true, length: { within: 1..10 }`. +- Omit outer `{}` on trailing options hashes: `user.set(name: 'John', age: 45)`. +- Use `&:method` shorthand: `names.map(&:upcase)`. +- `{...}` for single-line blocks, `do...end` for multi-line. Avoid `do...end` in chains. +- Avoid explicit `return` when unnecessary; avoid `self.` unless required. +- Use `||=` to init nil/unset vars; don't use `||=` for booleans. +- Use `&&=` to preprocess nullable values. +- Avoid `===` outside `case`. +- Use `==` not `eql?` unless strict type comparison is intended. +- No space between method name and opening paren: `f(x)`. +- No nested method defs; use lambdas. +- Lambda: `->(a, b) { ... }` with args (parens required); `-> { ... }` with no args (omit parens); `lambda do ... end` for multi-line. +- Prefer `proc` over `Proc.new`; use `.call()` not `[]` or `.()`. +- Use shorthand self-assignment: `x += y`, `x **= y`, etc. +- Use explicit `&block` to forward blocks rather than wrapping them. +- Don't shadow methods with local variables (e.g. naming an arg `options` when an accessor already exists). +- Don't use character literals (`?x`) — use `'x'`. +- Avoid Perl-style special vars (`$;`, `$,`); prefer `English` library aliases (`$LOAD_PATH`, etc.). +- Don't use `BEGIN`/`END` blocks; use `Kernel#at_exit` instead. +- Use `warn` over `$stderr.puts`. +- Favor `sprintf`/`format` over `String#%`; `Array#join` over `Array#*`. +- Use `Array(var)` to coerce possibly-single values into arrays. +- Use ranges or `between?` instead of `x >= a && x <= b`. +- Predicate methods (`.even?`, `.zero?`, `.nil?`) over `== 0`, `== nil`. +- Avoid `!x.nil?` when `if x` suffices. +- Guard clauses over nested conditionals; `next` over `if` in loops. +- Prefer: `map` over `collect`, `select` over `find_all`, `find` over `detect`, `reduce` over `inject`, `size` over `length`/`count` (note: `count` on non-Array Enumerables iterates the full collection). +- `flat_map` over `map.flatten(1)`; `reverse_each` over `reverse.each`. + +## Naming +- Identifiers in English, `snake_case` for methods/vars/symbols/files/dirs. +- `CamelCase` for classes/modules; keep acronyms uppercase (`XMLParser`). +- `SCREAMING_SNAKE_CASE` for constants. +- Predicate methods end with `?`; **do not prefix** with `is_`/`can_`/`does_`. +- Bang methods (`!`) exist only when a safe counterpart exists. +- Name binary operator params `other` (exceptions: `<<` and `[]`). +- No numeric separation: `some_var1` not `some_var_1`. +- One class/module per file, file named `snake_case` after the class/module. +- Define non-bang methods in terms of bang when possible: `def flatten_once; dup.flatten_once!; end`. + +## Comments +- Prefer self-documenting code; comments in English, capitalized, one space after `#`. +- Refactor bad code instead of explaining it. +- Avoid superfluous comments (`counter += 1 # Increments counter by one`). +- Keep comments up to date — an outdated comment is worse than none. +- Annotations: `TODO`, `FIXME`, `OPTIMIZE`, `HACK`, `REVIEW` + `:` + description, above the relevant code. Document custom annotation keywords in the project README. +- Magic comments (`# frozen_string_literal: true`) at top, one per line, blank line before code. + +## Classes & Modules +- Layout order: `extend`/`include` → inner classes → constants → `attr_*` → other macros → public class methods → `initialize` → public instance methods → protected → private. +- Separate `include` per mixin. +- Don't nest multi-line classes; use matching folder structure. +- Prefer modules (`extend self`) over classes with only class methods. +- Use `def self.method` (not `def ClassName.method`). +- Within class methods calling siblings, omit `self.`. +- Always supply `to_s` for domain objects. +- Use `attr_reader`/`attr_accessor`; avoid `attr`; don't prefix with `get_`/`set_`. +- `Struct.new` for trivial value objects; don't inherit from it. +- Avoid class variables (`@@var`); prefer class instance variables. +- Proper visibility (`private`/`protected`); indent modifiers at method level with blank lines. +- Prefer duck-typing over inheritance. +- Apply SOLID principles; respect the Liskov Substitution Principle (subclasses should be substitutable for their parents). +- Encourage factory methods for clearer object creation APIs. +- Use `alias` in lexical scope; `alias_method` for runtime/module aliasing. Note: `alias` binds at definition time, so subclass overrides won't be picked up unless re-aliased. + +## Exceptions +- Prefer `raise` over `fail`; `raise SomeException, 'message'` (not `.new(...)`, not `RuntimeError`). +- Never `return` from `ensure`. +- Use implicit `begin` in method bodies (`def foo ... rescue ... end`). +- Don't suppress exceptions; avoid `rescue` in modifier form. +- No exceptions for flow control. +- **Never `rescue Exception`** — use `rescue StandardError => e` or bare `rescue => e`. +- Specific exceptions higher in the rescue chain. +- Release resources in `ensure` or block form (`File.open('f') { |f| ... }`). +- Favor stdlib exceptions over new classes. +- Extract repeated rescue patterns into contingency methods (`with_io_error_handling { ... }`) to DRY up error handling. + +## Collections +- Use literals `[]` and `{}` (not `Array.new`, `Hash.new`). +- `%w[one two three]` for word arrays, `%i[a b c]` for symbol arrays (2+ elements). +- Prefer `first`/`last` over `[0]`/`[-1]`; `Set` for unique collections. +- Symbols as hash keys; 1.9 syntax `{ one: 1 }`; don't mix with hash rockets. +- `Hash#key?` not `has_key?`; `Hash#each_key` not `keys.each`. +- `Hash#fetch` for required keys; block form for expensive defaults: `hash.fetch(:k) { expensive }`. +- `Hash#values_at` for multi-key lookup. +- Don't mutate a collection while iterating. +- Don't use mutable objects as hash keys. +- Rely on ordered hashes (Ruby 1.9+); insertion order is preserved. +- When providing collection-returning APIs, offer an alternate accessor to avoid `nil[]`: prefer `Regexp.last_match(1)` over `Regexp.last_match[1]`. + +## Numbers +- `Integer` (not `Fixnum`/`Bignum`) for type checks. +- `rand(1..6)` over `rand(6) + 1`. + +## Strings +- Interpolation `"#{x}"` over concatenation. +- Pick single or double quotes consistently (guide prefers single when no interpolation). +- `{}` around `@var`/`$var` in interpolation; don't call `.to_s` inside. +- `String#<<` to build large strings, not `+=`. +- `sub`/`tr` over `gsub` when simpler. +- Squiggly heredocs `<<~END` for multi-line indented strings. + +## Date & Time +- `Time.now` over `Time.new`; use `Date` or `Time`, not `DateTime`. +- (See also `rails-conventions` — in Rails, use `Time.zone.now`/`Time.current`.) + +## Regular Expressions +- Plain string ops (`string['text']`) over regex when possible; also `string[/regexp/]` and `string[/text(grp)/, 1]` forms. +- `(?:...)` for non-capturing; named groups `(?...)` over numbered. +- `Regexp.last_match(n)` not `$1`. +- `\A` / `\z` (not `^`/`$`) for full-string boundaries. +- `/x` modifier for complex, commented regexes. +- In character classes `[]`, only `^`, `-`, `\`, `]` need escaping; don't escape `.` or brackets. +- Use `sub`/`gsub` with a block or hash for complex replacements. + +## Percent Literals +- `%()` only for single-line strings needing both interpolation and `"`. Heredocs for multi-line. +- Avoid `%q()` unless the string has both `'` and `"`. +- `%r{...}` only when the regex contains `/`. +- Brackets: `()` for strings, `[]` for `%w`/`%i`, `{}` for `%r`. +- Avoid `%x` unless invoking a command whose string contains backticks. +- Avoid `%s` — prefer `:"some string"` for symbols needing spaces. + +## Metaprogramming +- Avoid needless metaprogramming; don't monkey-patch core classes in libraries. +- Prefer block `class_eval` over string form; prefer `define_method`. +- If you must use string-form `class_eval`/`eval`, pass `__FILE__` and `__LINE__` for sensible backtraces. +- Avoid `method_missing`; if needed, also define `respond_to_missing?`, call `super`, and only catch well-defined prefixes (e.g. `find_by_*`) — delegating to non-magical methods. +- `public_send` over `send`; `__send__` over `send` when receiver may define `send`. + +## Misc +- Write `ruby -w` clean code (run with warnings). +- Avoid hashes as optional params (except initializers). +- Keep methods small (~10 LOC, ideally <5); params 3–4 max. +- Avoid more than 3 levels of block nesting. +- Code functionally; don't mutate parameters unless that's the method's purpose. +- Prefer module instance variables over globals (`$foo`). +- Use `OptionParser` for complex CLI options; `ruby -s` only for trivial cases. +- If adding global methods, put them in `Kernel` and make them `private`. +- Be consistent and use common sense — within a file, prefer matching the surrounding style over strict rule-following. diff --git a/README.md b/README.md index ddc4d9d7..171b98b6 100644 --- a/README.md +++ b/README.md @@ -163,6 +163,16 @@ For more details about the CI process and SonarQube setup, check the [CI documen ## More linters - [Hadolint](https://github.com/hadolint/hadolint) Install with `brew install hadolint` and run `hadolint Dockerfile*`. Edit `.hadolint.yml` to omit additional rules. +## Claude Code setup + +This template ships with project-level [Claude Code](https://www.claude.com/product/claude-code) skills under `.claude/skills/` that auto-activate while Claude edits code in your project: + +- `ruby-conventions` — triggers on Ruby files; applies Rootstrap's Ruby style guide. +- `rails-conventions` — triggers on Rails code (controllers, models, migrations, config); applies Rootstrap's Rails conventions. +- `rspec-conventions` — triggers on spec files; applies Rootstrap's RSpec style guide. + +They are distilled from [rootstrap/tech-guides](https://github.com/rootstrap/tech-guides/tree/master/ruby). No setup required — Claude Code loads them automatically based on the files being edited. If you don't use Claude Code, or want to customize the conventions, simply edit or remove the files under `.claude/skills/`. + ## Impersonation The `rails_api_base` incorporates a user impersonation feature, allowing `AdminUser`s to assume the identity of other `User`s. This feature is disabled by default.