Reviewing Changes

Process

Always run the three passes in order. Findings flow into one combined verdict.

1. Read the rules

Before any pass, internalise the language-rule skill that matches the diff (python-conventions, go-conventions, solidity-conventions, …) and engineering-philosophy. The rule skills are the source of truth — don't invent additional standards.

2. See the change

git diff <base>...HEAD
git log <base>..HEAD --oneline

For a GitHub PR:

gh pr view <N>
gh pr diff <N>

3. Pass 1 — Code quality

Check, in order:

• Philosophy violations — over-engineering (KISS, YAGNI), duplication (DRY), magic behaviour (No Magic), copy-paste-modified blocks.
• SOLID violations — Single Responsibility first; flag classes/files that grew a second responsibility.
• Naming, readability, complexity — function lengths, parameter lists, deeply nested conditionals, clever one-liners that hide intent.
• Test coverage — was the change tested? If TDD discipline applied, was the failing test committed first?
• Tooling compliance — ruff/mypy strict for Python, golangci-lint for Go, solhint:all for Solidity, forge fmt --check for Solidity formatting.
• Configuration safety — production timeouts, connection pools, missing retries, missing rate limits.

4. Pass 2 — Security audit

Check, in order, against OWASP Top 10:

• Injection — SQL, command, LDAP, template, header injection via unsanitised input.
• Broken authentication — weak token handling, missing MFA, fragile session management.
• Broken access control — missing authz checks, privilege escalation, IDOR (insecure direct object reference).
• Sensitive data exposure — secrets in logs, error messages, or response bodies; missing TLS; weak ciphers.
• Misconfiguration — overly permissive CORS, missing security headers, debug endpoints exposed.
• Vulnerable components — pip-audit / npm audit / govulncheck / dep CVEs.
• XSS — unencoded output rendered as HTML/JS; missing CSP.
• Insecure deserialisation — pickle.loads on untrusted input, similar in JS/Java.
• Insufficient logging and monitoring — security-relevant events not logged, no alerting.
• Cryptographic issues — weak algorithms, hardcoded keys, missing key rotation, predictable IVs.
• Smart-contract specific (if Solidity) — reentrancy, integer over/underflow, unchecked external calls, access control on onlyOwner-style modifiers, front-running, MEV exposure, signature replay.

See reference/owasp-checklist.md for the canonical mapping with attack-vector notes.

5. Pass 3 — Architecture consistency

• Architecture map — does the diff respect the docs/architecture.md (or equivalent) responsibility split? See reference/architecture-map-pattern.md.
• Layer violations — dependencies pointing the wrong way (e.g., domain importing infrastructure).
• Boundary erosion — public methods sneaking into private packages; circular dependencies.
• Missing abstractions — same logic implemented twice with minor variations.
• Custom code where a library exists — flag reinvented validators, parsers, ORMs, retry logic, etc.
• Pattern compliance — clean architecture / DDD bounded contexts, only when the project documents a pattern.

Output

## Quality Gate Summary

| Review       | Verdict        | Critical | Major | Minor |
|--------------|----------------|----------|-------|-------|
| Code         | pass/warn/fail | N        | N     | N     |
| Security     | pass/warn/fail | N        | N     | N     |
| Architecture | pass/warn/fail | N        | N     | N     |

**Overall**: PASS / NEEDS WORK / FAIL

### Action items
1. <Critical/Major items, ordered>

For each individual finding:

• Rule — which rule was violated (with the language-rule skill or engineering-philosophy reference) or "best practice" if no codified rule.
• Severity — Critical / Major / Minor.
• Location — file:line.
• Issue — what's wrong and (for security) the attack vector.
• Fix — concrete suggestion, with a short code example when it clarifies the change.

Behavioural traits

• Constructive, educational tone. Teach; don't just flag.
• Specific, actionable feedback. "This is too complex" without a fix is useless.
• Severity matches reality. Critical for "this could ship a bug or a CVE today"; Major for "this will hurt within six months"; Minor for style and polish.
• Practical over theoretical security risks. If an attack requires three impossible preconditions, mark Minor.
• Defence in depth. Multiple weak controls beat one perfect control.
• Read-only. This skill never edits the diff itself; it reports.

Cross-references

• running-tdd-cycles — preceding workflow; review confirms TDD discipline.
• committing-changes — commit-message + branch hygiene checks fold into the code-quality pass.
• python-conventions / go-conventions / solidity-conventions — the language rule the diff is being checked against.
• engineering-philosophy — KISS, YAGNI, DRY, SOLID weights for code-quality and architecture passes.

Reference

• reference/code-reviewer-agent.md — original Claude-Code code-reviewer agent verbatim (model: opus).
• reference/security-auditor-agent.md — original Claude-Code security-auditor agent verbatim (model: opus).
• reference/architect-review-agent.md — original Claude-Code architect-review agent verbatim (model: opus).
• reference/owasp-checklist.md — canonical OWASP Top 10 mapping with attack vectors and fix patterns.
• reference/architecture-map-pattern.md — the optional docs/architecture.md convention this skill expects.