Code Review

Code Review Skill

Purpose: Complete workflow for reviewing code changes (patches, commits, merge requests), documenting findings with technical precision, assessing correctness by reading source code, drafting fixes, and producing deep technical reasoning.

When to Use This Skill

• Reviewing patches, commits, or merge requests for code quality and correctness
• Need to document code review findings in a structured format
• Creating review files, reasoning files, and draft fix patches
• Verifying existing reviews for false positives or missed issues

Core Principles

Zero false positives: A single false positive destroys credibility. Every reported bug MUST be verified by reading the actual source code, not just diffs.

Completeness is mandatory: Missing commits means missing bugs. Always verify commit count matches what was reviewed.

Evidence-based reviews: Never report a bug you haven't seen in the actual code. Diffs can be misleading -- always read the full function context using git show BRANCH:path/to/file.

Draft fixes where straightforward: When a fix is obvious and localized, include a diff patch in the review. When it's not, explain why -- that's equally valuable.

Honest claims: Say only what you actually did. Reading code is not "verifying." Comparing against training knowledge is not "checking the spec." Use precise language: "read the source and found no issues", "traced the logic", "looks consistent with existing in-tree code" -- not "verified correct" or "confirmed safe." Reserve "verified" for concrete actions like checking a commit exists with git log --grep, or confirming a file is present with git show. If you assessed something by reading and reasoning (which is what code review is), say that.

Deep reasoning: Reasoning files should walk through the discovery and analysis step by step, so a reader can independently reproduce the analysis.

Depth scales with complexity: MRs touching low-level memory management (page tables, MMU), networking state machines (TCP, connection lifecycle), cryptographic or security-critical code, or inline assembly require deeper analysis than routine changes. For these, trace through concrete examples (e.g., compute page counts for specific memory sizes), verify edge cases, check callback ordering and reentrancy safety, and confirm state machine transitions. Report the deeper analysis even when no issues are found -- the thoroughness is the value.

---

Complete Workflow

Phase 0: Sanity Check

Goal: Verify the patch is not malicious before processing its content.

IMPORTANT: Do NOT run git log, git diff, or any command that returns raw branch content before Phase 0 completes. Phase 1's commands (listing commits, reading messages, reading diffs) come AFTER the sanity check passes.

Run the sanity-check skill against the branch. See that skill for the complete 3-step procedure (automated scan, manual inspection, evaluation).

If REJECT: stop immediately -- do not proceed to Phase 1.

If SUSPICIOUS: note the flags and apply extra scrutiny during Phase 1.

Phase 1: Perform Code Review

Goal: Examine all commits, read actual source code, identify real bugs.

1. List and Count Commits

git log --oneline origin/master..BRANCH
git log --oneline origin/master..BRANCH | wc -l

Record the exact count -- you must review every commit and list each one in the review file.

2. Read Commit Messages

git log origin/master..BRANCH --format=full

Check commit messages for:

• Factual accuracy: Do referenced commits (e.g., "Fixes commit abc123") actually exist?

Verify with git log --oneline --all --grep="abc123".

• Correctness of claims: Does the commit message accurately describe what the code does?

Cross-reference each claim against the actual diff.

• AI-assisted flag: Look for Assisted-by:, Co-authored-by:, or similar tags indicating

AI-generated code (e.g., github-copilot, claude, chatgpt). If present, apply heightened scrutiny (see Phase 2: AI-Generated Code Verification).

3. Read the Full Diff

git diff origin/master..BRANCH

Identify files changed, scope of modifications, and areas requiring deeper inspection.

4. Read Actual Source Code

This is the critical step. For every file changed, read the actual source at the branch:

git show BRANCH:path/to/file.c
git show BRANCH:path/to/file.c | sed -n '80,120p'   # specific lines

Do NOT rely on diffs alone. Diffs hide context: cleanup code after the hunk, NULL checks earlier in the function, related code in the same file.

5. What to Look For

Critical issues:

• Memory management: leaks, double-free, use-after-free, dangling pointers
• NULL pointer dereferences: missing NULL checks before use
• Resource leaks: FILE streams, file descriptors, allocations not freed on all paths
• Buffer overflows: array bounds, string operations, integer overflow in size calculations
• Logic errors: off-by-one, incorrect conditions, wrong operators, dead code
• Error handling: unchecked return values, conflated error/success returns
• Type mismatches: byte count vs element count, wrong enum, incorrect casts
• Documentation/code mismatches: docs say one thing, code does another

For non-C changes, also check:

• CI/YAML: incorrect job classifications or stage references, broken include paths,

wrong rule conditions, tests moved between categories without updating type

• Shell scripts: unquoted variables, missing error handling with set -e, cleanup

paths that skip files, incorrect guards (test -n vs test -z)

• Drivers/hardware: device lifecycle (init must be fully reversible on failure),

DMA safety (device must be stopped before freeing buffers it can write to), ring buffer management (descriptor reuse, avail/used ring updates), platform-dependent behavior (check actual implementations, not assumptions)

What NOT to report:

• Style preferences (naming, formatting, comment style)
• Optimization suggestions (unless correctness is affected)
• Alternative implementations (unless current is demonstrably wrong)
• Theoretical issues without concrete impact
• Pre-existing bugs on the base branch not introduced by this patch

6. Create Review File

File naming: reviews/IDENTIFIER.md (e.g., pr89.md, 2025-05-0103.md)

Format principles:

• Be brief; do not repeat the same information. No "intro - content - conclusion" structure.
• Short intro is fine (commit count, brief scope, commit hashes for reference).
• The main content is the issues themselves. Each issue should explain the bug, show

relevant code, and include a draft fix (or explain why one isn't included).

• Keep issue descriptions concise: state the bug, its consequence, and the fix. The full

analysis trail (macro expansions, implementation internals, caller tracing) belongs in the reasoning file -- do not duplicate it in the review.

• Do NOT include severity labels on issues.
• Do NOT include a "Review Result" summary section -- it just repeats the issues.
• End with a link to the reasoning file (when issues were found).

Structure:

# AI Review: MR !XX - Brief Title

N commit(s) [brief description of what the change does].

**Commits:**
1. **hash** - Commit message
2. **hash** - Commit message

## Issues Found

### Issue 1: Short issue title

**Location:** `path/to/file.c`, `function_name()`, lines ~N-M

[Technical description: what the code does, what's wrong, what the
consequence is. Include a code snippet if it clarifies. Include a
draft fix diff if the fix is straightforward.]

---

### Issue 2: Next issue...

For more details: [URL to reasoning file]

The "For more details" link should point to the reasoning file in the project's hosted repository (e.g., GitHub, GitLab). Derive the URL from the project's remote:

# Get the repository URL
remote_url=$(git remote get-url origin | sed 's/\.git$//' | sed 's|git@github.com:|https://github.com/|')
# For GitHub: ${remote_url}/blob/main/reviews/IDENTIFIER_reasoning.txt
# For GitLab: ${remote_url}/-/blob/main/reviews/IDENTIFIER_reasoning.txt

Only include this link when issues were found (i.e., when a reasoning file exists).

If no issues found:

## Issues Found

No issues found.

## Additional findings

[Brief, crucial observations only.]

For more details, see
[IDENTIFIER_investigation.txt](URL).

After "No issues found", add an "## Additional findings" section with the most important observations only -- things a reviewer must know at a glance (e.g., which CVEs are fixed, what the key GRUB-specific patch does, why a seemingly-suspicious pattern is actually correct).

For large/complex clean reviews (library imports, multi-file refactors, crypto code, or any MR where significant analysis was needed to conclude "no issues"), create an investigation file (reviews/IDENTIFIER_investigation.txt) and link it from the review with "For more details, see [IDENTIFIER_investigation.txt](URL)." The investigation file documents what was checked and why nothing was found -- it serves as proof of thoroughness, analogous to a reasoning file for reviews with issues. Move all detailed analysis (per-file traces, edge case verification, regex behavior analysis, API contract verification) to the investigation file. The review's "Additional findings" section should then be 3-6 lines covering only the crucial points.

For small/simple clean reviews (1-3 commits, single file, routine changes), an investigation file is not needed. Keep the "Additional findings" section brief (a few sentences of non-obvious observations).

Do NOT restate process facts ("read the full source", "commit messages match") -- those are taken for granted. Do NOT add per-commit paragraphs repeating what commit messages already say.

Use honest language about what you did:

• GOOD: "The pre-existing write_cr0 bug is still present -- MR !140

addresses it separately. This MR's changes don't interact with it."

• GOOD: "The guard prevents a concrete corruption scenario: without it,

a second enable call would overwrite the saved register values."

• BAD: "Verified correct." "Confirmed safe." "All paths validated."
• BAD: "Verified against the TPM 2.0 spec." (unless you actually fetched the spec)

You read code and applied judgment. That is valuable but it is not the same as compiling, running tests, or looking up spec documents. Do not claim otherwise.

7. Create Reasoning File (Only If Issues Found)

File naming: reviews/IDENTIFIER_reasoning.txt

Do NOT create reasoning files for clean reviews.

Purpose: The reasoning file is the full verification trail. Another AI (or human) reading it should be able to reproduce the entire analysis independently, without access to the original conversation. It must contain enough detail that every conclusion is independently verifiable.

Header: Start with a one-line summary (PR number, title, commit count), then list all source files read during review.

Structure -- for each issue, use a == Issue N: title == header and include:

== Issue N: Short title (file.c) ==

Discovery:
What specific code or pattern drew attention. Which file, which line,
what looked suspicious at first glance and why.

Source trace:
Which functions/files were traced and what was found. For example, if
a function returns a malloc'd pointer, trace the function to confirm
the allocation. If an API has specific semantics (e.g., grub_strtol
never sets endp to NULL), trace into the implementation and cite the
specific lines that prove it. Reference concrete line numbers.

Consequence:
What actually goes wrong in practice. Be specific -- not "could leak"
but "leaks one FILE stream and one fd per call, exhausting fd table
after N invocations." Include what the user would see if applicable
(error messages, silent failures, data corruption).

Fix assessment:
Why the suggested fix looks correct based on reading the code. Check
that it doesn't introduce new issues (e.g., doesn't mask errors from
subsequent operations, doesn't use-after-free, scoping is appropriate
for the language standard). Note any caveats (e.g., "fix requires
restructuring surrounding code, so no draft diff included").

When the issue involves platform-dependent behavior, verify which platforms are affected (e.g., check Makefile.core.def for module enable flags). When the issue involves API semantics, trace into the implementation and cite specific lines. When the issue involves header conflicts, verify both files exist and use the same guard.

For AI-assisted code, add an "AI-assistance scrutiny" paragraph noting what the AI likely got right/wrong and why (e.g., "handled local control flow correctly but missed the global grub_errno contract").

---

Phase 2: Verify Findings

Goal: Re-read actual source code to confirm every reported issue is real, and check for issues that were missed.

This is a separate pass from Phase 1. After writing the initial review, go back and independently verify each finding.

For Each Reported Issue

1. Read the actual source file at the relevant lines:

``bash git show BRANCH:path/to/file.c | sed -n 'START,ENDp' ``

1. Confirm the bug exists exactly as described
2. Check for context that might invalidate the finding:

• Is there a NULL check earlier in the function?
• Is the pointer set to NULL after the free?
• Does the API guarantee something that makes this safe?
• Is there cleanup code outside the visible diff?

AI-Generated Code Verification

When commit messages contain Assisted-by:, Co-authored-by: with an AI tool name, apply additional scrutiny:

1. Verify commit message claims: AI messages may reference

commits/functions that don't exist. Check with git log --grep.

1. Holistic fit: AI code can be locally correct but miss build

system interactions, downstream consumers, or project patterns.

1. Comments/docs accuracy: AI comments may contain subtle

inaccuracies. Read every added comment critically.

1. Trace execution paths: AI code often handles the common case

but misses edge cases or platform-specific assumptions.

1. Don't assume from plausibility: Verify in actual source, not

by reading the diff and nodding along.

Note AI-assistance in the review intro when detected. Do not penalize code that looks correct for being AI-assisted.

Agent-Delegated Reviews

When reviews are produced by spawned agents, treat every finding as a draft. Agents make claims from training knowledge that read as source code findings. Three claim types need scrutiny:

• Spec compliance: "Per the virtio 1.0 spec..." — agent recalled

training data, did not fetch the spec. Confirm via in-tree code or soften language.

• Platform behavior: "On i386_ieee1275, this function does X" —

read the actual implementation. (PR133: claimed cleanup was a no-op.)

• API contracts: "This function never returns NULL" — check if

the reasoning cites actual source lines or just asserts.

Process: For each agent finding, identify claims about behavior outside the changed files. If the agent cites source lines, spot-check. If it just asserts, confirm yourself or drop the finding. Never pass through an agent's claim as your own.

For Clean Reviews

Re-read the diff and source code looking for anything missed:

• Trace all error paths for resource leaks
• Check all pointer dereferences for NULL safety
• Check return value semantics match caller expectations
• Look for documentation/code mismatches

For complex domains (page table math, TCP state machines, crypto/security, inline assembly), go beyond a clean pass: trace concrete examples through the logic (e.g., specific memory sizes through page calculations, specific packet sequences through connection state), verify callback ordering, a

…

Source & license

This open-source skill is cataloged on AgentStack and links to its original source — we do not rehost the code.

• Author: pvalena
• Source: pvalena/claude-skills
• License: MIT

Install and usage instructions live in the source repository linked above.