Fix: github-pr-review reviews in a tree the PR author may own (caused data loss) #4

Re-filed from rossoctl/agent-skills#40 (originally opened by @huang195) after RepoMan was cloned into this repo. Original discussion/history lives there.

Still applies in repoman: skills/github-pr-review/SKILL.md has no worktree/isolation guidance.

Summary

skills/github-pr-review/SKILL.md §1.5 tells the reviewer to cd <local-clone-path> and verify
claims against a local checkout, but it never says which tree to use or that the tree must not
be one somebody else owns. In practice the reviewer picks whichever local worktree already sits at
the PR head — which, for an actively developed branch, is the author's own working tree.

That is unsafe in both directions: the author's commits move the ground under the review, and the
review's subagents write into the author's tree. I hit all three failure modes in a single review
of rossoctl/cortex#1004, including uncommitted work being destroyed.

A review is inherently read-only work, so none of this is necessary. A review never needs a mutable
tree; it needs an immutable snapshot of one commit.

What happened

Reviewing rossoctl/cortex#1004, I selected a local worktree because it happened to be at the PR
head. During the review:

1. Head drift produced wrong citations. The tree advanced e0f8c6f8 → c0c1c23d → 6cc4888b → d65ffeeb while subagents were reading it. One reviewer re-verified its line numbers against the
   moved tree and reported findings against code that was not in the PR — including a defect in a
   function arm that only existed in an unpushed commit.
2. A concurrent edit almost became a false finding. A reviewer's go test run failed on another
   session's uncommitted edit to plugins/inferenceparser/plugin.go. It nearly reported that as a
   defect at the PR head; it was caught only because the reviewer re-ran in a pristine checkout.
3. Data loss. That same reviewer then ran git bisect in the shared tree, detached HEAD, and
   discarded the other session's uncommitted change.

Failure 3 is the serious one. Failures 1 and 2 cost a wasted pass and risked a wrong review comment.

Root cause

Two gaps in the skill:

• §1.5 optimises for freshness (git fetch upstream main) but says nothing about tree ownership
  or exclusivity. Its stated anti-pattern is "stale checkout"; the opposite hazard — a live
  checkout — is not mentioned.
• The Phase 2 / Phase 3 subagent templates hand agents a repo path with no isolation. Subagents are
  where the destructive commands ran (go test, scratch files, git bisect), not the main loop.

Proposed fix

1. §1.5 — snapshot the pinned SHA into a review-only tree; never reuse an existing worktree.

   SHA=$(gh api repos/$OWNER/$REPO/pulls/$PR --jq .head.sha)
   git worktree add --detach /tmp/review-$PR-${SHA:0:8} "$SHA"

   Detached, review-owned, disposable. Immune to the author committing, rebasing, or force-pushing.

2. Phase 2/3 — pass isolation: "worktree" on every review subagent. This is the change that
   would have prevented the data loss, since the damage came from subagents. It keeps what makes
   these reviews valuable — the highest-value findings in #1004 came from agents actually running
   tests and mutation testing — while making a stray bisect or scratch file harmless.

3. Pin the SHA as a hard invariant. Capture head_sha in Phase 1, put it in every subagent
   prompt ("verify with git show <SHA>:<path>, never the working tree"), and re-read it before
   Phase 4. If it moved, re-scope and say so rather than silently mixing two heads.

4. §1.5 — add a contention pre-check before selecting any tree. git worktree list plus
   git status --porcelain on each candidate; a dirty tree, or a branch checked out elsewhere,
   disqualifies it. This is the check whose absence caused all three failures.

5. Optional, for co-located sessions. When the PR author is a reachable peer session, announce
   the pinned SHA and ask to be told about pushes. Useful mainly for self-authored PRs, where the
   author-session and reviewer-session are the same person working in parallel.

What not to do

Do not make the review subagents read-only. Executing tests and mutation testing produced the two
strongest findings in #1004 (a reproduced retention-overflow that unlinks every day file, and a
measured case of a rejected response being billed as a success). Isolate the execution; do not
remove it.

Acceptance criteria

• §1.5 documents tree selection and exclusivity, with the live-checkout anti-pattern named
  alongside the existing stale-checkout one.
• The review runs from a detached, review-owned snapshot at a pinned SHA by default.
• Phase 2/3 subagent templates specify isolation: "worktree".
• The pinned SHA appears in subagent prompts and is re-verified before the verdict, with drift
  reported explicitly.
• A contention pre-check runs before any local tree is used.

Notes

The skill is consumed through the plugin marketplace cache
(~/.claude/plugins/cache/rossoctl-agent-skills/<rev>/skills/github-pr-review/), so a local edit is
overwritten on the next plugin update — which is why this is filed here rather than patched locally.

Assisted-By: Claude Code