fix(review,cso): SHA-pin PR diff to prevent worktree-flip-during-review

[habassa5]

Summary

/review and /cso use working-tree-dependent git refs (origin/<base>, <base>..HEAD) when computing the PR diff. Inside Agent SDK sessions where nested subagents share the worktree, a stray git checkout anywhere in the call tree (e.g., to inspect a sibling branch's file) silently re-targets every later git diff command, and the review reports findings on the wrong branch. Observed empirically three times in one session in a downstream project's PR reviewers.

This PR introduces a {{PR_DIFF_PIN}} template fragment (Step 0.5 of the affected skills) that resolves BASE_SHA and HEAD_SHA to immutable commit SHAs at the start of the skill — via gh pr view --json baseRefOid,headRefOid in PR context, or git rev-parse origin/<base> plus local HEAD otherwise — and rewrites every subsequent git diff/git log/git show to use those SHAs by value. Commit SHAs are immutable across worktree flips.

The bug, by example

A /review session is in progress on branch feature-B. A nested subagent runs git checkout feature-A to inspect a sibling branch's implementation, then doesn't switch back. From that point on, every git diff origin/main in the review skill renders against feature-A, and the review reports vulnerabilities and dead code from feature-A while purporting to review feature-B.

# Before the fix (working-tree dependent — bug)
git diff origin/main           # whatever branch HEAD currently points to
git log origin/main..HEAD      # whatever branch HEAD currently points to
git diff origin/main...HEAD    # whatever branch HEAD currently points to
git diff --name-only origin/HEAD...   # same — Claude Code's built-in /security-review uses this exact pattern

# After the fix (SHA-pinned — correct)
git diff "$BASE_SHA" "$HEAD_SHA"
git log "$BASE_SHA..$HEAD_SHA"
git show "$HEAD_SHA:VERSION"

Empirical evidence (from the downstream project that surfaced this)

Three back-to-back PR reviewers in the same session observed wrong-branch diffs:

• Hit 1 (PR feat: contributor mode, session awareness, universal RECOMMENDATION format (v0.3.11) #82 reviewer): invoked /security-review while on PR feat: contributor mode, session awareness, universal RECOMMENDATION format (v0.3.11) #82's branch but received the diff for an unrelated branch the worktree had been flipped to mid-session. Reviewer correctly detected and refused the false-positive findings.
• Hit 2 (PR feat(qa): add browse subcommand reference table to SKILL.md #88 reviewer): noted "/security-review skill not installed locally as a callable executable; performed manual security review per the rubric." Different surface symptom — same underlying class of bug.
• Hit 3 (PR feat: contributor mode, session awareness, recommendation format #90 reviewer): flagged "/security-review ... NOT RUN. Skipped per known stale-git-context bug guidance." Bug is now actively avoided rather than tolerated.

What's NOT changed

• /security-review (Claude Code built-in) has the exact same class of bug — cli.js ships the prompt git diff --name-only origin/HEAD... literally — but lives in Anthropic's binary and is out of gstack's reach. After this PR, /cso is a strictly safer alternative for security audits run from inside multi-agent sessions. The Claude Code bug is being filed separately upstream with Anthropic.
• /ship, /codex, /document-release have similar bare-ref patterns. They are reachable from outside multi-agent reviews where the flip risk is lower; this PR leaves them on bare refs for back-compat. Adopting {{PR_DIFF_PIN}} there is one-line each in their .tmpl. Tracked as follow-up.
• scripts/slop-diff.ts internally uses ${base}...HEAD and walks the working tree. Properly fixing it requires changing the script's interface to accept a head-SHA argument. Tracked as follow-up.

Test plan

• bun test test/pr-diff-pin-regression.test.ts — 8 tests, ~6s, free tier
  • Includes a real two-branch-fixture test that reproduces the failure mode end-to-end (proves the bug is real, then proves SHA-pinning fixes it)
  • Smell tests catch regressions where someone re-introduces a bare-ref pattern into review/SKILL.md.tmpl or cso/SKILL.md.tmpl — both fenced bash blocks AND inline backtick-quoted commands
  • Verified empirically: deliberately injecting Run \git diff origin/`` into review/SKILL.md.tmpl makes the smell test fail loudly
• bun test test/skill-collision-sentinel.test.ts — confirms no skill-name collisions introduced
• bun run gen:skill-docs --dry-run — confirms generated SKILL.md files are in sync with templates
• Adversarial review by independent codex CLI at HIGH reasoning effort (per CLAUDE.md 全ファイル日本語化: スコープ整理と実施計画 #67) surfaced 7 substantive concerns; all addressed in commit 9404fa9a. Re-running the same adversarial framing on the fixed branch is recommended before merge.
• End-to-end fresh-clone validation (per CLAUDE.md /plan-ceo-review stack in the planning mode and started implementing the code #66): gh repo clone <fork> -- --branch fix/... to a clean directory, bun install, run regression test, all pass.

Adversarial codex findings addressed

Severity	Finding	Resolution
P1	Resolver-emitted commands still used bare refs	Threaded ctx.skillName === 'review' through 6 affected resolvers; pinned-SHA forms inside /review, bare refs preserved for /ship
P2	Uncommitted local edits dropped in local-/review path	Added $REVIEW_DIRTY env var (default 0 in PR context, 1 for local pre-PR), Step 3 instructs combining git diff "$BASE_SHA" with git diff "$HEAD_SHA" when set
P2	Stale base ref via fetch + rev-parse	Now uses gh pr view --json baseRefOid directly (immutable) inside PR context
P2	Stale base ref when fetch fails	Warn explicitly + add git cat-file -e probe for both pinned SHAs
P2	gh pr diff blessed as immutable but isn't	Replaced with explicit warning; recommend gh api /repos/.../compare/$BASE_SHA...$HEAD_SHA if user wants GitHub-rendered diff
P2	Smell test only checked fenced bash blocks	Extended to also catch inline backtick-quoted git/gh/bun imperatives
P2	/cso exit(1) on missing SHAs broke non-diff scope flags	Soft-fail with WARN; /review re-asserts requirement with explicit check

Files changed

CHANGELOG.md                        |  55 ++
VERSION                             |   2 +-
package.json                        |   2 +-
scripts/resolvers/utility.ts        |  ~140 (+ generatePrDiffPin)
scripts/resolvers/index.ts          |   3
scripts/resolvers/review.ts         |  51 ± (skillName-conditional pinned vs bare)
scripts/resolvers/review-army.ts    |  18 ± (skillName-conditional pinned vs bare)
review/SKILL.md.tmpl                |  29 ± (Step 0.5 inclusion + Step 1/3 rewrite)
review/SKILL.md                     | 108 ± (regenerated)
review/checklist.md                 |   4 (doc reference to pinned form)
review/greptile-triage.md           |   2 (doc reference to pinned form)
cso/SKILL.md.tmpl                   |  13 ± (Step 0.5 inclusion + Phase 2 diff-mode rewrite)
cso/SKILL.md                        | 129 ± (regenerated)
test/pr-diff-pin-regression.test.ts | 280 (new — 8 tests, ~6s, free tier)

Failure mode tracking

This change closes the failure mode named shared-checkout-branch-flip-during-review in CLAUDE.md tracking. The same name appears in Step 0.5 documentation and the regression test's docblock so future maintainers can grep for it.

Compatibility / breaking changes

None. The bare-ref forms are preserved for /ship and other skills that don't yet adopt {{PR_DIFF_PIN}}. Users running /review and /cso will see a new "Pinned review context:" log line at the start of the skill (Step 0.5), but the rest of the workflow is functionally identical except in the buggy worktree-flip case.

🤖 Generated with Claude Code

---

^{Need help on this PR? Tag @codesmith with what you need.}

• Let Codesmith autofix CI failures and bot reviews