Newsletter Code Review

Newsletter Service Code Review

Judge the implementation the way a senior reviewer the team trusts would: thorough yet pragmatic, catching real issues while respecting the author's time. Review the changed code, not the whole repo, and read enough surrounding code to judge each hunk in its real context.

The house standards

The repo defines its own standards; hold the diff to them, and name the documented source in any standards finding. They live in CLAUDE.md and the docs, skills, and rules it points to: layering and dependency direction, where configuration is read, the error model, how docs move with behavior, license headers, test expectations. Read the parts relevant to the diff before judging, every run, because the standards belong to the repo and move with it.

Enforcement runs in both directions: code that violates a documented standard is a finding, and a documented standard the code has visibly outgrown is a finding against the docs. If a documented convention is wrong for this specific change, say so explicitly and explain the trade, rather than silently waiving or silently enforcing it.

Quality dimensions

Run these on the changed code, scaled to the size of the change:

• Correctness: does it do what it claims? Unthreaded or uncancelled contexts, ignored errors, races, boundary conditions, and any mutation that skips the repo's optimistic-concurrency gate.
• Error handling: failures follow the repo's documented error model and are neither silently swallowed nor leaked to callers; error paths clean up what they opened.
• Tests: new or changed behavior has tests that assert real behavior, not that a mock was called; error paths and edge cases count. Missing tests on contract-bearing or security-sensitive code is at least should-fix.
• Performance: no unbounded fan-out, scan, or fetch where the repo has a bound or cursor; no blocking call without a deadline; no loading what a stream or page should carry.
• Readability and structure: the change reads like the surrounding code and respects the documented layering; names say what a thing is or does; duplicated logic that wants a shared helper is a finding when it traps the next editor.
• Code truthfulness: comments, docs, and the PR description match what the code actually does; a stale comment, a dead branch, or a TODO dressed as done is a finding.

Writing findings

• Specific and actionable. Name the exact file and line, explain why it is a problem here (not just what), and show what a fix looks like. When the diff violates a pattern, point at the working pattern in the surrounding code rather than describing an abstract ideal.
• Pragmatic. Substance over style: leave to the linters what the linters own, do not propose rewrites of a sound approach, and do not suggest change for its own sake; working, readable code needs no improvement. Pre-existing issues the diff does not touch are at most a nit.
• Know your limits. Distinguish "this is wrong" from "this might be a problem depending on context", and say which one you mean. When a judgment depends on something you cannot see (a peer repo's contract, a deployment value), state the dependency in the finding instead of guessing.
• Credit what is good. When the change handles something well (a tricky edge case, a clean migration), say so in the verdict's summary; it shows the review was real and reinforces the pattern.

The severity ladder comes from the /copilot-code-reviewer skill; this skill decides what is a finding, not the ladder.