quality-review

name	quality-review
description	Code review focused on code quality and craft — readability, structure, and style — not correctness, security, or naming. Use when the user says "/quality-review", "review jakości", "sprawdź jakość kodu", "przejrzyj pod kątem jakości", "quality review", "czy ten kod się dobrze czyta", or asks whether a change reads cleanly and is well structured. Especially apt for code freshly generated by a strong model, which is correct but glued together: blocks with no blank lines, helpers ordered randomly, OOP and functional styles mixed, pointless barrel exports, unnecessary casts, and two near-identical functions that should be one. Reviews the current branch diff by default (auto-detects the base branch, or pass --base), or explicit file/dir paths. Returns per-finding severity + a concrete suggested fix, and offers to apply the safe ones.
allowed-tools	Read, Bash, Grep, Glob, Edit, AskUserQuestion, Task

quality-review — review how the code reads, not whether it works

You review code quality and craft: readability, vertical structure, the ordering of functions, stylistic consistency, and needless complexity. You do not review correctness, security, performance, naming, or test coverage — other tools own those. If the code reads cleanly, say so and stop; do not invent findings to look thorough.

The motivating case: code written by a strong model is usually correct but glued together. Six small functions stacked with no blank line between them, an if whose body is jammed against the next statement, helpers in random order, two functions that differ by one line. None of it is a bug. All of it slows the next human (or model) who has to read it. Your job is to find that friction and propose the smaller, calmer version.

Step 0 — Read the project's own conventions first

Before judging structure, read CLAUDE.md, AGENTS.md, and any .cursor/rules or CONTRIBUTING.md at the repo root (and in the directory being reviewed). They override the structural rules below: if the project documents barrel exports as its public-API style, or a layered file ordering, that is the standard here and you must not flag it. A rule you'd otherwise raise becomes a non-finding when the project has deliberately chosen it. Note which conventions you picked up.

Step 1 — Resolve scope

Parse the invocation arguments:

• Arguments are file or directory paths → review those targets in full. Expand directories to their source files. Skip the diff machinery below.

• --base <branch> → pass it straight through to the script as --base <branch>.

• No path arguments → review the current branch diff. Detect what exists with the bundled script, which resolves the base defensively (@{upstream}, then origin/main, origin/master, main, master) and covers committed, uncommitted, and untracked changes:

  python3 ${CLAUDE_PLUGIN_ROOT}/skills/quality-review/scripts/get_changes.py --scope uncommitted
  python3 ${CLAUDE_PLUGIN_ROOT}/skills/quality-review/scripts/get_changes.py --scope committed
  

  (append --base <branch> to both when the user passed one.) Read the count of each:

  • both zero → tell the user there is nothing to review and stop;
  • exactly one non-zero → use that scope automatically;
  • both non-zero → ask with AskUserQuestion which to review — Uncommitted (working tree vs HEAD), Committed (HEAD vs base), or Both (base → working tree) — putting the file counts you just saw in each option's description.

  Re-run the script once with the chosen --scope to get the canonical file list. Each entry carries path, status, binary, an optional untracked, plus the run's diff_args. To see a file's change:

  • tracked → git diff <diff_args> -- <path>;
  • untracked ("untracked": true) → git diff shows nothing, so read the file directly and treat every line as added.

  Base resolution lives in the script, which computes the fork point internally (via a subprocess git call the Bash hook never sees) — so there is no git merge-base command to run here, and nothing for a "block the word merge" hook to catch.

If the script exits with "could not resolve a base ref", tell the user and offer to review uncommitted changes only or to pass --base <branch> — never guess silently.

Read the whole file for context, but score the changed lines

Read the whole changed file — ordering, style-mix, barrel, and over-complex are whole-file and cross-file properties you cannot see in a diff hunk, so you need the surrounding code to judge them. But the review targets what this change added or modified. Sort every finding into one of two buckets, and keep them apart:

• Primary findings — the problem is in code this change touched: the added/modified lines, or structure the change introduced or made worse (a new helper placed out of stepdown order, a freshly duplicated function). This is what the review is about.
• Extra clean-up (boy-scout) — the problem is in untouched code you only noticed while reading for context. The boy-scout rule ("leave the code cleaner than you found it") makes it worth surfacing, but it is optional, clearly separated, and never mixed into the primary findings. The author asked you to review their change, not to rewrite the file.

Don't let a pile of pre-existing issues drown the few the change actually introduced — that inversion is exactly what makes a review feel like noise.

In scope vs skip

Review source files: .ts .tsx .js .jsx .py .go .rs .java .kt .swift .c .cpp .h .rb .php .vue .scala .cs and similar. Skip: JSON, lockfiles, generated/minified files (.d.ts from a generator, *_pb.*, anything under dist/, build/, node_modules/), .md/docs, and config. When you skip a changed file, note it in one line so coverage is honest.

Step 1.5 — Pick the review mode: inline by default, fan-out for large diffs

Default: inline. One agent (you) reads every in-scope file and judges it against all seven rules. This is correct and cheapest for a normal change, and it keeps the cross-file rules (ordering, style-mix, over-complex, barrel) in a single head. Use inline unless the diff is genuinely large.

Fan out only when the diff is too big to hold at once — roughly more than ~20 in-scope source files, or a set so large you would be re-reading files to keep context while judging. This is a judgment call, not a hard gate; a 13-file change reviews fine inline. Below the threshold, do not fan out — the coordination and merge cost is not worth it, and parallel agents each tend to inflate their own findings, which is the noise this skill exists to suppress.

When you do fan out, dispatch parallel subagents with the Task tool, one per rule-family lens (not per "role" — naming, correctness, and security are out of scope and get no lens):

• Lens A — structure & tests: openness, test-structure, ordering
• Lens B — simplification & waste: over-complex, needless-cast
• Lens C — module shape: style-mix, barrel

Each lens keeps whole files, so the cross-file rules in its family still see the surrounding code. Give every subagent: the in-scope file list (paths), the verbatim rule sections for its lens from Step 2 (including their calibration paragraphs), the severity table, and the finding contract — `name` severity · L<lines> — what the reader loses → the fix, primary vs boy-scout split. A subagent returns findings only; it does not render the Step 3 skeleton, does not edit files, and does not run Step 4.

You then merge: collect all lenses' findings, dedup overlaps (most-specific wins, exactly as inline), and re-grade every finding yourself against the Step 2 severity table — do not trust a subagent's severity, since a single-lens agent is the one most prone to the anchoring the table forbids. Then render the one Step 3 report and run Step 4. The fan-out changes who reads, never the output contract: the user sees the same single skeleton either way.

Step 2 — Judge against the rules

Each finding gets one rule name and one severity. Use these seven names verbatim in the report — they are the fixed vocabulary, so the reader (and a diff between two reviews) sees the same label every time, never a code number or a paraphrase you invented this run:

name	what it catches
openness	logical blocks jammed together with no blank line
test-structure	arrange/act/assert (given/when/then) interleaved or out of order
ordering	helpers not in stepdown / newspaper order under their caller
style-mix	OOP and functional mixed ad hoc (a misplaced free function or class)
barrel	a pointless re-export index.* that narrows nothing
needless-cast	a type cast the value's type already guarantees
over-complex	code that collapses into something smaller (duplication → one parameter)

Severity:

• high — a wrong structural decision or real waste: collapsible duplication (over-complex), an OOP/functional break that misplaces code (style-mix), a cast that masks a stale type (needless-cast). These cost the most to live with.
• medium — readability friction a reader feels every time: ordering, test-structure interleaving, pointless indirection (barrel).
• nit — local openness / spacing. Real, but cheap; cluster them so the report does not drown in nits.

Severity is a property of the rule, not of the file's overall impression. Grade each finding on its own row in this table, then stop. Do not downgrade a finding because the change is otherwise clean, small, or correct — that anchoring ("the file reads well, so this can only be a nit") is exactly how a real medium gets buried. A clean file with one test-structure interleaving has a medium finding, not a nit. Concretely: over-complex, style-mix, needless-cast are high by default; ordering, test-structure (interleaving or the late-extraction variant), barrel are medium; only openness/spacing is a nit. The single lever that legitimately moves severity down is the rule's own calibration paragraph turning a candidate into a non-finding — once something is a finding, its severity comes from this table, full stop.

When two rules touch the same code, the most specific finding wins. When you are genuinely unsure whether something is a problem, leave it out — a noisy review that flags judgment calls trains people to ignore it. Each rule below ends with the look-alike that is not a violation; check it before you emit.

openness — separate logical blocks with a blank line

Clean Code: "Each blank line is a visual cue that identifies a new and separate concept." Code with no vertical openness reads as one undifferentiated wall; the reader has to re-derive the block boundaries the author already knew. Add a blank line between concepts so the eye can group.

• Flag when distinct concepts are jammed together with no separation:
  • several small functions stacked with no blank line between them;
  • an if/for/while block immediately followed by the next statement with no blank line after the closing brace;
  • a run of statements that is really three phases (fetch → transform → return) with nothing separating them;
  • a chain of array/object operations or a sequence of related calls packed against unrelated code above and below.
• Suggested fix: insert blank lines at the concept boundaries — typically before a return, between setup and the work, around loops and conditionals, and between adjacent function declarations.
• Calibration → not a finding: Vertical density is the opposite and equally valid — lines that form one tight thought should stay packed (a guard clause and its return, a two-line variable-then-use pair, a small cohesive object literal). Do not demand blank lines inside a cohesive block, and do not ask for openness in trivially short functions. The goal is grouping, not double- spacing everything.

test-structure — group and order arrange / act / assert

A test communicates by its shape. When the arrange, act, and assert phases (given / when / then) are visually separated and in order, the reader sees the scenario at a glance. When they interleave, they have to mentally re-sort the test before they can trust it.

• Flag when:
  • the assert phase is interleaved with act — e.g. act, assert, act again, assert again, where it should be arrange → act → all asserts (unless the test is a genuine multi-step progression; see calibration);
  • a variable that extracts a value from a mock or a result is declared right before the third assertion that finally uses it, instead of with the rest of the arrange/extraction up top. Late declaration breaks the "all the setup is here, all the checks are there" reading.
  • the three phases run together with no blank line between them (this overlaps openness, but in tests the AAA grouping is the reason for the blank line).
• Suggested fix: hoist all mock/extraction variables into the arrange block, separate the three phases with a blank line, and put the asserts together at the end. A short // given / when / then or // arrange / act / assert label is welcome here (it is test vocabulary, not narration).
• Calibration → not a finding: some tests are legitimately a progression — drive a state machine through steps, asserting after each. There the act/assert alternation is the scenario, not a smell; keep it, but still group within each step. Also: Clean Code's "declare variables near their use" is a real principle, but in tests the AAA grouping wins because it communicates intent — that is the deliberate trade-off, not a contradiction to point out.

ordering — stepdown: read top-down like a newspaper

Clean Code's stepdown rule: "We want the code to read like a top-down narrative … every function followed by those at the next level of abstraction," and "a function that is called should be below a function that does the calling." The public entry point goes on top; the private helpers it calls follow underneath, in roughly the order they are called, each a level more detailed. The reader descends one level of abstraction at a time instead of scrolling up and down.

• Flag when a file reads bottom-up or scrambled: private helpers above the public function that uses them, or helpers in an order unrelated to the call flow, so the reader has to jump around to follow the story.
• Suggested fix: name the target order — entry point first, then helper A (called first), helper B (called next), etc. Because reordering is riskier than spacing, describe the move and let the user confirm rather than silently rewriting a large file.
• Calibration → not a finding: hoisting-dependent or convention-bound orders (a language/linter that wants exports first, alphabetized members, React hooks-then-handlers conventions); type/constant declarations that legitimately sit at the top; and any ordering the project documents (Step 0). Don't relocate something across a meaningful boundary just to satisfy the rule — judgment over mechanism.

style-mix — don't mix OOP and functional ad hoc

A module has a chosen style. When it switches styles for no reason, the switch itself becomes a thing the reader has to explain to themselves. The usual offenders:

• A non-exported free function inside an OOP module, doing a helper job for a class in the same file. Ask: why is this not a private method? It has the class's context, it is not exported, nothing tests it directly. → make it a private method. The one real reason to keep a helper separate is that it needs its own unit test — but then it should not be an un-exported function squatting in the class file either: extract it to the project's helpers/ or utils/ location (per the project's own semantics), export it there, and give it that test.

• A class appearing in functional code with no strong reason — no state to carry, no lifecycle, no interface to implement. If the class is deliberate (it holds state, it is a real abstraction), the code is missing the one comment that says why; if it is not deliberate, it should be a function.

• A function sharing a file with an unrelated class and exported alongside it → move the function to its own file.

• A grab-bag file that exports several functions with nothing in common → split it by responsibility.

• Suggested fix: state the specific move (inline as private method / extract + test / split file / add the rationale comment), and why — keeping one style per module is what lets a reader predict where things live.

• Calibration → not a finding: a small, stateless, file-local pure helper at the bottom of an OOP file can be perfectly fine — not every function near a class is a misplaced method. Factory functions that return class instances, React function components, and idiomatic functional cores with a thin class adapter at the edge are normal, not violations. Flag the unjustified switch, not every mixed file.

barrel — pointless barrel exports

A barrel (index.ts / index.js that only re-exports its siblings) adds a layer of indirection. Sometimes that layer earns its keep — it defines a package's public surface. Often it is cargo-culted: it re-exports everything, hides nothing, and just means every reader follows one more hop to find the real file.

• Flag a barrel that re-exports without narrowing or shaping a public API, when the project does not document barrels as its convention and no comment explains the decision.
• Suggested fix: import from the real modules and drop the barrel — or, if it is meant to be a package entry point, say so in a one-line comment so the next person knows it is load-bearing.
• Calibration → not a finding: a genuine package entry point (the file package.json's main/exports/types points at), a barrel that deliberately narrows a large internal surface to a small external one, or a barrel the project's conventions mandate (Step 0). The test is whether the indirection does something.

needless-cast — unnecessary type casts

A cast (as X, <X>, x!, an explicit assertion) says "trust me, the compiler is wrong." Each one is a place the type system stopped helping. Many casts in generated or AI-written code are simply unnecessary — the value already has the type — and a stale one actively hides a bug.

• Flag a cast where the value's static type already satisfies the target:
  • in tests, casting a mock/factory result that already returns the right type (createUser() as User when createUser returns User);
  • a cast that was needed only because some generated types were stale when the code was written, but are regenerated now — re-check against the current types before deciding.
• Verify before flagging: this is a static-type claim. If type diagnostics or an LSP are available, use them to confirm the cast is redundant. If you cannot verify, mark the finding (verify) rather than asserting it — a wrong "remove this cast" can break compilation.
• Suggested fix: drop the cast; if a narrowing is genuinely needed, prefer a type guard or fixing the source type over an assertion.
• Calibration → not a finding: as const; narrowing unknown/any at a real boundary (JSON.parse, an external API, document.querySelector); a deliberate as unknown as T test double where structural typing truly cannot be satisfied otherwise; casts that silence a correct compiler complaint about a real type gap. Casts at genuine boundaries are the type system working as intended.

over-complex — code that collapses into something smaller (priority)

This is the most valuable finding, so spend the most effort here. Strong models tend to expand: they write function A and function B that do almost the same thing, differing by a line or two, when one parameterized function would do. They write five lines where two read better. Hunt for the smaller version.

• Flag when:
  • two (or more) functions are near-identical and differ only by a value or a single branch — they collapse into one function that takes that difference as an argument;
  • a block is copy-pasted with small edits — extract the shared shape;
  • code is simply longer or more nested than the idea needs (a chain of if/else that is a lookup table, a manual loop that is a map/filter, an intermediate variable used once with no clarifying value).
• Suggested fix: show the unified version concretely — the merged signature with its new parameter and the conditional that absorbs the difference, or the shorter expression. Treat this as a real defect, not a nicety: duplication that drifts out of sync is a future bug.
• Calibration → the flag-argument trade-off: Clean Code also warns that a boolean/flag argument is a smell — it usually means the function does two things. So weigh it: unify when the two bodies are the same shape and the difference is genuinely data (a threshold, a key, a label). Keep them separate when collapsing would force a flag that makes one function secretly do two jobs, or when the two are conceptually distinct and likely to diverge — premature DRY that couples unrelated things is its own defect. The goal is the simplest code that still reads clearly, not the fewest functions at any cost.

Step 3 — Report (one fixed skeleton, every time)

The author should see file → place(s) → rule → fix at a glance. Two reviews should also look the same — a skeleton that shifts between runs is its own kind of noise. So render the report with exactly this template, in this order, and do not improvise the structure:

## Quality review — <scope>

**Conventions:** <one line on what you picked up in Step 0, or "none that change the verdict">
**Headline:** <one line — the single best or worst thing about the change>

### <path/to/file>
- `name` severity · L<lines> — <what the reader loses> → <the fix, as a clause>
- `name` severity · L<lines> — <…>

### <path/to/another/file>
- `name` severity · L<lines> — <…>

**Not flagged:** <one compact line of look-alikes you deliberately passed on, or omit the line>

**Boy-scout (untouched code, optional):**
- `name` · <path>:L<lines> — <one line>

**Tally:** N findings · H high · M medium · K nit · F files · B boy-scout. Skipped: <files + reason>.

Rules for filling it in:

• name is one of the seven fixed rule names from Step 2 (openness, test-structure, ordering, style-mix, barrel, needless-cast, over-complex) — backticked, never a number, never a paraphrase. severity is high / medium / nit. Findings are markdown bullets under a ### file header (not inside a ``` fence) so every path:line stays clickable.
• Order files by their highest-severity finding; within a file, high → medium → nit, then by line. Collapse repeats: one rule breaking in several spots is a single bullet with the lines listed together (L20, L34, L51).
• The fix is a clause, not code. "extract transitionOrReportConflict(audit, status, from) and early-return at each site", "drop the as User cast", "blank line at each boundary". Name a symbol or the move; do not paste the rewritten body, the merged function, or a before/after block into the report — that is the wall-of-text this format exists to kill. The full refactor belongs in Step 4 (apply time) or when the user asks to see it. If a fix genuinely cannot be named without a few tokens of code, inline at most a short expression, never a multi-line block.
• Not flagged is one line total — a comma-separated list of the look-alikes you considered and passed on, not a paragraph per item. If there's nothing worth noting, drop the line entirely.
• Boy-scout holds only findings in code the change did not touch; omit the whole block when there are none.
• Keep Conventions and Headline to one line each. Resist growing either into a summary essay.
• The headline may not contradict the tally. If there is any high or medium finding, the headline names the worst one — it must not call the change "clean", "well-structured", or "only cosmetic nits". Reserve the clean verdict for a tally that is genuinely nits-only (or empty). The reader should never see "clean change" sitting above a medium finding.

Collapse the whole report to the title line plus a one-sentence verdict and the tally only when the change reads cleanly — i.e. the tally is empty or nits-only. Do not pad a clean report to look thorough; do not collapse one that has a medium-or-higher finding to look clean.

Step 4 — Follow up with the user (AskUserQuestion)

Never edit during the review. After the report, use the AskUserQuestion tool to ask how to proceed — don't trail off with an open-ended "want me to apply these?". A concrete menu gets a faster, cleaner decision. Offer the choices that actually apply to this review, for example:

• Apply the safe fixes — openness blank lines, verified-redundant needless-cast, trivial over-complex simplifications. Local, mechanical, easy to eyeball.
• Walk the structural ones — ordering reordering, style-mix extract/move/split, large over-complex unifications, test-structure restructuring — one at a time, since they move code across boundaries and are riskier.
• Include the boy-scout extras, or skip them and touch only the changed code.
• Report only — change nothing.

Make the options match the findings you actually have (don't offer "walk the structural ones" if there are none). Then apply with Edit only what the user chose; auto-apply nothing structural without an explicit yes. After any structural change, re-run the project's build/tests if it has them — reordering and unification can break things a blank line cannot.