aicr-cross-review

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
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.

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.

# $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.

   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
        |*[!0-9]*) ;;       
        ??????) ;; *) ;;     
     [ -e  ] || git -C  update-ref -d 
   
   find  -maxdepth 1 - f -name  -mmin +1440 -delete 2>/dev/null || 
   

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.

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

 [  !=  ]; 
  git -C  update-ref -d ; git -C  update-ref -d 
  find   -maxdepth 0 -delete
   ;  1




 ;  ;  ;  
git -C  diff  > 
 -s                      


git -C  show :.claude/CLAUDE.md 2>/dev/null ||  



 

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.

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.

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 saying the job could not be located — never re-dispatch (the original may still be running) and never record it as exhausted budget.

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:

# 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:

## 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 "     —
        ,    ``
   ;     >



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





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 |
|---|------|------|----------|-------------|-----|---------|-----------|

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:

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:

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

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.