pr-review

Files (3)

• SKILL.md
• children.md
• guide.md

SKILL.md (first lines)

# PR Review

You are the orchestrator of the PR review. You run the `reviewer` and `oracle` sub-agents directly as your own children — there is no intermediate review agent. This flat shape is intentional: both children appear in your fleet with live per-child detail (model, context mode, tool/token/elapsed counters, current activity), so a working-but-slow child is distinguishable from a stuck one, and a stuck leg can be steered or re-dispatched individually without losing the other leg's work. Never reintroduce a wrapper agent for this flow.

**Before starting any review, read `guide.md`** — it contains hard-won lessons and constraints that prevent real failures (timeout budgets, context mode, baseline rules). This is not optional.

## Review artifacts directory

Review artifacts (context file, report) must live in a RAM disk, not in the project directory. The correct location is `${XDG_RUNTIME_DIR}/pr-review/$number/` where `XDG_RUNTIME_DIR` is the user-specific runtime directory (typically `/run/user/$(id -u)`). Check it with `echo $XDG_RUNTIME_DIR` — it defaults to `/run/user/1000` on most Linux systems. Do NOT create review artifacts inside the project worktree — it pollutes the working tree and risks them being committed.

## Workflow

### 1. Parse the request

Extract the PR number and optional owner/repo from:

| Input format | Extracted |
|---|---|
| `Review PR #44` | number=44, repo from git remote |
| `Review PR #44 on owner/repo` | number=44, owner/repo given |
| `https://github.com/owner/repo/pull/44` | number=44, owner/repo parsed from URL |
| `owner/repo#44` | number=44, owner/repo parsed |

If the owner/repo cannot be determined from the task or the git remote, ask the user before proceeding.

### 1.5. Resolve the PR, then verify it targets master (hard gate)

Fetch the PR's branches **before** any workspace decision. Step 2's "already on the PR branch" check compares the current branch against `headRefName`, so fetching it later makes that check impossible to evaluate — that ordering bug is why an already-checked-out PR branch gets missed.

```bash
gh pr view <number> --repo <owner/repo> --json number,title,url,state,headRefName,baseRefName
```

Hold `PR_HEAD`, `PR_BASE`, and `PR_TITLE` for the rest of the workflow. If `state` is not `OPEN`, say so and confirm the user still wants a review.

**Now the target-branch gate. Do not skip it.**

**If `PR_BASE` is not `master`:**

1. Check whether the user explicitly mentioned the non-master target in their request (e.g., "review PR #123 which targets develop").
2. **If the user did NOT explicitly mention it:** Stop immediately. Report:
   > ⚠️ **PR #<number> targets `<PR_BASE>`, not `master`.** This PR appears to be aimed at the wrong branch. Aborting review to avoid wasted effort. Please verify with the PR author.
   Do **not** proceed with the review. Do **not** merge master. Do **not** run any review steps.
3. **If the user DID explicitly mention it:** Continue, but note the non-master target in the context file and in your final report.

**If `PR_BASE` is `master`:** proceed to step 2.

Running this before the workspace questions means a mis-targeted PR is rejected without ever asking the user where to work.

### 2. Choose the review workspace (current directory first)

Check whether the current working directory is inside a git repo:

```bash
TOPLEVEL="$(git rev-parse --show-toplevel 2>/dev/null)" || TOPLEVEL=""
CURRENT_REMOTE="$(git remote get-url origin 2>/dev/null)"
CURRENT_BRANCH="$(git rev-parse --abbrev-ref HEAD 2>/dev/null)"
WORKTREE_PATH=""
```

**Check 1 — Already on the PR branch. This is the expected case, not a lucky one.** The user normally checks out the PR branch into a worktree and starts the session there, so check the current directory first and prefer it. A match requires all three:

- `CURRENT_REMOTE` is the same repo as `<owner/repo>` — compare owner/repo, not the raw URL (`git@github.com:<owner>/<repo>.git` and `https://github.com/<owner>/<repo>.git` both count)
- `CURRENT_BRANCH` equals `PR_HEAD`
- `git status --porcelain` shows no modified *tracked* files (see the guard below)

**On a match: set `WORKTREE_PATH="$TOPLEVEL"` and continue. Do not ask the user where to work, do not offer alternatives, do not create or switch anything.** Asking here is the exact failure this check exists to prevent. Then bring the branch up to date:

```bash
git fetch origin "$CURRENT_BRANCH"
BEHIND="$(git rev-list --count HEAD..origin/"$CURRENT_BRANCH" 2>/dev/null)"
if [ "$BEHIND" != "0" ]; then
  git merge --ff-only origin/"$CURRENT_BRANCH"
fi
```

If the fast-forward fails, stop and ask the user rather than forcing anything.

**Guard — dirty working tree.** Before doing any review work, verify the review directory is clean. Run `git status --porcelain` and stop if there is *any* output — modified tracked files, untracked files, new files, deleted files, or staged changes. Previous reviews, abandoned experiments, or other branches may have left debris. Report the full `git status --porcelain` output and ask the user whether to commit, stash, or discard everything before proceeding. Ignored build artifacts (see step 2b) do not count — they are expected. Do not review over a dirty tree; it produces false findings and makes it impossible to tell which changes belong to the PR.

**Check 2 — Same repo, different branch.** The directory is the right repository but not the PR branch. **Compare the directory's path with the branch it is holding before deciding anything** — the user keeps one worktree per branch, named for it **in full** (`src/master`, `src/develop`, then one per ticket branch under whatever prefix that branch uses: `src/chore/<ticket>`, `src/feat/<ticket>`, `src/tickets/<ticket>`), so the name is an assertion about the contents and their navigation depends on it. Never parse the prefix — it is `chore/`, `fix/`, `bugfix/`, `hotfix/`, `docs/`, `refactor/`, or nothing at all, and it changes — so compare the whole path suffix against the whole branch name. Only the generic ones (`src/wip`, `src/review/wip`) change branch by design. That gives one test with no list of magic names to maintain: **does the directory's path name the branch it holds?**

| Path names… | What it means | What to do |
|---|---|---|
| **`PR_HEAD`** | This worktree is the PR branch's home; the checkout was simply forgotten | Confirm, then check out `PR_HEAD` here, and recommend it plainly |
| **the branch it holds, not `PR_HEAD`** (`master`, `develop`, `staging`, or any other branch under any prefix) | Another branch's home. The name promises those contents | **Do not offer this directory and do not switch it.** Name the mismatch and ask where to work — this is the case that needs a scratch worktree or a new one |
| **neither** | Scratch space, reused across reviews on purpose | Offer it as the default (below): a clean tree means the switch was forgotten, not that the directory is taken |

Compare words, not characters — case and separator style are cosmetic, so `hotfix/Payments-Down` and `hotfix/payments_down` are one name. Compare the whole path, not the basename, and keep the prefix in the comparison: `src/feat/issue-96-dashboard` names `feat/issue-96-dashboard` and `src/chore/install-vitest` names `chore/install-vitest`, though neither basename carries the prefix.

```bash
norm() { printf '%s' "$1" | tr 'A-Z' 'a-z' | tr '_-' '//'; }
ends_with() { case "$1" in *"$2") return 0;; esac; return 1; }
held="$(git branch --show-current)"   # the branch this directory currently holds
if [ -z "$held" ]; then echo "generic (detached HEAD) -> offer, default yes"
elif ends_with "$(norm "$PWD")" "$(norm "$PR_HEAD")"; then echo "names PR_HEAD -> switch here"
elif ends_with "$(norm "$PWD")" "$(norm "$held")"; then echo "another branch's home -> do not offer"
else echo "generic -> offer, default yes"; fi
```

For a generic directory or one named for `PR_HEAD`:

1. **Check out `PR_HEAD` in the current directory — the default whenever `git status --porcelain` is empty.** Confirm with the user and recommend it. Run `git fetch origin "$PR_HEAD"`, then `git checkout "$PR_HEAD"`. If the checkout is refused, fall through to option 2 or 3 rather than forcing it.
2. **Use a different existing worktree** — especially one already named for `PR_HEAD`. Run `git worktree list` and go there.
3. **Let the user create one** — ask them to create/check out a worktree and tell you the path.

**Do not create worktrees yourself** — creation needs project-specific setup beyond git — but checking out an existing branch in an existing worktree is not creation. If the user nonetheless directs you to use a directory named for another branch, follow the instruction and say which branch it will hold, so the exception is theirs and on the record rather than silent.

A dirty tree blocks the switch: clear it through the guard above first, then offer this choice. Set `WORKTREE_PATH` to whichever path results, and announce it in the pre-flight line below before any further step.

Step 16 re-checks the branch before any later read, for the same reason this step may switch it: the directory is shared, and it can move under you between turns.

**Check 3 — Not in a repo with the right remote.** Ask the user: diff-only review (fast, GitHub API) or full clone (slower, better). Diff-only means both children use `gh pr diff <number> --repo <owner/repo>` and have no surrounding codebase.

**Workspace pre-flight — announce it before continuing.** Post one line stating the resolved workspace and branch before any further step, for example:

> Workspace: `/path/to/worktree` — branch `feat/x`, PR #<number> head `feat/x` ✓, base `master` ✓

This makes a wrong workspace visible immediately instead of surfacing 40 minutes later as bogus findings. If the announced line disagrees with the PR, stop and fix it before step 2a.

From ceilidhboy/skills, MIT.

More from ceilidhboy/skills