diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index ed28b8c08..c347afea5 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -138,24 +138,57 @@ Three-dot diff with pathspec exclusions is unreliable. Instead, either: 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 via `git show`.** Sprint branches -use the naming pattern `sprint-{N}/{team}`. To read a file from the -branch being reviewed: +**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 )/.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. ```bash -git show origin/: +# Determine worktree path +SPRINT_NUM=$(echo "" | sed -E 's|sprint-([0-9]+)/.*|\1|') +TEAM=$(echo "" | 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/` ``` -For example: -```bash -git show origin/sprint-31/server:server/src/bin/atlas.rs -``` +If the worktree exists and its HEAD matches `origin/`, 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/:` — explicitly flag this +fallback in the reviewer prompt so the reviewer knows Read/Grep on +any local path would be wrong. -If the sprint branch has an active worktree (under `.sprint/`), reviewers -can also use the Read tool with the worktree path. But `git show` is -the reliable default — it works whether or not a worktree exists. +In the reviewer prompt, state the rule non-negotiably: -Also tell agents to read relevant `decisions/*.md` files for context. +> **Read source from `` 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 `` +> 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