code-review

Name: code-review
Author: UnpaidAttention

84stars18forks

How to install code-review?

unpaidattention/fable5-methodology/code-review

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

Installs into the current project.

Summary

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 commi

Files · 1

View on GitHub

SKILL.md

1	# Code Review
2
3	Review in order of what costs most to get wrong. A style nit found while a correctness bug
4	goes unnoticed is a review that failed. Do the passes in sequence — do not let a formatting
5	distraction pull you off the correctness pass.
6
7	## Setup: get the whole diff and its intent
8
9	1. Read the full change set (`git diff <base>...HEAD`, or the PR diff) — not just the latest
10	commit. Review the change as it will land.
11	2. Know the intent: the PR description, the linked issue, or the task. A diff can be
12	internally perfect and still wrong — because it does the wrong thing. Judge against intent.
13	3. Read big diffs in dependency order (types → logic → interfaces), not file-alphabetical.
14
15	## Pass 1: Correctness (highest priority)
16
17	For each hunk ask:
18	- Does it do what the intent says? Are all stated requirements actually met by this diff?
19	- Edge cases: empty inputs, boundaries (off-by-one, `<` vs `<=`), null vs empty vs zero,
20	duplicates, malformed input. Walk the same edge list as verification-and-review against the
21	new code.
22	- Logic errors: inverted conditions, wrong operator, wrong variable, swapped arguments,
23	incorrect loop bounds, mishandled async (unawaited promises, missing error propagation).
24	- Are there tests, and do they assert concrete behavior (not just "ran without throwing")?
25	Does a test exist for the bug this PR claims to fix, failing on the old code?
26
27	## Pass 2: Safety and security
28
29	- Trust boundaries: is new external input validated? New sinks (query, command, path, HTML,
30	deserializer) safe? If the diff touches auth, input handling, or sensitive data, run the
31	full security-review checklist rather than a glance.
32	- Failure behavior: fallible operations handled, not silently swallowed; errors carry context;
33	resources cleaned up on all paths.
34	- Secrets: none introduced in code or fixtures. Concurrency: new shared state or check-then-act
35	races? Data safety: migrations reversible, destructive operations guarded.
36
37	## Pass 3: Design
38
39	- Right place: does the change belong where it was put, following the codebase's existing
40	patterns? Or does it fight them / duplicate an existing utility?
41	- Scope: does the diff exceed its intent (drive-by refactors, unrelated changes)? Flag scope
42	creep — it's a review finding, not a courtesy.
43	- Complexity: simpler equivalent available? Speculative abstraction (interface with one impl,
44	config for a constant) to cut? Duplication that should be extracted (rule of three)?
45	- Interfaces: are new public signatures/names clear and consistent with the codebase? Any
46	breaking change to an existing contract, intended or accidental?
47
48	## Pass 4: Style (lowest priority)
49
50	Naming clarity, comment quality (why-not-what, no process narration), formatting consistency.
51	Rule: never let Pass 4 findings crowd out or outnumber Pass 1–2 findings in the report.
52	If the linter enforces it, don't hand-review it — say "linter covers formatting" and move on.
53
54	## Reporting
55
56	Rank by severity, most severe first. Each finding: `file:line`, what's wrong, why it matters
57	(the failure it causes), and a concrete fix or a specific question.
58
59	- CRITICAL — will break in production / security hole / data loss. Must fix before merge.
60	- HIGH — bug under realistic conditions, missing error handling on a real path,
61	requirement unmet. Fix before merge.
62	- MEDIUM — design smell, missing test, scope creep, maintainability drag. Should fix.
63	- LOW — style, naming, nit. Optional.
64
65	End with a one-line verdict: approve / approve-with-nits / changes-requested, and the count by
66	severity. If there are zero substantive findings, say so plainly — don't manufacture nits to
67	look thorough.
68
69	## Reviewing your own diff
70
71	Same passes, plus: read it as a stranger's (you're blind to your own intent — lean on the
72	written requirements instead), and specifically hunt your signature failure modes: unverified
73	API calls stated as correct, requirements quietly narrowed, success implied without a run.
74	Collect all findings in one read-through before fixing any — fixing mid-read hides structural
75	problems.
76
77	## Worked example finding
78
79	```
80	HIGH src/auth/session.ts:88 Missing expiry check
81	refreshSession() issues a new token whenever the old one decodes, without checking exp.
82	An expired token still refreshes → sessions never actually expire (defeats th

People who liked this also viewed

6 picks

Type

code-review

84stars18forks

View on GitHub

How to install code-review?

unpaidattention/fable5-methodology/code-review

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

Installs into the current project.

Summary

Files · 1

View on GitHub

SKILL.md

1	# Code Review
2
3	Review in order of what costs most to get wrong. A style nit found while a correctness bug
4	goes unnoticed is a review that failed. Do the passes in sequence — do not let a formatting
5	distraction pull you off the correctness pass.
6
7	## Setup: get the whole diff and its intent
8
9	1. Read the full change set (`git diff <base>...HEAD`, or the PR diff) — not just the latest
10	commit. Review the change as it will land.
11	2. Know the intent: the PR description, the linked issue, or the task. A diff can be
12	internally perfect and still wrong — because it does the wrong thing. Judge against intent.
13	3. Read big diffs in dependency order (types → logic → interfaces), not file-alphabetical.
14
15	## Pass 1: Correctness (highest priority)
16
17	For each hunk ask:
18	- Does it do what the intent says? Are all stated requirements actually met by this diff?
19	- Edge cases: empty inputs, boundaries (off-by-one, `<` vs `<=`), null vs empty vs zero,
20	duplicates, malformed input. Walk the same edge list as verification-and-review against the
21	new code.
22	- Logic errors: inverted conditions, wrong operator, wrong variable, swapped arguments,
23	incorrect loop bounds, mishandled async (unawaited promises, missing error propagation).
24	- Are there tests, and do they assert concrete behavior (not just "ran without throwing")?
25	Does a test exist for the bug this PR claims to fix, failing on the old code?
26
27	## Pass 2: Safety and security
28
29	- Trust boundaries: is new external input validated? New sinks (query, command, path, HTML,
30	deserializer) safe? If the diff touches auth, input handling, or sensitive data, run the
31	full security-review checklist rather than a glance.
32	- Failure behavior: fallible operations handled, not silently swallowed; errors carry context;
33	resources cleaned up on all paths.
34	- Secrets: none introduced in code or fixtures. Concurrency: new shared state or check-then-act
35	races? Data safety: migrations reversible, destructive operations guarded.
36
37	## Pass 3: Design
38
39	- Right place: does the change belong where it was put, following the codebase's existing
40	patterns? Or does it fight them / duplicate an existing utility?
41	- Scope: does the diff exceed its intent (drive-by refactors, unrelated changes)? Flag scope
42	creep — it's a review finding, not a courtesy.
43	- Complexity: simpler equivalent available? Speculative abstraction (interface with one impl,
44	config for a constant) to cut? Duplication that should be extracted (rule of three)?
45	- Interfaces: are new public signatures/names clear and consistent with the codebase? Any
46	breaking change to an existing contract, intended or accidental?
47
48	## Pass 4: Style (lowest priority)
49
50	Naming clarity, comment quality (why-not-what, no process narration), formatting consistency.
51	Rule: never let Pass 4 findings crowd out or outnumber Pass 1–2 findings in the report.
52	If the linter enforces it, don't hand-review it — say "linter covers formatting" and move on.
53
54	## Reporting
55
56	Rank by severity, most severe first. Each finding: `file:line`, what's wrong, why it matters
57	(the failure it causes), and a concrete fix or a specific question.
58
59	- CRITICAL — will break in production / security hole / data loss. Must fix before merge.
60	- HIGH — bug under realistic conditions, missing error handling on a real path,
61	requirement unmet. Fix before merge.
62	- MEDIUM — design smell, missing test, scope creep, maintainability drag. Should fix.
63	- LOW — style, naming, nit. Optional.
64
65	End with a one-line verdict: approve / approve-with-nits / changes-requested, and the count by
66	severity. If there are zero substantive findings, say so plainly — don't manufacture nits to
67	look thorough.
68
69	## Reviewing your own diff
70
71	Same passes, plus: read it as a stranger's (you're blind to your own intent — lean on the
72	written requirements instead), and specifically hunt your signature failure modes: unverified
73	API calls stated as correct, requirements quietly narrowed, success implied without a run.
74	Collect all findings in one read-through before fixing any — fixing mid-read hides structural
75	problems.
76
77	## Worked example finding
78
79	```
80	HIGH src/auth/session.ts:88 Missing expiry check
81	refreshSession() issues a new token whenever the old one decodes, without checking exp.
82	An expired token still refreshes → sessions never actually expire (defeats th

People who liked this also viewed

6 picks

Type