Metadata

Skill ID

4eeb5eac-8f1a-478e-8a39-2e50b588be78

Version

1

Owner

387274b7-2891-478b-81b8-e11d5adb9319

Tags

code-reviewseverityblockingnitpicktriagefeedback

Signature

verified

Integrity

OK

Content hash

b02da1cfaa688b2e782d8347d86a558eb861a7b416ad6d5a98b98b3b2faedb2b

Created

2026-08-15T05:27:15Z

Skill file

# Review Comment Severity Triage

Use when writing or responding to review feedback. Classifying every comment by
severity prevents review fatigue (everything is blocking) and review negligence
(everything is a nit, bugs slip through).

## 1. The severity rubric — three tiers, no more

| Severity | Prefix | Definition | Example |
|---|---|---|---|
| **Blocking** | `[blocking]` | A bug, security issue, data-loss risk, broken API contract, or incorrect behavior. **Must** be fixed before merge. | "The token comparison uses `==` instead of constant-time compare — timing attack." |
| **Should-fix** | `[should-fix]` | Maintainability, consistency, readability, or future-risk issue. The code works but will cause problems later. Not a merge blocker. | "Duplicate logic between these two functions — extract into a shared helper before the next change." |
| **Nit** | `[nit]` | Style preference, optional improvement, "I'd write it differently." The code is correct. Author can take it or leave it. | "Consider a list comprehension here instead of the loop — more idiomatic but not required." |

Every comment must carry one of these three prefixes. If you cannot decide which
one, it is a `[nit]`.

## 2. Decision flowchart

```
Is this a bug, security risk, or incorrect behavior?
  YES → [blocking]
  NO  → Will this cause real pain (maintenance, consistency, risk) if left?
           YES → [should-fix]
           NO  → [nit]
```

If you are unsure, default **one tier down** from your instinct. Over-classifying
nits as blocking is review fatigue for the author; under-classifying blocks is a
bug in production. When truly uncertain, ask: "would I be upset if this shipped
as-is and I had to fix it later?" If yes, blocking. If "mildly annoyed," should-fix.
If "I'd barely notice," nit.

## 3. Writing feedback at the right level

```text
# GOOD — severity clear, problem stated, fix suggested
[blocking] src/payments.py:89
The charge amount is passed as a float. Floating-point arithmetic with currency
will cause rounding errors.
Suggestion: use `int` cents throughout; convert to decimal only for display.

# BAD — severity missing, vague, no suggestion
This looks wrong. Can you fix it?
```

For `[should-fix]` and `[nit]`, the author can respond with a single-line
acknowledgment or a quick fix — no rounds of debate.

## 4. Responding to feedback as the author

| Severity | Your response |
|---|---|
| `[blocking]` | Fix it. Do not argue unless the reviewer is factually wrong. |
| `[should-fix]` | Fix it or record a follow-up issue. Do not ignore. |
| `[nit]` | Batch nits and address them in one pass. Do not argue style. |

Never respond to a `[nit]` with a paragraph-long justification. "Done" or
"Will address in a follow-up" is enough.

## 5. Reviewer hygiene

- If a PR has more than 3 `[blocking]` comments, pause and consider: is the PR
  too large, or is the author out of their depth? A PR with many blockers should
  not be in review.
- If you find yourself writing 10+ `[nit]` comments, pick the top 3 and drop the
  rest. Nit-spam erodes trust.
- Never use `[blocking]` for "I prefer different naming." Style is a nit.

## 5. Worked example: triaging 10 comments on a PR

Imagine a PR review with these findings. Triaging them:

| Finding | Severity | Why |
|---|---|---|
| Missing validation on `amount` field — negative values pass through | `[blocking]` | Data integrity bug |
| Password compared with `==` instead of constant-time compare | `[blocking]` | Security issue |
| Duplicate logic between `create_order()` and `update_order()` | `[should-fix]` | Maintainability, not a bug |
| A `try/except: pass` swallowing a 500 error | `[blocking]` | Bug — silent failure |
| Variable `d` instead of `discount_amount` | `[nit]` | Style, no bug |
| Commented-out code block left in | `[should-fix]` | Clutter, confusing to next reader |
| `print()` debug statement left in production path | `[should-fix]` | Not incorrect but pollutes output |
| Could use list comprehension instead of for-loop | `[nit]` | Preference, correct as-is |
| Missing docstring on public function | `[should-fix]` | Contract is undocumented |
| "I'd rename this class to X" | `[nit]` | Naming preference |

Result: 3 blocking, 4 should-fix, 3 nits. The blockings must be fixed; the
should-fixes should be addressed; the nits the author can take or leave.

## 6. When to escalate a nit to blocking

Two cases where a nit becomes a blocker:

1. **Pattern, not an instance** — "this variable name is bad" is a nit, but "every
   variable in this module uses single-letter names" is a `[should-fix]`; if the
   names cause actual comprehension errors, it is `[blocking]`.
2. **Consistency with project norms** — a naming choice that violates AGENTS.md or
   the project's explicit conventions is `[should-fix]`, not a nit.

Style is a nit; consistency is a should-fix; correctness is blocking.

## Guardrails

- Do **not** mark everything blocking — that is review fatigue and trains authors
  to ignore your feedback.
- Do **not** mark everything a nit — that lets real bugs through.
- Do **not** argue about `[nit]` severity. If the reviewer called it a nit, it is
  a nit.
- Do use `[should-fix]` for real maintainability issues that are not merge-
  blockers; they must be recorded, not ignored.
- Do prefix every comment with exactly one of `[blocking]`, `[should-fix]`, `[nit]`.

## Pitfalls

- **Everything-is-blocking** — review fatigue; authors start resenting and ignoring
  reviews.
- **Everything-is-a-nit** — real bugs are called nits and merge into production.
- **Arguing over severity** — "that's not blocking, it's a nit" wastes more time
  than fixing the issue.
- **No prefix at all** — the author has to guess what is required vs optional.
- **Nit-spam** — 15 nit comments on a 50-line PR; pick the top 3.

## Verify / Checklist

- [ ] Every comment carries exactly one of `[blocking]`, `[should-fix]`, or `[nit]`.
- [ ] Every `[blocking]` is a genuine bug, security issue, or incorrect behavior.
- [ ] No `[blocking]` is about style or preference.
- [ ] The review ends with an explicit verdict (approve / request changes / comment).
- [ ] Nits were batched, not dribbled out one by one.
- [ ] The author's response matches the severity tier (fix blockers, batch nits).

Attached files

No attached files.