review-changes

Files (1)

• SKILL.md

SKILL.md (first lines)

# Review Changes

Review a branch or PR against its base. Report findings by severity.

## Step 0: PR eligibility (PR only)

```bash
gh auth status 2>&1
```

Not logged in: **STOP**, tell the user to run `gh auth login`.

```bash
gh pr view $ARGUMENTS --json number,state,author,headRefOid,baseRefName
gh api user --jq .login
gh api "repos/{owner}/{repo}/pulls/<number>/reviews"
```

Skip, and say why, if the PR is closed, automated (e.g. a dependency bump), or your latest review's `commit_id` equals `headRefOid`. Filter reviews to your own login. Don't compare timestamps: your review bumps `updatedAt`, and a force-push can carry an older `committedDate`.

## Step 1: Resolve base, read the change

PR: base is `baseRefName`; diff is `gh pr diff $ARGUMENTS`; intent is `gh pr view $ARGUMENTS --json title,body` plus the commits.

Branch: derive the base from the tracked remote. Never assume `origin` or `main`.

```bash
remote=$(git config "branch.$(git branch --show-current).remote" || git remote | head -1)
base=$(git symbolic-ref --short "refs/remotes/$remote/HEAD" 2>/dev/null \
  || gh repo view --json defaultBranchRef -q "\"$remote/\" + .defaultBranchRef.name" 2>/dev/null)
git log --oneline "$base..HEAD"
git diff "$base...HEAD"
```

If `base` is empty, ask the user.

## Step 2: Review

Read the root CLAUDE.md and any in touched directories; flag clear violations only.

| Category | Check |
| -------- | ----- |
| Correctness | Logic errors, off-by-one, missing null/error checks, resource leaks |
| Security | Secrets, insecure permissions, exposed credentials, injection |
| Shell safety | Unquoted variables, missing `set -euo pipefail`, unsafe globbing |
| Portability | Platform, path, or tool-version assumptions the repo doesn't guarantee |
| Concurrency | Shared mutable state, missing synchronization |
| Tests | Changed behavior without test changes; tests deleted or weakened |
| Style | Naming, dead code, inconsistency with existing patterns |
| Conventions | CLAUDE.md violations |

History: `git blame` the modified code — does the change contradict why it was written that way, or a comment in the file (invariants, "must", "do not", ordering notes)? If blame shows it landed via a reviewed PR (`gh api "repos/{owner}/{repo}/commits/<sha>/pulls"`, `gh pr view <number> --comments`), does that feedback apply again?

Breaking changes: for each changed signature, export, script, build target, config key, env var, CLI flag, or output format, `git grep` its consumers and confirm each still works. Schema changes need a migration.

Docs: if the diff touches paths, symbols, endpoints, config keys, or build targets, invoke `repo-verify-docs` and report its findings. If ARCHITECTURE.md, DEVELOPER_GUIDE.md, README.md, or CLAUDE.md changed, check them against `repo-lint-docs` rules and report violations. Never edit during a review.

## Step 3: Verify

Send every Critical and Important finding to its own `Agent`, at most 20 per message (Claude Code's concurrent subagent limit) so they run in parallel. Give each the base ref and `git diff "$base...HEAD"` (or the PR number), the change's intent from Step 1, the finding with its file:line, and the CLAUDE.md paths read in Step 2, and have it score its confidence 0-100 that the finding is real, using this rubric verbatim:

- 0: false positive that fails light scrutiny, or pre-existing
- 25: might be real, could not verify; or stylistic and not called out in CLAUDE.md
- 50: verified real, but a nitpick or rare in practice
- 75: double-checked, very likely hit in practice, or directly called out in CLAUDE.md
- 100: certain; the evidence confirms it and it will happen often

For a CLAUDE.md finding, the agent must confirm the file calls out that issue specifically. Drop findings below 75, except Critical findings scoring 50 or more: keep those, marked "(low confidence)". Suggestions, Documentation, and Out of scope entries skip this step.

## Step 4: Report

```text
## Critical (must fix before merge)
- deploy.sh — unquoted $TARGET_DIR breaks on paths with spaces

## Important (should fix)
- config/loader.py — new RETRY_LIMIT has no default; a missing env var raises KeyError

## Suggestions
- scripts/fetch.sh — duplicates scripts/download.sh

## Documentation
- scripts/new-tool.sh is not mentioned in ARCHITECTURE.md

## Out of scope
- bin/legacy.sh — pipes curl to bash (predates this change)
```

If the change is good, say so. Don't manufacture issues.

## Rules

- Reference file:line and quote the code
- Verify every finding is real and matters; Step 3 decides which Critical and Important findings stay, drop any other you can't confirm
- Flag anything this change introduces or triggers, wherever it manifests
- Keep out of main findings: pre-existing issues, formatter-level nits, anything the repo's CI linter/typechecker would catch, lint-silenced issues. Serious pre-existing issues (security, data loss, broken auth) go under "Out of scope", one line each; omit the section if empty
- A possibly intentional behavior change is a question, not a finding
- Use the repo's default `gh` host — no `--hostname` flag

From offspring/offspring-claude-skills, MIT.

More from offspring/offspring-claude-skills