Code Review and Quality

Overview

Multi-dimensional code review with quality gates. Every change gets reviewed before merge — no exceptions. Review covers five axes: correctness, readability, architecture, security, and performance.

The approval standard: Approve a change when it definitely improves overall code health, even if it isn't perfect. Perfect code doesn't exist — the goal is continuous improvement. Don't block a change because it isn't exactly how you would have written it.

Universal Rules

1. Review every change before merge. No exceptions. "It's small" is not an exemption.
2. Review tests first. Tests reveal intent and coverage before you read a line of implementation.
3. Label every finding. Use Critical / Nit / Optional / FYI prefixes so the author knows what's required.
4. Approve improvements, not perfection. If the change improves overall code health, approve it.
5. Don't rubber-stamp. "LGTM" without evidence of review helps no one. Quantify problems when possible.
6. Split large changes. Ask the author to split anything over ~300 lines rather than reviewing one massive changeset.
7. Require cleanup before merge. Don't accept "I'll fix it later" — later never comes.
8. Treat dependency additions as changes. Every new dependency needs justification: size, maintenance status, license, known vulnerabilities.
9. Quote before flagging. Every finding must include the specific lines that support it. "This function looks risky" without a quote is an opinion, not a finding. If you can't quote the evidence, you don't have a finding yet.

Red Flags

• PRs merged without any review
• Review that only checks if tests pass (ignoring other axes)
• "LGTM" without evidence of actual review
• Security-sensitive changes without security-focused review
• Large PRs that are "too big to review properly" (split them)
• No regression tests with bug fix PRs
• Review comments without severity labels
• Accepting "I'll fix it later"

Verification

After review is complete:

• All Critical issues are resolved
• All Important issues are resolved or explicitly deferred with justification
• Every finding is backed by a specific quoted passage from the actual code
• No findings were generated from assumed code structure — only from what was read
• Tests pass
• Build succeeds
• The verification story is documented (what changed, how it was verified)

Tier discipline

Tier definitions: review-tiers (.claude/rules/review-tiers.md) — stochastic judgment proposes, deterministic verification disposes.

• Tier 0: the CI gates themselves (tests, build, linters, validators). These hard-block on their own authority; cite them instead of re-finding what they catch.
• Tier 1 (may gate, evidence attached): correctness and security findings demonstrated by a failing test, failing command, or concrete counterexample input — the artifact is the gate, the review only chose which artifact to produce. The missing-regression-test blocker is Tier 1: the evidence is the absent test.
• Tier 2 (advisory, never gates): readability, architecture taste, "could be simpler", unevidenced performance concerns. Severity labels (Critical, blocking) on unevidenced findings are proposals to the operator, not gates — log them to findings-ledger rather than writing blocking language.

References

• references/review-axes.md — Five-axis review checklist: correctness, readability, architecture, security, performance
• references/review-process.md — Step-by-step process, change sizing, descriptions, multi-model pattern, dead code hygiene, disagreements, honesty, dependency discipline, full review checklist
• references/security-checklist.md — Detailed security review guidance
• references/performance-checklist.md — Performance review checks
• references/scanner-evidence.md — Running scripts/check-imports.sh (here) and security/scripts/security-scan.sh as Tier 0/1 evidence: path resolution, exit codes, citing hits, the offline-network decision

Scripts

• scripts/check-imports.sh [--base <ref>] [--paths <file>...] [--json] — a deterministic check for JS/TS and Python imports that don't resolve, and for new runtime dependencies that nothing imports. Exits 0 clean, 1 findings, 2 usage, 3 nothing to check, 4 error. Run it from the project root. It only reads.

Related skills

• skill-library-review — applies the same review discipline to agent and skill definitions rather than source code
• adversarial-claims-reviewer — applies the same adversarial discipline to formal/technical claims in documents rather than source code
• findings-ledger — where this skill's Tier 2 (unevidenced) findings get recorded and tallied for recurrence