Code Review Agent Rules
Project Context
You are conducting or receiving code reviews on pull requests. Reviews improve code quality, share knowledge, and maintain consistency. The goal is constructive collaboration, not gatekeeping.
Author Responsibilities
- Keep PRs small and focused — under 400 lines of diff is ideal; split larger changes into a stack of smaller PRs.
- Write a clear PR description: what changed, why it was needed, how to test it, and what alternatives were considered.
- Link to the relevant issue, ticket, or design document with a closing keyword: `Closes #123`.
- Self-review before requesting others — run the linter, tests, and read your own diff as if you were the reviewer.
- Add comments to non-obvious decisions in the code itself, not just the PR description.
- Respond to every review comment — acknowledge, implement, or explain disagreement respectfully.
- Push fixes as new commits during review; squash into clean commits before merging.
Reviewer Responsibilities
- Complete review within 24 hours of assignment — stale PRs block the entire team.
- Read the PR description and understand the goal before reading the diff.
- Start with the overall structure and design before diving into line-level details.
- Comment on what needs changing, why, and optionally how — provide context for every blocking comment.
- Use explicit prefixes to communicate comment severity:
- `blocker:` Must be fixed before merge.
- `nit:` Minor style preference, optional to address.
- `question:` Seeking understanding, not requesting a change.
- `suggestion:` Non-blocking improvement idea.
- Acknowledge good work and clever solutions — reviews should not be purely critical.
- Approve once all blockers are resolved — do not block on nits or personal preferences.
Correctness Checklist
- Does the code do what the description claims? Trace through the logic manually for complex paths.
- Are all error cases handled explicitly — not silently swallowed or left to propagate unexpectedly?
- Are there off-by-one errors, null/undefined dereferences, or race conditions in concurrent code?
- Are edge cases tested: empty collections, zero values, maximum values, concurrent execution?
- Does the change handle partial failure correctly — if step 3 of 5 fails, is state left consistent?
Security Checklist
- No hardcoded secrets, API keys, tokens, or credentials — not even in tests.
- All user-supplied input is validated and sanitized before processing or storing.
- SQL queries use parameterized queries or an ORM — no string concatenation with user data.
- Authentication and authorization checks are applied on every relevant endpoint and action.
- Sensitive data is not logged, echoed in error messages, or exposed via API responses.
- New dependencies are checked for known vulnerabilities via `npm audit`, `snyk`, or Dependabot.
Performance Checklist
- Are there N+1 query patterns introduced by new data access code?
- Are unbounded result sets properly paginated or limited?
- Are expensive operations (API calls, heavy computation) cached or moved to background jobs?
- Are new database queries backed by appropriate indexes?
- Do any changes affect render-blocking behavior in frontend code?
Readability Checklist
- Are functions small and focused on a single responsibility? Flag anything over 20 lines.
- Are variable, function, and type names self-documenting?
- Are non-obvious decisions documented with comments explaining the why?
- Is there dead code, commented-out code, or debugging statements left in?
- Is the code consistent with existing patterns and conventions in the codebase?
Architecture Checklist
- Does the change follow the established architectural patterns of the codebase?
- Does it introduce unnecessary coupling or circular dependencies between modules?
- Are abstractions appropriate — not over-engineered, not leaking implementation details?
- Are new dependencies justified? Does something equivalent already exist in the project?
- Are breaking changes to public APIs or data schemas documented and backwards-compatible where possible?
Testing Checklist
- Is the new or changed behavior covered by tests?
- Are tests testing behavior (what the code does) rather than implementation (how it does it)?
- Are edge cases and error paths tested, not just the happy path?
- Are tests independent, deterministic, and free of shared mutable state?
- Do test names clearly describe what they verify without reading the assertion?
Code Review
