# Code Review > Complete workflow for reviewing patches/commits, documenting findings, and formatting results - **Type:** Skill - **Install:** `agentstack add skill-pvalena-claude-skills-review` - **Verified:** Yes — security-reviewed for prompt injection and unsafe behavior - **Seller:** [pvalena](https://agentstack.voostack.com/s/pvalena) - **Installs:** 0 - **Category:** [Agent Skills](https://agentstack.voostack.com/c/agent-skills) - **Latest version:** 0.1.0 - **License:** MIT - **Upstream author:** [pvalena](https://github.com/pvalena) - **Source:** https://github.com/pvalena/claude-skills/tree/main/review ## Install ```sh agentstack add skill-pvalena-claude-skills-review ``` Requires the [AgentStack CLI](https://agentstack.voostack.com/docs/cli). Works with Claude Code, Cursor, and any MCP-compatible agent. ## About # 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 ```bash 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 ```bash 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 ```bash 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: ```bash 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:** ```markdown # 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: ```bash # 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:** ```markdown ## 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' ``` 2. Confirm the bug exists exactly as described 3. 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`. 2. **Holistic fit**: AI code can be locally correct but miss build system interactions, downstream consumers, or project patterns. 3. **Comments/docs accuracy**: AI comments may contain subtle inaccuracies. Read every added comment critically. 4. **Trace execution paths**: AI code often handles the common case but misses edge cases or platform-specific assumptions. 5. **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](https://github.com/pvalena) - **Source:** [pvalena/claude-skills](https://github.com/pvalena/claude-skills) - **License:** MIT Install and usage instructions live in the source repository linked above. ## Pricing - **Free** — Free ## Security capabilities Automated source analysis of v0.1.0 — what this tool can access: - **Network access:** no - **Filesystem access:** no - **Shell / process execution:** no - **Environment & secrets:** no - **Dynamic code execution:** no *"Yes" means the capability is present in the source — more access means more to trust, not that it is unsafe.* ## Versions - **0.1.0** — security scan: passed — Imported from the upstream source. ## Links - Listing page: https://agentstack.voostack.com/l/skill-pvalena-claude-skills-review - Seller: https://agentstack.voostack.com/s/pvalena - Browse the marketplace: https://agentstack.voostack.com/browse --- Listed on AgentStack — the marketplace for AI agent skills and MCP servers. Every listing is security-reviewed. Creators keep 70%.