Respond to Code Review

Review feedback is input to evaluate, not orders to obey or an attack to deflect. The goal is the right code, so you verify each point against the codebase, fix what is correct, explain with evidence what is not, and keep the conversation short and specific. No performative agreement, no silent partial fixes, no force-pushing over a reviewer's context.

When to use

• A pull request you opened has review comments or "changes requested".
• The owner or a teammate says "this isn't right", "why did you do it this way?", or lists fixes.
• A reviewer subagent returned findings on your work.

What you need

• All the feedback in one place: inline comments, the review summary, and any discussion. Fetch it with the GitHub or GitLab connected app, gh pr view <n> --comments and gh api repos/OWNER/REPO/pulls/<n>/comments, or the browser.
• The branch checked out, up to date with its remote (git pull --ff-only).
• The project's checks (tests, lint, types) so you can verify each fix.

Method

1. Read all of it before changing anything. Items are often related: the fix for comment 4 may change the answer to comment 2. List every comment in a todo list, one item each, with its link or path:line.

2. Restate each item in your own words. "Reviewer wants the refund amount capped at the captured total." If you cannot restate it, it is unclear. Ask about all unclear items first, in one message, before implementing any of them: "I understand 1, 2, 3 and 6. For 4, do you mean X or Y? For 5, which caller did you have in mind?"

3. Check each item against the code. For every suggestion:

   • Is it correct for this codebase? read_file the code it refers to; search_files for the callers or uses it mentions.
   • Would it break something? Other callers, platforms, older clients, a test that encodes a deliberate behaviour.
   • Is there a reason for the current code? git log -L <start>,<end>:<file> or git blame shows why it was written that way.
   • Does it conflict with something the owner decided? If so, the owner decides, not you or the reviewer.
   • Is it in scope? A request to also rebuild a neighbouring module is a follow-up, not part of this change.
   • Is the "proper" version needed at all? If the reviewer asks to build something out fully, search whether anything uses it; unused code is a case for removal, not expansion.

4. Sort the items.

   Type	Action
   Valid blocker (bug, security, data)	Fix first, with a test
   Valid, simple (naming, missing check, typo)	Fix
   Valid, larger (restructure)	Fix if in scope; otherwise propose a follow-up issue and say so
   Unclear	Ask (step 2)
   Incorrect for this codebase	Push back with evidence
   Out of scope or a matter of taste	Say so politely, offer a follow-up, or accept if it is cheap and harmless

5. Fix one item at a time. For each: make the change (read the file first, patch, check the returned diff), add or adjust a test when behaviour changes, run the relevant tests, then commit. Prefer small commits that map to comments. If the repo squashes on merge, plain follow-up commits are fine. If it expects a tidy history, use fixup commits and squash them before merge, not during active review:

   git commit --fixup <sha-of-original-commit>
   # later, when the reviewer is done and the owner agrees:
   GIT_SEQUENCE_EDITOR=: git rebase -i --autosquash origin/main
   

6. Push back well. When a suggestion is wrong, explain with facts the reviewer can check: the caller it would break, the test that encodes the behaviour, the platform constraint, the benchmark result, the earlier decision. Offer an alternative if there is one. Example: "Checked this: legacy_export is still called by the nightly job in jobs/export.py:44, so removing it would break the export. I can add a deprecation warning and an issue to remove it after the job moves to v2. Does that work?" If you turn out to be wrong, say so plainly and fix it: "You're right, I checked X and it does Y. Fixed in abc123."

7. Reply in each thread. Reply to the inline comment where it was made, not as one top-level wall:

   gh api repos/OWNER/REPO/pulls/<n>/comments/<comment_id>/replies -f body="Fixed in abc1234: capped at captured minus refunded, test added."
   

   Keep replies to what changed and where, or the reason you did not change it. No thanks-padding or "great point!": the fix shows you listened. Resolve threads only if the project lets authors resolve their own; otherwise leave that to the reviewer.

8. Verify before asking for another look. Run the full test suite, linter and type checker; push; wait for CI to go green (gh pr checks <n> --watch). Then re-request review (gh pr edit <n> --add-reviewer <login> or the button in the browser) with a short summary: what was fixed, what was answered, what was deferred to which issue.

9. Never rewrite history under an active review without agreement. Force-pushing a rebased branch hides what changed since the last review and can drop a reviewer's suggested commits. If you must (the owner asks for a rebase), use git push --force-with-lease and tell the reviewer what moved.

Output

Updated branch with CI green, a reply in every thread, and one summary comment:

Ready for another look. Changes since your last review (abc123..def456):
- Fixed: refund cap (#c1), provider error handling (#c2), test name (#c4)
- Answered: #c3 (kept `legacy_export`, still used by the nightly job; follow-up #812)
- Need your input: #c5 (two options in the thread)
Checks: 418 passed, lint and types clean, CI green.

Checks before you finish

• Every comment has a fix, a reasoned reply, or an open question; none is silently ignored.
• Every fix was verified with the relevant tests, and the full suite ran after the last change.
• Pushback is backed by something the reviewer can check.
• History was not rewritten under review without agreement.
• CI is green on the head commit before you re-request review.

Pitfalls

• Implementing the clear half and ignoring the unclear half. Ask first; related items change each other.
• Agreeing to everything. A reviewer without full context can be wrong. Verify, then decide.
• Arguing from feelings. "I think it's fine" is not a reason. Show the caller, test or measurement.
• Batching fixes untested. One fix at a time keeps a regression traceable to one change.
• One giant "address review" commit. It forces the reviewer to re-read everything.
• Scope creep from review. Big new asks become follow-up issues unless the owner wants them now.
• Re-requesting review with red CI. It wastes the reviewer's time and signals carelessness.