Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 18 additions & 11 deletions claude-plugins/kampus-pipeline/skills/gh-issue-intake-formats.md
Original file line number Diff line number Diff line change
Expand Up @@ -1869,19 +1869,26 @@ ad hoc.
**The invariant — a review gate MUST source ALL code/prose under review from the PR head, and
assert it did:**

1. **Resolve the live head SHA up front, via REST/porcelain — never GraphQL** (§Target repo
resolution / the all-`gh api` rule). This is the SHA the verdict binds to (§5/ADR 0058):
1. **Resolve + materialize the head via the shared `pipeline-cli review-head` verb — never
re-derive it inline** (#3690 / #793 / #1807; `packages/pipeline-cli/src/tools/review-head/`).
The verb owns the deterministic mechanism this section used to spell out inline: it resolves the
live head SHA up front via REST (never GraphQL — the SHA the verdict binds to, §5/ADR 0058),
fetches the head into a per-run ref (never the launched checkout — the §RO read-only path), and
**asserts the fetched ref resolves to exactly that SHA before you review** — aborting on a moved
head rather than binding a stale verdict. Two modes, same core:
```bash
HEAD_SHA="$(gh pr view "$PR" --repo "$REPO" --json headRefOid -q .headRefOid)"
```
2. **Materialize the head into a per-run ref (never the launched checkout)** — the §RO
read-only path — and confirm it resolves to exactly that SHA before reviewing:
```bash
PR_REF="refs/pr/$PR-$(uuidgen)"
git fetch origin "pull/$PR/head:$PR_REF"
FETCHED="$(git rev-parse "$PR_REF")"
[ "$FETCHED" = "$HEAD_SHA" ] || { echo "FATAL: fetched head $FETCHED != resolved $HEAD_SHA — aborting" >&2; exit 1; }
# ref-only (review-doc reads via `git show "$PR_REF:<path>"`):
eval "$(pipeline-cli review-head materialize --pr "$PR" | jq -r '"PR_REF=\(.prRef); HEAD_SHA=\(.headSha)"')"
# full detached head worktree (review-code / review-skill), emitted as `.worktreeDir`:
pipeline-cli review-head materialize --pr "$PR" --worktree | jq -r '"REVIEW_WT=\(.worktreeDir)\nPR_REF=\(.prRef)\nHEAD_SHA=\(.headSha)"' > "$WT_FILE"
```
`pull/<pr>/head` resolves same-repo AND cross-fork, and the checkout is always DETACHED onto the
per-run ref — never `gh pr checkout` / `git checkout` / `git switch`, which would land the head in
the shared PRIMARY the harness resets a subagent's cwd to and detach the human's `main`
(#2270/#1103; the verb refuses this outright). Run `iso_preflight` (§RO-iso) BEFORE the verb.
2. **(owned by the verb, step 1)** — the per-run-ref fetch + the fetched-ref-IS-the-head assertion
are the verb's, not a step each gate re-derives; `review-design` reviews a preview URL rather than
a tree, so it uses the lighter `pipeline-cli review-head resolve --pr "$PR"` (the head SHA only).
3. **Read every file under review FROM THE HEAD, never from CWD.** Route full-file reads
through `git show "$PR_REF:<path>"` (or read from the throwaway head worktree the gate
already materializes — `git worktree add "$(mktemp -d)/…" "$PR_REF"`, `$REVIEW_WT`), and
Expand Down
50 changes: 21 additions & 29 deletions claude-plugins/kampus-pipeline/skills/review-code/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -322,36 +322,28 @@ BASE_REF="$(gh api repos/$REPO/pulls/$PR --jq '.base.ref')" # normally main
# whose consumers had merged minutes earlier was FAILed against a stale local main.)
git fetch origin "$BASE_REF"

# Fetch the PR head into a dedicated ref WITHOUT touching the session tree. pull/$PR/head
# resolves for same-repo AND cross-fork PRs, so there is no separate cross-fork branch to
# check out into your own tree (the trust inversion ADR 0052 closes — never run
# `gh pr checkout` / `git checkout` / `git switch`, which materialize the head into a working
# tree: the harness resets this cwd to the shared PRIMARY between Bash calls, so a bare checkout
# lands there and detaches the human's `main` (#2270/#1103 detach class) — §RO forbids it outright).
# Per-run ref: the PR number alone is shared, so a second review of the same PR would
# overwrite the ref mid-review and you'd verify the wrong SHA — uniquify it per invocation.
PR_REF="refs/pr/$PR-$(uuidgen)"
git fetch origin "pull/$PR/head:$PR_REF"

REVIEW_WT="$(mktemp -d)/review-head-${PR}"
# Persist the run-unique worktree path to a per-run mktemp handle so it survives the harness
# cwd/shell reset between Bash calls — $REVIEW_WT is a shell var lost across calls, and the leaf
# `review-head-${PR}` is PR-namespaced, so a later step re-deriving it from `git worktree list`
# would match a SIBLING reviewer's `review-head-<otherPR>` and pin the wrong head (the collision
# #1807: a reviewer re-read a shared pointer and found it flipped to a sibling's worktree). Mirror
# the VERDICT_FILE (#1465) / report BODY_FILE mktemp discipline: `. "$WT_FILE"` at the start of
# each later step re-sources $REVIEW_WT/$PR_REF from the run-unique handle, NEVER from
# the shared leaf name. WT_FILE itself is `$(mktemp)` — never a fixed/PR-only path. HEAD_SHA is
# NOT persisted here: it is re-resolved fresh against the live head at each later step (§HEAD),
# so only the run-unique tree/ref handles need to survive.
# §HEAD steps 1–3 (resolve the live head SHA, fetch pull/$PR/head into a per-run ref WITHOUT
# touching the session tree, assert the fetched ref IS that head, add a throwaway DETACHED head
# worktree) are the shared `pipeline-cli review-head materialize` verb (#3690 / #793 / #1807) —
# cite it, don't re-derive it. `pull/<pr>/head` resolves for same-repo AND cross-fork PRs, so there
# is no separate cross-fork branch to check out (the trust inversion ADR 0052 closes); the verb
# never runs `gh pr checkout` / `git checkout` / `git switch` (which would land the head in the
# shared PRIMARY the harness resets this cwd to and detach the human's `main` — #2270/#1103), and it
# nonce-uniques the ref per invocation so a concurrent review of the same PR can't overwrite it
# (#1807). It emits the resolved head + ref + worktree as JSON. Persist those to a per-run mktemp
# handle so they survive the harness cwd/shell reset between Bash calls (a shell var is lost across
# calls); re-source them with `. "$WT_FILE"` at each later step — NEVER re-derive from a shared leaf
# name (a `git worktree list` re-derivation matches a SIBLING reviewer's tree and pins the wrong head).
# The `--worktree` tree is a FULL checkout (ADR 0067): biome.jsonc + biome-plugins/, patches/, the
# catalog/lockfile, and everything `fate generate` needs are present, so the typecheck bootstrap is
# whole. A full checkout also lands the head's root CLAUDE.md + .claude/.decisions/.patterns — that
# leak is closed by the explicit denylist removal + absence-assert below, NOT by any pattern set.
WT_FILE="$(mktemp /tmp/review-code-wt.XXXXXX)"
{ echo "REVIEW_WT='$REVIEW_WT'"; echo "PR_REF='$PR_REF'"; } > "$WT_FILE"
# Cone-mode-minus-denylist (ADR 0067): a FULL checkout materializes the head's whole tree, so
# biome.jsonc + biome-plugins/, patches/, the catalog/lockfile, and everything `fate generate`
# needs are present — the typecheck bootstrap is whole. A full checkout (like cone mode) also
# lands the head's root CLAUDE.md + .claude/.decisions/.patterns — that leak is closed by the
# explicit denylist removal + absence-assert below, NOT by any include/cone pattern set.
git worktree add "$REVIEW_WT" "$PR_REF"
pipeline-cli review-head materialize --pr "$PR" --worktree \
| jq -r '"REVIEW_WT=\(.worktreeDir)\nPR_REF=\(.prRef)\nHEAD_SHA=\(.headSha)"' > "$WT_FILE"
. "$WT_FILE"
[ -n "${REVIEW_WT:-}" ] && [ -n "${PR_REF:-}" ] && [ -n "${HEAD_SHA:-}" ] || {
echo "FATAL: review-head materialize did not yield a head worktree — aborting (never review the base tree; §HEAD)." >&2; exit 1; }

# LAYER-2 containment (#2666): a freshly materialized worktree MUST come up pristine — assert it
# clean NOW, before the denylist `rm --cached` below deliberately dirties it. A dirty materialization
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,10 @@ CONTROL_PLANE_TOUCHED="$(gh api --paginate "repos/$REPO/pulls/$PR/files?per_page
```bash
gh api repos/$REPO/pulls/$PR \
--jq '{number, state, draft, merged, head: .head.ref, base: .base.ref, body}'
HEAD_SHA="$(gh api repos/$REPO/pulls/$PR --jq .head.sha)" # the head your verdict binds to (ADR 0058)
# Resolve the current head SHA the verdict binds to (ADR 0058) via the shared
# `pipeline-cli review-head` verb (#3690 / #793 / #1807) — `resolve` is REST-only (this gate
# reviews the preview URL, not a checked-out tree), fail-safe on a missing/closed/partial head:
HEAD_SHA="$(pipeline-cli review-head resolve --pr "$PR" | jq -r .headSha)"
```

Find the linked issue from the PR body's `Fixes #N` / `Closes #N` (the seam `write-code` writes) —
Expand Down
47 changes: 25 additions & 22 deletions claude-plugins/kampus-pipeline/skills/review-doc/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -340,13 +340,13 @@ branch cut from `origin/main` (the **base**) — so a plain full-file `Read`/`ca
reads the **pre-PR base**, and you would review the wrong file version while binding the verdict
to the right head SHA (issue [#793](https://github.com/kamp-us/phoenix/issues/793); the
false-PASS hazard). Obey [`../gh-issue-intake-formats.md`](../gh-issue-intake-formats.md) §HEAD
**before** verifying any criterion — cite it, don't re-derive the steps: resolve the live head
via REST (`gh pr view $PR --repo "$REPO" --json headRefOid -q .headRefOid`), fetch it into the
per-run `$PR_REF` + assert `git rev-parse "$PR_REF"` equals it, read every full file from the
head (`git show "$PR_REF:<path>"`) and **never** from CWD, and re-check the live head before
posting (§HEAD #4). The fetch-into-a-ref read mechanism below *is* §HEAD's read path for this
diff-only gate; the verdict (§5) must bind to the SHA whose files you actually read and assert
it read the PR head.
**before** verifying any criterion — cite it, don't re-derive the steps: run
`pipeline-cli review-head materialize --pr "$PR"` (the shared verb, #3690 / #793 / #1807) to
resolve the live head SHA via REST, fetch it into the per-run `$PR_REF`, and assert the fetched
ref IS that head; then read every full file from the head (`git show "$PR_REF:<path>"`) and
**never** from CWD, and re-check the live head before posting (§HEAD #4). The verb's
fetch-into-a-ref is §HEAD's read path for this diff-only gate; the verdict (§5) must bind to the
SHA whose files you actually read and assert it read the PR head.

Verification is grounded in the **diff**, not the PR's self-description. There is **no
test-running here** — a doc PR has no behavior to exercise; the artifact *is* the prose,
Expand All @@ -364,12 +364,15 @@ the hunk alone — and read it **read-only**, without ever switching the checkou
**Read-only on git working state** below). Fetch the head into a ref and read off that ref:

```bash
# Land the head in a per-run ref WITHOUT touching the working tree (resolves for same-repo
# AND cross-fork PRs — never `gh pr checkout` / `git checkout` / `git switch`: the harness
# resets this cwd to the shared PRIMARY between Bash calls, so a checkout lands there and
# detaches the human's `main` (#2270/#1103), and §RO forbids switching your tree outright):
PR_REF="refs/pr/$PR-$(uuidgen)"
git fetch origin "pull/$PR/head:$PR_REF"
# Land the head in a per-run ref via the shared `pipeline-cli review-head materialize` verb
# (#3690 / #793 / #1807) — cite it, don't re-derive it. Ref-only mode (no `--worktree`): it
# resolves the live head SHA (REST), fetches `pull/<pr>/head` into a nonce-uniqued per-run ref
# WITHOUT touching the working tree, and asserts the fetched ref IS that head. It never runs
# `gh pr checkout` / `git checkout` / `git switch` (which would land the head in the shared PRIMARY
# the harness resets this cwd to and detach the human's `main` — #2270/#1103; §RO). It emits the
# head + ref as JSON:
eval "$(pipeline-cli review-head materialize --pr "$PR" \
| jq -r '"PR_REF=\(.prRef); HEAD_SHA=\(.headSha)"')"

# Read the head's files off the ref — read-only, no checkout:
git show "$PR_REF:<path>" # the file's content at the PR head
Expand All @@ -378,18 +381,18 @@ git grep -n "<pattern>" "$PR_REF" # search the head tree without checking it
git update-ref -d "$PR_REF" # drop the throwaway ref when done
```

If a check genuinely needs a materialized tree (rare for a doc PR), add a **throwaway
worktree** from `$PR_REF` and **persist its run-unique path to a per-run `mktemp` handle**
so a later step recovers the exact own tree rather than re-deriving it from the shared,
PR-namespaced `review-doc-head-$PR` leaf (which would match a sibling reviewer's
`review-doc-head-<otherPR>` under a parallel fan-out and pin the wrong head — the #1807
collision; mirror the `VERDICT_FILE` (#1465) / report `BODY_FILE` `mktemp` discipline):
If a check genuinely needs a materialized tree (rare for a doc PR), pass `--worktree` to the
same verb — it adds a throwaway DETACHED head worktree (named `review-head-<pr>-*`) and emits
its path. **Persist that path to a per-run `mktemp` handle** so a later step recovers the exact
own tree rather than re-deriving it from a shared, PR-namespaced leaf (which would match a
sibling reviewer's tree under a parallel fan-out and pin the wrong head — the #1807 collision;
mirror the `VERDICT_FILE` (#1465) / report `BODY_FILE` `mktemp` discipline):

```bash
REVIEW_WT="$(mktemp -d)/review-doc-head-$PR"
WT_FILE="$(mktemp /tmp/review-doc-wt.XXXXXX)" # run-unique handle, never a fixed/PR-only path
{ echo "REVIEW_WT='$REVIEW_WT'"; echo "PR_REF='$PR_REF'"; } > "$WT_FILE"
git worktree add "$REVIEW_WT" "$PR_REF"
pipeline-cli review-head materialize --pr "$PR" --worktree \
| jq -r '"REVIEW_WT=\(.worktreeDir)\nPR_REF=\(.prRef)"' > "$WT_FILE"
. "$WT_FILE"
# Register teardown as a trap so a mid-block error still tears the throwaway tree down:
trap 'rm -rf "$REVIEW_WT"; git worktree prune' EXIT
# … later steps: `. "$WT_FILE"` re-sources $REVIEW_WT/$PR_REF after a between-call reset,
Expand Down
42 changes: 18 additions & 24 deletions claude-plugins/kampus-pipeline/skills/review-skill/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -70,31 +70,25 @@ reviewing (below), read all skill text from the head, and re-check the live head
BASE_REF="$(gh api repos/$REPO/pulls/$PR --jq '.base.ref')" # normally main — your trusted config
git fetch origin "$BASE_REF"

# §HEAD: resolve the live head via REST (never GraphQL) — the SHA the verdict binds to (ADR 0058).
HEAD_SHA="$(gh pr view "$PR" --repo "$REPO" --json headRefOid -q .headRefOid)"

# Fetch the PR head into a dedicated ref WITHOUT touching the session tree (same-repo AND
# cross-fork; never `gh pr checkout` / `git checkout` / `git switch`, which materialize the head
# into a working tree — the harness resets this cwd to the shared PRIMARY between Bash calls, so a
# bare checkout lands there and detaches the human's `main` (#2270/#1103), and §RO forbids it).
PR_REF="refs/pr/$PR-$(uuidgen)"
git fetch origin "pull/$PR/head:$PR_REF"
# §HEAD #2: confirm the fetched ref IS the resolved head before reviewing — else you'd review a
# different SHA than the verdict claims. Abort on mismatch rather than bind a stale verdict.
[ "$(git rev-parse "$PR_REF")" = "$HEAD_SHA" ] || { echo "FATAL: fetched head != resolved $HEAD_SHA — aborting" >&2; exit 1; }

REVIEW_WT="$(mktemp -d)/review-skill-head-${PR}"
# Persist the run-unique worktree path to a per-run mktemp handle so it survives the harness
# cwd/shell reset between Bash calls — $REVIEW_WT is a shell var lost across calls, and the leaf
# `review-skill-head-${PR}` is PR-namespaced, so a later step re-deriving it from `git worktree
# list` would match a SIBLING reviewer's `review-skill-head-<otherPR>` and read the wrong head's
# skill text (the #1807 collision: a reviewer re-read a shared pointer and found it flipped to a
# sibling's worktree). Mirror the VERDICT_FILE (#1465) / report BODY_FILE mktemp discipline:
# `. "$WT_FILE"` at the start of each later step re-sources these from the run-unique handle,
# NEVER from the shared leaf name. WT_FILE itself is `$(mktemp)` — never a fixed/PR-only path.
# §HEAD steps 1–3 (resolve the live head SHA via REST, fetch pull/$PR/head into a nonce-uniqued
# per-run ref WITHOUT touching the session tree, assert the fetched ref IS that head, add a
# throwaway DETACHED head worktree) are the shared `pipeline-cli review-head materialize` verb
# (#3690 / #793 / #1807) — cite it, don't re-derive it. `pull/<pr>/head` resolves same-repo AND
# cross-fork; the verb never runs `gh pr checkout` / `git checkout` / `git switch` (which would land
# the head in the shared PRIMARY the harness resets this cwd to and detach the human's `main` —
# #2270/#1103; §RO), and it internally aborts on a fetched-ref ≠ resolved-head mismatch (§HEAD #2)
# so you never review a different SHA than the verdict claims. Persist its emitted head/ref/worktree
# to a per-run mktemp handle so they survive the harness cwd/shell reset between Bash calls (a shell
# var is lost across calls); re-source them with `. "$WT_FILE"` at each later step — NEVER re-derive
# from a shared leaf name (a `git worktree list` re-derivation matches a SIBLING reviewer's tree and
# reads the wrong head's skill text — the #1807 collision). Mirror the VERDICT_FILE (#1465) / report
# BODY_FILE mktemp discipline; WT_FILE itself is `$(mktemp)`, never a fixed/PR-only path.
WT_FILE="$(mktemp /tmp/review-skill-wt.XXXXXX)"
{ echo "REVIEW_WT='$REVIEW_WT'"; echo "PR_REF='$PR_REF'"; echo "HEAD_SHA='$HEAD_SHA'"; } > "$WT_FILE"
git worktree add "$REVIEW_WT" "$PR_REF"
pipeline-cli review-head materialize --pr "$PR" --worktree \
| jq -r '"REVIEW_WT=\(.worktreeDir)\nPR_REF=\(.prRef)\nHEAD_SHA=\(.headSha)"' > "$WT_FILE"
. "$WT_FILE"
[ -n "${REVIEW_WT:-}" ] && [ -n "${PR_REF:-}" ] && [ -n "${HEAD_SHA:-}" ] || {
echo "FATAL: review-head materialize did not yield a head worktree — aborting (never review the base tree; §HEAD)." >&2; exit 1; }

# Enforce the instruction denylist EXPLICITLY (a full checkout lands the head's root CLAUDE.md
# + .claude/.decisions/.patterns): remove them, then ASSERT absent — the load-bearing isolation
Expand Down
5 changes: 5 additions & 0 deletions packages/pipeline-cli/src/registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ import {readmeGuardCommand} from "./tools/readme-guard/command.ts";
import {redactLeaksCommand} from "./tools/redact-leaks/command.ts";
import {refGuardCommand} from "./tools/ref-guard/command.ts";
import {resumePolicyCommand} from "./tools/resume-policy/command.ts";
import {reviewHeadCommand} from "./tools/review-head/command.ts";
import {roadmapCommand} from "./tools/roadmap/command.ts";
import {roadmapGuardCommand} from "./tools/roadmap-guard/command.ts";
import {settingsEnvGuardCommand} from "./tools/settings-env-guard/command.ts";
Expand Down Expand Up @@ -141,6 +142,10 @@ export const registeredTools: ReadonlyArray<RegisteredTool> = [
designTokenGuardCommand,
designInventoryCommand,
resumePolicyCommand,
// The shared PR-head-checkout the review gates cite instead of hand-copying the §HEAD
// materialization (#793 / #1807): resolve + fetch the PR's current head into a per-run ref
// (+ optional detached worktree), asserting the fetched ref IS the resolved head (ADR 0058).
reviewHeadCommand,
evalHarnessCommand,
mainSyncCommand,
refGuardCommand,
Expand Down
Loading
Loading