github-code-review

# 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 (see `github-auth` skill) - Inside a git repository ### Setup (for PR interactions) ```bash if command -v gh &>/dev/null && gh auth status &>/dev/null; then AUTH="gh" else AUTH="git" if [ -z "$GITHUB_TOKEN" ]; then if [ -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 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 ```bash # 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:** ```bash 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: ```bash git diff main...HEAD -- src/auth/login.py ``` 3. **Check for common issues:** ```bash # 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:** ```bash gh pr view 123 gh pr diff 123 gh pr diff 123 --name-only ``` **With git + curl:** ```bash 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 This works with plain `git` — no `gh` needed: ```bash # Fetch the PR branch and check it out git fetch origin pull/123/head:pr-123 git checkout pr-123 # Now you can use read_file, search_files, run tests, etc. # View diff against the base branch git diff main...pr-123 ``` **With gh (shortcut):** ```bash gh pr checkout 123 ``` ### Leave Comments on a PR **General PR comment — with gh:** ```bash gh pr comment 123 --body "Overall looks good, a few suggestions below." ``` **General PR comment — with curl:** ```bash 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):** ```bash 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:** ```bash # 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:** ```bash gh pr review 123 --approve --body "LGTM!" gh pr review 123 --request-changes --body "See inline comments." gh pr review 123 --comment --body "Some suggestions, nothing blocking." ``` **With curl — multi-comment review submitted atomically:** ```bash 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 from Hermes Agent\", \"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? - 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? ### 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 ```bash source "${HERMES_HOME:-$HOME/.hermes}/skills/github/github-auth/scripts/gh-env.sh" # Or run the inline setup block from the top of this skill ``` ### Step 2: Gather PR context Get the PR metadata, description, and list of changed files to understand scope before diving into code. **With gh:** ```bash gh pr view 123 gh pr diff 123 --name-only gh pr checks 123 ``` **With curl:** ```bash PR_NUMBER=123 # PR details (title, author, description, branch) curl -s -H "Authorization: token $GITHUB_TOKEN" \ https://api.github.com/repos/$GH_OWNER/$GH_REPO/pulls/$PR_NUMBER # Changed files with line counts curl -s -H "Authorization: token $GITHUB_TOKEN" \ https://api.github.com/repos/$GH_OWNER/$GH_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. ```bash