Code review

Skill UnpaidAttention/fable5-methodology/skills/code-review

Review a diff or pull request — including your own before delivery — in a fixed pass order (correctness, then safety/security, then design, then style) and report findings ranked by severity. Trigger this when asked to review a PR or diff, when checking over a change before committing or handing it off, and as the design-and-correctness half of your own pre-delivery review. Do NOT trigger for the mechanical did-it-run gate (use verification-and-review) or for a dedicated vulnerability sweep (use security-review, which this skill's safety pass defers to) — this skill is holistic human-style review of a change set.From its SKILL.md

Install

npx -y skills add UnpaidAttention/fable5-methodology --skill code-review

Assembled from the repository path, not quoted from the project. Check it against their README if it does not work.

One thing to look at

no licenseNo license file was found in the repository. Code published without one is not open source by default, so using it at work is a question for whoever answers licensing questions where you are.

SKILL.md

5.5 KB, as published. Nobody here has run it

Code Review

Review in order of what costs most to get wrong. A style nit found while a correctness bug goes unnoticed is a review that failed. Do the passes in sequence — do not let a formatting distraction pull you off the correctness pass.

Setup: get the whole diff and its intent

Read the full change set (git diff <base>...HEAD, or the PR diff) — not just the latest commit. Review the change as it will land.
Know the intent: the PR description, the linked issue, or the task. A diff can be internally perfect and still wrong — because it does the wrong thing. Judge against intent.
Read big diffs in dependency order (types → logic → interfaces), not file-alphabetical.

Pass 1: Correctness (highest priority)

For each hunk ask:

Does it do what the intent says? Are all stated requirements actually met by this diff?
Edge cases: empty inputs, boundaries (off-by-one, < vs <=), null vs empty vs zero, duplicates, malformed input. Walk the same edge list as verification-and-review against the new code.
Logic errors: inverted conditions, wrong operator, wrong variable, swapped arguments, incorrect loop bounds, mishandled async (unawaited promises, missing error propagation).
Are there tests, and do they assert concrete behavior (not just "ran without throwing")? Does a test exist for the bug this PR claims to fix, failing on the old code?

Pass 2: Safety and security

Trust boundaries: is new external input validated? New sinks (query, command, path, HTML, deserializer) safe? If the diff touches auth, input handling, or sensitive data, run the full security-review checklist rather than a glance.
Failure behavior: fallible operations handled, not silently swallowed; errors carry context; resources cleaned up on all paths.
Secrets: none introduced in code or fixtures. Concurrency: new shared state or check-then-act races? Data safety: migrations reversible, destructive operations guarded.

Pass 3: Design

Right place: does the change belong where it was put, following the codebase's existing patterns? Or does it fight them / duplicate an existing utility?
Scope: does the diff exceed its intent (drive-by refactors, unrelated changes)? Flag scope creep — it's a review finding, not a courtesy.
Complexity: simpler equivalent available? Speculative abstraction (interface with one impl, config for a constant) to cut? Duplication that should be extracted (rule of three)?
Interfaces: are new public signatures/names clear and consistent with the codebase? Any breaking change to an existing contract, intended or accidental?

Pass 4: Style (lowest priority)

Naming clarity, comment quality (why-not-what, no process narration), formatting consistency. Rule: never let Pass 4 findings crowd out or outnumber Pass 1–2 findings in the report. If the linter enforces it, don't hand-review it — say "linter covers formatting" and move on.

Reporting

Rank by severity, most severe first. Each finding: file:line, what's wrong, why it matters (the failure it causes), and a concrete fix or a specific question.

CRITICAL — will break in production / security hole / data loss. Must fix before merge.
HIGH — bug under realistic conditions, missing error handling on a real path, requirement unmet. Fix before merge.
MEDIUM — design smell, missing test, scope creep, maintainability drag. Should fix.
LOW — style, naming, nit. Optional.

End with a one-line verdict: approve / approve-with-nits / changes-requested, and the count by severity. If there are zero substantive findings, say so plainly — don't manufacture nits to look thorough.

Reviewing your own diff

Same passes, plus: read it as a stranger's (you're blind to your own intent — lean on the written requirements instead), and specifically hunt your signature failure modes: unverified API calls stated as correct, requirements quietly narrowed, success implied without a run. Collect all findings in one read-through before fixing any — fixing mid-read hides structural problems.

Worked example finding

HIGH  src/auth/session.ts:88  Missing expiry check
  refreshSession() issues a new token whenever the old one decodes, without checking exp.
  An expired token still refreshes → sessions never actually expire (defeats the timeout the
  PR's issue asks for — requirement unmet).
  Fix: reject when payload.exp < now() before issuing; add a test with an expired token
  asserting 401.

Done when

All four passes ran in order over the complete diff judged against its stated intent; findings are ranked by severity with file:line, impact, and a concrete fix; Pass 1–2 findings are not buried under Pass 4 nits; and the report ends with an explicit verdict and severity counts. For your own work, the signature-failure-mode hunt was done before delivery.