reviewing-code
npx machina-cli add skill oryanmoshe/agent-skills/reviewing-code --openclawFiles (1)
SKILL.md
4.3 KB
Reviewing Code
Overview
Review code changes systematically using priority-based rules. Focus on what automated linters miss — performance, architecture, test coverage, and security patterns.
Review Workflow
- Identify changed files — focus review on actual changes, not entire codebase
- Check P0 rules first — blocking issues stop the review
- Check P1 rules — important issues that need discussion
- Optionally check P2 — only if user wants a thorough review
- Provide fixes — include code suggestions for each issue
Priority Levels
- P0: Blocking — must fix before merge
- P1: Important — should fix, discuss if not
- P2: Nice-to-have — suggest but don't block
What to Check
P0 — Blocking Issues
| Category | What to look for |
|---|---|
| N+1 queries | Database/API call inside a loop; resolver without batching |
| Security | Missing auth check on endpoint; fail-open permission pattern; SQL injection; XSS |
| Data loss | Missing error handling on write operations; no transaction for multi-step mutations |
| Memory leaks | Uncleaned subscriptions, timers, or event listeners in effects |
P1 — Important Issues
| Category | What to look for |
|---|---|
| Performance | O(n²) when O(n) is possible; expensive computation in render path; missing memoization for derived data; large bundle imports |
| Error handling | Missing try/catch on async operations; swallowed errors; missing error boundaries |
| Test coverage | New business logic without tests; untested error paths; missing edge cases |
| Null safety | Accessing nested properties without null checks; missing optional chaining |
| React hooks | Missing dependencies in useEffect; missing cleanup functions; hooks inside conditions |
P2 — Nice to Have
| Category | What to look for |
|---|---|
| Naming | Unclear variable/function names; inconsistent naming patterns |
| Code organization | Logic in wrong layer (UI doing business logic); duplicated code |
| Types | Missing type annotations on public APIs; any types |
| Clean code | Magic numbers; deeply nested conditionals; functions doing too many things |
Human Blind Spots — Prioritize These
These are rarely caught by human reviewers. The skill must catch them:
- Performance — O(n) vs O(1) lookups, unnecessary re-renders, bundle size impact
- Test coverage — missing tests for new logic, untested error paths
- Memory leaks — uncleaned subscriptions, timers, event listeners
Explanation Style
DO: Provide full verbal explanations
"This calls the user service inside a loop. With 50 users, that's
51 database calls instead of 2. Batch the IDs and make a single
query with a WHERE IN clause."
DON'T: Leave terse comments without context
"N+1 query"
Output Format
For each issue:
### [P0] Short Description
**File:** path/to/file.ts:42
**Issue:** [Full explanation of the problem and its impact]
**Fix:**
// Before
[problematic code]
// After
[fixed code]
Inline Code Markers
When reviewing code in-place, use these markers:
// FIXME(review): [P0] N+1 query — will cause performance degradation at scale
// TODO(review): [P1] Add error handling for network failure case
// NIT(review): Consider renaming for clarity
After Review
Present the summary with issue counts by severity, then ask the user what to do next:
- Post comments to GitHub PR
- Create tasks to track fixes
- Just show the summary
Red Flags — STOP and Review
| Thought | Reality |
|---|---|
| "This is too simple to review" | Simple changes break things. Review. |
| "Tests pass so it's fine" | Tests can't catch what they don't cover. Review. |
| "It's just a refactor" | Refactors introduce subtle bugs. Review. |
| "The linter will catch it" | Linters miss logic, performance, and architecture. Review. |
Source
git clone https://github.com/oryanmoshe/agent-skills/blob/main/skills/reviewing-code/SKILL.mdView on GitHub Overview
Reviewing code changes systematically using priority-based rules helps catch issues automated linters miss, including performance, architecture, test coverage, and security patterns. It prioritizes blocking (P0) issues, important (P1) problems, and optional (P2) enhancements, with concrete fixes and code suggestions.
How This Skill Works
Identify changed files to focus the review on actual changes. Apply P0 blocking issues first, then P1 important issues, and optionally P2 nice-to-have checks, documenting each finding with fix proposals. Use inline markers (FIXME, TODO, NIT) to convey context and guide follow-up actions.
When to Use It
- Review PRs before merging to catch blocking issues like N+1 queries, missing auth checks, or data loss risks.
- Review changes before committing to ensure alignment with quality standards and security patterns.
- After making code changes to validate fixes, ensure test coverage, and confirm no new issues were introduced.
- When a user asks you to review, check, or look over code snippets to provide quick, actionable feedback.
- During ongoing reviews to identify performance, test coverage, and security gaps that automated tools may miss.
Quick Start
- Step 1: Identify changed files to focus the review on actual changes.
- Step 2: Run through P0 (Blocking) first, then P1 (Important), and optionally P2 (Nice-to-have).
- Step 3: Provide fixes for each issue with concrete code suggestions and inline markers (FIXME, TODO).
Best Practices
- Limit the review to changed files only to stay efficient and focused.
- Triage issues by severity: start with P0 blocking issues, then P1 important issues, and consider P2 for enhancements.
- Prioritize catching N+1 queries, missing error handling, and security vulnerabilities as you review.
- Provide actionable fixes with concrete code suggestions and before/after snippets when possible.
- Annotate issues with inline markers (FIXME, TODO, NIT) and summarize findings with a follow-up plan.
Example Use Cases
- Detect an N+1 query by identifying a database call inside a loop without batching it into a single query.
- Missed authentication check on an API endpoint that could expose data to unauthorized users.
- Missing transaction handling for a multi-step mutation, risking data inconsistency or loss.
- Uncleaned subscriptions or timers in a React useEffect causing memory leaks or stale data.
- New business logic with no tests or untested error paths, leaving edge cases uncovered.
Frequently Asked Questions
Add this skill to your agents
