Code Review

Code Review

Use this skill when the main question is "is this specific change ready, what evidence do we trust, and what should a reviewer actually say?"

The job is not to dump a giant clean-code checklist. The job is to: 1. normalize the review packet, 2. choose the right review mode, 3. inspect the highest-risk behavior first, 4. separate missing evidence from proven defects, 5. classify findings by severity, 6. route non-review work out immediately.

Read references/intake-packets-and-escalations.md before handling an unfamiliar review packet. Read references/review-modes.md for deeper heuristics by change type. Read references/handoff-boundaries.md when deciding whether code-review, git-workflow, debugging, testing-strategies, web-design-guidelines, web-accessibility, or repo/PR workflow skills should own the next step.

When to use this skill

• Reviewing a PR, MR, local diff, patch stack, or self-review packet before merge
• Deciding what reviewer comments matter most and how severe they are
• Checking a change for correctness, security, migration/rollout risk, maintainability, and missing validation evidence
• Writing a concise approve / request-changes / block / route-out review brief
• Reviewing backend, frontend, CLI, fullstack, or game-programming changes where the core task is judgment on the change rather than implementation

When not to use this skill

• The real task is splitting commits, rebasing, conflict resolution, or push recovery → use git-workflow
• The real task is reproducing or isolating a live failure → use debugging
• The real task is choosing long-term coverage shape, CI gates, or flaky-suite policy → use testing-strategies
• The real task is pure design, accessibility, or visual-governance critique → use web-design-guidelines or web-accessibility
• The real task is reviewer assignment, CODEOWNERS interpretation, labels, merge queue, or repo settings → use a repo / PR workflow skill
• The real task is measurement-led bottleneck analysis or tuning → use performance-optimization

Instructions

Step 1: Normalize the review packet

Start from the evidence already present instead of asking for an idealized packet.

Capture:

• review surface: PR / MR / local diff / patch stack / self-review
• goal of the change
• hotspots: API, UI, schema, auth, config, build/release, game runtime, tooling, unknown
• packet shape: diff only | diff + tests | schema/auth rollout notes | screenshots/preview | CI bot findings | game/runtime validation notes | mixed
• obvious evidence present or missing

Minimum frame:

Review surface: PR
Goal: add coupon support to checkout
Hotspots: discount logic, schema migration, auth edge cases
Packet: diff + tests, no rollout notes

If the packet is still mostly branch hygiene or repo-admin work, route out before pretending review has started.

Step 2: Choose one primary review mode

Pick one primary mode from references/review-modes.md:

• general change review
• backend / platform review
• frontend / UX-adjacent review
• game-programming / engine review
• policy / meta review

Rule: one primary mode, optional secondary mode. Do not flatten every diff into the same checklist.

Step 3: Inspect the highest-risk path first

Prioritize in this order: 1. broken correctness or edge-case handling 2. security / privacy / trust-boundary mistakes 3. schema, migration, config, rollout, or compatibility risk 4. missing or misleading tests / screenshots / previews / rollout proof 5. maintainability problems that will slow future work 6. style and readability nits

High-value questions:

• Can the change behave incorrectly even if current tests are green?
• Did a trust boundary, permission rule, secret path, or user-controlled input change?
• Did the change alter schemas, contracts, jobs, rollout behavior, or game/runtime state without enough safeguards?
• Is the packet missing the one artifact needed to judge the risky path honestly?

Step 4: Separate findings from missing evidence

A review can fail because the code is wrong or because the packet is not convincing enough.

Evidence sources to check:

• the diff itself
• nearby code paths and existing invariants
• tests and fixtures
• schema / contract / migration notes
• screenshots, recordings, or preview links for behavior/layout-sensitive frontend work
• rollout notes, config changes, and CI bot findings
• playtest or engine-validation notes for game/runtime work

Good finding shape:

[Blocker] The new API still trusts the client-provided discount amount. Recompute discount server-side and add a regression test for mismatched input.

Good missing-evidence shape:

[Major] The diff changes responsive navigation states, but the packet has no screenshots or preview link for mobile/tablet open-close behavior.

Step 5: Classify severity and route-outs

Use a small, explicit severity model.

• Blocker — merge should not proceed: correctness break, security issue, data loss, broken migration, or clearly missing validation for a risky path
• Major — important but fixable in the current review round: missing tests/evidence for a core path, incomplete rollout/migration story, or a high-maintenance design choice
• Minor — readability, naming, local cleanup, optional simplification
• Route-out — the concern is real, but another skill owns the next step

Typical route-outs:

• commit cleanup / rebase / push safety → git-workflow
• reproduce and isolate live failure → debugging
• broader coverage policy or flaky-suite direction → testing-strategies
• visual/accessibility/product polish review → web-design-guidelines or web-accessibility
• reviewer assignment, CODEOWNERS, branch rules, merge queue, PR operations → repo / PR workflow skill

Step 6: Produce a reviewer-grade decision brief

Preferred shape:

# Code Review Brief

## Decision
- Approve | Request changes | Block pending investigation | Needs follow-up from another skill

## Review frame
- Surface:
- Goal:
- Primary mode:
- Packet:

## Key findings
1. [Severity] ...
2. [Severity] ...
3. [Route-out] ...

## Missing evidence
- ...

## Recommended next step
- merge
- patch specific issues
- collect one missing artifact
- split the diff
- route next to another skill

If approving, say why the change looks safe:

• risky areas reviewed
• evidence that exists
• residual concerns, if any

Step 7: Escalate confidence honestly

• If the diff is too large, say review confidence is limited and focus on the highest-risk slice.
• If frontend or marketing-site behavior depends on rendering states, ask for preview evidence instead of bluffing.
• If backend or rollout risk is high, demand migration/config/rollback proof before approval.
• If game/runtime behavior still needs playtest or engine validation, state that clearly.
• If bot findings exist (reviewdog, CI comments, static-analysis annotations), treat them as evidence inputs, not as the final review judgment.

Output format

Always return a concise review brief or review-comment set.

Required qualities:

• identify the review surface and change goal
• focus on the highest-risk findings first
• separate concrete defects from missing evidence
• choose an explicit decision
• name the correct neighboring skill when the task has shifted
• avoid generic checklist filler

Examples

Example 1: Backend PR with migration risk

Input

Review this PR that adds coupon support to the checkout API. There is a schema migration and a few tests.

Output sketch

• Decision: Request changes
• Review frame: backend / platform review, packet = diff + tests + migration
• Key findings:

1. [Blocker] discount value is still accepted from the client instead of recomputed server-side 2. [Major] migration lacks rollback/backfill notes and no compatibility test covers old rows 3. [Major] no test for invalid or expired coupon race conditions

• Recommended next step: patch validation + add migration/test evidence, then re-review

Example 2: Frontend diff that needs preview evidence

Input

Can you code-review this responsive navbar change before I merge it?

Output sketch

• Decision: Needs follow-up before approval
• Summary: implementation may be maintainable, but behavior cannot be fully judged from the diff alone
• Key findings:

1. [Major] missing mobile/tablet screenshots or preview link for menu states 2. [Minor] duplicated breakpoint logic should be centralized 3. [Route-out] accessibility or visual-polish checks should go through web-design-guidelines / web-accessibility

Example 3: Request that should route away

Input

Before review, help me split this huge branch into smaller commits and rebase it cleanly.

Output sketch

• Decision: Route out
• Summary: this is primarily a Git-structure problem, not review judgment yet
• Route: git-workflow

Example 4: Review packet with bot annotations

Input

reviewdog already commented on the lint and static-analysis issues. Can you do the final review pass?

Output sketch

• Treat the bot comments as inputs, not the full answer
• Re-check the risky behavior, missing evidence, and merge decision
• Route repo-admin follow-up elsewhere if the request shifts into PR operations

Best practices

1. Review the highest-risk behavior before style or formatting. 2. Tie every serious finding to evidence from the diff, nearby code, tests, or one clearly missing artifact. 3. Distinguish missing evidence from proven bugs. 4. Use severity labels so authors know what blocks merge. 5. Keep one primary review mode instead of flattening every diff into one checklist. 6. Ask for previews/screenshots when rendered behavior matters. 7. Demand rollout or migration proof when backend/platform risk is high. 8. Treat CI bots and static-analysis comments as evidence inputs, not as the reviewer. 9. Route Git, debugging, test-policy, UI-governance, and repo-admin tasks out instead of absorbing everything. 10. If approving, say why the change looks safe — not just "LGTM".

References

• GitHub Docs — About pull request reviews
• GitHub Docs — About code owners
• GitHub Docs — About protected branches
• GitLab Docs — Merge request approvals
• reviewdog
• Danger JS

Handoff Boundaries

Use code-review when the main question is

• Is this diff or PR ready to merge?
• What reviewer comments matter most?
• Should this be approved, blocked, or sent back for changes?
• What evidence is missing before approval?

Route to git-workflow when the main question is

• how to split or reorder commits
• rebase vs merge choice
• conflict resolution mechanics
• safe push / recovery after history rewrite

git-workflow prepares a reviewable diff. code-review judges the diff once it exists.

Route to debugging when the main question is

• why a failure is happening now
• how to reproduce or isolate the issue
• what the likely root cause is
• how to verify a fix after a live bug or flaky failure

code-review is preventive. debugging is diagnostic.

Route to testing-strategies when the main question is

• what the long-term coverage model should be
• how to shape unit/integration/E2E layers
• how to reduce flaky suites or set CI policy
• what broader testing program the team should adopt

code-review can say a change needs more evidence. testing-strategies decides the broader testing shape.

Route to web-design-guidelines / web-accessibility when the main question is

• visual consistency
• accessibility compliance
• layout polish
• interaction behavior that needs UX/design critique beyond implementation risk

code-review should still note when preview evidence is missing, but it should not absorb pure design governance.

Route to repo / PR workflow skills when the main question is

• assign reviewers or labels
• interpret CODEOWNERS or branch protection
• create/edit/manage the PR itself
• merge queue, auto-merge, or repository settings

Rule of thumb

If the user is asking for judgment on a change, stay in code-review. If the user is asking for how to shape, diagnose, administer, or visualize the work around the change, route to the neighboring skill.

Intake Packets and Escalations

Start from the review packet the user already has. Do not demand a perfect PR template before doing useful work.

Diff only

Use when the packet is mostly code changes with little supporting evidence.

Look for:

• the riskiest changed path
• obvious trust-boundary or state-transition changes
• whether one missing artifact blocks confident review

Escalate when:

• the change is too large to review confidently → recommend splitting or narrowing
• the real work is branch cleanup or commit surgery → git-workflow

Diff + tests

Use when unit/integration tests are present and the main question is whether the evidence actually covers the risky path.

Look for:

• test coverage of the changed invariant
• false confidence from green tests that skip edge cases
• fixtures or mocks that hide the real risk

Escalate when:

• the conversation becomes broader coverage policy or flaky-suite design → testing-strategies

Schema / auth / rollout packet

Use when the diff changes migrations, auth behavior, config, background jobs, release flow, or compatibility boundaries.

Look for:

• rollback and backfill assumptions
• backward compatibility
• permission or secret-handling changes
• missing migration or rollout notes

Escalate when:

• a live failure already exists and root-cause diagnosis is now the main task → debugging
• the next task is wider auth architecture or API contract redesign → route after naming the review concern

Screenshots / preview / recording packet

Use when frontend, marketing-site, or product-facing interaction behavior matters.

Look for:

• mobile/tablet/desktop state coverage
• hover/focus/error/loading states
• whether the diff alone can explain the behavior
• maintainability issues even if the visuals look fine

Escalate when:

• the next task is pure design governance → web-design-guidelines
• the next task is accessibility remediation → web-accessibility

CI bot / reviewdog / static-analysis packet

Use when automated findings already exist in the PR.

Look for:

• whether the bot surfaced a real blocker or a noisy policy issue
• risky behavior the bot cannot judge
• missing evidence that still blocks approval even after lint/static-analysis passes

Escalate when:

• the next task is repo-admin or CI policy maintenance rather than review judgment

Game / runtime validation packet

Use when the diff affects gameplay, engine integration, runtime state, or asset-driven behavior.

Look for:

• determinism or state-sync risks
• config/data coupling
• whether code review can judge the change without playtest or engine validation
• what still needs manual validation on target hardware or in-editor/runtime

Escalate when:

• the next task is engine-specific profiler reading or runtime bottleneck analysis → game-performance-profiler
• the next task is reproducing a live bug → debugging

Repo policy / admin packet

Use when the request mixes review with reviewer assignment, CODEOWNERS, branch protection, labels, merge queue, or PR operations.

Look for:

• whether there is still a real diff-review task to complete
• whether the request should be split into review judgment plus repo/PR workflow administration

Escalate when:

• the main task is PR lifecycle or repository administration rather than review judgment

Rule of thumb

A good review packet is the smallest artifact set that lets you make one honest merge decision. If the packet cannot support that decision yet, ask for one missing artifact or route to the neighboring skill that owns the next step.

Review Modes

Use code-review as a decision skill, not a giant static checklist.

1. General change review

Use for ordinary diffs / PRs where the main question is whether the change is safe and maintainable.

Primary checks:

• correctness of the changed behavior
• edge cases and failure paths
• maintainability of the implementation
• missing validation evidence

2. Backend / platform review

Reach for this mode when the diff changes APIs, jobs, schemas, permissions, or deployment behavior.

Look especially at:

• backward compatibility
• schema migration safety
• authz/authn or trust-boundary changes
• rollback assumptions
• missing tests for failure paths and edge cases

3. Frontend / UX-adjacent review

Use when the diff changes interaction states, responsive behavior, forms, or UI rendering.

Primary rule:

• code review can judge implementation quality
• screenshots, previews, or recordings may still be required to judge behavior/layout

Ask for additional evidence when the diff alone cannot show:

• hover/focus/error states
• mobile vs desktop behavior
• animation or timing behavior
• before/after visual changes

4. Game-programming / engine review

Use when the code is part of gameplay, tooling, engine integration, or data-driven game logic.

Focus on:

• determinism / state sync assumptions
• config and data coupling
• gameplay side effects
• what still requires manual playtest or engine validation

Do not pretend binary assets or editor-side behavior are fully reviewable from the code diff alone.

5. Policy / meta review

Use when the highest-value review finding is not inside a single line of code.

Examples:

• change is too large to review confidently
• PR lacks clear description or evidence
• migration has no rollout notes
• screenshots or previews are missing
• tests are absent for the core risk

Core heuristic

If the most important comment is about how the change should be judged or evidenced, start there before offering line-level nits.