references/rails.md
# Opinionated Rails Review Profile
Use this opinionated profile when reviewing a Rails application. Apply it to the changed behavior and its execution paths. Explicit repository instructions and documented architectural decisions take precedence, but do not discard a concern merely because the repository has not documented it.
This is an investigation checklist, not a finding checklist. Every item below is a place to look; none of them is a defect on its own. Return a finding only when the diff introduces or materially exposes a concrete defect, operational risk, or maintenance cost, and only when you can name a realistic path that reaches it — a caller, request, job run, migration, or deploy that actually produces the conditions. A preference that does not affect the change is not a finding, and neither is a risk this application's data, scale, or usage does not expose it to.
Calibrate to the application in front of you. Concurrency, lock duration, N+1 cost, rolling-deploy compatibility, and cache-key scope matter in proportion to real table sizes, traffic, tenancy, and deploy model; check the repository for that evidence instead of assuming the worst case.
## Rails shape and boundaries
- Follow Rails conventions before adding custom infrastructure. Prefer the framework's lifecycle, naming, and composition mechanisms unless the code demonstrates a reason to depart from them.
- Keep controllers, jobs, mailers, and callbacks as delivery or lifecycle boundaries. Move multi-step business workflows into explicit collaborators when doing so clarifies transaction ownership, reuse, failure handling, or tests; do not demand a service object by name.
- Give each class or module its own file. Namespace-only wrapper chains may share a file, but nested implementation classes hide dependencies and fight Zeitwerk conventions.
- Check that file paths, constant names, autoload/eager-load configuration, and inflection rules agree. Development autoloading must not conceal a production eager-load failure.
- Prefer explicit collaborators at I/O boundaries. HTTP, storage, email, payment, and other external integrations should centralize request construction, authentication, serialization, response handling, timeouts, and error translation when the change would otherwise duplicate or scatter them.
- Prefer reader methods and normal method dispatch over direct instance-variable reads outside the reader itself. This preserves an interception point for validation, memoization, typing, and tests.
## POODR-inspired object design
Use the changeability principles from Sandi Metz's [Practical Object-Oriented Design](https://www.poodr.com/) as review guidance, not as a demand for patterns or more objects.
- Prefer composition over inheritance. Use inheritance only for a stable, genuine type relationship whose subclasses can honor the superclass contract without conditionals, refused behavior, or knowledge of sibling classes. Keep hierarchies shallow and favor composed role players when behavior varies independently.
- Give each object one coherent responsibility and a small public interface. Judge responsibility by its reasons to change, not by line count; extract an object when the change reveals a distinct concept or isolates volatile behavior.
- Depend on roles and messages rather than concrete class names. Inject collaborators when doing so lets objects vary independently, and isolate object construction at an application edge or small factory instead of mixing creation with use.
- Ask collaborators for what the sender needs without reaching through them to direct how the work is done. Long message chains, exposed internal data, and repeated navigation indicate that responsibilities or interfaces may be misplaced.
- Use duck typing and polymorphism when multiple objects can honor the same role. Repeated conditionals that select behavior by type or state may reveal a missing role; isolate variant selection rather than scattering the branching across callers.
- Arrange dependencies toward objects that are less likely to change. Wrap unstable framework, vendor, and policy details behind interfaces owned by the application when that boundary reduces the cost of a demonstrated change.
- Do not speculate abstractions into existence. Prefer simple, concrete code until duplication or changing requirements reveal the stable concept, then refactor toward the smallest abstraction that makes the next change cheaper.
- Treat painful tests as design feedback. Excessive setup, knowledge of private methods, or many unrelated stubs often signals hidden dependencies or an object with too many responsibilities; improve the design rather than cementing it with more test machinery.
## Persistence and domain state
- Enforce durable invariants in the database as well as the model where concurrency or non-Rails writers can violate them. Inspect nullability, foreign keys, unique and check constraints, indexes, and the behavior of bulk APIs that skip validations and callbacks.
- Trace transaction boundaries across every write and side effect. Avoid external I/O inside a database transaction; ensure jobs and notifications that require committed data are dispatched after commit.
- Treat callbacks as local invariant maintenance, not as a hidden workflow engine. Check ordering, recursion, skipped-callback APIs, rollback behavior, and whether the initiating code can understand the side effects.
- Prefer timestamp-backed state over boolean columns when the domain benefits from knowing when a transition happened. Check predicate semantics, scopes, backfills, and whether repeated transitions are meaningful before accepting a boolean.
- Inspect query cardinality, eager loading, counter caches, pagination, and tenant scoping. Watch for N+1 queries, unbounded relations, per-row writes, unnecessary object allocation, and calculations that belong in SQL.
- Check time handling for application-zone versus UTC semantics, date-boundary behavior, daylight-saving transitions, and comparisons against database timestamps.
## Migrations and deploy safety
- Review migrations as production operations, not only schema transformations. Check table size, locks, statement duration, index creation, constraint validation, and whether a backfill should be separated from the schema change.
- Preserve compatibility across rolling deploys. Old and new application versions may run against the same schema; destructive renames, removals, type changes, and new required values usually need expand-and-contract sequencing.
- Prefer reversible migrations, but do not confuse a syntactic `down` path with safe recovery. State what happens to data and running application versions during rollback.
- Keep `structure.sql` when database features cannot be represented faithfully by `schema.rb`. Review changes to the checked-in schema artifact and ensure migrations reproduce it from an empty database.
- Use Strong Migrations or equivalent safeguards when present. Bypasses such as `safety_assured` require concrete, change-specific justification.
## Requests, authorization, and rendering
- Treat strong parameters as assignment filtering, not authorization. Verify authentication, action-level authorization, record scoping, and tenant ownership for every changed read or write path.
- Trace identifiers supplied through routes, forms, Turbo frames, and JSON APIs. Look for insecure direct object references, mass assignment, unsafe dynamic ordering or SQL, and enumeration across account boundaries.
- Check CSRF behavior, HTTP method semantics, redirects, return URLs, and content negotiation. State-changing actions should not be reachable through GET, and redirects should not accept untrusted external destinations without validation.
- Treat `html_safe`, `raw`, dynamic partial paths, user-authored rich text, and JavaScript interpolation as trust-boundary changes. Verify escaping and sanitization at the final rendering context.
- For Turbo and Stimulus changes, inspect full-page and frame/stream behavior, stable DOM identifiers, duplicate submissions, browser history, validation failures, focus, and no-JavaScript fallbacks where the product requires them.
- For ViewComponent changes, keep rendering logic and variants inside the component boundary, use explicit inputs, and cover meaningful rendered output rather than private implementation details.
## Jobs, caching, and asynchronous behavior
- Assume jobs can be delivered more than once, retried after partial work, and run after records have changed or disappeared. Require idempotent effects or an explicit deduplication strategy where duplicates matter.
- Pass stable identifiers and small serializable values to jobs rather than mutable object graphs. Re-authorize or re-check relevant state at execution time instead of assuming enqueue-time state remains true.
- Calibrate retry, discard, and rescue behavior to the failure. Do not retry permanent validation or authorization failures; preserve useful context without logging secrets or personal data.
- Check queue choice, priority, concurrency limits, transaction commit timing, and operational visibility. A job is not complete merely because it was enqueued.
- Review cache keys for every input that can change the result, including tenant, locale, authorization, variants, and versioned records. Verify invalidation and stampede behavior; never share sensitive cached output across scopes.
## Tests and verification
- Prefer behavioral RSpec coverage at the narrowest useful boundary. Model specs should exercise domain behavior and persistence; request specs should cover routing, authorization, responses, and side effects; system specs should be reserved for browser interactions that lower layers cannot prove.
- Cover success, validation failure, authorization failure, missing records, boundary values, and relevant retry or concurrency behavior. Match the test to the demonstrated risk rather than mechanically adding one example per method.
- Prevent accidental network access in tests. Stub at an owned integration boundary when available and use WebMock for request construction and protocol behavior. Exercise timeouts, malformed responses, non-success statuses, and network failures when the change handles them.
- Use verified partial doubles. Build valid records with FactoryBot when persistence matters, but avoid large factory graphs and `create_list` calls that hide test cost or irrelevant setup.
- Use `ClimateControl.modify` for environment-dependent behavior instead of stubbing `ENV` directly, so changes are scoped and restored reliably.
- Check time-dependent behavior with an explicit clock or Rails time helpers. Include zone and boundary cases when they affect semantics.
- Keep tests order-independent and compatible with randomized execution. Inspect leaked global configuration, cached constants, thread-local state, jobs, files, and database records.
## Quality defaults
Treat these as the preferred baseline when the repository has not intentionally chosen another standard:
- RSpec, FactoryBot, Shoulda Matchers, SimpleCov, WebMock, Capybara, and Selenium for the testing stack.
- Bullet configured to raise on N+1 queries in tests and to report them during development.
- Rails Omakase plus relevant RSpec, FactoryBot, and ViewComponent RuboCop plugins.
- Sorbet `typed: strict` for application and library Ruby, with committed Tapioca gem and DSL RBIs. Review signatures at public boundaries and avoid using `T.untyped` or unsafe casts to silence design problems.
- RubyCritic as a maintainability signal with a high project threshold; use its output to investigate complexity rather than treating the score alone as a defect.
- Brakeman, Bundler Audit, importmap audit when applicable, and bounded dependency requirements. Git dependencies should pin a full commit SHA.
- PostgreSQL with `structure.sql`, Strong Migrations, and explicit database constraints for durable invariants.
- Timestamp-backed state instead of boolean database columns when transition time carries domain value.
- One implementation class or module per file, direct instance-variable reads hidden behind readers, and environment changes in tests isolated with ClimateControl.
These are defaults, not requirements. Do not report the absence of a tool from this list as a finding; the review is static and CI owns execution. Read the repository's `Gemfile`, `bin/`, and CI workflows to learn which checks it already runs, and consult their existing output when it is relevant evidence. Raise a missing tool only when its absence creates a specific, named risk in the reviewed change.
SKILL.md
---
name: team-code-review
description: Review an agent's own code changes or another contributor's pull request by dispatching specialist agents, verifying every finding, and reporting only evidence-backed issues. Use when the user asks for a code review, PR review, review of a diff, or a multi-agent quality audit.
---
# Team Code Review
Run a read-only, static, evidence-driven code review with specialist agents. The main agent owns scope, verification, deduplication, severity, editorial judgment, and the final report. Evidence comes from reading code, not executing it: the review does not run tests, linters, type checkers, or builds — CI owns that feedback loop.
## 1. Establish the review target
Determine whether the target is:
- **Own work:** changes produced by the current agent or agent team.
- **Another contributor's pull request:** a PR authored outside the current agent team.
Read the repository instructions first. Identify the exact review base and head, then inspect the complete diff and relevant surrounding code. Do not silently broaden the review to unrelated working-tree changes. If authorship is unclear, infer it from the conversation and PR metadata when available; ask only when the answer changes the workflow and cannot be discovered.
Detect the application stack from repository evidence such as dependency manifests, framework configuration, and directory structure. Load any matching review profile before dispatching specialists. For a Rails application, read [references/rails.md](references/rails.md) and incorporate its applicable checks into the specialist assignments. Treat profiles as opinionated investigation guidance, not automatic findings; explicit repository instructions and documented architectural decisions take precedence.
Record explicit non-goals and constraints. Keep the review read-only unless the user separately asks for fixes.
Completion criterion: the review target, authorship branch, base/head, repository guidance, changed files, application stack, and applicable review profiles are known.
## 2. Dispatch the review team
Use subagents when the runtime supports them. Give each specialist the same target, base/head, repository instructions, and read-only constraint. Assign non-overlapping primary lenses:
1. **Architecture and code quality** — assess whether responsibilities and abstractions are coherent; identify duplication or misplaced behavior; check for appropriate API clients, service objects, collaborators, boundaries, and reuse without demanding abstractions that the code does not yet need.
2. **Readability** — assess naming, control flow, local complexity, consistency, error handling, and whether intent is easy to recover from the code.
3. **Testing** — trace each changed behavior to meaningful coverage, including success and failure paths, boundary cases, regressions, and integration points. For HTTP behavior, check request construction, headers/authentication, serialization, response parsing, relevant status codes, malformed responses, timeouts, retries/idempotency where applicable, and network failures. Prefer behavioral tests over implementation-coupled assertions.
4. **Maintainability** — assess coupling, extension cost, dependency direction, operational burden, compatibility, observability, and likely future failure modes.
5. **Security** — trace trust boundaries and data flow; assess authentication, authorization, validation, injection, secret exposure, sensitive logging, dependency risk, unsafe defaults, and failure behavior. Do not inflate hypothetical risks beyond what the code supports.
6. **Documentation** — check public interfaces, setup/configuration, migrations, operational changes, examples, changelog/release notes, and repository architecture or runbook documentation affected by the change.
Ask every specialist to return only actionable findings with:
- severity, likelihood, and confidence;
- exact file and tight line range;
- the concrete failure mode or maintenance cost;
- the specific conditions that trigger it, and evidence that a real caller, input, configuration, or schedule can produce those conditions;
- evidence from the diff and relevant surrounding code;
- the reasoning path that validates the claim, grounded in the cited code;
- a concise remediation direction.
Every candidate must pass a reachability test before a specialist reports it. Name what has to be true for the failure to happen, then look for the upstream guard that makes it impossible: validation, database constraint, type signature, route, authorization check, feature flag, caller contract, or the fact that no caller passes that shape. If such a guard exists, there is no finding — drop it rather than reporting it with a hedge. A specialist whose own explanation concedes the path cannot be hit has written a note to itself, not a review comment.
Specialists must also omit praise, style preferences without impact, speculative concerns, defensive handling for states the system already prevents, and hardening, abstraction, error handling, or test coverage beyond what this change demonstrably needs. Findings outside the changed code belong in the review only when the change directly activates them. Three findings the author will act on beat ten the author will dismiss; an empty return is a legitimate result.
They inspect code, tests, and repository context statically and must not run tests, linters, type checkers, builds, or other checks — CI already produces that signal and duplicating it wastes review time and budget. Reading existing CI output is fine when directly relevant. They must not edit files or publish review comments.
Run specialists concurrently when capacity allows; batch them when it does not. The main agent must retain enough context and time to verify their work.
Completion criterion: every lens has been investigated and each specialist has either returned candidate findings or explicitly reported no actionable findings.
## 3. Verify every candidate finding
The main agent must independently verify every candidate before reporting it:
1. Open the cited lines and enough surrounding code to understand the execution path.
2. Confirm the issue is introduced by or materially exposed by the target diff.
3. Try to kill the finding. Search callers, shared abstractions, tests, validations, constraints, configuration, generated-code boundaries, and documentation for anything that already prevents the failure. Verification is an attempt to disprove the claim, not to dress it up; a candidate that survives a genuine attempt to invalidate it has earned its place.
4. Validate with a concrete reasoning path from the diff to the failure mode, grounded in the opened code and traced callers. Do not execute tests, linters, or builds to confirm a finding; run something only when the user explicitly asked for runtime verification.
5. Establish who actually hits this and how often, then calibrate severity to impact times likelihood. Likelihood bounds severity: a failure that needs conditions the system does not produce is not a blocker, and is usually not a finding at all. Drop it rather than reporting it at reduced severity.
6. Merge duplicates across specialists and discard unverified, speculative, or non-actionable items.
Do not report a specialist's claim merely because it sounds plausible. The main agent is accountable for every final finding, including the cost of a false one: findings that turn out to be unreachable teach the author to stop reading the review.
Completion criterion: every reported finding has independently checked evidence, an accurate location, a realistic trigger path, a defensible severity, and a concrete impact; every rejected candidate is omitted.
## 4. Apply editorial judgment before reporting
Verification proves a finding is true. It does not prove the finding is worth the author's attention. Before anything reaches the user or a pull request, the main agent reviews the surviving set as a whole and cuts.
Ask of each survivor: would a thoughtful author change this code because of it? If the honest answer is no, remove it. Specifically, cut:
- findings whose realistic trigger conditions are rare enough that the author would reasonably defer them, unless the impact when they do occur is severe;
- restatements of a tradeoff the author clearly made on purpose, where the review simply prefers the other side;
- "consider" and "you may want to" items with no demonstrated cost attached;
- theoretical robustness, extensibility, and coverage gaps that no planned or likely change would exercise;
- anything that survives only because of how it is worded. A finding that needs a hedge such as "unlikely in practice, but" has already answered the question — drop it.
Then judge the set, not just the items. A long list against a small diff means the bar drifted during the review; re-apply it rather than presenting the volume. Separate what should block from what is optional, and do not let a stack of minor items bury the one finding that matters. Never pad a report to look thorough, and never inflate severity to make a finding feel worth writing. Reporting two real problems and saying the rest of the change looks sound is a stronger review than listing eight.
A clean or near-clean review is a legitimate outcome. Say so plainly rather than reaching for something to report.
Completion criterion: every remaining finding is one the main agent would defend to the author in person, blocking and non-blocking items are distinguished, and the cut items are gone rather than softened.
## 5. Report the review
Lead with findings ordered by severity, then confidence. For each finding include:
- a short imperative title;
- severity;
- file and tight line range;
- why it matters in this specific code path, including what has to happen for it to trigger and how likely that is in real use;
- the smallest useful remediation direction.
Then include:
- a concise overall assessment;
- that verification was static — findings rest on code reading and reasoning — plus any uncertainty that introduces;
- remaining risks or coverage gaps;
- a brief note when no actionable findings were verified.
For **own work**, report the findings directly. Do not fix them unless the user asked for implementation.
For **another contributor's pull request**, provide the verified summary first, then ask whether the user wants a pull request review drafted on their behalf. Do not draft or publish the review before they agree.
If the user agrees to a PR review draft, invoke the `conventional-comments` skill and translate only the verified findings into Conventional Comments. Draft first; publish only with explicit authorization. Preserve severity, precise locations, and the distinction between blocking and non-blocking feedback.
A published comment carries a higher bar than a terminal report: it spends another person's attention, stays on the record, and is read as the user's own judgment. Apply the cut from step 4 again, harder. Post as blocking only what genuinely should block the merge; fold the remaining non-blocking observations into the summary comment or leave them out. If that second pass empties the list, say so and offer an approving or neutral summary instead of manufacturing inline comments.
## Review standard
- Review the change in repository context, not as isolated snippets.
- Execution belongs to CI: review by reading code and tests, not by running them. Consult existing CI results when they are relevant evidence.
- Prefer concrete defects and material design costs over taste.
- Every finding must answer: what breaks, for whom, and how often. A finding that cannot answer the third question is not ready to report.
- Do not ask for defensive code against states the system already prevents, error handling for errors the dependency does not raise, or configurability, abstraction, and validation the change has not shown it needs. Resilience the code does not need is a cost, not a safeguard.
- Judge the change against the standard the repository actually holds itself to, not an idealized one. A pattern the codebase consistently uses is the baseline; deviating from it is the finding, matching it is not.
- Do not confuse missing tests with a proven production defect; describe the actual coverage risk.
- Do not require a pattern such as an API client or service object by name. Recommend it only when it resolves demonstrated duplication, coupling, inconsistency, or testability problems.
- Under-reporting a marginal issue costs less than a review the author learns to discount. When a finding is genuinely borderline, leave it out.
- A clean review is valid when all six lenses were investigated and no candidate survived verification.