review-implementation

Files (2)

• SKILL.md
• ratings.md · references/

SKILL.md (first lines)

# Review implementation

Review the requested PR or local changes. Keep the assessment read-only: do not
edit source files or write review artifacts. Inspect existing test code and
recorded results; run tests only when the user explicitly authorizes test
execution for this review. A request to review, verify, or post findings is not
that authorization. For authorized tests, follow `run-checks` and any tool
restrictions; a coordinator can run checks a read-only reviewer cannot run.

Use this skill for the assessment even when the user also asks to post it on
GitHub. Complete the review first, then use `github-pr-review` to publish it.
The read-only assessment ends before that separately authorized posting step.
For a review or draft requested only in chat, report here without loading the
posting skill.

When a plan exists, its review standard, scope, contracts, required proof,
forbidden patterns, and acceptance checklist are authoritative. Default a
missing review standard to `GENERAL`. For `SCIENTIFIC_ANALYSIS`, load
`scientific-analysis`; do not load it for `GENERAL`. Plans follow the `planning` format; load it if a
section or field is unclear.

## Inputs

- **PR** — use the supplied URL or number, or an unambiguous PR identified by
  the request. Record its base and head commits.
- **Plan path** — use the supplied plan or one linked from the PR. For a PR
  without a plan, use its stated requirements and repository instructions;
  report `PLAN: none` and do not invent acceptance IDs. If the user explicitly
  asks for comparison against a particular plan that cannot be found, ask for
  its location.
- **Base or range** — honor an explicitly requested comparison and any file
  scope. Otherwise choose the comparison below.
- **Previous review** — optional. If supplied, this is a re-review (see below).

## Choose the diff

Resolve commit refs to commit IDs and state the comparison before inspecting
findings. Use the request and any earlier review to choose the scope:

| Requested review | Comparison |
| --- | --- |
| PR | Merge base of the PR base and head, through the PR head. Exclude unrelated local edits. |
| Branch | Merge base with the requested target branch, or the repository's default branch, through the branch head. |
| Uncommitted changes | `git diff HEAD --` for staged and unstaged tracked changes; also inspect relevant files from `git ls-files --others --exclude-standard`. |
| Staged changes only | `git diff --cached --`; inspect surrounding source from the index, not a newer working copy. |
| Unstaged changes only | `git diff --`; inspect relevant untracked files if the requested scope includes them. |
| Explicit commits or range | Honor the requested comparison, including two-dot or three-dot meaning; do not substitute the default branch. |
| Re-review after fixes | Compare the previously reviewed head with the current head, and revisit unresolved findings (see below). |

For PR and branch reviews, use `git diff <merge-base> <head> --` when the refs
are available locally, or obtain the equivalent PR diff through GitHub. Inspect
surrounding source from the reviewed head. Record both the target branch's
commit and the merge base used for comparison.

For an unspecified local review, infer scope from the request and `git status`:
prefer uncommitted changes when present, otherwise the branch diff. State that
choice. If multiple targets remain equally plausible or the necessary refs
cannot be identified, ask for the missing target instead of guessing a range.

Start with the changed files and lines. Follow callers, related code, tests,
and requirements when needed to understand their effects. Report defects the
changes introduce or expose, and failures of explicit requirements. Do not
expand into an audit of unrelated existing problems. An unchanged file can
still supply evidence for a required change that is missing.

## Inspecting

Batch independent reads and git inspections; don't reread an unchanged file.
Prefer symbol-aware search tools, when available, over paging whole files.

Inspect the diff, scope, contracts, required proof, forbidden patterns,
visible failure behavior, and agreement between analogous paths. Do not reward
partial work because tests pass. Also check:

- **Required estimates:** Trace each required estimate from where it is
  computed to where it is saved, through unchanged code as well as the diff.
  Look for branches that silently skip it, such as a `continue`, an empty
  default, or a filter that drops rows without raising.
- **Dropped units:** When a whole unit drops out, such as a participant with
  no sessions left after exclusions, check what happens to every count,
  denominator, and index that depends on it.
- **Output sets:** A script that writes several output files must publish all
  of them or none. A failure partway through must not leave a mix of new and
  stale files that looks like a complete result.
- **Teaching comments:** Key scientific steps need the teaching comments the
  global policy requires: what the step does, why it is needed, and how it
  affects the result.

Read or glob every plan-named implementation and test path directly. A file
missing from `git status` or the diff is not missing from the worktree — an
unchanged tracked file is real and present. A test that an implementation
acceptance item explicitly requires is a blocking defect only when it is
genuinely absent from the worktree or fails to establish the required behavior.

Inspect every acceptance ID and relevant contract even after finding a
blocker. `REVIEW_COVERAGE` must name each one you inspected.

Use [review ratings](references/ratings.md) for every finding and risk
candidate. State severity, likelihood, and a short evidence-based reason for
each. The ratings describe impact and exposure; they do not replace the
blocking rules below.

## Test quality

Inspect new and changed tests alongside nearby coverage and the behavior they
claim to verify. Assess this from the code and recorded results; these checks
do not authorize test execution.

- **Useful coverage:** Identify the meaningful result, required behavior, or
  plausible failure each test protects, and why existing tests would miss it.
  Flag overlapping cases, obsolete assertions, and tests of trivial details
  that add upkeep without distinct protection. Prefer extending or
  consolidating existing coverage over adding more tests.
- **Independent expectations:** Look for tautological tests: comparing a value
  with itself, deriving the expected answer through the same calculation being
  tested, or replacing the behavior under test with a mock and asserting its
  configured answer. Expected results should have an independent basis, such
  as a hand-calculated example, a trusted reference, or a scientific property.
  Sharing setup helpers is not itself a defect; trace whether the same mistake

From tobywise/agent-skills, MIT.

More from tobywise/agent-skills