settled-reach/.claude/skills/pr-review/SKILL.md

name, description, user-invocable, allowed-tools

name	description	user-invocable	allowed-tools
pr-review	Review a branch diff with team-appropriate agents before merge. Use when the user says "review-pr", "review this PR", "review this branch", or invokes /pr-review. Spawns reviewers matched to the branch type (code, copy, visual, audio) in parallel. Reports approve/reject with inline comments.	true	Bash, Read, Grep, Glob, Task

PR Review Skill

Multi-agent review of a branch diff against main. Reviewer composition depends on the branch type. All reviewers must approve for a clean review.

Workflow

0. Branch guard — MUST be run from the main branch

git branch --show-current

If the current branch is not main, stop immediately and tell the user: "PR reviews must be run from the main branch." Do NOT proceed with the review. Stop and wait for the user to invoke /pr-review from main.

0b. Verify runtime smoke test was performed

Before spawning reviewers, check that the pushing team performed basic runtime verification. This was the #1 process failure of Sprint 28 — 3 review rounds without anyone launching the game missed critical bugs.

Ask: "Did the team run make game or a headless smoke test before pushing this PR?"

If the PR description or commit messages don't mention runtime testing, note this in the review output as a process gap. Reviewers should still proceed (the PR exists and needs reviewing) but the gap should be visible.

For client/visual branches, run a quick headless parse check from main:

godot --headless --path client --quit 2>&1 | grep -i "SCRIPT ERROR"

If script errors appear in the branch diff files, flag them immediately before spawning reviewers — no point reviewing code that doesn't parse.

0b-i. Merge-path smoke test gate

When the branch diff touches any of:

• pre-game flow (main menu → character creation → connect)
• scene transitions (change_scene_to_file, scene autoloads)
• save / load / new-game paths
• connection handshake (sim_bridge, protocol decode/encode)
• any code path executed in the first 30 seconds of a new session

...the reviewer output MUST explicitly call out the state of author-side manual smoke boxes in the PR test plan. If any merge-path smoke box is unchecked, include a top-level note:

Merge-path smoke not performed. PR test plan has unchecked manual smoke box(es): [list]. A reviewer or the team must run the smoke before merge approval. Sprint 36 bug #872 (New Game hangs on 'connecting') landed exactly here — do not skip.

Unchecked merge-path smoke boxes downgrade the verdict from APPROVED to REQUEST_CHANGES even if reviewers have no code comments. The smoke is a deliverable, not a suggestion.

0c. Zero warnings check

The project enforces a zero warnings policy. Before spawning reviewers, check if the branch introduces lint warnings:

• client/visual: gdlint client/scripts/ client/ui/ should report 0 issues
• server: cargo clippy -- -D warnings should be clean
• ci/tooling: ruff check tooling/ should be clean

If warnings exist, note the count in the review output. Reviewers should flag any new warnings introduced by the branch as warning severity. Pre-existing warnings are not PR blockers but should be tracked for cleanup.

1. Determine the branch to review

Always start fresh. Even if you reviewed this branch before in this conversation, the branch may have new commits, a new PR, or main may have moved. Do NOT skip steps or reuse earlier results. Every /pr-review invocation is a full review cycle.

Always fetch and check for PRs first:

git fetch --all
tea pr list --login schweitz --repo jpmschweitzer/settled-reach --state open --output simple

Then determine the branch:

• If the user provided a branch name as argument, match it to an open PR. If a PR exists for that branch, note the PR number. If no PR exists, proceed with the branch diff but note "no PR found" in the output.
• If no argument was given, list open PRs and ask which to review.

2. Determine reviewer team

Map the branch name to a reviewer set. Use the branch prefix (before any / or - suffix) to classify:

Branch type	Branches	Reviewers
code	server, client, ci, or unknown	Hoshe (code quality) + Tyre (architecture)
copy	copy	Hoshe (QA) + Paula (narrative depth) + Miri (world consistency)
visual	visual	Hoshe (QA) + Araminta (art direction)
audio	audio	Hoshe (QA) + Ozzie (player experience)

If the branch name doesn't match any known type, default to code reviewers.

3. Generate the diff and read source files

git log --oneline main..<branch>
git diff main...<branch> --stat

If the diff is empty, report "No changes to review" and stop.

Exclude generated/vendor files from the review diff. Common exclusions:

• Cargo.lock (auto-generated)
• client/addons/gdUnit4/ (vendor test framework)
• *.uid (Godot-generated)
• docs/backups/settledreach.db.backup (binary)

Three-dot diff with pathspec exclusions is unreliable. Instead, either:

1. Use git diff main...<branch> (full diff) and filter in the prompt, or
2. Read source files directly from the team directory (see below).

For large diffs (>1000 lines of source), provide source files rather than raw diff to reviewers — cleaner context, better reviews.

Reviewer agents read source files from the team worktree.

Sprint branches follow sprint-{N}/{team}. Mid-sprint, a worktree of each branch exists at $(dirname <repo_root>)/.sprint/sprint-{N}/{team}/ — a sibling of the repo root, not a child. This worktree IS the branch: Read/Grep on paths rooted there resolve against the branch's checkout, not main's.

Why this matters: Sprint 37 PR #138 review produced 6 false- positive findings because the reviewer defaulted to Read/Grep on the main repo path (/var/mnt/data/projects/settled-reach/main/) instead of the branch worktree. Every finding was a verbatim match against main's state but irrelevant to the branch — the branch had already cleaned the residue the reviewer flagged as "still present." Sending those findings to the team would have caused busywork on already-clean code, and more dangerously, the same drift hides false negatives (branch-introduced bugs the reviewer never saw because it never read the branch).

Fix: before spawning reviewers, resolve the worktree path and pass it into every reviewer prompt with prominent language. The reviewer reads from the worktree, not from main.

# Determine worktree path
SPRINT_NUM=$(echo "<branch>" | sed -E 's|sprint-([0-9]+)/.*|\1|')
TEAM=$(echo "<branch>" | sed -E 's|sprint-[0-9]+/||')
REPO_ROOT=$(git rev-parse --show-toplevel)
WORKTREE="$(dirname "$REPO_ROOT")/.sprint/sprint-${SPRINT_NUM}/${TEAM}"

# Verify it exists and matches the branch tip
git -C "$WORKTREE" rev-parse HEAD   # should equal `git rev-parse origin/<branch>`

If the worktree exists and its HEAD matches origin/<branch>, use it as the reviewer's source of truth. If it doesn't exist (e.g. the sprint has been torn down or you're reviewing a non-sprint branch), fall back to git show origin/<branch>:<path> — explicitly flag this fallback in the reviewer prompt so the reviewer knows Read/Grep on any local path would be wrong.

In the reviewer prompt, state the rule non-negotiably:

Read source from <WORKTREE_PATH> only. Do NOT Read or Grep paths under the main repo root (/var/mnt/data/projects/settled-reach/main/). Those resolve to main, not the branch. The worktree at <WORKTREE_PATH> IS the branch — point all file tools there.

Also tell agents to read relevant decisions/*.md files for context (these can be read from either path — they're usually identical — but for consistency, use the worktree path).

4. Spawn reviewers in parallel

Use the Task tool to spawn all reviewers simultaneously in a single message.

Read references/reviewer-profiles.md for the full per-branch-type reviewer specifications (agent types, models, prompt focus areas). Match the branch type from step 2 to the corresponding section.

All reviewers: request structured verdict: APPROVE or REQUEST_CHANGES with file-specific comments.

5. Present results

Format the combined review as a table per reviewer. Include one section per reviewer that was spawned (2 for code/visual/audio, 3 for copy):

## Review: <branch> -> main  (type: code|copy|visual|audio)

### <Reviewer Name> (<Focus>): [APPROVE | REQUEST_CHANGES]
[Summary]
| # | File | Issue |
|---|------|-------|
| 1 | path:line | description |

### <Reviewer Name> (<Focus>): [APPROVE | REQUEST_CHANGES]
...

### Verdict: [APPROVED | CHANGES REQUESTED]

The overall verdict is APPROVED only if all reviewers approve.

Prompt template for reviewers

Use this structure when constructing the agent prompts (adapt as needed):

Review the following {branch_type} branch diff for merge into main.

Branch: {branch}
Branch type: {branch_type} (code|copy|visual|audio)
Commits:
{commit_log}

Diff stats:
{diff_stat}

[Source files or diff here — exclude vendor/generated code]

Your review focus: {focus_area}

Respond with:
1. Verdict: APPROVE or REQUEST_CHANGES
2. Summary: 2-3 sentence overall assessment
3. Comments: List of specific issues, each with:
   - File path and approximate location
   - Description of the issue

Verdict rules:
- **Any comment at all → REQUEST_CHANGES.** Every issue is actionable.
  There is no "suggestion" tier that gets skipped. If it's worth
  mentioning, it's worth fixing before merge.
- If no issues found, say APPROVE with a brief positive summary.
- Do NOT flag something unless you expect it to be addressed. If you
  wouldn't fix it yourself, don't mention it.
- **Quality and polish is the golden standard.** We are not optimizing
  for speed — we are optimizing for a product we're proud of. If
  something can be better, say so.

6. Posting results to Gitea

After presenting results to the user, post the review as a PR comment.

Note: tea pr reject does not work on your own PRs. Use tea comment instead.

Post using the tea-comment wrapper (handles temp files and cleanup). Write the review to a temp file first, then pass via @filepath syntax:

# Write review to file, then post — avoids $() in the command which breaks permissions
cat > /tmp/pr-review-<NUMBER>.md << 'EOF'
...review content...
EOF
tooling/tea-comment <PR_NUMBER> @/tmp/pr-review-<NUMBER>.md

7. Merging approved PRs

tea pr merge fails (405) when branches have conflicts with main. Merge locally instead:

git fetch --all
git merge origin/<branch>        # resolve conflicts if any
git push origin main
tea pr close --login schweitz --repo jpmschweitzer/settled-reach <PR_NUMBER>

Gitea does not auto-close PRs when you push a local merge — always close manually with tea pr close after pushing.

8. Post-review team actions

If a sprint team is active and you are the team lead, handle the review outcome:

CHANGES_REQUESTED: The sprint-start lifecycle (step 9c) handles dispatching review comments to agents. After presenting results, remind the lead: "Review requested changes. Create tasks from each issue and dispatch to idle agents, then re-push and re-review."

The team may push back on specific comments. When a team agent disagrees with a reviewer comment, the process is:

1. The team agent explains why the comment should be retracted — with a concrete technical rationale, not just "I disagree."
2. The team lead (you) evaluates the pushback. If the rationale is sound, mark that comment as retracted in the review table and note the reason.
3. If the team lead is unsure, escalate to the user for a ruling.
4. Retracted comments do NOT need to be fixed. The re-review should note which comments were retracted and why.

This prevents reviews from becoming dogma while maintaining the rule that every comment is taken seriously. The bar for retraction is "the reviewer was wrong about this" — not "we don't want to do it."

APPROVED: The sprint-start lifecycle (step 9c) handles shutdown. After presenting results, remind the lead: "Review approved. Proceed with team shutdown per sprint-start step 9c."

Tips from practice

• Vendor code: Explicitly note vendor code in the prompt so reviewers focus on project code. Mention it as a separate architectural concern (should it be gitignored? submoduled?).
• Large PRs: For PRs touching many files, provide file-by-file source code rather than a single massive diff. Reviewers give better feedback.
• Multiple PRs: When reviewing several PRs, spawn all reviewers in one parallel batch (4 agents for 2 PRs). This is faster than sequential.
• Tyre reads decisions: Always tell Tyre to read the relevant decisions/*.md files — this grounds the review in project-specific architectural choices.
• Binary/DB files: Exclude binary files from the diff. Note them in the prompt as "also changed" if relevant.