github-code-review

name	github-code-review
description	Review code changes by analyzing git diffs, leaving inline comments on PRs, and performing thorough pre-push review. Works with the gh CLI or falls back to git + GitHub REST API via curl. Use when: review this PR, review a pull request, look at this PR, code review, review my changes before I push, pre-push review, leave inline comments on a pull request.

GitHub Code Review

Perform code reviews on local changes before pushing, or review open PRs on GitHub. Most of this skill uses plain git — the gh/curl split only matters for PR-level interactions.

Prerequisites

• Authenticated with GitHub: run gh auth login, or export a $GITHUB_TOKEN with repo scope
• Inside a git repository

Pitfalls (don't revert these)

• Never check out a PR with a bare git checkout / gh pr checkout in a repo's main checkout. It clobbers in-progress work and destroys parallel agents' state. Always add an isolated worktree under ~/projects/_worktrees/pr-N.
• Never clean up by deleting branches or worktrees you didn't create unless the user explicitly asks. Banned: git checkout -- ., git restore, git reset --hard, git clean, git branch -D on the PR branch. Leave the worktree in place by default.
• Don't assume the default branch is main. Detect it: BASE=$(git symbolic-ref refs/remotes/origin/HEAD | sed 's@.*/@@') (some repos use master), then diff against origin/$BASE...HEAD.

Setup (for PR interactions)

if command -v gh &>/dev/null && gh auth status &>/dev/null; then
  AUTH="gh"
else
  AUTH="git"
  if [ -z "$GITHUB_TOKEN" ]; then
    # Try the gh CLI first, then fall back to a local env file
    GITHUB_TOKEN=$(gh auth token 2>/dev/null)
    if [ -z "$GITHUB_TOKEN" ] && [ -f ~/.hermes/.env ] && grep -q "^GITHUB_TOKEN=" ~/.hermes/.env; then
      GITHUB_TOKEN=$(grep "^GITHUB_TOKEN=" ~/.hermes/.env | head -1 | cut -d= -f2 | tr -d '\n\r')
    elif [ -z "$GITHUB_TOKEN" ] && grep -q "github.com" ~/.git-credentials 2>/dev/null; then
      GITHUB_TOKEN=$(grep "github.com" ~/.git-credentials 2>/dev/null | head -1 | sed 's|https://[^:]*:\([^@]*\)@.*|\1|')
    fi
  fi
fi

REMOTE_URL=$(git remote get-url origin)
OWNER_REPO=$(echo "$REMOTE_URL" | sed -E 's|.*github\.com[:/]||; s|\.git$||')
OWNER=$(echo "$OWNER_REPO" | cut -d/ -f1)
REPO=$(echo "$OWNER_REPO" | cut -d/ -f2)

---

1. Reviewing Local Changes (Pre-Push)

This is pure git — works everywhere, no API needed.

Get the Diff

Examples below say main, but some repos default to master and others vary. Detect it first and substitute: BASE=$(git symbolic-ref refs/remotes/origin/HEAD | sed 's@.*/@@'), then use origin/$BASE...HEAD.

# Staged changes (what would be committed)
git diff --staged

# All changes vs main (what a PR would contain)
git diff main...HEAD

# File names only
git diff main...HEAD --name-only

# Stat summary (insertions/deletions per file)
git diff main...HEAD --stat

Review Strategy

1. Get the big picture first:

git diff main...HEAD --stat
git log main..HEAD --oneline

2. Review file by file — use read_file on changed files for full context, and the diff to see what changed:

git diff main...HEAD -- src/auth/login.py

3. Check for common issues:

# Debug statements, TODOs, console.logs left behind
git diff main...HEAD | grep -n "print(\|console\.log\|TODO\|FIXME\|HACK\|XXX\|debugger"

# Large files accidentally staged
git diff main...HEAD --stat | sort -t'|' -k2 -rn | head -10

# Secrets or credential patterns
git diff main...HEAD | grep -in "password\|secret\|api_key\|token.*=\|private_key"

# Merge conflict markers
git diff main...HEAD | grep -n "<<<<<<\|>>>>>>\|======="

4. Present structured feedback to the user.

Review Output Format

When reviewing local changes, present findings in this structure:

## Code Review Summary

### Critical
- **src/auth.py:45** — SQL injection: user input passed directly to query.
  Suggestion: Use parameterized queries.

### Warnings
- **src/models/user.py:23** — Password stored in plaintext. Use bcrypt or argon2.
- **src/api/routes.py:112** — No rate limiting on login endpoint.

### Suggestions
- **src/utils/helpers.py:8** — Duplicates logic in `src/core/utils.py:34`. Consolidate.
- **tests/test_auth.py** — Missing edge case: expired token test.

### Looks Good
- Clean separation of concerns in the middleware layer
- Good test coverage for the happy path

---

2. Reviewing a Pull Request on GitHub

View PR Details

With gh:

gh pr view 123
gh pr diff 123
gh pr diff 123 --name-only

With git + curl:

PR_NUMBER=123

# Get PR details
curl -s \
  -H "Authorization: token $GITHUB_TOKEN" \
  https://api.github.com/repos/$OWNER/$REPO/pulls/$PR_NUMBER \
  | python3 -c "
import sys, json
pr = json.load(sys.stdin)
print(f\"Title: {pr['title']}\")
print(f\"Author: {pr['user']['login']}\")
print(f\"Branch: {pr['head']['ref']} -> {pr['base']['ref']}\")
print(f\"State: {pr['state']}\")
print(f\"Body:\n{pr['body']}\")"

# List changed files
curl -s \
  -H "Authorization: token $GITHUB_TOKEN" \
  https://api.github.com/repos/$OWNER/$REPO/pulls/$PR_NUMBER/files \
  | python3 -c "
import sys, json
for f in json.load(sys.stdin):
    print(f\"{f['status']:10} +{f['additions']:-4} -{f['deletions']:-4}  {f['filename']}\")"

Check Out PR Locally for Full Review

Always use a worktree — never git checkout in the repo's main checkout. A raw checkout clobbers any in-progress work and risks destroying parallel agents' state. Add an isolated worktree for the PR head instead:

# Fetch the PR head to FETCH_HEAD, then add/reuse a detached repo-scoped worktree
REPO_SLUG=$(gh repo view --json nameWithOwner -q '.nameWithOwner' | tr '/' '-')
WT=~/projects/_worktrees/${REPO_SLUG}-pr-123
git fetch origin pull/123/head
if git -C "$WT" rev-parse --is-inside-work-tree >/dev/null 2>&1; then
  cd "$WT"
  test -z "$(git status --short)" || { echo "Existing PR worktree has local changes; stop before updating it"; exit 1; }
  git fetch origin pull/123/head
  git switch --detach FETCH_HEAD
else
  git worktree add --detach "$WT" FETCH_HEAD
  cd "$WT"
fi

# Now you can use read_file, search_files, run tests, etc.

# View diff against the base branch (detect default branch, don't assume main)
BASE=$(git symbolic-ref refs/remotes/origin/HEAD | sed 's@.*/@@')
git diff "origin/$BASE"...HEAD

Leave the worktree in place when you're done unless the user explicitly asks you to remove it — see "Clean up" below.

Leave Comments on a PR

General PR comment — with gh:

gh pr comment 123 --body "Overall looks good, a few suggestions below."

General PR comment — with curl:

curl -s -X POST \
  -H "Authorization: token $GITHUB_TOKEN" \
  https://api.github.com/repos/$OWNER/$REPO/issues/$PR_NUMBER/comments \
  -d '{"body": "Overall looks good, a few suggestions below."}'

Leave Inline Review Comments

Single inline comment — with gh (via API):

HEAD_SHA=$(gh pr view 123 --json headRefOid --jq '.headRefOid')

gh api repos/$OWNER/$REPO/pulls/123/comments \
  --method POST \
  -f body="This could be simplified with a list comprehension." \
  -f path="src/auth/login.py" \
  -f commit_id="$HEAD_SHA" \
  -f line=45 \
  -f side="RIGHT"

Single inline comment — with curl:

# Get the head commit SHA
HEAD_SHA=$(curl -s \
  -H "Authorization: token $GITHUB_TOKEN" \
  https://api.github.com/repos/$OWNER/$REPO/pulls/$PR_NUMBER \
  | python3 -c "import sys,json; print(json.load(sys.stdin)['head']['sha'])")

curl -s -X POST \
  -H "Authorization: token $GITHUB_TOKEN" \
  https://api.github.com/repos/$OWNER/$REPO/pulls/$PR_NUMBER/comments \
  -d "{
    \"body\": \"This could be simplified with a list comprehension.\",
    \"path\": \"src/auth/login.py\",
    \"commit_id\": \"$HEAD_SHA\",
    \"line\": 45,
    \"side\": \"RIGHT\"
  }"

Submit a Formal Review (Approve / Request Changes)

With gh (top-level review text only; create inline comments separately with REST):

gh pr review 123 --approve --body "LGTM!"
gh pr review 123 --request-changes --body "Requesting changes; details are in this top-level review body or in REST-created inline comments."
gh pr review 123 --comment --body "Some suggestions, nothing blocking."

With curl — multi-comment inline review submitted atomically:

HEAD_SHA=$(curl -s \
  -H "Authorization: token $GITHUB_TOKEN" \
  https://api.github.com/repos/$OWNER/$REPO/pulls/$PR_NUMBER \
  | python3 -c "import sys,json; print(json.load(sys.stdin)['head']['sha'])")

curl -s -X POST \
  -H "Authorization: token $GITHUB_TOKEN" \
  https://api.github.com/repos/$OWNER/$REPO/pulls/$PR_NUMBER/reviews \
  -d "{
    \"commit_id\": \"$HEAD_SHA\",
    \"event\": \"COMMENT\",
    \"body\": \"Code review\",
    \"comments\": [
      {\"path\": \"src/auth.py\", \"line\": 45, \"body\": \"Use parameterized queries to prevent SQL injection.\"},
      {\"path\": \"src/models/user.py\", \"line\": 23, \"body\": \"Hash passwords with bcrypt before storing.\"},
      {\"path\": \"tests/test_auth.py\", \"line\": 1, \"body\": \"Add test for expired token edge case.\"}
    ]
  }"

Event values: "APPROVE", "REQUEST_CHANGES", "COMMENT"

The line field refers to the line number in the new version of the file. For deleted lines, use "side": "LEFT".

---

3. Review Checklist

When performing a code review (local or PR), systematically check:

Correctness

• Does the code do what it claims?
• Compare the PR's claimed scope against the exact condition in code. If the PR says "explicitly enabled" but the code checks a resolved/available state (valid_*, loaded_*, effective_*), look for silent scope widening. For this class of bug, read references/semantic-scope-regression.md.
• Edge cases handled (empty inputs, nulls, large data, concurrent access)?
• Error paths handled gracefully?

Security

• No hardcoded secrets, credentials, or API keys
• Input validation on user-facing inputs
• No SQL injection, XSS, or path traversal
• Auth/authz checks where needed

Code Quality

• Clear naming (variables, functions, classes)
• No unnecessary complexity or premature abstraction
• DRY — no duplicated logic that should be extracted
• Functions are focused (single responsibility)

Testing

• New code paths tested?
• Happy path and error cases covered?
• Tests readable and maintainable?

Regression boundary checks

For narrow bug-fix PRs, review the boundary between the claimed fix and adjacent behavior. Do not only verify the positive repro. Look for places where the implementation uses a resolved/effective state that is broader than the user's intent.

Common pattern: a fix says "when X is explicitly enabled," but the code checks "X is available after default expansion." That can accidentally widen behavior for default/all-tool paths, batch jobs, ignore-rules sessions, or delegated agents. Require at least one negative regression test for the closest adjacent path that should remain unchanged.

Example review question: "Does this condition mean explicitly requested, or merely present after defaults/plugins expanded it?"

Performance

• No N+1 queries or unnecessary loops
• Appropriate caching where beneficial
• No blocking operations in async code paths

Documentation

• Public APIs documented
• Non-obvious logic has comments explaining "why"
• README updated if behavior changed

---

4. Pre-Push Review Workflow

When the user asks you to "review the code" or "check before pushing":

1. git diff main...HEAD --stat — see scope of changes
2. git diff main...HEAD — read the full diff
3. For each changed file, use read_file if you need more context
4. Apply the checklist above
5. Present findings in the structured format (Critical / Warnings / Suggestions / Looks Good)
6. If critical issues found, offer to fix them before the user pushes

---

5. PR Review Workflow (End-to-End)

When the user asks you to "review PR #N", "look at this PR", or gives you a PR URL, follow this recipe:

Step 1: Set up environment

Run the inline setup block from the top of this skill ("Setup (for PR interactions)"). It detects the gh CLI, falls back to $GITHUB_TOKEN, and derives $OWNER/$REPO from the origin remote. ($GH_OWNER/$GH_REPO below are aliases for $OWNER/$REPO.)

If the user gives a full PR URL for a different repo than the current checkout, parse OWNER/REPO/PR_NUMBER from the URL instead of trusting origin (e.g. https://github.com/acme/widgets/pull/42 → OWNER=$(echo "$URL" | cut -d/ -f4), REPO=$(echo "$URL" | cut -d/ -f5), PR_NUMBER=$(echo "$URL" | cut -d/ -f7)).

Step 2: Gather PR context

Get the PR metadata, description, and list of changed files to understand scope before diving into code.

With gh:

gh pr view 123
gh pr diff 123 --name-only
gh pr checks 123

With curl:

PR_NUMBER=123

# PR details (title, author, description, branch)
curl -s -H "Authorization: token $GITHUB_TOKEN" \
  https://api.github.com/repos/$OWNER/$REPO/pulls/$PR_NUMBER

# Changed files with line counts
curl -s -H "Authorization: token $GITHUB_TOKEN" \
  https://api.github.com/repos/$OWNER/$REPO/pulls/$PR_NUMBER/files

Step 3: Check out the PR locally

This gives you full access to read_file, search_files, and the ability to run tests. Use a worktree, never a bare git checkout (protects in-progress work and parallel agents):

REPO_SLUG=$(gh repo view --json nameWithOwner -q '.nameWithOwner' | tr '/' '-')
WT=~/projects/_worktrees/${REPO_SLUG}-pr-$PR_NUMBER
git fetch origin pull/$PR_NUMBER/head
if git -C "$WT" rev-parse --is-inside-work-tree >/dev/null 2>&1; then
  cd "$WT"
  test -z "$(git status --short)" || { echo "Existing PR worktree has local changes; stop before updating it"; exit 1; }
  git fetch origin pull/$PR_NUMBER/head
  git switch --detach FETCH_HEAD
else
  git worktree add --detach "$WT" FETCH_HEAD
  cd "$WT"
fi

Step 4: Read the diff and understand changes

# Use the PR's actual base branch (it may target a non-default branch like release/1.2).
# For a specific PR: BASE=$(gh pr view $PR_NUMBER --json baseRefName -q '.baseRefName')
# For local pre-push review with no PR yet, detect the default branch instead:
BASE=$(gh pr view "$PR_NUMBER" --json baseRefName -q '.baseRefName' 2>/dev/null \
  || git symbolic-ref refs/remotes/origin/HEAD | sed 's@.*/@@')
git fetch origin "$BASE" --quiet

# Full diff against the PR's base branch
git diff "origin/$BASE"...HEAD

# Or file-by-file for large PRs
git diff "origin/$BASE"...HEAD --name-only
# Then for each file:
git diff "origin/$BASE"...HEAD -- path/to/file.py

For each changed file, use read_file to see full context around the changes — diffs alone can miss issues visible only with surrounding code.

Step 5: Run automated checks locally (if applicable)

# Run tests if there's a test suite
python -m pytest 2>&1 | tail -20
# or: npm test, cargo test, go test ./..., etc.

# Run linter if configured
ruff check . 2>&1 | head -30
# or: eslint, clippy, etc.

Step 6: Apply the review checklist (Section 3)

Go through each category: Correctness, Security, Code Quality, Testing, Regression boundary checks, Performance, Documentation. For bug-fix PRs, explicitly compare the PR's stated scope against the condition used in code, then ask for one negative test that proves nearby behavior did not change.

Step 7: Post the review to GitHub

Collect your findings and submit them as a formal review. If the review needs inline comments, use the REST review-comments flow below; gh pr review can only submit top-level review text.

With gh (top-level review text only; gh pr review has no path/line flags):

# If no issues — approve
gh pr review $PR_NUMBER --approve --body "Reviewed. Code looks clean — good test coverage, no security concerns."

# If issues found but you only need a top-level review summary
gh pr review $PR_NUMBER --request-changes --body "Found a few issues; details are in this top-level review body."

With curl/REST — request changes with multiple inline comments atomically:

HEAD_SHA=$(curl -s -H "Authorization: token $GITHUB_TOKEN" \
  https://api.github.com/repos/$OWNER/$REPO/pulls/$PR_NUMBER \
  | python3 -c "import sys,json; print(json.load(sys.stdin)['head']['sha'])")

# Build the review JSON — event is APPROVE, REQUEST_CHANGES, or COMMENT
curl -s -X POST \
  -H "Authorization: token $GITHUB_TOKEN" \
  https://api.github.com/repos/$OWNER/$REPO/pulls/$PR_NUMBER/reviews \
  -d "{
    \"commit_id\": \"$HEAD_SHA\",
    \"event\": \"REQUEST_CHANGES\",
    \"body\": \"## Review\n\nFound 2 issues, 1 suggestion. See inline comments.\",
    \"comments\": [
      {\"path\": \"src/auth.py\", \"line\": 45, \"body\": \"🔴 **Critical:** User input passed directly to SQL query — use parameterized queries.\"},
      {\"path\": \"src/models.py\", \"line\": 23, \"body\": \"⚠️ **Warning:** Password stored without hashing.\"},
      {\"path\": \"src/utils.py\", \"line\": 8, \"body\": \"💡 **Suggestion:** This duplicates logic in core/utils.py:34.\"}
    ]
  }"

Step 8: Also post a summary comment

In addition to inline comments, leave a top-level summary so the PR author gets the full picture at a glance. Use the review output format from references/review-output-template.md.

With gh:

gh pr comment $PR_NUMBER --body "$(cat <<'EOF'
## Code Review Summary

**Verdict: Changes Requested** (2 issues, 1 suggestion)

### 🔴 Critical
- **src/auth.py:45** — SQL injection vulnerability

### ⚠️ Warnings
- **src/models.py:23** — Plaintext password storage

### 💡 Suggestions
- **src/utils.py:8** — Duplicated logic, consider consolidating

### ✅ Looks Good
- Clean API design
- Good error handling in the middleware layer
EOF
)"

Step 9: Clean up

Leave the worktree and branch in place by default — don't delete work you didn't create. Only remove the review worktree if the user explicitly asks, and even then never with git checkout -- . / reset --hard / clean:

# Only when explicitly asked to clean up:
REPO_SLUG=$(gh repo view --json nameWithOwner -q '.nameWithOwner' | tr '/' '-')
git worktree remove ~/projects/_worktrees/${REPO_SLUG}-pr-$PR_NUMBER

Decision: Approve vs Request Changes vs Comment

• Approve — no critical or warning-level issues, only minor suggestions or all clear
• Request Changes — any critical or warning-level issue that should be fixed before merge
• Comment — observations and suggestions, but nothing blocking (use when you're unsure or the PR is a draft)