code-review

Files (19)

• SKILL.md
• check_large_fixture.py · evals/
• evals.json · evals/
• approved-rates.csv · evals/files/
• large-pr.diff · evals/files/
• .editorconfig · evals/files/project-audit/
• AGENTS.md · evals/files/project-audit/
• README.md · evals/files/project-audit/
• package.json · evals/files/project-audit/
• batch.js · evals/files/project-audit/src/
• customer.js · evals/files/project-audit/src/
• ids.js · evals/files/project-audit/src/
• customer.test.js · evals/files/project-audit/test/
• generate_large_fixture.py · evals/
• trigger-eval.json · evals/
• change-hygiene.md · references/
• dependencies.md · references/
• project-audit.md · references/
• structural-remedies.md · references/

SKILL.md (first lines)

# Code Review and Quality

## Overview

Review the entire change and the surrounding code it affects before you recommend a merge, whether the diff is one line or more than 15,000. For a whole-project audit, inspect the existing repository instead of looking for a diff. Cover six axes: correctness, overengineering, readability, architecture, security, and performance. Review only; do not edit the code under review.

**Approval standard for changes:** Approve when the change meets its requirements and no Critical or Required findings remain, even if you would have written it differently. Don't block a change on style preferences or unrelated pre-existing problems; in a whole-project audit, existing problems are in scope. State verification gaps honestly.

## When to Use

- Before merging a pull request or change, including one produced by another agent or model
- After a feature, refactor, or bug fix (review both the fix and its regression test)
- When a change adds or upgrades dependencies: also read [references/dependencies.md](references/dependencies.md)
- When a change is very large, has a weak description, or the author disputes a finding: also read [references/change-hygiene.md](references/change-hygiene.md)
- When asked to audit the entire repository, including existing source, tests, and relevant configuration: also read [references/project-audit.md](references/project-audit.md)
- When you flag a structural problem and need a concrete remedy: see [references/structural-remedies.md](references/structural-remedies.md)

## The Six-Axis Review

Evaluate each axis where it is relevant. Report concrete findings rather than filling a quota for each axis. For a change, focus on what the change causes; for a whole-project audit, cover existing issues throughout the in-scope repository.

### 1. Correctness

- Does the code match the spec or task requirements, including edge cases, boundary values, and error paths?
- Do the tests exercise the required behavior, and were they actually run?
- Are there off-by-one errors, race conditions, or state inconsistencies?

### 2. Overengineering

Does the change add more moving parts than the required behavior needs?

- Can new code, an option, or a compatibility path be omitted without losing required behavior?
- Does the codebase, standard library, or platform already solve the problem?
- Does a new abstraction remove duplication or clarify ownership, or does it only add wrappers, modes, and indirection for hypothetical uses?
- Does a refactor remove branches and concepts, or only move the same complexity elsewhere?
- For each finding, name what to delete or simplify and what replaces it. Respect real correctness, security, and performance requirements; fewer lines alone is not a reason to flag code.

### 3. Readability

- Are names descriptive and consistent with project conventions?
- Is the control flow direct, without nested ternaries, deep callbacks, or clever tricks?
- Is non-obvious intent explained, and is obvious code left uncommented?
- Did the change leave dead code or unnecessary compatibility shims?

### 4. Architecture

- Does the change follow existing patterns? Is any new pattern justified?
- Do dependencies flow in the right direction, without cycles or new coupling?
- **Does feature-specific logic leak into a shared module?** Keep logic in its owning layer and reuse the canonical helper instead of a near-duplicate.
- **Are type boundaries explicit?** Question gratuitous `any`, `unknown`, optional fields, casts, and silent fallbacks that hide an unclear invariant. An explicit boundary often removes downstream branches.
- **Is a new conditional bolted onto an unrelated flow?** Prefer a focused helper or policy when it makes ownership clearer.

### 5. Security

- Is every external input (users, APIs, files, logs, configuration) treated as untrusted and validated at the boundary before use in logic, queries, or rendering?
- Are authentication and authorization checked where needed?
- Are secrets kept out of code, logs, and version control?
- Are queries parameterized and outputs encoded?

### 6. Performance

- Are there N+1 queries, unbounded loops or fetches, or missing pagination?
- Is expensive work repeated in a hot path, such as rebuilding data on every call?
- Is blocking work done where asynchronous work is expected?

## Review Process

### Step 1: Understand the Context and Conventions

Before judging code, read the applicable repository instructions and conventions: root and nested `AGENTS.md` or equivalent files, README and contribution or architecture guidance, lint and formatter settings, test conventions, and representative existing code. Apply the guidance that covers each file. Distinguish documented rules from inferred patterns, and don't report a convention violation without evidence. Then set the scope:

- **Change review:** What requirement does the diff implement, which files and lines changed, and what surrounding code and tests do they affect?
- **Whole-project audit:** What are the entry points, components, tests, and relevant configuration, and which existing behaviors must be checked?

### Step 2: Review the Tests First

Tests show intent and coverage. Check that they exist, test behavior rather than implementation details, cover edge and error cases, have descriptive names, and would catch a regression.

### Step 3: Review the Implementation

For each changed file or in-scope component, walk through the six axes above.

### Step 4: Categorize Findings

Label every finding with its severity so the author knows what must change:

| Prefix | Meaning | Effect on the verdict |
|--------|---------|-----------------------|
| **Critical:** | Security vulnerability, data loss, or broken required behavior | Blocks approval |
| **Required:** | A change that must be made before merge | Blocks approval |
| **Consider:** | A worthwhile suggestion | Does not block |
| **Nit:** | Minor style or formatting point the author may ignore | Does not block |
| **FYI:** | Context only; no action needed | Does not block |

The verdict for a change review follows from the labels: **Request changes** when any Critical or Required finding remains; otherwise **Approve**.

**Lead with what matters.** Order findings by impact: correctness and security first, then structural regressions and missed simplifications, then the rest. A few high-conviction findings beat a long list of nits. Anchor every finding to the exact current file path and the smallest relevant line or range (a changed line for a pull request), and cite every affected location when one root cause spans files. Explain the failure or the unnecessary complexity, and give an actionable remedy. Verify line numbers from the file or diff; if a location is unavailable, say so. Don't invent impact figures or present unverified risks as facts.

### Step 5: Verify the Verification

Check the evidence supplied with the change or available in the project: which tests and build checks ran, manual testing, screenshots for UI changes, and before-and-after comparisons. Require only verification relevant to the change. When you cannot run a check or evidence is missing, say so instead of claiming it passed.

## Coverage and Consolidation

### Change reviews

Start with an inventory of every changed file and hunk against the intended base. Record added and removed lines, the change's purpose, and relevant tests or callers. A one-line edit may take one pass; above about 1,000 changed lines, plan bounded batches organized by related files or behavior. Size is never a reason to skip, sample, or reject a review; if a change also grows a file past about 1,000 lines, check whether it should extract a module. Do not substitute a diff summary, a sample, or a size-based rejection for reading the change.

### Whole-project audits

Inventory in-scope source, tests, configuration, manifests, and behavior-defining docs; inspect every in-scope file in subsystem batches; and report coverage by category with exclusions and reasons. Read [references/project-audit.md](references/project-audit.md) for the full procedure.

### Both modes

For each batch:

1. Inspect every changed hunk or in-scope project file and enough surrounding code to understand its behavior. Follow affected call sites, interfaces, tests, and data flows across files. For mechanical or generated changes, validate the generator or transformation against the full output and inspect exceptions; document the method rather than treating unexamined lines as reviewed.
2. Record which files and hunks were examined, what was verified, and candidate findings with precise file paths, line numbers or ranges, and concrete remedies. Keep a compact coverage ledger outside the final report if the review exceeds the current context; use it to resume rather than rereading or silently dropping batches.
3. Reconcile the ledger with the initial inventory and examine cross-batch interactions before writing the verdict. Combine duplicate symptoms under their shared root cause, but retain distinct actionable findings and cite each affected path.

From akash07k/skills, AGPL-3.0.

More from akash07k/skills