code-review-and-quality

name	code-review-and-quality
description	Review code changes before merge or handoff. Use when the user asks to review a PR, branch, commit, diff, working tree, agent-written code, human-written code, bug fix, refactor, or implementation for correctness, regression risk, architecture fit, maintainability, security, reliability, performance, UX/API contracts, and test coverage.

Code Review And Quality

Review Stance

Act as a strict production reviewer. Prioritize real bugs, regressions, security/privacy issues, data loss, broken contracts, missing tests, and operational risk.

Do not rubber-stamp. Do not invent issues. Do not block on personal style unless it hides a real risk. Prefer exact evidence from source, tests, diffs, logs, docs, CI, and runtime configuration.

Review-only mode means do not edit files unless the user explicitly asks for fixes.

Workflow

1. Identify the review target:
   • PR, branch, commit range, staged diff, unstaged working tree, or specific files.
   • If unclear, inspect git status, git branch --show-current, and available local context before asking.
2. Load local instructions:
   • Read relevant AGENTS.md, README.md, docs, build files, CI files, package files, and nearby tests.
   • Prefer current source over old docs, memory, or assumptions.
3. Understand intent before judging code:
   • What behavior is supposed to change?
   • What user, API, data, auth, job, deployment, or UI contract is affected?
   • Which layer owns the behavior?
4. Inspect tests first:
   • New or changed tests.
   • Nearby regression tests and fixtures.
   • Whether tests assert behavior instead of implementation details.
5. Inspect implementation:
   • Trace upstream callers and downstream effects for shared behavior.
   • Check boundaries between API/presentation, application/use case, domain, infrastructure, worker/job, external client, config, and deployment.
6. Use independent subagent review for non-trivial changes:
   • Start multiple read-only subagents in parallel when the diff is broad, high-risk, cross-module, security-sensitive, data-affecting, contract-changing, generated by another agent, or explicitly requested as a strict/deep review.
   • Give each subagent the same raw diff/context plus one focused lens. Do not give them your suspected findings or conclusions.
   • Reconcile their outputs by checking source evidence yourself. Do not accept or reject findings by majority vote.
7. Verify when cheap and safe:
   • Run read-only checks relevant to the review when available.
   • Do not run migrations, deploys, destructive DB operations, or external writes unless explicitly requested.
8. Report findings first:
   • Findings before summary.
   • Highest severity first.
   • Include exact file and line whenever possible.

Review Axes

Correctness

• Does the change match the task/spec?
• Are null, empty, boundary, timezone, locale, permission, and error cases handled?
• Are async state, retries, cancellation, transactions, idempotency, and race conditions correct?
• Does the code preserve intentional behavior instead of approximating it?
• Are generated artifacts, schemas, API specs, or docs updated when behavior requires it?

Tests

• Are meaningful tests added or updated for changed behavior?
• Would the tests fail on the old bug?
• Are edge cases and error paths covered?
• Are snapshots or mocks masking the real contract?
• Are skipped checks explained with residual risk?

Architecture And Maintainability

• Does the change fit existing architecture, naming, folder structure, and error/logging patterns?
• Are ownership boundaries respected?
• Is business logic in the owning layer?
• Is there unnecessary abstraction, flexibility, compatibility code, or dependency churn?
• Is dead code introduced or left behind by this change?

Security And Privacy

• Are inputs validated at trust boundaries?
• Are auth/authz checks preserved?
• Are secrets, tokens, private data, or sensitive IDs kept out of code, logs, errors, and telemetry?
• Are SQL, shell, path, template, HTML, markdown, URL, and serialization sinks safe?
• Are external data sources treated as untrusted?

Reliability And Operations

• Are retries, timeouts, backpressure, pagination, batching, and failure modes appropriate?
• Are side effects explicit and safe to repeat?
• Are logs useful without leaking sensitive data?
• Does config/deployment behavior match code expectations?
• Could rollout order, cache freshness, or stale clients break this?

Performance

• Any N+1 queries, unbounded scans, unbounded memory growth, or unnecessary network calls?
• Any hot-path allocations, blocking I/O, or avoidable re-renders?
• Are list endpoints paginated and filtered appropriately?
• Does the dependency or bundle impact justify itself?

UX/API Contracts

• Are public APIs, DTOs, event payloads, webhook contracts, DB schemas, SDK contracts, and UI states preserved or intentionally changed?
• Are loading, empty, error, permission-denied, partial-data, and mobile/responsive states handled when relevant?
• Are user-facing strings, status values, and semantic distinctions preserved?

Severity

• P0: must block. Security exploit, data loss/corruption, auth bypass, production outage, irreversible migration risk.
• P1: must fix before merge. Likely bug, regression, broken contract, missing critical test, unsafe operational behavior.
• P2: should fix. Maintainability, reliability, performance, security hardening, or coverage gap with plausible impact.
• P3: optional. Nit, readability preference, minor cleanup, or follow-up suggestion.

Only mark optional comments as P3. Everything else should describe why it matters and what would fail.

Output Format

Use this structure:

Findings
- [P1] Short title - path/to/file.ext:123
  Explain the failing scenario, why current code is wrong, and the smallest useful fix.

Open Questions
- Question or assumption that affects correctness, if any.

Tests And Verification
- Pass/fail/unknown for exact commands reviewed or run.
- State important skipped checks and remaining risk.

Summary
- One or two bullets only, after findings.

If there are no findings, say that clearly:

Findings
- No blocking issues found.

Tests And Verification
- ...

Residual Risk
- ...

Finding Quality Bar

Every actionable finding must include:

• Evidence from code, diff, test, config, or runtime contract.
• The condition that triggers the issue.
• The user/system impact.
• A concrete fix direction.

Avoid:

• Style-only comments without impact.
• Vague comments such as "this could be cleaner".
• Repeating lint output without explaining risk.
• Asking for broad rewrites when a local fix is enough.
• Treating absent tests as a finding unless the changed behavior needs coverage.

Large Or Risky Changes

For broad diffs, review by ownership boundary:

1. Public contract changes.
2. Data/model/schema changes.
3. Auth/authz and trust boundaries.
4. Side effects and jobs/workers.
5. UI/user-visible behavior.
6. Tests and generated artifacts.

If the diff is too large to review credibly, say so and recommend a split by feature slice, layer, or file group. Still report any concrete issues already found.

Independent Subagent Review

For substantial reviews, default to multiple independent read-only subagents instead of a single linear review. The goal is variance reduction: catch blind spots, reduce anchoring on one interpretation, and force every finding back to source evidence.

Use subagents when any of these are true:

• The user asks for strict, deep, multi-angle, or high-confidence review.
• The diff crosses repo, service, module, API, worker, data, auth, deployment, or UI boundaries.
• The change touches money, identity, permissions, secrets, migrations, caches, concurrency, external clients, generated artifacts, or production config.
• The change was produced by another agent/model and needs independent validation.
• The main agent already has a strong hypothesis and bias risk is high.

Launch 2-4 focused passes when tools and time allow:

• Correctness/regression: behavior, edge cases, state, contracts.
• Security/reliability: trust boundaries, auth/authz, secrets, retries, idempotency, concurrency, operations.
• Architecture/maintainability: ownership boundaries, coupling, abstractions, dead code, dependency direction.
• Tests/coverage: regression tests, fixtures, mocks, generated artifacts, verification gaps.

Keep subagent prompts independent:

• Pass raw artifacts: diff, target branch/commit, relevant files, test output, task/spec.
• Ask for severity, file/line evidence, trigger condition, impact, and fix direction.
• Prohibit file edits and destructive/external-write commands.
• Do not include your own suspected issues, desired answer, or another subagent's findings.

Reconcile outputs yourself:

• Verify every proposed finding against current source before reporting it.
• Drop hallucinated, stale, style-only, or unsupported findings.
• Merge duplicate findings and keep the clearest evidence.
• If subagents disagree, inspect the source path and report the verified conclusion plus any remaining uncertainty.
• If subagents were skipped, state why: trivial diff, no available tool, time/approval risk, or review target too small.

Do not outsource final judgment. Review subagent findings against the source before reporting.

Source Note

This skill is an adapted, shorter local workflow inspired by Addy Osmani's MIT-licensed code-review-and-quality skill: https://github.com/addyosmani/agent-skills/tree/main/skills/code-review-and-quality