Code Review With Files
Overview
This skill defines a findings-first, read-only code review protocol.
Inspect the complete review boundary, then report every actionable issue supported by evidence. Prioritize correctness, security, regressions, and runtime risk over style.
When to Use
- The user asks to review source files, a diff, commit, branch, pull request, or bug fix.
- The goal is to find correctness, regression, security, performance, edge-case, or test problems.
Do not use it for pure explanation, rewriting, or style-only feedback.
Scope and Safety
Identify the target and baseline first. Inspect the entire supplied file or patch. For a commit, branch, or pull request, compare the intended base to the complete head change, not only the latest commit. Honor explicit scope and exclusions.
A change finding must be introduced, worsened, or made newly reachable by the review target. Do not misattribute pre-existing defects; report only an immediate critical one separately as a Pre-existing Risk.
Review is read-only unless changes are separately requested:
- Do not edit, stage, unstage, reset, or regroup files.
- Run only safe, targeted checks that materially improve confidence.
- Inspect the full requested boundary rather than stopping at the first issue.
Read callers, contracts, types, schemas, configuration, lifecycle code, and the original bug path only when a conclusion depends on them. Read relevant existing tests early enough to establish intent, but reconcile them with contracts and callers rather than treating them as automatic truth.
If critical context is unavailable, state what is missing and what cannot be confirmed. Report only supported conclusions.
Review Workflow
- Define the boundary: changed behavior, entry points, callers, data structures, contracts, and tests.
- Establish intended behavior from code, types, documentation, callers, and relevant existing tests.
- Walk execution paths: inputs, state transitions, errors, returns, side effects, cleanup, and rollback.
- Check cross-cutting risks: security, concurrency, resources, performance, compatibility, and observability.
- Evaluate test coverage after understanding the implementation: happy path, failure path, edge cases, and regression proof.
- Complete the full boundary pass. Consolidate repeated manifestations of one root cause instead of duplicating findings.
Do not perform a mechanical line-by-line nitpick pass. Focus on behavior that can actually break.
Subjective style is not a finding unless the user explicitly requests style feedback or it creates a concrete maintenance risk.
Finding Threshold
A finding must be:
- Attributable: caused, worsened, or exposed by the target
- Verifiable: identifies a trigger, violated contract, call path, or failure
- Consequential: explains the user, system, security, or maintenance impact
- Actionable: gives a realistic fix direction
- Atomic: covers one root cause; combine duplicate instances with the same fix
Resolve assumptions from available context before reporting. If missing evidence can materially change the judgment, use an open question instead of inventing a finding.
Severity and Merge Readiness
Use impact, likelihood, blast radius, and recoverability. Defect type alone never determines severity: crashes, security issues, and performance problems can occur at any level.
| Label |
When to use it |
| critical |
Reachable data loss, corruption, broad outage, exploitable boundary failure, or severe unrecoverable results |
| high |
Likely regression or security failure on a supported/common path with important impact |
| medium |
Conditional or limited incorrect behavior, material maintenance risk, weak regression proof, or bounded degradation |
| low |
Localized low-impact issue or optional polish with a concrete benefit |
Unless project policy says otherwise, critical and high findings block merge; medium and low findings do not. Do not raise severity merely because evidence is uncertain or lower it because the fix is easy.
Output Contract
Use a findings-first structure by default unless the user explicitly asks for another format:
**Findings**
1. [severity] [path:line] Conclusion
Trigger/evidence and impact
Fix direction
2. ...
**Pre-existing Risks** (optional)
**Open Questions** (optional)
**Summary**
Overall risk, merge readiness, tests run, and testing gaps
Requirements:
- Sort findings by severity, then by impact and confidence within the same severity
- Include a location, trigger/evidence, impact, and fix direction; use locating context if no exact line is available
- Suggest rather than implement fixes unless the user asks
- Omit empty Pre-existing Risks and Open Questions sections
- Use the language of the most recent review request
- With zero findings, say No actionable findings found and note residual risk or testing gaps
- With only non-blocking findings, say No blocking findings only in the summary
Example:
1. [high] `foo/bar.ts:87`
This path updates local state to success even when `save()` fails, so callers can observe "saved" while persistence never completed.
That turns a retriable failure into silent data inconsistency.
Commit state only after persistence succeeds, or roll back and surface the error on the failure path.
Common Mistakes
- Reporting an unchanged defect as if the reviewed change introduced it
- Assigning severity from labels such as "crash" without calibrating realistic impact
- Finalizing before reading context or tests on which the conclusion depends
- Stopping after the first finding or duplicating one root cause across several findings
Quick Heuristics
- For bug fixes, check whether the patch fixes only one symptom while leaving sibling paths broken
- For new flags or branches, enumerate meaningful combinations and preserved invariants
- For cache, retry, concurrency, or async callback code, check ordering and idempotency
- For parsing, SQL, HTML, filesystem, or permission logic, check security boundaries first
- For schema, DTO, or interface field changes, check compatibility and default behavior
Final Check Before Sending
Before sending:
- Are findings first, attributable, evidence-backed, actionable, and ordered by calibrated severity?
- Was the complete boundary inspected without duplicate root causes?
- Is any context required for a conclusion still unread?
- Are tests run and testing gaps stated accurately?
- With zero findings, does the response say No actionable findings found?
1---2name: code-review-with-files3description: Use when reviewing source files, diffs, commits, pull requests, or bugfixes for correctness issues, regressions, security problems, performance risks, edge cases, or missing tests across any language or framework.4---56# Code Review With Files78## Overview910This skill defines a findings-first, read-only code review protocol.1112Inspect the complete review boundary, then report every actionable issue supported by evidence. Prioritize correctness, security, regressions, and runtime risk over style.1314## When to Use1516- The user asks to review source files, a diff, commit, branch, pull request, or bug fix.17- The goal is to find correctness, regression, security, performance, edge-case, or test problems.1819Do not use it for pure explanation, rewriting, or style-only feedback.2021## Scope and Safety2223Identify the target and baseline first. Inspect the entire supplied file or patch. For a commit, branch, or pull request, compare the intended base to the complete head change, not only the latest commit. Honor explicit scope and exclusions.2425A change finding must be introduced, worsened, or made newly reachable by the review target. Do not misattribute pre-existing defects; report only an immediate critical one separately as a **Pre-existing Risk**.2627Review is read-only unless changes are separately requested:28- Do not edit, stage, unstage, reset, or regroup files.29- Run only safe, targeted checks that materially improve confidence.30- Inspect the full requested boundary rather than stopping at the first issue.3132Read callers, contracts, types, schemas, configuration, lifecycle code, and the original bug path only when a conclusion depends on them. Read relevant existing tests early enough to establish intent, but reconcile them with contracts and callers rather than treating them as automatic truth.3334If critical context is unavailable, state what is missing and what cannot be confirmed. Report only supported conclusions.3536## Review Workflow37381. Define the boundary: changed behavior, entry points, callers, data structures, contracts, and tests.392. Establish intended behavior from code, types, documentation, callers, and relevant existing tests.403. Walk execution paths: inputs, state transitions, errors, returns, side effects, cleanup, and rollback.414. Check cross-cutting risks: security, concurrency, resources, performance, compatibility, and observability.425. Evaluate test coverage after understanding the implementation: happy path, failure path, edge cases, and regression proof.436. Complete the full boundary pass. Consolidate repeated manifestations of one root cause instead of duplicating findings.4445Do not perform a mechanical line-by-line nitpick pass. Focus on behavior that can actually break.4647Subjective style is not a finding unless the user explicitly requests style feedback or it creates a concrete maintenance risk.4849## Finding Threshold5051A finding must be:52- **Attributable:** caused, worsened, or exposed by the target53- **Verifiable:** identifies a trigger, violated contract, call path, or failure54- **Consequential:** explains the user, system, security, or maintenance impact55- **Actionable:** gives a realistic fix direction56- **Atomic:** covers one root cause; combine duplicate instances with the same fix5758Resolve assumptions from available context before reporting. If missing evidence can materially change the judgment, use an open question instead of inventing a finding.5960## Severity and Merge Readiness6162Use impact, likelihood, blast radius, and recoverability. Defect type alone never determines severity: crashes, security issues, and performance problems can occur at any level.6364| Label | When to use it |65|------|----------|66| critical | Reachable data loss, corruption, broad outage, exploitable boundary failure, or severe unrecoverable results |67| high | Likely regression or security failure on a supported/common path with important impact |68| medium | Conditional or limited incorrect behavior, material maintenance risk, weak regression proof, or bounded degradation |69| low | Localized low-impact issue or optional polish with a concrete benefit |7071Unless project policy says otherwise, critical and high findings block merge; medium and low findings do not. Do not raise severity merely because evidence is uncertain or lower it because the fix is easy.7273## Output Contract7475Use a findings-first structure by default unless the user explicitly asks for another format:7677```markdown78**Findings**791. [severity] [path:line] Conclusion80 Trigger/evidence and impact81 Fix direction82832. ...8485**Pre-existing Risks** (optional)8687**Open Questions** (optional)8889**Summary**90Overall risk, merge readiness, tests run, and testing gaps91```9293Requirements:94- Sort findings by severity, then by impact and confidence within the same severity95- Include a location, trigger/evidence, impact, and fix direction; use locating context if no exact line is available96- Suggest rather than implement fixes unless the user asks97- Omit empty **Pre-existing Risks** and **Open Questions** sections98- Use the language of the most recent review request99- With zero findings, say **No actionable findings found** and note residual risk or testing gaps100- With only non-blocking findings, say **No blocking findings** only in the summary101102Example:103104```markdown1051. [high] `foo/bar.ts:87`106 This path updates local state to success even when `save()` fails, so callers can observe "saved" while persistence never completed.107 That turns a retriable failure into silent data inconsistency.108 Commit state only after persistence succeeds, or roll back and surface the error on the failure path.109```110111## Common Mistakes112113- Reporting an unchanged defect as if the reviewed change introduced it114- Assigning severity from labels such as "crash" without calibrating realistic impact115- Finalizing before reading context or tests on which the conclusion depends116- Stopping after the first finding or duplicating one root cause across several findings117118## Quick Heuristics119120- For bug fixes, check whether the patch fixes only one symptom while leaving sibling paths broken121- For new flags or branches, enumerate meaningful combinations and preserved invariants122- For cache, retry, concurrency, or async callback code, check ordering and idempotency123- For parsing, SQL, HTML, filesystem, or permission logic, check security boundaries first124- For schema, DTO, or interface field changes, check compatibility and default behavior125126## Final Check Before Sending127128Before sending:129- Are findings first, attributable, evidence-backed, actionable, and ordered by calibrated severity?130- Was the complete boundary inspected without duplicate root causes?131- Is any context required for a conclusion still unread?132- Are tests run and testing gaps stated accurately?133- With zero findings, does the response say **No actionable findings found**?
HeathWang