91 lines
4.2 KiB
Markdown
91 lines
4.2 KiB
Markdown
---
|
|
name: code-review
|
|
description: >
|
|
Review existing code for bugs, security problems and risky design: scope the
|
|
review (git diff or files), outline and lint with a helper script, read
|
|
function by function, verify each suspected bug with a concrete failing input,
|
|
and write a ranked REVIEW.md. Use when the task asks to review, audit, check,
|
|
or find bugs/problems in code - not for writing new features.
|
|
---
|
|
|
|
# Code review
|
|
|
|
The goal is a short list of REAL problems the author would want to fix, each
|
|
with evidence. Ten verified findings beat fifty guesses. A review does not
|
|
change the code unless the task explicitly asks for fixes.
|
|
|
|
## 1. Scope and groundwork (one command)
|
|
```
|
|
python <skill-dir>/review_prep.py # git: changed files; else all source files
|
|
python <skill-dir>/review_prep.py a.py b.js # only these files
|
|
python <skill-dir>/review_prep.py --base main # diff against a branch/commit
|
|
```
|
|
`<skill-dir>` is the absolute directory at the top of this text. Per file you
|
|
get: changed line ranges, an outline (`start-end def name`), compiler/linter
|
|
output and hints. Linter output and hints are LEADS, not findings - many are
|
|
harmless in context. `NOT CHECKED` means no compiler was available: say so in
|
|
the report.
|
|
|
|
## 2. Understand intent before judging
|
|
Read what the code is supposed to do: the task text, README/AGENTS.md, module
|
|
docstrings, the tests. A "bug" is a difference between intent and behaviour -
|
|
without knowing the intent you can only report style.
|
|
|
|
## 3. Read function by function
|
|
- Use the outline: `read_file(path, start_line=a, end_line=b)` for ONE function
|
|
at a time. Never read a whole large file; never re-read a range you have.
|
|
- Order: changed ranges first, then the functions they call and the callers
|
|
of changed functions (`search_files("name\\(")`), then hotspots: input
|
|
parsing, file/network I/O, error handling, concurrency, anything that builds
|
|
shell commands, SQL, HTML or paths from input.
|
|
- For each function ask the questions in [checklist.md](./checklist.md).
|
|
Load it once at the start: `load_skill("code-review", "checklist.md")`.
|
|
|
|
## 4. Verify every finding
|
|
For each suspected problem, write down a concrete failure scenario:
|
|
**input/state -> what happens -> what should happen**. Then:
|
|
- If it is cheap, PROVE it: a 1-5 line repro with `run_command`
|
|
(`python -c "from calc import average; average([])"`, a tiny test file,
|
|
`node -e ...`). Mark it **CONFIRMED** and quote the output.
|
|
- If a repro is not practical (needs network, GUI, a race), mark it
|
|
**PLAUSIBLE** and state the exact preconditions.
|
|
- If you cannot state a concrete scenario, it is not a finding - drop it or
|
|
list it under "Questions for the author".
|
|
Never put repro files into the project's source tree: use a `_review/` folder
|
|
and mention it in the report.
|
|
|
|
## 5. Report: write REVIEW.md
|
|
```markdown
|
|
# Review: <scope> (files, base, date)
|
|
Checked with: <linters/compilers that ran; what was NOT CHECKED>
|
|
|
|
## Findings (most severe first)
|
|
### 1. [HIGH] <one-line claim> - `file.py:42` - CONFIRMED
|
|
**Scenario:** `average([])` -> ZeroDivisionError is swallowed, returns None;
|
|
callers format None as "None %" (report.py:88).
|
|
**Evidence:** `python -c ...` -> `None`
|
|
**Fix:** raise ValueError for empty input (or return 0.0 if that is the intent).
|
|
|
|
## Questions for the author
|
|
- ...
|
|
|
|
## Not reviewed
|
|
- vendor/, generated files, ...
|
|
```
|
|
Severity: **HIGH** = wrong results, data loss, crash on normal input, security
|
|
hole. **MEDIUM** = crash/misbehaviour on edge cases, resource leaks, silent
|
|
error swallowing. **LOW** = maintainability problems that will likely cause
|
|
bugs later. Pure style (naming, formatting, import order) only if asked.
|
|
|
|
At most ~10 findings; if there are more, keep the most severe and say how many
|
|
smaller ones exist. End your answer with the count per severity and the path
|
|
to REVIEW.md - do not paste the whole report into the answer.
|
|
|
|
## Pitfalls
|
|
- Reporting a linter line verbatim without reading the code around it.
|
|
- "Could be None" without showing a call path where it IS None.
|
|
- Suggesting a rewrite instead of the smallest fix.
|
|
- Flagging framework conventions as bugs (read how the codebase does it elsewhere).
|
|
- Running out of steps on a big codebase: after the changed ranges and
|
|
hotspots, stop and list the rest under "Not reviewed".
|