code-review-checklist

Files (3)

• SKILL.md
• observability.md · references/
• security.md · references/

SKILL.md (first lines)

# Code Review Preflight Checklist

> **Pattern: Aviation preflight checklist.** Each phase is a system check.
> Every item is a concrete, verifiable action — read it, check it, confirm it.
> No item is skipped regardless of reviewer experience. Critical items carry
> WARNING markers. Phases are sequential — complete each before proceeding.

## When to Use

- Reviewing a PR before approving
- Performing a milestone or sprint review
- Self-reviewing before `git push`
- Checking a subagent's output before merging

## When NOT to Use

- Plan/spec feasibility review → `plan-review`
- Frontend architecture deep-dive → `frontend-code-review`
- Full milestone review methodology → `comprehensive-code-review`
- Pre-commit security scan → `requesting-code-review`

## Preflight Protocol

1. **Read each item aloud** (or mentally). Verify against actual code, not assumptions.
2. **Mark PASS or FAIL.** If any FAIL exists, do not approve until resolved.
3. **WARNING items are non-negotiable.** They represent real bugs from past incidents.
4. **Complete phases sequentially.** Phase 1 gates all others.
5. **Report every FAIL in one shape** — item or rule / severity (Critical, High, Medium, Low) /
   location (file, symbol, line) / evidence (the exact snippet) / impact (what goes wrong and who
   can trigger it) / fix (the smallest safe change) / false-positive notes (what to verify if you
   are unsure). A finding with no location and no evidence is an impression, and this checklist
   exists to replace impressions.
6. **A protection you cannot see in the diff is not a finding.** Security headers set at the edge,
   a WAF, TLS termination, gateway auth, network policy — these are routinely absent from
   application code and present in production. Report them as *"not visible in application code,
   verify at runtime"*, never as a vulnerability.

> **Stack-specific checks:** This template covers universal concerns. If your
> project uses dotnet, react, cosmos, azure, ts, or mcp, the corresponding
> `extensions/<name>/` phases are embedded automatically. Follow them after
> the generic phases. Project-specific checks (incident lessons, project
> conventions) go in `project-local.md` and are appended automatically.


---

## Phase 1: Pre-Takeoff Gates (ALL MUST PASS — gates everything else)

> These are absolute blockers. If any FAIL, do not proceed to Phase 2.

- [ ] **Build passes** — the project's build command completes with zero errors
- [ ] **Tests pass** — the project's test command shows no new failures vs baseline
- [ ] **Lint passes** — the project's linter runs clean
- [ ] **No hardcoded secrets** — no API keys, tokens, connection strings, or passwords in any tracked file
- [ ] **No unjustified warning suppressions** — every suppression must have a tracked TODO or documented reason
- [ ] **VERSION bumped** — if this is a release, the version has been bumped (semver) and a changelog entry exists
- [ ] **One PR = one task** — the PR does not bundle unrelated changes

---

## Phase 2: Architecture & Layering (Structural Integrity)

> These checks enforce clean boundaries between layers. Violations compound
> into unmaintainable codebases.

### 2.1 Layering

- [ ] **Domain has zero infrastructure dependencies** — domain files must not
  import HTTP clients, database SDKs, cloud provider SDKs, or any external
  service client.
- [ ] **Infrastructure implements domain interfaces** — the dependency direction
  is always: Infrastructure -> Domain (never reverse). Domain defines ports;
  infrastructure provides adapters.
- [ ] **API endpoints are thin** — delegate to domain services/engine.
  Endpoints > 50 lines should be refactored.
- [ ] **API surface maps 1:1 to domain operations** — no business logic in
  controllers, route handlers, or API gateway functions.

### 2.2 Domain Model

- [ ] **State transitions enforced by domain model** — state machine transitions
  are in the model, not the endpoint.
- [ ] **No string action parameters where an enum exists** — if a switch on a
  string action exists, it should be a typed enum.

### 2.3 Screaming architecture

- [ ] **Folders named by domain concept** — a new folder name should tell a
  reader what the system *does*, not what kind of file lives there.
  Avoid catch-all `Services/`, `Controllers/`, `Utils/` buckets.
- [ ] **Shared technical chassis is the only exception** — logging, DI wiring,
  cross-cutting middleware may use generic names.

---

## Phase 3: Cross-Cutting Concerns (TDD, Security, Docs)

### 3.1 TDD Compliance

- [ ] **Tests exist for all new production code** — no production code without
  a test that demanded it
- [ ] **Test-first order** — failing test written -> code to make it pass ->
  refactor. Not the reverse.
- [ ] **Edge cases tested** — not just happy path. Error paths, boundary
  conditions, concurrent scenarios.
- [ ] **Missing test scenarios documented** — if a test gap exists, it's noted
  as a follow-up, not silently skipped.

> WARNING: A test that cannot fail is not a test. Each item below is a FAIL on
> its own — the test is rewritten or deleted, never argued down. These are the
> patterns that make a suite report green while the behaviour it names is broken.

- [ ] **Every test asserts something** — a test that only calls the code under
  test passes as long as nothing throws, and reports coverage while verifying
  nothing.
- [ ] **No always-true assertion** — `Assert.IsTrue(true)`, `assert True`,
  `expect(true).toBe(true)`. Equivalent to having no assertion at all.
- [ ] **No self-referential or tautological assertion** — `Assert.AreEqual(x, x)`,
  `assert dto.name == dto.name`, or asserting that a value read back from the

From Arasz/ai-badger, MIT.

More from Arasz/ai-badger