74 lines
6.2 KiB
Markdown
74 lines
6.2 KiB
Markdown
# Review lenses
|
|
|
|
Use every core lens. Expand only the checks relevant to the changed surfaces.
|
|
|
|
## Contents
|
|
|
|
- Correctness and unintended behavior
|
|
- Test validity
|
|
- Security and defensive coding
|
|
- Architecture and maintainability
|
|
- Conditional lenses
|
|
|
|
## Correctness and unintended behavior
|
|
|
|
- Trace happy, error, empty, null, boundary, retry, cancellation, and partial-failure paths.
|
|
- Check state transitions, ordering, idempotency, caching, invalidation, and cleanup.
|
|
- Look for off-by-one errors, stale state, wrong defaults, lossy conversions, timezone or locale mistakes, and mismatched units.
|
|
- Check async ownership, missing awaits, races, deadlocks, double execution, resource leaks, and work after cancellation.
|
|
- Compare types and runtime values across boundaries; do not assume static types validate external data.
|
|
- Check compatibility with existing callers, serialized formats, database rows, events, CLI flags, environment variables, and public APIs.
|
|
- Follow error values and exceptions. Flag swallowed failures, misleading fallbacks, partial writes, and success reported before durable completion.
|
|
- Verify deletions and refactors leave no live caller, stale branch, duplicate path, or behavior split between old and new implementations.
|
|
- Distinguish behavior newly broken by the change from a nearby pre-existing issue.
|
|
|
|
## Test validity
|
|
|
|
- Identify the behavior each test claims to protect and the production branch that implements it.
|
|
- Mentally mutate or remove the production behavior. Determine whether the test fails for the intended reason.
|
|
- Detect tautological assertions, assertions derived from the same value under test, snapshots accepted without semantic inspection, and tests with no meaningful assertion.
|
|
- Check that the exercised path runs: awaited promises, consumed generators, subscribed streams, flushed transactions, rendered components, and invoked callbacks.
|
|
- Treat mocks as contracts. Compare mock methods, data, ordering, errors, and side effects with the real dependency.
|
|
- Flag tests that verify implementation details while missing observable outcomes.
|
|
- Check false positives from broad exception assertions, loose matchers, unconditional waits, retries that mask failure, or fixtures that bypass the behavior.
|
|
- For bug fixes, require a regression test that would fail on the old behavior when practical.
|
|
- Seek high-risk gaps: permissions, tenant isolation, malformed input, boundaries, failure recovery, concurrency, migrations, and cross-component integration.
|
|
- Do not demand a test for trivial wiring already proven by stronger coverage.
|
|
|
|
## Security and defensive coding
|
|
|
|
- Identify trust boundaries and attacker-controlled input before applying a checklist.
|
|
- Check authentication, authorization, object ownership, tenant isolation, and confused-deputy paths at the point of action.
|
|
- Validate and canonicalize input at boundaries; encode output for its destination context.
|
|
- Look for injection into SQL, commands, templates, HTML, URLs, headers, logs, paths, deserializers, and dynamic code.
|
|
- Check secrets, credentials, tokens, personal data, and sensitive metadata in source, logs, errors, caches, telemetry, URLs, and client-visible responses.
|
|
- Review cryptography, randomness, signature verification, token lifetime, replay resistance, secure comparison, and key handling when touched.
|
|
- Check filesystem traversal, unsafe temporary files, archive extraction, symlink behavior, permissions, and cleanup.
|
|
- Review SSRF, redirect validation, request smuggling surfaces, unbounded downloads, parser limits, and denial-of-service opportunities for network-facing changes.
|
|
- Check dependency provenance, pinned integrity, install or build scripts, dangerous configuration defaults, and privilege expansion.
|
|
- Prefer fail-closed behavior where authorization or integrity is uncertain, without turning recoverable availability failures into outages.
|
|
- Consider abuse cases and defense in depth; report only issues supported by a realistic capability and impact.
|
|
|
|
## Architecture and maintainability
|
|
|
|
- Compare the change with existing architectural boundaries and dependency direction.
|
|
- Check separation of orchestration, domain logic, persistence, transport, and presentation.
|
|
- Identify feature logic leaking into shared infrastructure or transport details leaking into the domain.
|
|
- Look for duplicated sources of truth, parallel implementations, inconsistent policy enforcement, and abstractions that conceal rather than remove complexity.
|
|
- Challenge both under-abstraction and premature abstraction. Require a named invariant or repeated variation before adding a general layer.
|
|
- Check cohesion, coupling, fan-in/fan-out, circular dependencies, global state, hidden temporal coupling, and unclear resource ownership.
|
|
- Prefer explicit invariants, narrow interfaces, meaningful types, and canonical helpers over casts, optional fields, catch-all objects, and silent fallbacks.
|
|
- Judge readability through control flow, naming, locality, concept count, and how much unrelated context a maintainer must retain.
|
|
- Check whether the design supports likely change without optimizing for hypothetical futures.
|
|
- When suggesting restructuring, name the move and why it lowers risk: split responsibility, invert dependency, centralize policy, remove pass-through abstraction, model state explicitly, or reuse a canonical path.
|
|
- Avoid requesting large refactors unless the current change creates or materially worsens the problem.
|
|
|
|
## Conditional lenses
|
|
|
|
- Performance: complexity, repeated I/O, N+1 access, allocations, hot-path blocking, pagination, cache correctness, and backpressure.
|
|
- Data changes: forward/backward compatibility, transactionality, rollbacks, backfills, mixed-version deployment, and irreversible loss.
|
|
- Distributed systems: retries, duplicate delivery, ordering, idempotency, clock assumptions, split brain, and partial availability.
|
|
- UI and accessibility: loading/error/empty states, keyboard and focus behavior, semantics, contrast, responsive behavior, and hydration.
|
|
- Observability: actionable errors, stable structured fields, correlation, sensitive-data hygiene, metrics on failure paths, and diagnosable fallbacks.
|
|
- Dependencies and configuration: default changes, environment parity, least privilege, reproducibility, version compatibility, and safe rollout.
|