code-review

Code Review

Purpose

This skill drives the Code Review agent through a structured, multi-dimensional review of a pull request. It goes beyond surface-level linting — it validates the PR against the original requirements, checks for OWASP security issues, evaluates performance characteristics, verifies test quality, and assesses code design. The review is published as a formal GitHub PR review with inline comments.

Inspired by Google's Engineering Practices and the OWASP Code Review Guide.

When to Use

• A PR is ready for review and the user asks "review PR #N"
• The user wants a pre-merge quality gate before approving
• After the Code Issue agent creates a PR and before the user merges
• The user asks to "check if PR #N meets the requirements for issue #M"
• As a handoff target from the Code Issue agent's delivery step

Agent

Execute with the Code Review agent (code-review.agent.md), which has the same tool access as Code Issue but operates with a reviewer mindset — read-heavy, comment-focused, doesn't modify code unless asked.

Workflow Overview

┌───────────────────────────────────────────────────────┐
│  1. ORIENT — Read PR, linked issues, and docs/        │
├───────────────────────────────────────────────────────┤
│  1b. SCANNER — Fetch CodeQL / GHAS bot comments       │
├───────────────────────────────────────────────────────┤
│  1c. PRIOR REVIEWS — Analyze human & Copilot comments │
├───────────────────────────────────────────────────────┤
│  1d. CI STATUS — Verify pipeline green (all failures  │
│      block, including pre-existing)                   │
├───────────────────────────────────────────────────────┤
│  2. REQUIREMENTS — Validate completeness against spec │
├───────────────────────────────────────────────────────┤
│  3. DESIGN — Evaluate architecture and design choices │
├───────────────────────────────────────────────────────┤
│  4. SECURITY — OWASP Top 10 + CVE scan + scanner xref │
├───────────────────────────────────────────────────────┤
│  5. QUALITY — Code quality, complexity, naming, style │
├───────────────────────────────────────────────────────┤
│  6. PERFORMANCE — Identify bottlenecks and anti-pats  │
├───────────────────────────────────────────────────────┤
│  7. TESTS — Coverage, quality, edge cases             │
├───────────────────────────────────────────────────────┤
│  8. DOCUMENTATION — Inline docs, README, changelogs   │
├───────────────────────────────────────────────────────┤
│  9. PUBLISH — Submit GitHub review with verdict       │
├───────────────────────────────────────────────────────┤
│  10. HANDOFF — Offer to fix issues via Code Issue     │
└───────────────────────────────────────────────────────┘

Detailed Steps

Step 1: Orient

Goal: Build a complete mental model before reviewing any code.

1. Read the PR using mcp_github_pull_request_read:
   • Title, description, branch, base branch
   • Extract Closes #N / Fixes #N references
   • Note the author and any previous reviews
2. Read the linked issue(s)/epic using mcp_github_issue_read:
   • Acceptance criteria and definition of done
   • Sub-issue scope and boundaries
   • Labels (bug, feature, security, etc.)
3. Scan docs/ folder — read any relevant specs, architecture docs, or user guides in the docs/ directory
4. Fetch the diff:

   gh pr diff {PR_NUMBER}
   

5. List changed files to understand the scope:

   gh pr view {PR_NUMBER} --json files --jq '.files[].path'
   

6. Read each changed file in full — not just the diff hunks. Context matters.
7. Impact analysis via graphify (if available). If graphify-out/graph.json exists, run graphify query "<symbol_or_path>" graphify-out/graph.json for each significantly changed module and graphify path <changed_file> <suspected_dependent> to surface non-obvious callers/importers. Flag any caller in the PR description that the diff didn't update — these are common review-blocking regressions. Skip if no graph is present (it's opt-in dev tooling, not a hard requirement).

Step 1b: Security Scanner Comments (CodeQL / GHAS)

Goal: Collect all findings from automated security scanners so they can be cross-referenced during the security review and tracked in the final verdict.

Note: In this repository, the CodeQL workflow (codeql.yml) only triggers on pushes to main and a weekly cron schedule — it does not run on pull requests. This means github-advanced-security bot comments will typically not exist on PRs. If no scanner comments are found, skip to Step 1c. Do NOT wait for or block on CodeQL results that will never arrive. Your manual OWASP review in Step 4 serves as the primary security gate for PRs.

GitHub Advanced Security (GHAS) runs CodeQL analysis on PRs and posts review comments from the github-advanced-security bot. These comments identify real vulnerabilities (injection, path traversal, missing rate limiting, XSS, etc.) that the human/agent reviewer must acknowledge.

Procedure:

1. Fetch all review comments on the PR:

   mcp_github_pull_request_read  method=get_review_comments  perPage=100
   

2. Filter for scanner bot comments — identify comments where author is github-advanced-security (CodeQL), dependabot, snyk, or any other known security bot. These are distinct from human reviewer comments.

3. Classify each scanner finding:

   Status	Meaning
   is_outdated: true	The code was changed after the comment — finding MAY be resolved. Read the current file at the flagged location to verify.
   is_outdated: false	The code has NOT changed since the comment — finding is likely still present.
   is_resolved: true	Explicitly resolved by a reviewer — can be skipped.

4. Build a scanner findings table for use in Step 4 and Step 9:

   | # | Bot | File:Line | Finding | Severity | Outdated? | Verified? |
   |---|-----|-----------|---------|----------|-----------|----------|
   | 1 | CodeQL | src/api/m365.ts:151 | Missing rate limiting | High | No | Pending |
   | 2 | CodeQL | src/m365/file-parser.ts:132 | Incomplete string escaping | Medium | No | Pending |
   

5. For each non-outdated, non-resolved finding, read the current file at the flagged line to understand the context. These are your pre-seeded security findings for Step 4.

6. For each outdated finding, still verify by reading the current code — "outdated" means the diff changed, NOT that the issue was fixed. A refactor might move the vulnerability rather than fix it.

Important: Scanner findings are authoritative signals. A CodeQL High/Critical that remains unaddressed after your review is a blocking issue that must appear in your verdict, even if your own manual analysis didn't independently flag it.

Step 1c: Existing Reviewer Comments (Human & Copilot)

Goal: Identify and analyze all prior review comments from human reviewers, GitHub Copilot, or any other non-scanner reviewer so their feedback is incorporated into the review verdict.

Other people or automated reviewers (e.g., GitHub Copilot code review) may have already left comments on the PR. These comments represent prior review work that should not be ignored or duplicated.

Procedure:

1. From the same review comments fetched in Step 1b, filter for all comments where author is NOT a known scanner bot (github-advanced-security, dependabot, snyk, sonarcloud) and is NOT the PR author themselves.

2. Identify the reviewer type for each comment:

   Author Pattern	Type	Trust Level
   copilot / github-copilot / copilot-pull-request-reviewer	GitHub Copilot automated review	Medium — good at patterns, may miss context
   Any human username	Human reviewer	High — understands business context
   Other bots (e.g., codecov, sonarcloud)	Quality/coverage bots	Medium — data-driven, verify claims

3. Classify each existing comment:

   Status	Meaning
   is_outdated: true + code changed	May be resolved — verify by reading current code
   is_outdated: false	Comment likely still applies
   is_resolved: true	Explicitly resolved — note but don't re-open unless the fix is wrong
   Thread has replies	Read the full thread to understand the discussion before judging

4. Build an existing comments table for tracking:

   | # | Reviewer | Type | File:Line | Comment Summary | Outdated? | Status |
   |---|----------|------|-----------|-----------------|-----------|--------|
   | 1 | mgcronin | Human | src/api/m365.ts:102 | Missing rate limiting on /cleanup | No | Unresolved |
   | 2 | copilot | Copilot | ui/lib/api.ts:42 | Unused import | Yes | Verify |
   

5. For each unresolved, non-outdated comment:

   • Read the current code at the flagged location
   • Determine if the comment is valid, already addressed, or a false concern
   • If valid and unaddressed: include it in your review findings (do NOT duplicate it as a new inline comment — reference the existing thread instead)
   • If addressed by subsequent commits: note it as resolved in your review summary

6. For outdated comments with replies:

   • Read the full thread to understand the discussion
   • Check if the code change that made it "outdated" actually addressed the concern
   • If the concern persists despite the code change, flag it as still-open

7. Cross-reference with your own findings:

   • If a prior reviewer already flagged something you also found, reference their comment rather than duplicating
   • If a prior reviewer flagged something you disagree with, explain your reasoning
   • If a prior re

---

Content truncated.