Receiving Code Review

Review feedback is technical input to evaluate, not a command stream to obey. Do not chase perfection: apply feedback that makes the change safer, clearer, or better aligned with the contract; push back on feedback that is wrong, speculative, or not worth the complexity it adds.

For coding-related work, load engineering-principles before this skill. Use it to decide whether feedback aligns with local standards for simplicity, boundaries, tests, validation, and tooling.

engineering-principles
  -> doing-code-review
  -> findings or reviewer comments
  -> receiving-code-review  (you are here)
       -> verify and classify feedback
       -> focused fixes / documented pushback / clarification
       -> focused verification
       -> doing-code-review          (if fixes are non-trivial)
       -> verification-before-completion

Core Rules

Response Pattern

1. Group comments by area: correctness, tests, architecture, security, performance, style.
2. Identify unclear, contradictory, or high-impact items.
3. Classify each item: valid, invalid, unclear, trade-off, or already handled.
4. Implement valid fixes in small coherent groups.
5. Run focused verification.
6. Report what changed, what was deferred, and what was verified.

If an unclear item could affect other fixes, clarify before partial implementation.

bad:  Fix clear items now, ask about related unclear items later.
good: Clarify related unclear items first, then edit once the shape is known.

Feedback Classification

Use doing-code-review when you need a fresh review of the updated diff.

Verification Gate

Before implementing feedback, check:

Does this suggestion...
  ├── match the original requirement?
  ├── preserve existing behavior?
  ├── fit project conventions?
  ├── avoid unrequested features?
  ├── avoid breaking public contracts?
  └── have a clear verification path?

If unclear, inspect more context or ask before editing.

Push Back When Needed

Push back when the suggestion:

Checked this against the generated client. Renaming `account_id` would break
`apps/web/src/api/accounts.ts`. We can keep the field or make a versioned
contract change. Which path do you want?

YAGNI Check

Reviewers sometimes ask for "proper" features that are not needed. Before a broad change, ask:

Is this path used now?
Was it in the requirement?
Is the data/traffic volume real?
Does a user path need this now?

If not, ask whether to remove, defer, or keep the simpler implementation.

Communication

Do not agree before checking.

weak:   "You're absolutely right, I'll fix it."
better: "Checking the call sites before changing this."
best:   "Valid. Fixed invalid-date handling and added a regression test."

If you pushed back and were wrong, correct cleanly:

Verified against `InvoiceSchema`; my initial read was wrong. Updating parser and test now.

Implementation Order

1. Clarify blockers and ambiguous items first.
2. Fix blocking correctness/security/data-loss issues.
3. Fix small mechanical issues that unblock tests/build.
4. Fix larger refactors or architecture changes.
5. Run focused verification.
6. Use doing-code-review for fresh review if fixes were non-trivial.

Keep fixes small enough that the reviewer can see the response to each finding.

Reply Format

## Addressed
- `src/invoices/date.ts`: added invalid-date guard and regression test.

## Deferred / Pushback
- Keeping `account_id` unchanged because generated web client consumes it.
  Versioned rename is possible, but it is a contract change.

## Verification
- `pnpm test invoices`: passed
- `pnpm typecheck`: passed

Reply to GitHub inline comments in-thread, not as unrelated top-level PR comments.

Related Skills

Common Mistakes