ia-receiving-code-review

Receiving Code Review

Core Principle

Verify before implementing. Technical correctness matters more than social comfort. A reviewer can be wrong -- blindly implementing bad suggestions creates bugs.

Response Pattern

For each piece of feedback, follow this sequence:

0. Prior feedback check (re-reviews only) -- if this is not the first review round, check whether previously flagged issues were addressed before processing new comments. Compare the current diff against prior review threads (gh api repos/{owner}/{repo}/pulls/{pr}/comments). Surface any that were ignored or only partially fixed -- these take priority over new feedback.

1. Read -- Understand what's being suggested and why 2. Verify -- Is the suggestion technically correct for THIS codebase? 3. Evaluate -- Does it improve the code, or is it preference/style? 4. Respond -- Agree with evidence, disagree with evidence, or ask for clarification 5. Implement -- Only after verification confirms the suggestion is correct

Triage all feedback first (see Implementation Order below), then implement one item at a time. Don't batch-implement everything at once.

Handling Unclear Feedback

When feedback is ambiguous or incomplete:

Batched clarification for critical-path ambiguity: When multiple ambiguous findings land on critical-path code (auth, payments, data migrations, permission checks) AND the AskUserQuestion tool is available, batch up to 4 of them into a single call rather than asking one at a time. Each question's header is the truncated filename and line, and the options are Valid / False positive / Defer. Skip the batched ask entirely when ambiguous findings are only on non-critical paths — just auto-triage those and move on. If AskUserQuestion is not available, fall back to a single prose block listing all ambiguous items numbered, asking for Valid/False-positive/Defer decisions. The batching limit is 4 because it caps cleanly at that size; asking more becomes noise rather than judgment.

Source-Specific Handling

From the user (project owner)

From automated review agents

From external reviewers (PR comments, open source)

When to Push Back

Push back (with evidence) when a suggestion:

Valid evidence: code references (file:line), test output, git blame/log, framework docs, reproduction steps, grep results showing usage patterns. Not evidence: "I think", "it should work", "it's fine", appeals to convention without citing the convention, or restating the original code as justification.

False-Positive Taxonomy (for dismissed suggestions)

When dismissing a suggestion (AUTO-DECLINE, manual push-back), tag the dismissal with one of four categories so the reviewer sees structured reasoning, not a bare "no":

| Category | Reviewer's response cited | Evidence required | Maps to "When to Push Back" | |----------|--------------------------|-------------------|------------------------------| | FP-ASSUMPTION | Reviewer assumed behavior that doesn't match the code | Quote the specific line that contradicts the assumption | "Is technically incorrect" | | FP-CONVENTION | Suggestion conflicts with this project's conventions | Cite the CLAUDE.md rule, ADR, or the established pattern in file:line | "Violates project conventions" | | FP-ALREADY-HANDLED | The concern is handled elsewhere (parent function, middleware, framework) | Show the existing handler in file:line | "Adds unnecessary complexity" | | FP-OUT-OF-SCOPE | Valid concern but belongs in a separate change | State where it will be tracked (issue, todo, next PR) | YAGNI / scope creep |

Use the tag in the reply: "FP-ALREADY-HANDLED: null check happens in auth/middleware.ts:42 before this handler runs. Keeping as-is." Structured tags prevent the "you're wrong because reasons" reply pattern and make future triage faster (if the same comment class keeps hitting FP-CONVENTION, the convention needs better documentation).

When NOT to Push Back

Accept feedback when:

Common Mistakes

| Mistake | Fix | |---------|-----| | Agreeing before verifying | Verify first, then state what you found | | Implementing without understanding impact | Trace the change through callers before editing | | Apologizing instead of fixing | State the correction factually, then implement | | Thanking the reviewer instead of responding technically | Delete "Thanks" -- state the fix instead | | Pushing back without evidence | Include the specific code path or test that proves your point | | Batch-implementing then testing | Test after each individual fix | | Can't verify the suggestion | Say so: "Can't verify this without [X]. Should I [investigate/ask/proceed]?" -- don't guess or implement blind |

Approved Response Templates

When feedback IS correct: "Verified -- [specific issue]. Implementing [specific fix]." When feedback is partially correct: "The [X part] is right because [reason]. The [Y part] doesn't apply here because [evidence]." When you need clarification: "Can you clarify [specific ambiguity]? The comment could mean [A] or [B], which changes the fix."

Implementation Order

After triaging all feedback:

1. Clarify -- resolve all unclear items first 2. Blocking issues -- fix things that break functionality 3. Simple fixes -- quick wins that are clearly correct 4. Complex fixes -- changes that need careful implementation

Test after each individual fix, not after implementing everything.

When Your Pushback Was Wrong

State the correction factually: "Checked this, you're correct because [reason]. Implementing." No extended apology, no self-deprecation -- just acknowledge and move on.

GitHub PR Reviews

Headless Mode

When invoked programmatically (by another skill or command with mode:headless), skip interactive prompts and return structured triage results. See headless-mode.md for the classification table (AUTO-FIX / AUTO-DECLINE / ESCALATE), output format, and constraints.

Scope vs ia-pr-comment-resolver Agent

This skill and the ia-pr-comment-resolver agent handle different situations:

| | This skill (interactive) | This skill (headless) | ia-pr-comment-resolver agent | |---|---|---|---| | When | Interactive review requiring judgment | Programmatic triage by another skill/command | Implementing a single pre-triaged comment | | Approach | Verify, evaluate, potentially push back | Auto-classify and return triage results | Implement a single pre-classified change | | Skepticism | High -- check correctness first | High -- same rules, automated classification | Low -- comments are pre-triaged | | Use for | Unclear suggestions, architectural feedback | Batch triage before dispatching resolvers | Clear-cut fixes, style nits, typos |

When the ia-pr-comment-resolver agent encounters feedback that requires judgment (architectural decisions, debatable trade-offs), it should escalate rather than implement.

Examples

Bad -- performative agreement: > Reviewer: "This should use a Map instead of an object for better performance." > Response: "Great catch! Switching to Map now."

Good -- verify first: > Reviewer: "This should use a Map instead of an object for better performance." > Response: "Checked this -- the object has at most 5 keys (bounded by the enum). Map overhead isn't justified here. Keeping as-is."

Bad -- blind implementation: > Reviewer: "Add null checks for the user parameter." > Response: "Done, added null checks."

Good -- verify the premise: > Reviewer: "Add null checks for the user parameter." > Response: "Traced the call chain -- user comes from the auth middleware which guarantees non-null (throws 401 otherwise). Adding a null check here would be dead code. Should I add it anyway for defense-in-depth?"

Good -- unclear item: > Reviewer: "This logic seems off." > Response: "Can you clarify which part? The filter predicate, the sort order, or the pagination logic?"

Verify

Integration

This skill pairs with:

Bad -- performative agreement: > Reviewer: "This should use a Map instead of an object for better performance." > Response: "Great catch! Switching to Map now."

Good -- verify first: > Reviewer: "This should use a Map instead of an object for better performance." > Response: "Checked this -- the object has at most 5 keys (bounded by the enum). Map overhead isn't justified here. Keeping as-is."

Bad -- blind implementation: > Reviewer: "Add null checks for the user parameter." > Response: "Done, added null checks."

Good -- verify the premise: > Reviewer: "Add null checks for the user parameter." > Response: "Traced the call chain -- user comes from the auth middleware which guarantees non-null (throws 401 otherwise). Adding a null check here would be dead code. Should I add it anyway for defense-in-depth?"

Good -- unclear item: > Reviewer: "This logic seems off." > Response: "Can you clarify which part? The filter predicate, the sort order, or the pagination logic?"