code-review-checklist v1.0.0B

Immutable. This exact content never changes and is served at /api/v1/blob/1a1c37b9c8c3f524.

---
name: code-review-checklist
description: >-
  Use when reviewing code — a PR, a diff, or freshly written changes — and you want concrete
  pass/fail checks rather than impressions. An aviation-style preflight checklist organised into
  sequential phases, with stack-specific items merged in from the project's config.
version: 1.0.0
author: ai-badger
license: MIT
platforms: [linux, macos, windows]
scope: default
metadata:
  hermes:
    tags: [code-review, checklist, fullstack]
    related_skills: [comprehensive-code-review, frontend-code-review, data-contract-audit, plan-review, requesting-code-review]
---

# 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
  store you just wrote it to is unchanged. Assert on the transformation the code
  performs, not on the fact that storage works.
- [ ] **Every async assertion is awaited** — `Assert.ThrowsAsync` without
  `await`, `expect(promise).resolves…` without `await`/`return`, an un-awaited
  coroutine in an async test. The assertion is never evaluated, so the test
  passes *silently* even when the behaviour it claims to check is broken — the
  hardest of these to spot by eye, and the one worth grepping the diff's test
  files for by name.
- [ ] **No swallowed exception** — `catch { }` (C#), `except: pass` (Python),
  `catch (e) {}` (JS/TS/Java), `defer recover()` with no re-panic (Go). The test
  passes on exactly the failure path it was written to detect.
- [ ] **No assert-in-catch** — `catch (Exception ex) { Assert.Fail(ex.Message); }`
  in place of asserting the expected exception. It turns "the wrong exception
  type was thrown" into a generic failure and hides which contract broke.

> These triggers are carried over from the `test-anti-patterns` and `grade-tests`
> skills in [dotnet/skills](https://github.com/dotnet/skills) (MIT, .NET
> Foundation). Their A–F letter grades are deliberately **not** carried: a band
> invites arguing about the grade instead of fixing the finding.

### 3.2 Repository & Contract Tests

- [ ] **Repository interfaces have contract tests** — not just in-memory fakes.
  Datastore implementations should be validated against the contract.
- [ ] **Repository filter methods cover spec requirements** — if the API spec
  defines filters, the repository must have a method that supports them.

> Full checklist: read references/security.md if the diff touches security surfaces (auth, secrets, input handling, redirects).

### 3.4 Documentation & Hygiene

- [ ] **Spec issues from reviews are fixed** — check if prior review findings
  are addressed in the implementation, not just noted in the spec.
- [ ] **Tracked TODOs for deferred work** — every suppression, known gap, or
  technical debt has a tracked issue or inline TODO with context.
- [ ] **No copy-paste duplication in specs/docs** — check for duplicated content
  blocks that should reference a single source.
- [ ] **Import paths are accurate** — all referenced modules, components, and
  utilities exist at the paths used in import statements.

## ts: Browser Security
> Applies to code that runs in a browser. For a server-side TypeScript project,
> mark the browser-only items N/A rather than skipping the section silently.
- [ ] **No untrusted value reaches a DOM XSS sink** — `innerHTML`/`outerHTML`,
  `insertAdjacentHTML`, `document.write`, `dangerouslySetInnerHTML`, `eval` /
  `new Function`, a string passed to `setTimeout`/`setInterval`, or an event
  handler attribute set from a string. Use `textContent` or the framework's
  escaping path, or sanitize with an audited sanitizer at the insertion point.
- [ ] **A markdown or rich-text renderer with raw HTML enabled has a sanitizer**
  — enabling raw HTML and trusting the source is the same finding as `innerHTML`.
- [ ] **`href`, `src` and programmatic navigation from untrusted data are
  scheme-checked** — `javascript:`, `data:` and `vbscript:` rejected, target
  origin decided explicitly rather than by string concatenation.
- [ ] **`postMessage` validates `event.origin` and passes an explicit
  `targetOrigin`** — never `"*"` for anything but public data, and the received
  payload is untrusted input, not a typed object.
- [ ] **No token in Web Storage** — `localStorage`/`sessionStorage` is readable
  by any injected script. Prefer `HttpOnly` `Secure` `SameSite` cookies, or
  in-memory with a silent-refresh path.
- [ ] **Cookie-authenticated state-changing requests carry CSRF protection** —
  `SameSite` is a mitigation, not the control.
- [ ] **New inline script or style has not quietly forced `unsafe-inline`** — the
  diff adding it is the moment to re-check the CSP; `unsafe-inline` and
  `unsafe-eval` each need a written reason, with nonces or hashes as the target.
- [ ] **A new third-party `<script src>` is self-hosted or carries `integrity`**
  — an unpinned CDN script, tag manager or analytics snippet is a supply-chain
  hole with no owner.
- [ ] **Source maps and dev-only overlays are absent from the production build**
  — they expose code structure and internal URLs.

> Distilled from the frontend references in the OpenAI `security-best-practices`
> skill ([openai/skills](https://github.com/openai/skills), Apache-2.0); rules
> that a reviewer cannot check against a diff were left there deliberately.

---

## Phase 4: Backend Runtime Behavior (Concurrency, Errors, Observability)

### 4.1 Concurrency & Idempotency

- [ ] **Optimistic concurrency via ETag** — every Save/Upsert that could be
  called concurrently must use ETag-based CAS.
- [ ] **Idempotent operations return 200, not 409** — if an operation is already
  in the target state, return 200 with current state. Only throw 409 for
  genuinely conflicting states.
- [ ] **Idempotency check comes BEFORE policy evaluation** — check
  disposition/early-exit before calling business logic that may return a
  misleading status for already-applied items.
- [ ] **TOCTOU gaps documented** — if a check-then-act pattern exists, note
  whether it's acceptable for current phase or needs a distributed lock.
- [ ] **Export/create operations have idempotency** — calling POST twice should
  either return the same resource or reject the second (not silently duplicate).

### 4.2 Error Handling

- [ ] **Problem type URIs / error codes are consistent** — the error identifier
  used by the backend must match what the client checks. Drift = silent failures.

> Full checklist: read references/observability.md if the diff touches metrics, logs, tracing, or readiness probes.

---

## Phase 5: Client-Server Contract Alignment

> WARNING: Mismatched routes, response shapes, or error codes cause
> silent failures — the app compiles and tests pass against mock data.

- [ ] **Client route paths match API route paths EXACTLY** — including resource
  prefix
- [ ] **Query parameter names match** — client query keys use the same parameter
  name as the API expects
- [ ] **Response shapes match field-for-field** — nested vs flat, wrapper
  objects, detail-only fields omitted from list responses
- [ ] **Error codes/types match** — client error detection uses the exact error
  identifiers the API returns.
- [ ] **Client types mirror backend types** — field names, optionality, nesting
  all match. Enum values use the wire format.
- [ ] **Config endpoints agree** — if the spec defines one path and the client
  calls another, resolve before implementation.
- [ ] **Types are explicitly defined** — every type referenced in an API call
  must have a corresponding type definition (not inline any or inferred).
- [ ] **Mock/test fixtures match actual API responses** — if the API shape
  changes, the test fixtures must update too. Wrong fixtures = tests pass
  against phantom data.

## ts: TypeScript Quality
- [ ] **No `any` types in application code** — zero tolerance
- [ ] **No `as` type assertions except for `JSON.parse` and `event.target.value`**
  — unsafe `as` casts bypass type checking
- [ ] **Route params are type-safe** — use `z.string().parse(param)` or a
  guard clause that returns early. No eslint-disable on route params.
- [ ] **Types are explicitly defined** — every type referenced in an API call
  must have a corresponding interface/type definition (not inline any).
- [ ] **Client types mirror backend record types** — field names, optionality,
  nesting all match. Enum values use the wire format.

---

## Phase 6: Cross-Feature Patterns

- [ ] **New code matches existing patterns in the same repo** — compare against
  established conventions in sibling modules/features.
- [ ] **Shared constants are not duplicated** — API base URLs, feature flags,
  config values should come from a single source, not redefined per module.
- [ ] **Cross-feature duplication extracted to shared utilities** — if two
  modules share ~70 lines of identical logic, extract it.
- [ ] **Bulk operations consider parallelism** — sequential processing of 10+
  items is slow. Document why sequential is required or use concurrent execution.

---

## Phase 7: Accessibility

- [ ] **Automated accessibility tests exist for every new page/section** — at
  minimum one axe smoke test per page-level component
- [ ] **All interactive elements have associated labels** — htmlFor/id pairs,
  aria-label, or aria-labelledby
- [ ] **Decorative icons have aria-hidden="true"**
- [ ] **Status badges have aria-live="polite"** — screen readers must announce
  changes to status indicators
- [ ] **Loading states have aria-busy="true" and aria-live="polite"**
- [ ] **Error containers have role="alert"**
- [ ] **Nav links have aria-current="page" on active state**
- [ ] **External links have target="_blank" rel="noopener"**

---

## Phase 8: Orchestration & Async Patterns

- [ ] **Long-running operations return async acknowledgment** — not synchronous
  completion. Return an operation ID for polling/tracking.
- [ ] **Orchestration code is deterministic** — no direct I/O in orchestration
  functions. Activities/workers do the I/O.
- [ ] **Retry loops are bounded** — every retry, poller, or concurrency gate
  has an explicit finite cap. An unbounded loop is a standing availability/cost risk.

---

## Phase 9: Post-Merge Verification (Smoke Test)

> After merging, verify the integration doesn't break the combined codebase.

- [ ] **Build clean on main** — zero errors after merge
- [ ] **All tests pass** — no regressions from merge conflicts
- [ ] **Lint clean** — integration clean
- [ ] **Merge conflicts resolved with intent** — not blindly auto-merged
- [ ] **Infrastructure definitions updated** — if new resources were added
  (database tables, containers, queues, storage)
- [ ] **Issues closed with PR reference comments** — traceability maintained

---

## Gotchas

No environment-specific gotchas known.

## Usage Tips

1. **Walk phases sequentially** — Phase 1 gates everything.
2. **Check each item against actual code**, not assumptions.
3. **Stack-specific extensions add items into each phase** — dotnet, react, cosmos, azure, ts, and mcp items are merged .
4. **Project-specific checks** go in `project-local.md` — incident lessons, project conventions, etc.
5. **After completing all phases**, the reviewer signs off: "Preflight complete. All phases PASS."