Code Review Loop
Overview
End-to-end code review etiquette. Covers the requesting side (preparing a PR
that's reviewable) and the receiving side (acting on feedback). The skill exists
because most code review failures aren't disagreement — they're noise. Reviewers
get PRs they can't reasonably review (1500 lines, mixed concerns, no
description) and authors get feedback they don't engage with seriously
(silent dismissals, "fixed" without explanation, defensive replies). The skill
enforces structure on both ends and dispatches claudekit:code-reviewer /
claudekit:security-auditor agents on the diff. Used after verification-gate,
before merge.
When to Use
- Opening a PR for review
- Responding to review comments on a PR you authored
- Reviewing a PR another engineer authored (the skill applies symmetrically)
- Re-requesting review after addressing feedback
When NOT to Use
- Quick fixes via direct push to a branch nobody else uses (no review needed)
- A PR is already merged and you have post-merge feedback (file a follow-up
issue, don't re-litigate)
- Reviewing infra/config that the project's policy explicitly auto-approves
Process
Step 1: Prepare the PR (requesting side)
Goal: A reviewable PR.
Inputs: A branch with verified changes (you've run verification-gate).
Actions:
- The PR title is one line, describing what changed. Not "Updates" or "Fix
stuff." Verb-led: "Add idempotency key to charge endpoint."
- The PR description has these sections:
- What: 1-3 sentences naming the change.
- Why: the spec link, the ticket, the bug being fixed.
- How: the design choice, especially if non-obvious. Cite the plan if
one exists.
- Verification: the output from
verification-gate (paste or link).
- Risk + rollback: if the change has any risk, name it and the rollback
procedure.
- Check the diff size. If >400 lines (excluding tests, generated files,
lockfiles), consider splitting the PR. Reviewers won't read the whole thing
carefully; they'll skim, miss issues, and approve.
- Tag the right reviewers. For sensitive paths (auth, payments, data), tag
the security-savvy reviewer too.
Output: A PR open for review with the description filled out.
Step 2: Dispatch the reviewer agents
Goal: A first pass before human reviewers spend their time.
Inputs: The open PR.
Actions:
- Dispatch
claudekit:code-reviewer on the diff. Returns: structural findings
(data flow, error handling, edge cases), style findings, complexity findings.
- If the diff touches
auth/, payments/, crypto/, users/, sessions/,
tokens/, or any path with sensitive-data semantics, also dispatch
claudekit:security-auditor. Returns: input-validation findings, OWASP-aligned
findings, secret-handling findings.
- Read both findings lists. Address obvious issues (typos, missing error
handling, easily-fixed structural notes) yourself before human reviewers see
the PR.
- Push the changes. Note in the PR description that automated reviewer agents
ran, plus any findings you intentionally deferred.
Output: A PR that has been pre-reviewed by agents; obvious findings already
addressed.
Step 3: Receive feedback (receiving side)
Goal: Engage with every comment.
Inputs: Reviewer comments on the PR.
Actions:
- Read every comment before responding to any. Get the full picture; don't
start replying piecemeal.
- For each comment, choose one of three responses:
- Agree + apply: make the change. Reply with the commit hash that
applied it. Don't reply "fixed" without the hash.
- Disagree + explain: explain why you disagree. Cite evidence (a test, a
constraint, a decision in the spec). Ask the reviewer if your reasoning
resolves their concern.
- Need more context: ask the reviewer for clarification. Don't guess at
what they meant.
- Never silently dismiss a comment. If you didn't apply it and didn't reply,
the reviewer assumes you missed it.
Output: Every comment has a response thread.
Step 4: Apply changes in coherent commits
Goal: Make the diff history easy to re-review.
Inputs: The agreed changes from Step 3.
Actions:
- Group changes by topic. One commit per topic, even if multiple comments
contributed to it.
- Each commit message names what changed and references the comment thread
("Address review: extract validation to ; thread #N").
- Don't squash before re-review unless the project's policy demands it.
Reviewers want to see what changed since their last pass.
Output: New commits on the branch addressing the agreed feedback.
Step 5: Re-request review
Goal: Hand back to the reviewer with a clear next step.
Inputs: The branch with applied changes.
Actions:
- Add a single comment on the PR summarizing what you addressed and what you
pushed back on:
- "Addressed: comments #1, #3, #5 (commits a1b2c, c3d4e)"
- "Pushed back: comments #2, #4 — see threads"
- Re-request review through the platform's mechanism (re-assign, request
re-review, etc.).
- Don't ping by Slack/IM unless the PR is blocking and reviewers are unaware.
Output: Reviewers re-engaged with a summary of what changed.
Step 6: Close the loop
Goal: Merge cleanly.
Inputs: Approval from required reviewers.
Actions:
- Confirm CI is green at the most recent commit (not the branch tip from when
review was requested).
- Resolve all comment threads. If a thread has unresolved disagreement, the PR
shouldn't merge yet — escalate or compromise.
- Merge using the project's standard method (squash, merge commit, rebase).
- If the PR introduced anything not yet rolled out (feature flag off, config
not flipped), the PR is merged but not delivered — track delivery
separately.
Output: PR merged. Any pending delivery steps tracked.
Rationalizations
| Excuse |
Why it sounds reasonable |
Why it's wrong |
What to do instead |
| "I'll write a quick PR description and the reviewer can read the diff for context." |
The diff is the source of truth; the description is metadata. |
The diff shows what changed, not why. A reviewer reading the diff cold has to reconstruct the intent, the constraints, the alternatives considered. They will reconstruct partially, miss something, ask questions you've already answered in the spec, and slow the review by hours. |
Write the description. The What/Why/How structure is short — 4-8 sentences — and saves the reviewer reconstruction time. The PR description is a contract: this is what I'm asking you to look for. |
| "The PR is large but the changes are mechanical — easy to review." |
Mechanical changes are real. A rename across 800 lines is genuinely simple. |
"Mechanical" is the line said before someone discovers a non-mechanical change buried in the mass: a slightly different signature, an off-by-one, a behavior tweak the rename quietly altered. Reviewers don't read 800-line "mechanical" PRs line-by-line; they spot-check and approve. The buried bug ships. |
Split the PR. Mechanical-only commit goes first, behavior changes (if any) go in a separate small PR after. If the PR is genuinely 100% mechanical, you can call that out explicitly and the reviewer can approve confidently — but don't ask them to take "mechanical" on faith. |
| "I'll reply 'fixed' to the comments — the reviewer can see the new commits." |
The reviewer can navigate the PR; making them re-derive the linkage feels like courtesy theater. |
The reviewer is reviewing many PRs that day; they don't remember which comment maps to which commit, and the PR UI doesn't always make it obvious. "Fixed" without a hash forces them to scan the diff hunting for your change, find it, verify it, and then react. The hash saves the search. |
Reply with the commit hash: "Fixed in a1b2c3d." Or, if it was multi-commit: "Fixed in a1b2c3d (extracted) and c3d4e5f (renamed param)." 10 seconds for you, 90 seconds saved per comment for the reviewer. |
| "The reviewer's comment is wrong — I'll just leave it and merge." |
Sometimes reviewers really are wrong. Defending against bad feedback is a real skill. |
Silently dismissing the comment doesn't tell the reviewer they're wrong; it tells them they were ignored. Next PR, they'll either escalate the same comment more aggressively or stop reviewing your PRs carefully. The disagreement is the data; suppressing it loses the data and the relationship. |
If you disagree, reply with your reasoning. Cite evidence. Ask if your reasoning resolves their concern. They may have context you don't, or vice versa — the comment thread is where that gets surfaced. |
| "Security review is overkill for this — the file is just a refactor." |
Refactors really don't usually change security posture. |
"Just a refactor" can move a sensitive call across a boundary, change which path a request takes, alter the ordering of validation and side effects. The security-auditor agent is automated and cheap; running it on a refactor that touches sensitive paths takes 30 seconds and catches the cases where "just a refactor" wasn't. |
If the diff touches any sensitive path (auth, payments, crypto, users, sessions, tokens), dispatch the security-auditor regardless of how mechanical the change feels. The cost is automated; the risk is asymmetric. |
| "CI ran when I opened the PR — that's still the source of truth." |
CI results don't usually change between open and merge. |
The branch typically has new commits between PR-open and merge (review feedback, conflict resolution, the dependency upgrade that sneaked into main). The CI run from PR-open is testing a state that no longer exists. Merging on stale green is how flaky-vs-broken slips through. |
Confirm CI is green on the current commit before merging. Most platforms show this; if yours doesn't, push a no-op or re-run CI to confirm. |
Evidence Requirements
| Checkpoint |
Required artifact |
What "no evidence" looks like |
| End of Step 1 |
PR description with What/Why/How/Verification/Risk sections; diff <400 lines (or split rationale) |
An empty PR description; "see ticket." |
| End of Step 2 |
Reviewer agent findings addressed or noted as deferred |
"I think the code is fine; let humans look." |
| End of Step 3 |
Every comment has a response |
Some comments left unanswered. |
| End of Step 4 |
New commits each named with topic + comment-thread reference |
One huge "address review" commit. |
| End of Step 5 |
A summary comment listing what was addressed and what was pushed back on |
Re-request without summary. |
| End of Step 6 |
CI green on the most recent commit; all threads resolved |
"Merged it; CI was green earlier." |
Red Flags
- The PR description is one sentence. The reviewer is reconstructing your work
from the diff alone.
- The diff exceeds 400 non-trivial lines and isn't split. Reviewers will skim.
- A comment thread has more than 5 back-and-forth replies. The disagreement
needs to be moved to a synchronous conversation.
- Multiple comments left without any reply from the author. The PR was abandoned
mid-review.
- The PR was merged with unresolved comment threads. The disagreement is now
hidden in the history.
- The PR has 20 commits each titled "fix review." Squash before merge or commit
with topical messages.
- The "Verification" section is missing. The PR jumped from work to merge
without the gate.
References
- Software Engineering at Google, Wright et al. (O'Reilly, 2020), Chapter 9
"Code Review" — "Small CL" principle and the case that review effectiveness
inversely correlates with diff size. Step 1's 400-line guideline derives
from Google's internal observations on review-found-defects vs CL size.
1---2name: code-review-loop3description: Use when opening a PR for review or when receiving review feedback. Activate for keywords like "code review", "PR review", "request review", "review feedback", "address comments", "reviewer said". Covers both ends of the loop: preparing a reviewable PR and acting on feedback rigorously. Always engage with every comment -- never dismiss feedback by silently ignoring it.4---56# Code Review Loop78## Overview910End-to-end code review etiquette. Covers the requesting side (preparing a PR11that's reviewable) and the receiving side (acting on feedback). The skill exists12because most code review failures aren't disagreement — they're noise. Reviewers13get PRs they can't reasonably review (1500 lines, mixed concerns, no14description) and authors get feedback they don't engage with seriously15(silent dismissals, "fixed" without explanation, defensive replies). The skill16enforces structure on both ends and dispatches `claudekit:code-reviewer` /17`claudekit:security-auditor` agents on the diff. Used after `verification-gate`,18before merge.1920## When to Use2122- Opening a PR for review23- Responding to review comments on a PR you authored24- Reviewing a PR another engineer authored (the skill applies symmetrically)25- Re-requesting review after addressing feedback2627## When NOT to Use2829- Quick fixes via direct push to a branch nobody else uses (no review needed)30- A PR is already merged and you have post-merge feedback (file a follow-up31 issue, don't re-litigate)32- Reviewing infra/config that the project's policy explicitly auto-approves3334## Process3536### Step 1: Prepare the PR (requesting side)3738**Goal:** A reviewable PR.3940**Inputs:** A branch with verified changes (you've run `verification-gate`).4142**Actions:**43441. The PR title is one line, describing what changed. Not "Updates" or "Fix45 stuff." Verb-led: "Add idempotency key to charge endpoint."462. The PR description has these sections:47 - **What:** 1-3 sentences naming the change.48 - **Why:** the spec link, the ticket, the bug being fixed.49 - **How:** the design choice, especially if non-obvious. Cite the plan if50 one exists.51 - **Verification:** the output from `verification-gate` (paste or link).52 - **Risk + rollback:** if the change has any risk, name it and the rollback53 procedure.543. Check the diff size. If >400 lines (excluding tests, generated files,55 lockfiles), consider splitting the PR. Reviewers won't read the whole thing56 carefully; they'll skim, miss issues, and approve.574. Tag the right reviewers. For sensitive paths (auth, payments, data), tag58 the security-savvy reviewer too.5960**Output:** A PR open for review with the description filled out.6162### Step 2: Dispatch the reviewer agents6364**Goal:** A first pass before human reviewers spend their time.6566**Inputs:** The open PR.6768**Actions:**69701. Dispatch `claudekit:code-reviewer` on the diff. Returns: structural findings71 (data flow, error handling, edge cases), style findings, complexity findings.722. If the diff touches `auth/`, `payments/`, `crypto/`, `users/`, `sessions/`,73 `tokens/`, or any path with sensitive-data semantics, also dispatch74 `claudekit:security-auditor`. Returns: input-validation findings, OWASP-aligned75 findings, secret-handling findings.763. Read both findings lists. Address obvious issues (typos, missing error77 handling, easily-fixed structural notes) yourself before human reviewers see78 the PR.794. Push the changes. Note in the PR description that automated reviewer agents80 ran, plus any findings you intentionally deferred.8182**Output:** A PR that has been pre-reviewed by agents; obvious findings already83addressed.8485### Step 3: Receive feedback (receiving side)8687**Goal:** Engage with every comment.8889**Inputs:** Reviewer comments on the PR.9091**Actions:**92931. Read every comment before responding to any. Get the full picture; don't94 start replying piecemeal.952. For each comment, choose one of three responses:96 - **Agree + apply:** make the change. Reply with the commit hash that97 applied it. Don't reply "fixed" without the hash.98 - **Disagree + explain:** explain why you disagree. Cite evidence (a test, a99 constraint, a decision in the spec). Ask the reviewer if your reasoning100 resolves their concern.101 - **Need more context:** ask the reviewer for clarification. Don't guess at102 what they meant.1033. Never silently dismiss a comment. If you didn't apply it and didn't reply,104 the reviewer assumes you missed it.105106**Output:** Every comment has a response thread.107108### Step 4: Apply changes in coherent commits109110**Goal:** Make the diff history easy to re-review.111112**Inputs:** The agreed changes from Step 3.113114**Actions:**1151161. Group changes by topic. One commit per topic, even if multiple comments117 contributed to it.1182. Each commit message names what changed and references the comment thread119 ("Address review: extract validation to <module>; thread #N").1203. Don't squash before re-review unless the project's policy demands it.121 Reviewers want to see what changed since their last pass.122123**Output:** New commits on the branch addressing the agreed feedback.124125### Step 5: Re-request review126127**Goal:** Hand back to the reviewer with a clear next step.128129**Inputs:** The branch with applied changes.130131**Actions:**1321331. Add a single comment on the PR summarizing what you addressed and what you134 pushed back on:135 - "Addressed: comments #1, #3, #5 (commits a1b2c, c3d4e)"136 - "Pushed back: comments #2, #4 — see threads"1372. Re-request review through the platform's mechanism (re-assign, request138 re-review, etc.).1393. Don't ping by Slack/IM unless the PR is blocking and reviewers are unaware.140141**Output:** Reviewers re-engaged with a summary of what changed.142143### Step 6: Close the loop144145**Goal:** Merge cleanly.146147**Inputs:** Approval from required reviewers.148149**Actions:**1501511. Confirm CI is green at the most recent commit (not the branch tip from when152 review was requested).1532. Resolve all comment threads. If a thread has unresolved disagreement, the PR154 shouldn't merge yet — escalate or compromise.1553. Merge using the project's standard method (squash, merge commit, rebase).1564. If the PR introduced anything not yet rolled out (feature flag off, config157 not flipped), the PR is *merged* but not *delivered* — track delivery158 separately.159160**Output:** PR merged. Any pending delivery steps tracked.161162## Rationalizations163164| Excuse | Why it sounds reasonable | Why it's wrong | What to do instead |165|---|---|---|---|166| "I'll write a quick PR description and the reviewer can read the diff for context." | The diff is the source of truth; the description is metadata. | The diff shows *what* changed, not *why*. A reviewer reading the diff cold has to reconstruct the intent, the constraints, the alternatives considered. They will reconstruct partially, miss something, ask questions you've already answered in the spec, and slow the review by hours. | Write the description. The What/Why/How structure is short — 4-8 sentences — and saves the reviewer reconstruction time. The PR description is a contract: this is what I'm asking you to look for. |167| "The PR is large but the changes are mechanical — easy to review." | Mechanical changes are real. A rename across 800 lines is genuinely simple. | "Mechanical" is the line said before someone discovers a non-mechanical change buried in the mass: a slightly different signature, an off-by-one, a behavior tweak the rename quietly altered. Reviewers don't read 800-line "mechanical" PRs line-by-line; they spot-check and approve. The buried bug ships. | Split the PR. Mechanical-only commit goes first, behavior changes (if any) go in a separate small PR after. If the PR is genuinely 100% mechanical, you can call that out explicitly and the reviewer can approve confidently — but don't ask them to take "mechanical" on faith. |168| "I'll reply 'fixed' to the comments — the reviewer can see the new commits." | The reviewer can navigate the PR; making them re-derive the linkage feels like courtesy theater. | The reviewer is reviewing many PRs that day; they don't remember which comment maps to which commit, and the PR UI doesn't always make it obvious. "Fixed" without a hash forces them to scan the diff hunting for your change, find it, verify it, and *then* react. The hash saves the search. | Reply with the commit hash: "Fixed in a1b2c3d." Or, if it was multi-commit: "Fixed in a1b2c3d (extracted) and c3d4e5f (renamed param)." 10 seconds for you, 90 seconds saved per comment for the reviewer. |169| "The reviewer's comment is wrong — I'll just leave it and merge." | Sometimes reviewers really are wrong. Defending against bad feedback is a real skill. | Silently dismissing the comment doesn't tell the reviewer they're wrong; it tells them they were ignored. Next PR, they'll either escalate the same comment more aggressively or stop reviewing your PRs carefully. The disagreement is the data; suppressing it loses the data and the relationship. | If you disagree, reply with your reasoning. Cite evidence. Ask if your reasoning resolves their concern. They may have context you don't, or vice versa — the comment thread is where that gets surfaced. |170| "Security review is overkill for this — the file is just a refactor." | Refactors really don't usually change security posture. | "Just a refactor" can move a sensitive call across a boundary, change which path a request takes, alter the ordering of validation and side effects. The security-auditor agent is automated and cheap; running it on a refactor that touches sensitive paths takes 30 seconds and catches the cases where "just a refactor" wasn't. | If the diff touches any sensitive path (auth, payments, crypto, users, sessions, tokens), dispatch the security-auditor regardless of how mechanical the change feels. The cost is automated; the risk is asymmetric. |171| "CI ran when I opened the PR — that's still the source of truth." | CI results don't usually change between open and merge. | The branch typically has new commits between PR-open and merge (review feedback, conflict resolution, the dependency upgrade that sneaked into main). The CI run from PR-open is testing a state that no longer exists. Merging on stale green is how flaky-vs-broken slips through. | Confirm CI is green on the *current* commit before merging. Most platforms show this; if yours doesn't, push a no-op or re-run CI to confirm. |172173## Evidence Requirements174175| Checkpoint | Required artifact | What "no evidence" looks like |176|---|---|---|177| End of Step 1 | PR description with What/Why/How/Verification/Risk sections; diff <400 lines (or split rationale) | An empty PR description; "see ticket." |178| End of Step 2 | Reviewer agent findings addressed or noted as deferred | "I think the code is fine; let humans look." |179| End of Step 3 | Every comment has a response | Some comments left unanswered. |180| End of Step 4 | New commits each named with topic + comment-thread reference | One huge "address review" commit. |181| End of Step 5 | A summary comment listing what was addressed and what was pushed back on | Re-request without summary. |182| End of Step 6 | CI green on the most recent commit; all threads resolved | "Merged it; CI was green earlier." |183184## Red Flags185186- The PR description is one sentence. The reviewer is reconstructing your work187 from the diff alone.188- The diff exceeds 400 non-trivial lines and isn't split. Reviewers will skim.189- A comment thread has more than 5 back-and-forth replies. The disagreement190 needs to be moved to a synchronous conversation.191- Multiple comments left without any reply from the author. The PR was abandoned192 mid-review.193- The PR was merged with unresolved comment threads. The disagreement is now194 hidden in the history.195- The PR has 20 commits each titled "fix review." Squash before merge or commit196 with topical messages.197- The "Verification" section is missing. The PR jumped from work to merge198 without the gate.199200## References201202- *Software Engineering at Google*, Wright et al. (O'Reilly, 2020), Chapter 9203 "Code Review" — "Small CL" principle and the case that review effectiveness204 inversely correlates with diff size. Step 1's 400-line guideline derives205 from Google's internal observations on review-found-defects vs CL size.
duthaho