---
name: aicr-cross-review
description: |
  Multi-agent PR review using Claude Code, Codex, and CodeRabbit. Runs
  parallel reviews with integration impact analysis, then one cross-review
  round to a 2-of-3 consensus, with every confirmed finding adversarially
  verified by a fresh agent. Never runs the reviewed commit's code, and
  never posts unless explicitly asked. Use when asked for a thorough
  cross-review or multi-reviewer
  analysis. Requires the Codex plugin; CodeRabbit is best-effort.
  Claude Code only — uses the Workflow and Agent tools, which are not
  available in other agents.
user-invocable: true
# Reviewer lanes must not reach a skill that posts. code-review permits `gh pr comment`
# and the CodeRabbit skill auto-triggers on review tasks, so denying Skill closes the
# automatic path that removing the explicit nested call did not.
disallowed-tools: Skill
argument-hint: "<PR-number-or-URL>"
version: 0.3.22
---

# AICR Cross-Review: Multi-Agent PR Review with Consensus

Three reviewers (Claude Code, Codex, CodeRabbit) plus a targeted integration impact
analysis, cross-reviewed to 2-of-3 consensus, with every confirmed finding
adversarially verified by a fresh agent. Orchestration runs as a **Workflow**
(`scripts/workflow.mjs`).

**Claude Code only.** If the `Workflow` tool is unavailable, stop and say why — do not
fall back to another review command. `/code-review` in particular posts its result to
the PR (see Phase 2), which this skill never does without an explicit request. Named
`aicr-cross-review` so it does not shadow a contributor's global `cross-review` skill.

**When in doubt, stop.** Every check below either passes or ends the review with an
explanation. The skill never executes the reviewed commit's code, and never posts to
the PR unless you explicitly ask (Phase 5).

## Input

Raw arguments: `$ARGUMENTS`

`$ARGUMENTS` must be a PR number or a URL; both are normalized in Phase 0. There is no
no-argument mode: a fork PR cannot be found from the local branch name alone, since the
branch lives on the contributor's fork while the PR lives on `NVIDIA/aicr`. Stop and ask
for a PR reference rather than guessing. Do not write a parser; `gh` accepts both forms.

## Phase 0: Pre-flight

Only the required lanes are hard requirements. Claude, Codex and integration analysis
must work — if one fails at runtime the review reports `incomplete` and stops.
CodeRabbit is best-effort: a missing or unauthenticated CLI is not an error, its vote
slot just records `NONE`.

```bash
for tool in gh git; do
  which "$tool" >/dev/null || { echo "$tool not found — install it and retry."; exit 1; }
done
ls ~/.claude/plugins/cache/openai-codex/codex/*/scripts/codex-companion.mjs >/dev/null 2>&1 \
  || { echo "Codex companion not found. Install the Codex plugin (Settings → Extensions → Codex)."; exit 1; }
echo "Pre-flight OK."
```

If either check fails, stop and report which tool is missing. Do not fall back to
another review command.

**Resolve the PR number — before anything else needs it.**

Every `gh` call and the ref fetch are scoped to `NVIDIA/aicr` **literally**, written out
in each command. Two reasons: in GitHub's standard fork layout the local repository is
the contributor's fork, which has neither the PR nor `refs/pull/*`; and a shell variable
would not survive anyway, since each Bash call is a fresh shell.

```bash
# $ARGUMENTS must be a PR number or URL — gh accepts either.
test -n "$ARGUMENTS" || { echo "usage: /aicr-cross-review <PR-number-or-URL>"; exit 1; }
gh pr view "$ARGUMENTS" --repo NVIDIA/aicr \
  --json number,title,body,baseRefName,headRefName,headRefOid,files
```

Take `<n>` = `.number` and use that numeric value for every later temp path, scoped ref
name and `gh` call — never the raw argument. Keep the rest of the response; Phase 1 does
not re-fetch it.

**Self-review guard.** From the `files` list just fetched: if any changed path is under
`.agents/skills/aicr-cross-review/`, **stop** — the scripts you would execute are the
ones under review. Ask for a trusted checkout. This catches the accidental case only;
`SKILL.md` lives inside the reviewed repo, so it is not a security boundary.

## Phase 1: Setup

**Batch A — one parallel message:**

1. From the Phase 0 response, pin `HEAD_SHA` = `headRefOid`. Every reviewer reviews
   this exact commit. `<n>` is already resolved in Phase 0; do not re-fetch.
2. Worktree hygiene: `git worktree prune`, then `git worktree list | wc -l`. If the
   count still exceeds ~15, **stop** and ask the user to clean up before retrying.
   Do not remove worktrees yourself — a clean detached-HEAD worktree may be another
   session's active review. (Each worktree adds sandbox deny-list paths; at ~70 the
   profile exceeded the OS spawn-arg limit and every sandboxed Bash call failed with
   `E2BIG`. Recovery needs a fresh session.)
3. Reap dead runs' pinned inputs. A session killed between Batch B and Phase 5 leaks
   its two `refs/cr/*` and its temp diff file permanently — nothing else reclaims
   them, and they accumulate in the same slow way the worktrees above do.

   ```bash
   RUNS="$(git -C "<repo-path>" rev-parse --path-format=absolute --git-common-dir)/cr-runs"
   find "$RUNS" -maxdepth 1 -type f -mmin +1440 -delete 2>/dev/null || true
   git -C "<repo-path>" for-each-ref --format='%(refname)' 'refs/cr/pr*' 'refs/cr/base*' |
   while read -r REF; do
     KEY=${REF#refs/cr/}                                  # want <n>-<SID>
     case "$KEY" in pr*) KEY=${KEY#pr};; base*) KEY=${KEY#base};; esac
     case "$KEY" in *-*) ;; *) continue;; esac             # must have both components
     case "${KEY%-*}" in ''|*[!0-9]*) continue;; esac      # <n> is a PR number
     case "${KEY##*-}" in ??????) ;; *) continue;; esac    # <SID> is mktemp's six chars
     [ -e "$RUNS/$KEY" ] || git -C "<repo-path>" update-ref -d "$REF"
   done
   find "${TMPDIR:-/tmp}" -maxdepth 1 -type f -name 'cross-review-pr*.??????' -mmin +1440 -delete 2>/dev/null || true
   ```

   Liveness comes from the per-run marker Batch B drops in `cr-runs/`, not from the
   ref and not from the diff file. Both alternatives are broken:

   - **Not the ref.** `git gc` packs `refs/cr/*` into `packed-refs`, after which the
     per-ref file under `.git/refs/cr/` no longer exists and an mtime gate silently
     stops reaping — and `git fetch`, which Batch B runs, triggers `gc --auto`. The
     two substitutes fail too: `refs/cr/*` has no reflog (`core.logAllRefUpdates`
     covers only `refs/heads`, `refs/remotes`, `refs/notes`, and `HEAD`), and
     `%(creatordate)` is the *commit's* date, so a ref created a minute ago on
     yesterday's `main` reports "21 hours ago".
   - **Not the diff file.** `TMPDIR` is not stable across sessions, or even within
     one: under Claude Code's sandbox it is `/tmp/claude-<uid>`, and with the sandbox
     bypassed it is the shell default (`/var/folders/…/T/` on macOS). A reaper that
     tested for the diff file would miss a live session's file whenever the two
     disagree and delete that session's pinned refs — the one thing this skill must
     never do.

   `cr-runs/` sits next to the refs it guards, under the **common** git dir, so every
   worktree of a clone shares one view, exactly as `refs/cr/*` are shared. Git ignores
   unknown entries there, so nothing packs or prunes it and the marker keeps a real
   creation timestamp. The last `find` is only a temp-file janitor: it reclaims diff
   files in whatever `TMPDIR` this session sees, and reclaiming none is harmless.

   **The three `case` guards are the safety boundary, and all three are load-bearing.**
   A candidate must have both components, a numeric `<n>`, and a six-character `<SID>`
   before it can be deleted. Prefix-stripping alone is not enough: `refs/cr/pr*` also
   matches `refs/cr/private-ABC123`, which strips to `ivate-ABC123` and would pass a
   suffix-only check. Hand-made bookmarks (`refs/cr/2183-r5`, `refs/cr/2187-test`) are
   deliberate, often pin active work, and must survive — the only names that now
   collide are a literal `pr<digits>-<six characters>` or `base<digits>-<six
   characters>`, since the loop accepts both prefixes, so do not use either shape
   for one. `find … -delete` rather than `rm` for the same reason as everywhere else in
   this skill: managed permission policies gate `rm:*` behind a prompt. The janitor
   is `-type f` so a directory that happens to match the diff-file pattern is never
   removed.

   **The marker key is `<n>-<SID>`, never `<SID>` alone.** `mktemp` guarantees the
   full filename it returns is unique; it does not reserve the suffix. Two concurrent
   reviews of *different* PRs pass different templates, so both can be handed the same
   six characters — their refs stay distinct, but a `<SID>`-keyed marker would be one
   shared file, and whichever run finished first would delete the other's protection.
   Reviews of the *same* PR are safe whenever they share a `TMPDIR`, since `mktemp`
   guarantees distinct names within one directory. It guarantees nothing across
   directories, so same-PR runs under different `TMPDIR` roots can still collide —
   but on `$SID` itself, which makes `PRREF` and `BASEREF` collide first. That is a
   property of how Batch B derives `$SID`, not of this reaper, and is left to a
   follow-up.

   **The gate is 24 hours, and it must stay far above any real run.** Nothing enforces
   an end-to-end limit on a review: Codex gets a five-wait, ~45-minute budget in the
   Review phase and again in Cross-review, with Verify on top. A gate near the
   expected duration would let a later Phase 1 reap a *live* run's marker, then its
   refs, and the temp-file janitor would take its `DIFFPATH` with them — the run would
   destroy itself. The gate measures **age since Batch B stamped the marker, not
   inactivity** — the marker is written once and never refreshed — so a run still
   alive a day later is outside every documented budget and is treated as dead. The
   temp-file janitor is gated independently, on each diff file's own mtime, so
   refreshing the marker would not cover `DIFFPATH` either way. Since this reaper
   exists for leaks that accumulate over days, waiting a day to collect one costs
   nothing. Do not tune it down toward the expected runtime.

**Batch B — after A** (needs `HEAD_SHA` and `baseRefName`). `gh pr diff` takes no
SHA argument, so pin the diff with `git fetch`. Refs and the diff file are
**session-scoped**: two sessions reviewing the same PR must not share, overwrite, or
delete each other's pinned input.

```bash
set -euo pipefail   # a failed fetch or diff must abort, not leave an empty diff file
BASE="<baseRefName>"                    # from step 1 — never hardcode "main"
DIFFPATH=$(mktemp "${TMPDIR:-/tmp}/cross-review-pr<n>.XXXXXX")   # must end in X on macOS
SID=${DIFFPATH##*.}                     # reuse mktemp's unique suffix to scope the refs
PRREF="refs/cr/pr<n>-$SID"; BASEREF="refs/cr/base<n>-$SID"
# Liveness marker for Batch A step 3's reaper, written BEFORE the refs exist so no
# concurrent reaper can ever see a ref without its marker. Keyed by <n>-$SID — mktemp
# guarantees the full filename is unique, not the suffix, so a $SID-only key would
# collide with a concurrent review of a DIFFERENT PR. Under the common git dir, so
# every worktree of this clone shares one view.
RUNS="$(git -C "<repo-path>" rev-parse --path-format=absolute --git-common-dir)/cr-runs"
RUNMARK="$RUNS/<n>-$SID"; mkdir -p "$RUNS"; : > "$RUNMARK"
# Fetch from the canonical repo by URL, not from `origin`: in GitHub's standard fork
# layout `origin` is the contributor's fork, and refs/pull/* exist only on the canonical
# repository.
git -C "<repo-path>" fetch "https://github.com/NVIDIA/aicr.git" \
  "+refs/pull/<n>/head:$PRREF" "+refs/heads/$BASE:$BASEREF"
# Head moved → stop. Clean the refs we just created before exiting; set -e would
# otherwise abort before the names are ever printed, leaving them unreclaimable.
if [ "$(git -C "<repo-path>" rev-parse "$PRREF")" != "<HEAD_SHA>" ]; then
  git -C "<repo-path>" update-ref -d "$PRREF"; git -C "<repo-path>" update-ref -d "$BASEREF"
  find "$DIFFPATH" "$RUNMARK" -maxdepth 0 -delete
  echo "HEAD moved since setup — restart the review"; exit 1
fi
# Echo the names FIRST: under `set -e` an empty or failing diff aborts, and any
# echo below it would never run — leaking the refs and the temp file with a random
# suffix nobody recorded, which Phase 5 then cannot clean up.
echo "DIFFPATH=$DIFFPATH"; echo "PRREF=$PRREF"; echo "BASEREF=$BASEREF"; echo "RUNMARK=$RUNMARK"
git -C "<repo-path>" diff "$BASEREF...$PRREF" > "$DIFFPATH"
test -s "$DIFFPATH"                     # a real PR diff is never empty
# repoNotes source, pinned to the BASE ref — a fork PR must not be able to rewrite
# the instructions fed to the reviewer. Absent on some repos; that is fine.
git -C "<repo-path>" show "$BASEREF":.claude/CLAUDE.md 2>/dev/null || echo "(no tracked CLAUDE.md)"
# BASE_SHA is the base branch tip. Its only consumer is CodeRabbit's --base-commit,
# and the CLI resolves the merge-base itself, so this stays consistent with the
# three-dot diff above without a second baseline to keep in sync.
echo "BASE_SHA=$(git -C "<repo-path>" rev-parse "$BASEREF")"
```

Capture `DIFFPATH`, `BASE_SHA`, `PRREF`, `BASEREF`, `RUNMARK` — shell variables do not persist
between Bash calls and Phase 5 needs the ref names.

Then build `repoNotes` for the Claude reviewer only (never fed to Codex — lean-context
rule): distill the base-pinned `CLAUDE.md` plus the local overlay into 3–6 lines of the
rules most likely to catch defects in the changed paths.

The check below reduces accidental exposure, but it is **not a trust boundary**:
reviewer subagents load the checkout's `CLAUDE.md` hierarchy automatically, before any
guard here runs. Treat `repoNotes` as a relevance digest, not a sanitiser.

**For an untrusted or fork PR, run this skill from a session started in a trusted
checkout** — the same operational remedy as the self-review guard in Phase 0. Git
overwrites *ignored* files during checkout without complaint, so checking out a fork
that force-added an ignored overlay silently replaces yours.

```bash
for f in AGENTS.local.md CLAUDE.local.md; do
  [ -e "<repo-path>/$f" ] || continue
  # Skip symlinks first. The tracked-status check applies to the link, not its target,
  # so an untracked symlink pointing at a PR-tracked file would otherwise be reported
  # TRUSTED while resolving to PR-controlled instructions.
  [ -L "<repo-path>/$f" ] && { echo "SKIP $f — symlink"; continue; }
  if git -C "<repo-path>" ls-files --error-unmatch -- "$f" >/dev/null 2>&1; then
    echo "SKIP $f — tracked by this PR, not a trusted local overlay"
  else
    echo "TRUSTED $f"      # regular untracked file: safe to read
  fi
done
```

Read only the paths reported `TRUSTED`. `AGENTS.local.md` is normally a symlink to
`CLAUDE.local.md`, so it is skipped and the overlay is read through the real file —
no content is lost.

**Verify the workflow script version before Phase 2.** The script about to be passed as
`scriptPath` must contain the sentinel identifier `codexResumeJobId`:
`grep -c codexResumeJobId "<skill-dir>/scripts/workflow.mjs"` — expect a non-zero count.
If it is absent, STOP: the file is a stale or reverted copy, and running it silently
restores the old semantics (observed live: a concurrent session's git operation reverted
uncommitted skill files in a shared checkout, and a full review round ran the old script
unnoticed). The sentinel detects staleness relative to this revision only — if that
identifier is ever renamed, update this check in the same change.

## Phase 1.5: Classify and extract the change list

**Classify** the PR: `code-change` | `adr` | `config-change` | `documentation-only`.

**Extract a bounded change list** so integration analysis verifies specific items
instead of fishing across the repo:

- Exported functions/types/constants added, removed, or modified
- Config keys added or changed (`.yaml`, `.toml`, `.json`)
- Workflow inputs/triggers added or changed
- File/manifest paths renamed or restructured
- Behaviorally significant defaults changed (timeouts, versions, namespaces)

> **This skill never runs the PR's code.** No build, test, or coverage step; every
> reviewer prompt forbids it. Only trusted tools run (`git`, `gh`, the CodeRabbit CLI,
> the Codex companion). Coverage is CI's job — see Phase 3.


## Phase 2: Run the review workflow

```
Workflow({
  scriptPath: "<skill-dir>/scripts/workflow.mjs",
  args: {
    pr: <number>,
    repo: "<owner>/<name>",
    repoPath: "<local checkout path>",
    headSha: "<HEAD_SHA>",
    baseSha: "<BASE_SHA>",
    diffPath: "<DIFFPATH>",
    prType: "<classification>",
    changeList: ["<item 1>", "<item 2>"],
    repoNotes: "<3-6 line digest, optional>"
  }
})
```

Pass `changeList` as a real JSON array, not a stringified one. Every lane is
`general-purpose` and inherits the session model, so there is no model argument to
pass.

**What the workflow does** (`scripts/workflow.mjs` is the single source of truth for
the consensus mechanics):

- **Review** — Claude Code (reviews the pinned diff directly; it deliberately does
  *not* delegate to the `code-review` command, whose step 8 instructs its agent to
  `gh pr comment` the result back to the PR), Codex (two chained agents: a dispatch
  agent starts the remote background job and hands back its id, which the workflow
  immediately writes to the progress log — `Codex job <id> dispatched — review running
  remotely` — then a wait agent runs a 9-min
  bounded wait plus up to four continuation waits when the job is still running — about 45 min
  for a live job), CodeRabbit (CLI against a detached worktree at `HEAD_SHA`, explicit
  600000 ms timeout — the Bash tool caps any single call at 10 minutes, which is why
  Codex exceeds it by waiting across several calls rather than waiting longer), and integration
  analysis (bounded to `changeList`). Every lane is a
  `general-purpose` agent. All
  parallel, schema-validated, and none may execute the reviewed commit's code.
- **Merge** — dedupe by `path:line:normalized-summary:consumerPath:consumerLine`;
  duplicates merge to the highest severity and union their sources; a finding citing a file
  the reporter never listed in `filesChecked` is flagged for extra scrutiny.

  **Two lanes wording one defect differently stay separate candidates, by design.** Keying
  on location alone was tried and reverted: it did merge those duplicates, but the
  evaluation schema permits exactly one verdict per candidate id, so a merged pair of
  *distinct* same-line defects has no correct verdict — confirming the real one also
  confirms the false one, and refuting the false one dismisses the real one. Retaining both
  summaries prevented data loss but not mis-adjudication, which is the worse failure.

  Instead, candidates sharing a location — `path:line` **and** the same
  `consumerPath`/`consumerLine` — are **flagged** as possible duplicates. The consumer half
  matters: one changed declaration breaking two callers is deliberately two candidates, and
  hinting that they might be duplicates would push reviewers to collapse a distinction the
  key exists to preserve. The flag
  reaches the cross-review candidate list and the refuter prompt, so reviewers decide
  whether the two are one defect and evaluate them consistently. Equivalence stays an
  explicit judgement rather than an assumption from a shared line number.

  Merging also stops once candidates are presented: a late finding that merged into an
  already-evaluated id would inherit votes cast before it existed. Late findings always
  become their own candidate and, being unpresented, stay contested for the human.

- **Cross-review (one round, Claude + Codex only)** — each re-reviews independently
  first (anti-anchoring), then returns AGREE/DISAGREE/OPEN_QUESTION per candidate.
  CodeRabbit does *not* take part: its CLI is a slow blocking cloud call and it
  reviews Git changes generically, so it cannot adjudicate our candidate ids and a
  second run over the same commit adds no signal. Its round-1 findings still stand as its AGREE votes, so
  it can still corroborate a split it independently reported. Anything still split
  afterwards is reported as contested for you to settle.
- **Consensus rule** — confirmed = 2 of the 3 reviewer slots AGREE **with evidence**;
  integration analysis is never a reviewer slot. A round-1 finding whose evidence is
  blank or whitespace-only is dropped at intake, so it never registers its reporter as
  a source. In the cross-review round an unevidenced AGREE/DISAGREE instead aborts the
  run (`incomplete`) — dropping it would leave the reviewer's round-1 source vote to
  decide the tally.
- **Verify** — every confirmed finding goes to a fresh adversarial refuter
  (REFUTED → dismissed; UNVERIFIABLE, no result, or a verdict without a citation →
  the `unresolved` array). `consensusReached` is true only when both `contested` and
  `unresolved` are empty — a finding that reached consensus but failed verification is
  an open question, not a settled one.

  **Read `adjudication` on every contested entry.** The bucket holds two different states
  and they need different things from you. `evaluated` means the finding was presented,
  the reviewers voted, and they did not reach 2-of-3 — a genuine split, so break the tie.
  `raised-late` means it was raised *during* the cross-review round, after candidates were
  presented, so nobody cross-evaluated it and its only position is its reporter's — it just
  needs reading. Measured on a real run: 8 of 8 contested findings were `raised-late`, each
  with a single AGREE and NONE elsewhere, so the count read as eight disagreements when
  there were none. `consensusReached` counts both, deliberately: a late finding is
  unadjudicated, and letting it report consensus would be the same overstatement this
  skill exists to avoid.
- **Report incomplete and stop** — Claude, Codex and integration analysis are required
  in round 1, and Claude and Codex must each return exactly one evaluation per
  candidate in the cross-review round. A missing lane, a missing evaluation, a
  duplicate, or an unknown candidate id returns `status: "incomplete"` with the reason
  and raw unverified findings. There is no degraded-consensus mode. CodeRabbit is the
  only best-effort lane: when it does not run, its vote slot records `NONE`, which
  raises the bar (Claude and Codex must then agree) rather than lowering it.

  One deliberate exception, at the level of a **finding** rather than a lane. An
  integration finding claims a specific consumer breaks, so one lacking
  `consumerPath`/`consumerLine` cannot be verified *as an integration claim* — but if it
  still locates a defect (its own path/line) with evidence, it is a perfectly reviewable
  ordinary finding. It is therefore **demoted** — consumer fields stripped, flagged in the
  candidate list, excluded from the integration severity escalation — rather than dropped,
  with a `log()` naming what was demoted. Dropping was tried twice and cost a whole run
  each time: first when the lane returned several findings and one legitimately
  consumer-less observation failed the run; then, after per-finding dropping replaced
  that, when the lane's ONLY finding was such an observation and the zero-survivor rule
  stopped the run with all four lanes `ok` and no report produced (observed on PR 2097).
  The run still stops when every integration finding lacks even a locatable defect or
  evidence — that is the case the fail-closed rule exists for: silently dropping the
  lane's only finding once yielded `consensusReached: true` while a required lane had
  contributed nothing.

  "Contributed nothing" is measured on what survives `intake()`: a demoted finding passes
  through the same coordinate and evidence gates as every other candidate, so a
  consumer-less, whitespace-evidence finding cannot slip through demotion into a
  false-clean. One rule covers the stop: a non-empty integration result that yields no
  accepted finding — neither as an integration claim nor as a demoted ordinary finding —
  stops the run, and the message says how many went for each reason.

  **Coordinates are validated by a single shared rule**, `hasCoords`, applied to a
  finding's own `path`/`line` in `intake()` — every lane, not just integration — and to
  `consumerPath`/`consumerLine` for the integration pair. The response schema **requires**
  `path` and `line` and leaves `consumerPath`/`consumerLine` optional — deliberately, since
  only integration findings carry a consumer — but it constrains none of the four, so
  `""`, `"   "`, `0` and `-1` all satisfy it and all passed a truthiness/null check.
  Tightening the schema instead would fail a whole lane on one bad field, which is the
  all-or-nothing behavior this section exists to remove. It is one helper rather than two
  call-site conditions for a specific reason: every earlier version of this guard fixed the
  pair it was shown and left the other, and a shared rule is what stops the next field pair
  from repeating that.

  **The zero-survivor rule applies to every required round-1 batch**, not just integration.
  Claude's and Codex's round-1 findings go through `intakeBatch`, which reports what
  survived. A required lane whose round-1 findings are *all* malformed contributed
  nothing, and letting them vanish silently is the same false-clean one lane over. Mixed
  batches still proceed. CodeRabbit is exempt: a total loss there records `NONE` and
  raises the bar, exactly as a lane that never ran. Rejection reasons are counted
  separately (`unlocatable` vs `unevidenced`) rather than inferred from a subtraction, so
  the message names the defect the reader should go looking for.

  **It deliberately does NOT apply to cross-review `newFindings`.** Those go through the
  same `intakeBatch` gate — a malformed late finding still never enters the tally — but a
  total loss there is not fatal. The rule tests "this lane contributed nothing", which
  round 1 can assert because findings are the lane's whole output. In the cross-review
  round the lane's output is its *evaluations*, and the completeness gate has already
  returned `incomplete` unless the lane evaluated every presented candidate; `newFindings`
  are supplementary and volunteered. Aborting there would throw away a full set of
  adjudications and the verification round over one imprecise extra finding — the same
  disproportionate total loss seen on PR 1908, one round over. Malformed late findings are
  dropped individually and each drop is logged with its reason.

**Operational notes:**

- The workflow runs in the background — wait for its completion notification.
- If it dies mid-run, **resume, don't restart**:
  `Workflow({scriptPath: ..., resumeFromRunId: "<wf_...>"})` — completed lanes replay
  from cache. Empty or odd result → read `<transcriptDir>/journal.jsonl` first.
- **The Codex round-1 lane is two agents, deliberately.** A dispatch agent composes the
  lean Codex task, starts the background job (and owns the fast-transient retry-once
  rule, decided inside a brief ~90-second launch watch that exactly covers the
  under-60s retry window), and returns `{jobId, dispatchNote}`; the workflow then logs
  `Codex job <id> dispatched — review running remotely` and hands the id to a wait
  agent that runs the continuation-wait protocol unchanged and translates the result.
  The split exists for progress visibility: a single opaque agent call shows "running"
  from spawn, which cannot distinguish "remote job dispatched and working" from
  "dispatch never happened" — a real run sat silent for 19 minutes with no way to tell
  which. The logged job id is the visible "started" signal, and it doubles as the
  recovery handle when everything after dispatch dies: a dispatch-agent failure or a
  wait-agent loss surfaces exactly like any Codex-lane unavailability (`incomplete`,
  with `codexJobId` whenever a live job id exists). The wait agent never dispatches.
- The Codex lane fails in three distinct ways, and the dispatch and wait protocols
  treat them differently:
  - **Lookup miss** — the status call exits 1 with empty stdout and `No job found` on
    stderr. Companion state is keyed by workspace root and each Bash call is a fresh
    shell, so an unpinned lookup resolves to a different workspace and reports a live job
    as unknown; the miss is **not** evidence the job died. **Always recheck exactly once**
    with `--cwd` pinned, whether or not the missing call already carried it — two causes
    produce the identical message and only one is settled by adding the flag. The other is
    transient: in companion v1.0.2 `saveState` writes `state.json` with a plain
    `fs.writeFileSync` (truncate-then-write, no temp-and-rename) while `loadState` wraps
    `JSON.parse` in a bare `catch` returning the **default** state, whose `jobs` list is
    empty. A read landing inside that write window yields a well-formed `No job found`
    rather than an error, and the background worker is rewriting that file precisely while
    the status call runs. So an identical repeat need not return an identical answer. Once
    a pinned recheck has also missed, return `unavailable` saying the job could not be
    located — never re-dispatch (the original may still be running) and never record it as
    exhausted budget.
  - **Fast transient failure** — retried **once**, in the dispatch agent's launch
    watch (the wait agent never dispatches), and only when all three hold:
    `.job.status` is `failed` (never `cancelled`), the job died in **under 60 seconds**
    by its own timestamps, and the error names a known-retryable cause such as an upstream
    capacity rejection (`Selected model is at capacity`) or a transient dispatch fault.
    The threshold is a number rather than a judgment because the observed cases are far
    apart — a capacity rejection at ~10s against a genuine timeout at 10m19s — and an
    undefined "quickly" is how a one-retry budget erodes.
    A retry then costs seconds and these clear on their own.
    **`cancelled` is never retried** — the companion emits it only for explicit
    cancellation, so a retry would restart work someone deliberately stopped. Nor is a
    late failure: `.waitTimedOut` false only means the job became terminal before the
    inner deadline, which a failure at 8:59 also satisfies while having burned the whole
    window. An unrecognised error is not assumed retryable either.
  - **Wait elapsed, job still alive** (`.waitTimedOut` true with `.job.status` still
    `queued`/`running`) — not the end of the lane. The job is dispatched in the background
    and outlives the Bash call waiting on it, so it needs more **time**, not another
    attempt — and the protocol now gives it exactly that: **up to four continuation waits** on the
    *same* job id in fresh calls. Those are not retries; nothing is re-dispatched and no
    work is duplicated. A fifth `.waitTimedOut` is then genuinely exhausted budget, and
    the lane returns `unavailable` with the job id in the structured `jobId` field —
    required, not prose — so the result can be fetched or the run resumed later.
    Measured on a real run: the lane timed out at the full 540000 ms while the job was
    demonstrably mid-work, the job was **still** `running` long after the review was
    abandoned, and its result stayed retrievable — so reporting exhausted budget there
    discarded a required lane, and the whole review with it, over a job that had merely
    not finished.
  - **Exhausted budget** — a fifth `.waitTimedOut`, or no parseable JSON at all because
    the outer timeout killed the call (a dead broker). No retry, no further wait.

  The ceiling is not tunable — the wait runs inside a Bash call, and that tool silently
  kills any foreground command at 600000 ms. Exceeding 10 minutes requires polling across
  several calls, which is exactly what the continuation wait above does: a live job gets
  five waits, roughly forty-five minutes, without a longer single call. The budget was
  three waits until a real ~53-minute job on a +1951/−153 PR outlasted even that — hence
  five waits, and a resumable `jobId` on exhaustion instead of lost work. The inner wait is
  therefore 540000 ms,
  deliberately **below** the outer cap: were the two equal, Bash could kill the command
  before it printed its JSON, leaving no `.waitTimedOut` to classify on. That is why an
  unclassifiable kill counts as exhausted-budget rather than a fast failure — guessing
  wrong there costs another full window for nothing. The lane still reports the job id
  it was waiting on (`jobId`), so even that kill stays resumable.
- Codex is required, so a lane that is still unavailable after its retry makes the run
  report `incomplete`; re-run rather than interpreting a partial result.
- **`incomplete` with a `codexJobId` usually means the review is NOT lost.** That field
  is the Codex job the run was waiting on, surfaced top-level (alongside
  `reviewerStatus`) precisely so recovery is mechanical, not improvised. The lane
  attaches it to every unavailable wait result deliberately — losing a live id costs a
  whole review, a wasted resume costs minutes — so it can also reference a job that
  already failed or was cancelled: a poll that shows a terminal non-completed state, or
  a resume that comes back unavailable again, confirms the job is dead and the review
  must be re-run rather than resumed. For a live job: poll it
  with the companion status command until it is terminal (a background 60-second loop is
  fine — polling is cheap once the workflow is no longer holding a lane open for it).

  Resolve the companion the same way the lane does, with `-t` — the lane's messages
  refer to `$comp` but cannot export it across Bash calls:

  ```bash
  comp=$(ls -t ~/.claude/plugins/cache/openai-codex/codex/*/scripts/codex-companion.mjs | head -1)
  node "$comp" status <job-id> --cwd "<repo-path>" --json
  ```

  `ls -t` picks the most recently installed companion, which is the version the plugin
  system has active. Dropping the `-t` sorts by version *name* instead and can select a
  stale cached copy — polling a job with a different companion version than dispatched it
  returns misleading results (`status` finding a job that `result` then reports as
  unknown), which reads exactly like a dead job and is not one.

  Then resume: `Workflow({scriptPath, resumeFromRunId: "<wf_...>", args: {...prevArgs,
  codexResumeJobId: "<job id>"}})` — `prevArgs` is the previous run's args object, unchanged. The three completed lanes replay from cache, the
  Codex dispatch agent is skipped entirely (the workflow logs
  `Codex resume: waiting on existing job <id>`), the wait agent collects the existing
  job without dispatching a second one, and the
  run proceeds to cross-review and verification normally. A run interrupted *between*
  dispatch and result needs no `codexResumeJobId` at all: on `resumeFromRunId` the
  dispatch agent's cached `{jobId}` replays instantly, so the wait prompt is
  byte-identical to the original run's and the resume lands in the same wait with no
  re-dispatch. Proven live on PR 2097: a
  ~53-minute job outlasted the then-three-wait budget, and the resumed run recovered it
  with zero re-dispatched work.
- **A dead job from broker teardown needs a private-broker resume, not a plain one.**
  When a Codex lane reports the job killed by concurrent-session broker teardown
  (`statusNote` names infra-kill-by-concurrent-session-teardown — the confirmed
  `sessionRuntime` fingerprint, which W2 and W4 both run; a late `failed` with a
  reaper/null error is a symptom, not the gate, and an UNCONFIRMED cause gets an
  ordinary re-run under the shared broker instead), the job is terminal — there is
  nothing to poll and `codexResumeJobId` does not apply. A plain `resumeFromRunId`
  does not help either: the lane *completed* with its unavailable result, so the
  cache replays the failure verbatim. And a re-dispatch under the same shared broker
  (keyed to the repo path) faces the same teardown risk while the concurrent sessions
  that caused it are still running. The remedy: copy `scripts/workflow.mjs` to a
  scratch path (never edit the checked-in file), add a one-line nonce to the affected
  lane's prompt, and in the scratch copy replace `${repoPath}` with the
  session-private worktree path in that lane's `--cwd` interpolations — every
  companion command: dispatch, status, result. Edit the interpolation itself, not an
  appended prompt override: the generated prompt's literal commands pin
  `--cwd "${repoPath}"` and insist on it for every call, so an override that
  contradicts them may lose, and any call that keeps the shared path lands the
  recovered job back under the broker being torn down. Then
  `Workflow({scriptPath: "<scratch copy>", resumeFromRunId:
  "<wf_...>", args: prevArgs})`. Every other lane replays from cache; only the edited
  lane re-runs, and its job lives under a private workspace broker that no concurrent
  session's SessionEnd hook will tear down. The task prompt's pinned `git -C` reads
  still name the original repo path, so the review context is unchanged. Proven live
  2026-08-08 on PR 2097: two evaluation jobs died to teardown under the shared broker;
  the third, dispatched under a private broker, completed and the run reached
  consensus with zero re-reviewed lanes.
- **Execute the CodeRabbit lane's STEP blocks verbatim** — same commands and paths, no
  substitutions; in particular never swap the `find … -delete` cleanup for `rm` (an
  invented `rm -rf` cleanup once blocked a run for hours on a managed-policy
  confirmation prompt). The no-`rm` rule covers **every** command composed in the lane,
  ad-hoc diagnostics included — a self-written `.git/worktrees` writability probe with
  `rm -f` cleanup once blocked a round the same way. `rmdir` for empty dirs,
  `find <path> -maxdepth 0 -delete` for files, always.
- CodeRabbit slow runs: check the newest file in `~/.coderabbit/logs/` (429/queue lines
  mean cloud-side queueing) and confirm `which -a coderabbit` resolves to the
  brew-managed binary — a stale `~/.local/bin` copy shadows it.
- **A sandboxed CodeRabbit run hangs instead of failing.** The sandbox can deny the CLI
  two independent ways, and both stall at `connecting_to_review_service` until the
  timebox kills it: `~/.coderabbit` outside the write allowlist (the CLI cannot create
  its log or review store), or the `coderabbit.ai` hosts outside the allowed-hosts list.
  The lane therefore **probes both** in step 1 — writability of `~/.coderabbit` *and*
  reachability of both CLI hosts, `cli.coderabbit.ai` (startup config fetch) and
  `ide.coderabbit.ai` (the review session's WebSocket) — and runs the `coderabbit`
  command (and only that command) with sandbox bypass when any check fails. The hosts
  are probed separately because the allowlist is per-host: an entry naming only the
  config host passes the first check and still hangs step 2 on the WebSocket connect.

  **Why probe-gated bypass rather than an allowlist assumption.** Adding `~/.coderabbit`
  to the sandbox write allowlist is the narrower grant, but this skill is checked into
  the repo and has to work on a contributor's machine as written: an allowlist entry
  lives in each person's local settings, so a lane that *assumed* it would hand anyone
  who has not made the edit the silent ten-minute hang above rather than a usable lane.
  And the hang means "try sandboxed, fall back on failure" without a probe costs a full
  timebox per wrong guess. The step-1 probe settles it in milliseconds: machines with
  the allowlist entry run step 2 fully sandboxed and pay **no bypass prompt at all**;
  every other machine gets the bypass from the start, portable and self-documenting at
  the call site. If you run this often, add **both** grants — `~/.coderabbit` (and
  `~/.claude/plugins/data`, for the Codex companion's job log) to your local sandbox
  `filesystem.allowWrite`, *and* the `coderabbit.ai` hosts to the network allowlist —
  since that pair is what removes the per-round approval prompts. The skill must not
  depend on either. Granting only the filesystem half is worse than granting neither:
  it satisfies the write probe while the network stays blocked, and an earlier
  writability-only probe read that state as sandbox-clean and hung the lane for a full
  ten-minute timebox. The probe now tests both for exactly this reason.

  **Diagnose it from the log directory**, since the two denials differ there. A stall at
  `connecting` *with no new file in `~/.coderabbit/logs/`* is **filesystem** denial — a
  process killed mid-run still flushes a partial log, so zero bytes means it never created
  one. A stall *with* a log naming the allowlist (`403 Forbidden` on
  `https://cli.coderabbit.ai/public-configs.json`, `"data":"Connection blocked by network
  allowlist"`, then repeated `wss://ide.coderabbit.ai/ws` retries) is **network** denial —
  do not read the presence of a log as proof the sandbox was not the cause. A real cloud
  problem leaves a log with 429/queue lines instead. Do not read either stall as an outage
  or as contention with another session: an unsandboxed run succeeding while a sandboxed
  one hangs looks exactly like contention and is not.
- **Persisted-store fallback.** If a run still fails, the lane checks
  `~/.coderabbit/reviews/*/*/reviews/*/git.json`, whichever session produced the record.
  Acceptance is `head` plus **the pinned change itself** — never `baseCommitId`. Measured:
  the CLI writes `baseCommitId` from the base it resolves locally in that working
  directory, not from the `--base-commit` the lane passes. In one real store the same head
  carried two different values (a main tip, and a commit not on main at all), while three
  other valid records carried the stale local `main` rather than the base Phase 1 fetched
  from the canonical repo. Gating on equality discarded good reviews whenever the local ref
  lagged — the common case in the fork layout this skill supports. `baseCommitId` is now
  reported in `statusNote` and never branched on.

  What identifies the review instead is the **blob OIDs the record already stores**.
  Acceptance is layered, cheapest first, and only the last step is identity:

  1. Same paths, from `git diff --numstat -z` over the **three-dot** range Phase 1 saved.
     Two-dot would compare the base tip to the head and pull in everything that landed on
     the base after branching — on a real pair, 13 files against the valid record's 59.
     `-z` matters independently: a plain `split()` shreds a path containing a space into
     separate members and would reject every record for such a PR.
  2. Same per-file line counts, against the record's `linesAdded` / `linesRemoved`.
  3. The committed-only `lanes` shape below.
  4. The **file modes and blob OIDs** the record's own patch states, equal to what
     `git diff --raw --no-abbrev` reports for the pinned range.

  Steps 1–2 are a prefilter, never a result: two different patches collide on a numstat
  trivially — a `beta`→`BETA` edit and a `gamma`→`GAMMA` edit in the same file both report
  one added and one removed, and a review of this head against another base can leave
  exactly such a record.

  Step 4 compares OIDs rather than the stored patch text, which was the obvious move and
  is wrong. Bare `git diff` output moves with the reader's gitconfig — measured, the text
  differs under `diff.noprefix` and `diff.context`, and fails outright under
  `diff.external` — so a contributor with ordinary settings gets a permanent mismatch and
  this fallback silently never fires. That is the same works-on-my-machine class the
  sandbox decision rejected an allowlist for. Pinning flags (`--no-ext-diff`, `--no-color`,
  `-U3`, `--src-prefix`) fixes *our* side only; if the CLI produced the record under that
  same config, hardening makes the mismatch worse. Blob OIDs are content hashes: stable on
  both sides, and exact content identity rather than a rendering of it. Both git calls
  still pass `--no-ext-diff --no-color`, since the `--raw` call needs them.

  **Modes are checked before OIDs, because OIDs alone are not identity.** With a `chmod +x`
  in one commit and an edit in the next, a stored review of the *edit alone* carries the
  same path, counts, `lanes` and **both** blob OIDs as the pinned range covering both
  changes — verified. Only the modes differ: pinned `100644 → 100755` against the record's
  `100755 → 100755`. Git states modes in one of four shapes (`old mode`/`new mode`,
  `new file mode`, `deleted file mode`, or the trailing mode on the `index` line), and the
  trailing mode is absent exactly when the `old`/`new` pair is present, so the cases do not
  overlap. A record that states no mode at all cannot prove identity and is skipped.

  An entry with **no** `index` line is not automatically rejected. Git omits that line
  exactly when the blob is unchanged — for a `chmod +x` the mode lines carry the
  information instead, and the mode check above has already matched them. Such an entry is
  accepted when the pinned diff agrees the blob is unchanged (`src == dst`), and rejected
  as `no-index-line` otherwise. Rejecting on sight would have disabled this fallback for
  any PR that merely marks a script executable, which this repo does routinely. A
  rename or copy in the pinned range is rejected outright as `rename-unverifiable`, by an
  explicit status check rather than incidentally: the record is keyed on the post-rename
  path and carries no source path, so an edit-only review of that path is indistinguishable
  from one that covered the rename — measured, a pinned `R077 old.txt -> new.txt` and a
  stored `M new.txt` share modes, blobs and line counts. The scope prefilter usually rejects
  such a record first, since the CLI scopes
  its stored patch and counts to the post-rename path, so the record's numstat differs from
  the pinned range's and the scope prefilter rejects it first.

  Every entry must additionally prove it was **committed-only** — `lanes` is a boolean map
  (`{"committed": true, "uncommitted": false}`), so the check tests the values and requires
  that exact shape. Merely rejecting a truthy `uncommitted` was not enough: a record from
  an older CLI that omits `lanes`, or carries a non-dict, would pass as clean and
  contribute comments on code outside the pinned head. Absent proof of clean, skip.

  The pinned diff itself is computed **fail-closed**: an unreachable base exits non-zero
  with empty output, and treating that as an empty result would reject every record and
  then report a clean "nothing matched" — so the scan aborts with an `ERROR` line instead.
  Several records can also pass every check at once (a re-run after a transient failure
  produces a twin), so candidates are ordered by mtime and exactly one `MATCH` is emitted:
  newest wins, since a re-run supersedes what it replaced.

## Phase 3: CI status for the pinned commit

Do **not** compute coverage locally and do **not** parse CI's coverage comment. The
Merge Gate already enforces the threshold from `.settings.yaml`, forks included. The
coverage comment is posted only for same-repo PRs, carries no head SHA, and is
baselined against the last *successful* main run.

`gh pr checks` reports the PR's **current** head, not `HEAD_SHA`, and the head can move
during a long review. Confirm it first:

```bash
# Separate `if`, not `A && B || C`: gh pr checks exits nonzero for pending (8) and
# failing checks, so chaining would report "head moved" whenever CI is simply red or
# still running.
if [ "$(gh pr view <n> --repo NVIDIA/aicr --json headRefOid -q .headRefOid)" = "<HEAD_SHA>" ]; then
  gh pr checks <n> --repo NVIDIA/aicr   # 0 = green, 8 = still running, other = failing
else
  echo "head moved during review — no CI status for the reviewed commit"
fi
```

If the head moved, omit the CI line rather than reporting another commit's result.
Otherwise report one line: passing, failing (name them), or still running.

Do **not** add `--required`: before the aggregate gate job exists it prints "no required
checks reported" and exits 1, so an ordinary in-progress run looks like an error. Plain
`gh pr checks` exits 8 while running and 0 when green, and handles `skipped`/`neutral`
correctly — unlike a raw check-runs query, which counts them as failures and returns
only the first page (on a green commit: 13 false failures across 30 of 37 checks).

## Phase 4: Consensus report

Build from the workflow's return value plus the CI status line from Phase 3:

```markdown
## Cross-Review Summary for PR #<number>

**Reviewers:** Claude Code, Codex, CodeRabbit + Integration Analysis
**Head commit:** <sha> | **Consensus reached:** Yes/No
**CI for this commit:** <passing | failing: check names | still running>
<note if CodeRabbit was unavailable — it is the only best-effort lane>

### Confirmed Issues (met consensus rule; survived adversarial verification)

| # | File | Line | Severity | Description | Confirmed By |
|---|------|------|----------|-------------|--------------|

### Integration Findings (cross-cutting impact)

| # | Changed File | Consumer File | Severity | Description | Confirmed By |
|---|--------------|---------------|----------|-------------|--------------|

<only findings with a verified consumer pair (non-null `consumerPath`/`consumerLine`)
belong here. A finding flagged "demoted from integration claim" has no consumer pair —
route it to Confirmed Issues like any ordinary finding, even though its `sources`
include the integration lane; omit the section if empty>

### Unresolved (no settled disposition)

| # | File | Line | Severity | Description | Why unresolved |
|---|------|------|----------|-------------|----------------|

<from the workflow's `unresolved` array: findings that reached consensus but did not
survive adversarial verification, plus findings no reviewer cast a valid vote on; omit
the section if empty>

### Contested Issues (no 2-of-3 disposition)

Split reviewers, a lone dissent, or a finding raised during the cross-review round and
therefore never presented for evaluation.

| # | File | Line | Severity | Description | For | Against | Reasoning |
|---|------|------|----------|-------------|-----|---------|-----------|

### Dismissed Findings

<finding, who flagged it, why dismissed (incl. "failed adversarial verification: ...")>

### Open Questions

<unverifiable findings + reviewers' open questions>

### Residual Risk

<from the workflow's residualRisk array — reviewer-flagged risks that are not
findings; omit the section if empty>

### Positive Observations

<noteworthy good patterns>
```

## Phase 5: Output

**Default: do NOT post.** Present the full report in chat and stop. Do not ask
whether to post.

**Only when explicitly asked to post**, publish two layers — one **brief** summary
comment first, then one inline comment per finding that anchors to a changed line.
The detail lives inline; the summary is an index, not a second copy.

**Classify anchors before posting anything.** A finding is anchorable when its
`path` is among the PR's changed files and its `line` falls inside the head commit's
diff hunks (check against the pinned diff from Phase 1, not the mutable working
copy). This classification decides where each finding's full text goes: anchorable →
its inline comment; unanchorable → the summary, which is the only place it will
appear.

**1. Summary comment (first, brief).** The overview a reader sees before the diff:
which commit was reviewed, a short overall assessment, and how many findings follow
as inline comments — **do not list or index the individual findings here**; the
inline comments are the findings. The only finding text that belongs in the summary
is the **full text of an unanchorable finding** (and any open questions, which have
no code anchor), since the summary is the only place those will appear.
Write it to a file with the Write tool, then post with `--body-file`. Never
interpolate the report into a double-quoted shell argument — findings quote PR
content, and backticks or `$(...)` in a finding would be executed by the shell
before `gh` ever runs:

```bash
gh pr comment <n> --repo NVIDIA/aicr --body-file "<report-file>"
```

`<report-file>` is the exact path you passed to Write — a Write-tool call cannot export
a shell variable, so substitute the literal path here.

**2. Inline comments — one per anchorable finding, full detail.** Each carries
exactly one finding: the defect statement, the failure scenario, and the evidence
`path:line`. Post each one as its own call — per-finding, so one rejected anchor
cannot take down the rest. The whole payload goes through a file for quoting safety:
`path` names a changed file in the PR under review and `line` comes from reviewer
output, so both are PR-controlled — a path containing `$(...)` or backticks would
execute if interpolated into shell source. Write the payload as JSON with the Write
tool (require `line` to be a plain integer — reject anything else) and pass it with
`--input`, so no finding-controlled value ever appears in the command line:

```json
{"body": "<finding text>", "commit_id": "<HEAD_SHA>",
 "path": "<path>", "line": <line>, "side": "RIGHT"}
```

```bash
gh api repos/NVIDIA/aicr/pulls/<n>/comments --input "<payload-file>"
```

If a call is rejected despite the pre-classification (the head moved between
classification and post, a renamed path), do NOT re-anchor to a nearby line — a
comment on the wrong line reads as a claim about that line. That finding's summary
entry is now its only trace, so append the full finding text to the summary comment
(`gh api --method PATCH repos/NVIDIA/aicr/issues/comments/<summary-comment-id>
--input <payload-file>` with the updated body; capture the summary comment's id when
posting it) so no finding is left as a bare one-liner.

**Content rules for everything posted (summary and inline):**

- Post **issues only**: Confirmed Issues (without the "Confirmed By" column),
  confirmed Integration Findings, Contested Issues, Unresolved, Open Questions.
  Never post Dismissed Findings or Positive Observations.
- **The multi-agent machinery must be invisible in posted text.** Write as one
  reviewer's plain findings: never use the words "cross-review", "review agent",
  "reviewer", "consensus", "lane", "adversarial", "verification round", or any
  agent name (Claude, Codex, CodeRabbit), and no severity-label prefixes or
  vote/attribution columns. State each finding and its evidence plainly. The
  machinery vocabulary belongs to the chat report only.

## Rules

- Never post to the PR without an explicit user request.
- The consensus rule, the required-lane contract, and the single-cross-review-round
  structure live in `scripts/workflow.mjs` — keep it and this doc in sync.
- **This skill never executes the reviewed commit's code.** No builds, tests, coverage,
  package managers, or repository scripts. If a claim can only be settled by running
  something, it is an open question.
- Confirmed integration findings identifying broken consumers (a verified
  `consumerPath`/`consumerLine` pair) escalate to at least **medium** severity
  (done in-script); findings demoted from the integration lane do not.
- Severity scale: critical (must fix) > major (should fix) > medium > minor.
- Keep the report concise — actionable findings, not noise.
- Never set `dangerouslyDisableSandbox` for reviewer or companion commands; they run
  fine sandboxed. **Exactly two exceptions**, both kept in sync with the protocols in
  `scripts/workflow.mjs`, both conditional on the sandbox actually denying the write,
  and both scoped to a single command that performs no Git operation, no working-copy
  mutation, and no GitHub write. (They do *read* files — CodeRabbit necessarily reads
  the detached worktree it was pointed at. The rule bars bypassing calls that **act on**
  the working copy, not calls that read a path):
  - **Codex companion** — it writes its job log under `~/.claude/plugins/data`, which is
    sandbox-denied by default. If dispatch fails on that write, bypass for that call
    only; a machine whose sandbox allowlist covers the path never needs the bypass.
  - **CodeRabbit review** — `~/.coderabbit` is outside the default write allowlist and
    the `coderabbit.ai` hosts are outside the default network allowlist; under either
    denial a sandboxed CLI hangs at `connecting_to_review_service` until the timebox
    kills it. Step 1 of the three-step CodeRabbit protocol probes writability of
    `~/.coderabbit` and reachability of both `cli.coderabbit.ai` and
    `ide.coderabbit.ai` (the WebSocket host); when any check fails, bypass
    **step 2 only**, which is a lone `coderabbit review` command. When the
    probe passes, step 2 runs sandboxed and no bypass happens at all. Worktree setup and
    cleanup live in steps 1 and 3 and stay sandboxed always.

  Anything else stays sandboxed. In particular, never bypass a call that also performs
  `git` operations — that is why the CodeRabbit protocol is split into three calls rather
  than the single invocation it used to be.
- **The tool shell is zsh, and every prompt that hands out a shell command says so.**
  `scripts/workflow.mjs` defines a single `SHELL_CONTRACT` constant, interpolated in
  **exactly one place** — `NO_EXECUTION`, which every prompt builder composes exactly
  once. So all seven assembled prompts carry it exactly once. Do not add a second
  interpolation: `NO_EXECUTION` already embeds `PINNED_READS`, so putting it in
  `PINNED_READS` or `CODEX_LEAN` as well silently doubles it in every lane. That is how
  the first attempt got it wrong, and no block-level check can see it — the duplication
  only appears once the prompts are assembled. It covers the
  three differences that bite silently: `shopt` and other bash builtins are simply absent,
  an unmatched glob is a hard error that aborts the command list rather than passing
  through literally, and `status` is readonly. Keep it in one place — the two blocks that
  once held hand-written copies each acquired theirs only after a zsh bug had already
  shipped in them, and a new lane must not be able to omit it by accident.
- **Clean up before finishing:** `find "<the DIFFPATH echoed in Phase 1>" -maxdepth 0 -delete
  2>/dev/null || true` — idempotent, because under `set -e` a diff file already removed by an
  earlier abort path would otherwise fail the call and skip the ref cleanup below —
  and delete the
  two scoped refs captured in Phase 1 (`git -C "<repo-path>" update-ref -d "$PRREF"`,
  same for `"$BASEREF"` — use the exact names echoed there, not a guess) and the
  `RUNMARK` liveness marker (`find "<the RUNMARK echoed in Phase 1>" -maxdepth 0
  -delete 2>/dev/null || true`). Delete the marker **last**: it is what tells Phase 1
  Batch A step 3 the refs are still in use, so removing it before the refs invites a
  concurrent session's reaper into the gap. If this run is killed before any of it
  runs, step 3 reaps all three on a later run. Confirm no
  `${TMPDIR:-/tmp}/cr-rabbit.*` worktree path remains in `git worktree list` — write the
  fallback out, since setup creates the worktree under `${TMPDIR:-/tmp}` and with `TMPDIR`
  unset a bare `$TMPDIR/cr-rabbit.*` names `/cr-rabbit.*` while the leak sits in `/tmp` —
  the CodeRabbit
  lane cleans its own in step 3, but verify, since a lane killed between steps 2 and 3
  leaves one behind. Step 1 of the next run reaps `cr-rabbit.*` directories older than
  120 minutes, which bounds the leak but does not clear it now. Do not compare total
  worktree counts; concurrent sessions change the total legitimately.
  Deletion is `find … -delete` rather than `rm` throughout this skill and its workflow —
  managed (admin-deployed) permission policies commonly gate `rm` behind a confirmation
  prompt (`Bash(rm:*)` ask rules match every sub-command and override allow rules), and a
  background lane blocked on a prompt stalls the review. Do not "simplify" back to `rm`.