Skip to content

feat(skills): add aicr-cross-review skill for multi-agent PR review#1915

Open
yuanchen8911 wants to merge 1 commit into
NVIDIA:mainfrom
yuanchen8911:add-cross-review-skill
Open

feat(skills): add aicr-cross-review skill for multi-agent PR review#1915
yuanchen8911 wants to merge 1 commit into
NVIDIA:mainfrom
yuanchen8911:add-cross-review-skill

Conversation

@yuanchen8911

@yuanchen8911 yuanchen8911 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds aicr-cross-review as a shared agent skill under .agents/skills/ — a three-reviewer consensus workflow (Claude Code, Codex, CodeRabbit) with integration impact analysis and adversarial verification of every confirmed finding. Also adds the skill to the Available Skills catalog in docs/contributor/skills.md.

Two files: SKILL.md (phases 0–5 orchestration, 395 lines) and scripts/workflow.mjs (the Workflow-tool consensus engine, 731 lines).

Motivation / Context

Contributes the cross-review workflow used in this repo to the shared skill set so all contributors can invoke it via /aicr-cross-review or $aicr-cross-review. Named aicr-cross-review (not cross-review) to avoid shadowing a contributor's global cross-review skill.

Fixes: N/A
Related: #1911

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Refactoring (no functional changes)
  • Build/CI/tooling

Component(s) Affected

  • CLI (cmd/aicr, pkg/cli)
  • API server (cmd/aicrd, pkg/server)
  • Recipe engine / data (pkg/recipe)
  • Bundlers (pkg/bundler, pkg/component/*)
  • Collectors / snapshotter (pkg/collector, pkg/snapshotter)
  • Validator (pkg/validator)
  • Core libraries (pkg/errors, pkg/k8s)
  • Docs/examples (docs/, examples/)
  • Other: .agents/skills/ (agent skill definitions)

Implementation Notes

The skill never executes the reviewed commit's code. There is no build, test, or coverage step. The execution rules appear in all six prompts — both round-1 review lanes, CodeRabbit, integration, the cross-review round, and the adversarial verifier — because these are shell-capable agents and running tests is normal review behaviour, so the prohibition has to be stated rather than assumed. CODEX_DISPATCH additionally requires every dispatched Codex task to carry the rules verbatim, since Codex runs in its own shell and inherits nothing from the wrapper prompt. The rule permits the trusted commands a prompt explicitly prescribes — notably CodeRabbit's detached worktree — so it does not contradict the lane it governs. Evidence comes from the saved diff plus pinned git show / git grep reads.

Coverage is deliberately left to CI. The Merge Gate already enforces the project-wide threshold from .settings.yaml and fails the run when coverage is below it, on fork PRs included. An earlier revision of this PR ran coverage locally in throwaway worktrees; that was the skill's only code-execution surface and the source of a disproportionate share of its complexity (fork gating, timeouts, baseline selection, floor attribution). Phase 3 reports CI status instead. It re-confirms headRefOid still equals the pinned HEAD_SHA before reporting, because gh pr checks reads the PR's current head and the head can move during a 20-minute review; if it moved, the CI line is omitted rather than attributed to the wrong commit. --required is deliberately not used — before the aggregate gate job exists it exits 1 with "no required checks reported", so a normal in-progress run looks like a failure. Plain gh pr checks exits 8 while running and 0 when green, and handles skipped/neutral correctly, unlike a raw check-runs query (on a green commit: 13 false failures across 30 of 37 checks). The head check is an explicit if rather than A && B || C, because chaining would report "head moved" whenever gh pr checks simply exited nonzero for pending or failing checks with the SHA still matching. It does not parse CI's coverage comment: that comment is posted only for same-repo PRs, carries no head SHA, and is baselined against the last successful main run, so it is absent on fork PRs and can be silently stale elsewhere.

Every gh call and the PR-ref fetch name NVIDIA/aicr literally rather than carrying a shell variable — each Bash call is a fresh shell, and the local repository is the contributor's fork in GitHub's standard layout, which has neither the PR nor refs/pull/*. A PR number or URL is required; there is no no-argument mode, because a fork PR cannot be identified from the local branch name alone. The reference is normalized in Phase 0, so .number is available to the self-review guard that runs immediately after — previously the guard consumed <n> a phase before it existed. Pre-flight and environment failures stop with a reason rather than suggesting /code-review, which posts its result to the PR.

Availability — report incomplete and stop:

  • All four lanes run as general-purpose agents, and the skill sets disallowed-tools: Skill so a reviewer lane cannot auto-invoke a skill that posts. The integration lane no longer uses Explore, which reads excerpts to locate code and is explicitly not a review agent; with Explore gone every lane inherits the session model and the model argument is removed.
  • Claude, Codex and integration analysis are required in round 1. Integration does not vote, but it produces candidates ordinary diff review misses, so making it optional would silently narrow the review this skill advertises.
  • Claude and Codex must each return exactly one evaluation per candidate in the cross-review round. A missing evaluation, a duplicate, or an unknown candidate id returns incomplete — without this, a candidate nobody actually evaluated could ride its initial source votes to confirmed.
  • Findings raised during the cross-review round stay contested. They were never presented to anyone, so two lanes independently raising the same late finding must not reach two AGREEs from source votes alone.
  • An AGREE or DISAGREE returned with blank evidence is fatal, not merely discarded. With no stored evaluation the reviewer falls back to its round-1 source vote, so a silently dropped verdict would still decide the tally.
  • CodeRabbit runs once and is best-effort. When it does not run, its fixed vote slot records NONE, which raises the bar (Claude and Codex must then agree) rather than lowering it. It is correspondingly not a pre-flight requirement, and it runs on a general-purpose agent rather than the CodeRabbit plugin agent type, so an unavailable CLI degrades instead of blocking. No dynamic participants, no quorum arithmetic, no retries, no arbitration.

Consensus integrity:

  • Confirmed = 2 of the 3 reviewer slots AGREE with evidence; integration analysis is never a reviewer slot. Dismissal is symmetric and needs two evidenced DISAGREEs — a lone dissent leaves the finding contested, so one slot cannot bury an issue only it ever judged. A finding whose evidence is blank or whitespace-only is rejected at intake, so it never registers its reporter as a source — JSON Schema required guarantees the field exists, not that it says anything.
  • Integration findings must carry both consumerPath and consumerLine; one lacking either stops the run. Integration is a required lane, so dropping its malformed output produced a result that looked clean and reported consensusReached: true while that lane had contributed nothing. JSON Schema cannot make them conditionally required, and the verifier only traces a consumer when the coordinates exist, so without them the finding's central claim would reach consensus unverified.
  • Pinned-read recipes put their placeholders in single quotes and send the pattern through git grep -e. Double quotes stop globbing but still expand $(...) and backticks, so a PR-controlled filename like docs/$(id).md would execute — single quotes are the only literal form in sh/bash/zsh. The recipe says so inline, because the earlier double-quoted version looked correct. A path that cannot be read this way (one containing a single quote) makes the lane return unavailable rather than an open question - open questions do not block consensus, so the alternative was a clean-looking run in which nothing was inspected.
  • One consumer per finding: the dedupe key includes the consumer coordinates, so one changed declaration breaking two callers stays two candidates and each is verified independently. Because those coordinates are part of the key, a key match implies they are equal — so no consumer merging is needed and none is done.
  • Every confirmed finding goes to a fresh adversarial refuter. UNVERIFIABLE, a dead verifier, or a CONFIRMED/REFUTED verdict with no citation lands in an emitted unresolved array that blocks consensusReached.

Commit pinning:

  • PR refs are fetched from the canonical repository by URL, not from origin: in GitHub's standard fork layout origin is the contributor's fork and carries no refs/pull/*. The diff is generated from refs/pull/N/head and the PR's actual baseRefName into session-scoped refs, asserted against the headRefOid captured at setup.
  • Every reviewer prompt, in every round, reads via git show <headSha>:<path> and searches via git grep <pattern> <headSha> — never the mutable working copy, which may be on a different branch during concurrent sessions.
  • repoNotes is distilled from the base-pinned CLAUDE.md. The local-overlay check is accident-reduction, not a trust boundary — reviewer subagents load the checkout's CLAUDE.md hierarchy before any guard runs, so an untrusted PR needs a session started from a trusted checkout. The overlay is read only after skipping symlinks and confirming git does not track the path: .gitignore does not stop git add -f, and the tracked-status check applies to a link rather than its target, so an untracked symlink resolving to a PR-tracked file would otherwise pass as trusted.

Safety:

  • The Claude lane performs its own review rather than delegating to the code-review command: that command's step 8 instructs its agent to gh pr comment the result back to the PR and carries Bash(gh pr comment:*) in allowed-tools. This skill never posts unless you explicitly ask (Phase 5).
  • Filesystem paths interpolated into generated shell commands are quoted — both in the workflow's reviewer prompts and in SKILL.md's setup, overlay-validation and cleanup blocks — so a checkout or temp directory containing spaces does not split into multiple shell arguments and fail a required lane.
  • Explicitly requested posting uses gh pr comment --body-file against a file written with the Write tool, never --body "...". Finding text quotes PR content, so backticks or $(...) in a finding would otherwise be evaluated by the shell before gh runs.
  • If the PR under review modifies this skill, the review stops and asks for a trusted checkout. Stated as an accident-catcher, not a security boundary — SKILL.md lives inside the reviewed repo.
  • The frontmatter, the SKILL.md consensus rule, and the in-prompt text are kept in sync with the implementation: they describe one cross-review round, CodeRabbit as best-effort, and an unevidenced cross-review verdict aborting the run. Drift between these and workflow.mjs has been the most frequent defect class in this PR's review, so the doc states the contract the code actually enforces rather than an earlier design.

Testing

No Go code affected — skill-only change. Skipping make qualify per project policy for skill/doc-only PRs (CLAUDE.local.md); no test, e2e, or Go lint surface can regress from it.

Verified:

  • workflow.mjs parses and was executed end-to-end against stubbed agent/parallel harnesses covering: CodeRabbit unavailable → review completes with its slot recorded NONE; integration lane unavailable → incomplete; a cross-review lane returning 0 of 1 evaluations → incomplete; an unknown candidate id → incomplete; a blank-evidence AGREE in the cross-review round → incomplete; blank-evidence rejection at intake; two consumers separated into distinct candidates verified independently; UNVERIFIABLE → unresolved; REFUTED → dismissed; consensusReached: false while unresolved is non-empty. Candidate accounting balanced in every run — no finding lost between the emitted arrays.
  • Dead code is removed on proof, not inspection: the consumer-backfill branch in addCandidate was shown unreachable by enumerating every key collision (0 of 9 could fire), and the CodeRabbit arbitration branch by enumerating all 128 reachable vote states.
  • That harness caught three defects static checks could not: a declaration-order (TDZ) ReferenceError; a null-dereference introduced when the participant list became a fixed slot list; and a candidate being confirmed from its round-1 source votes after both cross-review lanes returned blank-evidence verdicts that were logged as "not counted".
  • Prompt coverage of the execution rules is asserted programmatically rather than by inspection: a check confirms the fragment is present in all six prompt builders and that CODEX_DISPATCH carries the repeat-verbatim instruction. An earlier revision of this PR described that rule as applied when an edit had silently failed to write, so it is now verified by reading the file back.
  • The CodeRabbit arbitration branch was proven dead by exhaustive enumeration of all 128 reachable vote states, and removed rather than maintained.
  • Codex companion contract checked against the installed codex/1.0.2: status <job> --wait --timeout-ms --poll-interval-ms exists, and job state is at .job.status, not a top-level status field.
  • gh api placeholder substitution confirmed to cover {owner}/{repo}/{branch} only; the inline review-POST recipe that relied on {number} was removed rather than repaired.
  • CI behavior verified against on-push-comment.yaml (fork PRs skipped; baseline is the last successful main run) and .github/actions/go-coverage/action.yml (threshold enforced, forks included) before deciding to drop the local coverage lane. The Phase 3 command was validated against this PR's own head commit in both the in-progress and green states.

Not verified: no live end-to-end run against a real PR. The Codex --wait invocation and the CodeRabbit --base-commit block are still untested against the live tools.

Risk Assessment

  • Low — Isolated change, well-tested, easy to revert
  • Medium — Touches multiple components or has broader impact
  • High — Breaking change, affects critical paths, or complex rollout

Rollout notes: N/A

Checklist

  • Tests pass locally (make test with -race) — N/A, no Go code changed
  • Linter passes (make lint) — N/A, no Go or YAML source changed
  • I did not skip/disable tests to make CI green
  • I added/updated tests for new functionality — N/A, no Go code; behaviour is covered by the stubbed workflow harness described under Testing
  • I updated docs if user-facing behavior changed
  • Changes follow existing patterns in the codebase
  • Commits are cryptographically signed (git commit -S) — GPG signing info

@yuanchen8911
yuanchen8911 requested a review from a team as a code owner July 24, 2026 20:45
@yuanchen8911 yuanchen8911 added the theme/ci-dx CI pipelines, developer experience, and build tooling label Jul 24, 2026
@yuanchen8911
yuanchen8911 force-pushed the add-cross-review-skill branch from 8da1b78 to 3b76441 Compare July 24, 2026 20:49
@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds the aicr-cross-review skill and documents its phased multi-agent review process. The new workflow coordinates Claude Code, Codex, CodeRabbit, and integration analysis; aggregates findings through cross-review consensus and adversarial verification; and emits structured results. Supporting scripts monitor Codex jobs and measure Go coverage in isolated worktrees. The contributor skills table now lists the new skill.

Estimated code review effort: 4 (Complex) | ~60 minutes

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly states the main change: adding the aicr-cross-review skill for multi-agent PR review.
Description check ✅ Passed The description matches the changeset and summarizes the new skill, workflow, and docs update.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@yuanchen8911
yuanchen8911 force-pushed the add-cross-review-skill branch 2 times, most recently from 0058899 to 0ab5828 Compare July 24, 2026 20:59

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.agents/skills/aicr-cross-review/scripts/codex-watch.sh:
- Around line 57-67: Move the STATEKEY and OUR_JOB_ID environment-variable
assignments from the node side to the consuming python3 side of both pipelines:
the reaper probe around probe and the job-status probe around job_phase. Ensure
the Python filters read the actual state key and job ID from os.environ,
preserving the existing CODEX_DEAD, CODEX_ALIVE, and job-phase behavior.
- Line 40: The polling calls in the watchdog, including the workspace-root
lookup around node "$comp" and the reaper invocation near the corresponding
polling logic, lack bounded execution. Wrap each node "$comp" and node "$REAPER"
call with the repository’s supported timeout mechanism, preserving their
existing output parsing, error suppression, and polling behavior while ensuring
a hung IPC call cannot block the watchdog indefinitely.

In @.agents/skills/aicr-cross-review/scripts/coverage-check.sh:
- Around line 22-23: Make PRREF unique for each invocation of the coverage-check
script instead of deriving it solely from PRNUM. Update the PRREF assignment
near WORK and the corresponding ref cleanup/use around the later ref-handling
logic, incorporating the per-run WORK identifier or another invocation-unique
value while preserving the existing PR association and lifecycle.

In @.agents/skills/aicr-cross-review/scripts/workflow.mjs:
- Around line 60-83: Update EVAL_SCHEMA to conditionally require evidence
whenever an evaluation verdict is AGREE or DISAGREE, while allowing it for
OPEN_QUESTION to remain optional. In tally(), filter or otherwise gate
AGREE/DISAGREE counts through evidenceFor() before applying the confirmation and
disagreement thresholds, including the primary two-of-three path.
- Around line 316-341: Update addCandidate so findings sharing the same path and
line are merged only when their substantive content matches; preserve separate
candidates for divergent summaries, evidence, or impact. Ensure each distinct
finding retains its own data and source, while identical findings continue to
share a candidate.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 7a28a6c8-ef05-48e9-806f-0a9c05fa6af9

📥 Commits

Reviewing files that changed from the base of the PR and between 7e0f4a1 and 3b76441.

📒 Files selected for processing (5)
  • .agents/skills/aicr-cross-review/SKILL.md
  • .agents/skills/aicr-cross-review/scripts/codex-watch.sh
  • .agents/skills/aicr-cross-review/scripts/coverage-check.sh
  • .agents/skills/aicr-cross-review/scripts/workflow.mjs
  • docs/contributor/skills.md

Comment thread .agents/skills/aicr-cross-review/scripts/codex-watch.sh Outdated
Comment thread .agents/skills/aicr-cross-review/scripts/codex-watch.sh Outdated
Comment thread .agents/skills/aicr-cross-review/scripts/coverage-check.sh Outdated
Comment thread .agents/skills/aicr-cross-review/scripts/workflow.mjs
Comment thread .agents/skills/aicr-cross-review/scripts/workflow.mjs Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.agents/skills/aicr-cross-review/scripts/codex-watch.sh:
- Around line 72-80: Update the job_phase probe’s JSON parsing in the heredoc to
fail closed: when node status execution fails, produces empty output, or JSON
parsing fails, emit RUNNING (or the established distinct error token) instead of
DONE. Preserve DONE only when valid status data is parsed and no matching job
remains, and align this behavior with the reaper probe’s safe failure default.

In @.agents/skills/aicr-cross-review/scripts/coverage-check.sh:
- Around line 81-82: Update the coverage-threshold comment in the script to
match the current .settings.yaml value of 80%, while preserving the instruction
to rely on the configured quality.coverage_threshold rather than hardcoding it
in logic.

In @.agents/skills/aicr-cross-review/scripts/workflow.mjs:
- Around line 89-98: Update REFUTE_SCHEMA validation and the verify-phase
handling around the verdict application logic so CONFIRMED and REFUTED responses
require non-empty evidence, while UNVERIFIABLE may retain its existing evidence
behavior. Reject or route verdicts missing required evidence before accepting
them, preserving the existing reason requirement and verdict-specific handling.

In @.agents/skills/aicr-cross-review/SKILL.md:
- Around line 167-211: Add a dedicated Residual Risk section to the Phase 4
consensus report template in SKILL.md, positioned alongside the other findings
sections. Define a placeholder that renders the aggregated residualRisk entries
returned by workflow.mjs, ensuring this collected data has an explicit
destination in the generated markdown.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 74048a37-be66-44d4-8458-392c2154c70b

📥 Commits

Reviewing files that changed from the base of the PR and between 3b76441 and 0ab5828.

📒 Files selected for processing (5)
  • .agents/skills/aicr-cross-review/SKILL.md
  • .agents/skills/aicr-cross-review/scripts/codex-watch.sh
  • .agents/skills/aicr-cross-review/scripts/coverage-check.sh
  • .agents/skills/aicr-cross-review/scripts/workflow.mjs
  • docs/contributor/skills.md

Comment thread .agents/skills/aicr-cross-review/scripts/codex-watch.sh Outdated
Comment thread .agents/skills/aicr-cross-review/scripts/coverage-check.sh Outdated
Comment thread .agents/skills/aicr-cross-review/scripts/workflow.mjs
Comment thread .agents/skills/aicr-cross-review/SKILL.md

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.agents/skills/aicr-cross-review/SKILL.md:
- Around line 111-121: Update the Go coverage instructions around
coverage-check.sh to require explicit AskUserQuestion confirmation before
executing PR-supplied Go code, unless the user has already explicitly authorized
it. If confirmation is not granted, skip coverage and report that it was skipped
due to missing authorization; retain the existing background execution and
HEAD_SHA validation for authorized runs.
- Line 90: Update the workflow instructions around the pinned diff and Phase 2
verification to create a detached worktree at HEAD_SHA, pass that immutable
checkout as the file-read workspace, and clean it up after review. Replace the
current repoPath/git show preference with this required workflow so all reads
correspond to the pinned commit.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: f82047a1-a6ff-4cc1-bec4-5407ec89cac8

📥 Commits

Reviewing files that changed from the base of the PR and between 0ab5828 and f526f2f.

📒 Files selected for processing (4)
  • .agents/skills/aicr-cross-review/SKILL.md
  • .agents/skills/aicr-cross-review/scripts/codex-watch.sh
  • .agents/skills/aicr-cross-review/scripts/coverage-check.sh
  • .agents/skills/aicr-cross-review/scripts/workflow.mjs

Comment thread .agents/skills/aicr-cross-review/SKILL.md Outdated
Comment thread .agents/skills/aicr-cross-review/SKILL.md Outdated
@yuanchen8911
yuanchen8911 force-pushed the add-cross-review-skill branch from f526f2f to 22c062a Compare July 24, 2026 22:07

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.agents/skills/aicr-cross-review/scripts/codex-watch.sh:
- Around line 33-35: Validate MAX as a non-negative integer and INTERVAL as a
positive integer immediately after parsing the arguments, exiting with the usage
message for invalid values. In the polling loop, calculate the remaining budget
from elapsed time and sleep for the smaller of INTERVAL and that remaining
duration, avoiding sleep once MAX has been reached.
- Around line 61-70: Update the watchdog’s reaper probe and companion-status
pipeline assignments so each runs within an if-not-success branch, preventing
set -euo pipefail from terminating the script. Treat reaper or JSON parsing
failures as no probe result so the loop continues, and treat companion-status
failures as RUNNING. Preserve the existing successful status parsing and
fallback behavior.

In @.agents/skills/aicr-cross-review/scripts/workflow.mjs:
- Around line 310-341: Update the filesChecked membership test in the
participant findings loop around addCandidate so suffix matches require a
path-separator boundary, preventing basename-only matches across unrelated
files. Preserve matching for exact paths and valid relative-path suffixes, while
ensuring genuinely unchecked files still receive the scrutiny flag.

In @.agents/skills/aicr-cross-review/SKILL.md:
- Around line 164-170: Update the “Merge” bullet to document the deduplication
key used by workflow.mjs addCandidate: path, line, and the normalized summary
via normSummary(summary), while preserving the existing multi-source tracking
behavior.
- Around line 97-107: Update the diff-generation block in the cross-review setup
to use the base branch captured as baseRefName in step 1 instead of hardcoding
origin/main. Resolve or construct the corresponding remote base reference for
the git diff while preserving the HEAD_SHA pinning and temporary ref cleanup
behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 7b2164ee-d330-4739-b0cf-2a54fa1b3607

📥 Commits

Reviewing files that changed from the base of the PR and between f526f2f and 22c062a.

📒 Files selected for processing (5)
  • .agents/skills/aicr-cross-review/SKILL.md
  • .agents/skills/aicr-cross-review/scripts/codex-watch.sh
  • .agents/skills/aicr-cross-review/scripts/coverage-check.sh
  • .agents/skills/aicr-cross-review/scripts/workflow.mjs
  • docs/contributor/skills.md

Comment thread .agents/skills/aicr-cross-review/scripts/codex-watch.sh Outdated
Comment thread .agents/skills/aicr-cross-review/scripts/codex-watch.sh Outdated
Comment thread .agents/skills/aicr-cross-review/scripts/workflow.mjs
Comment thread .agents/skills/aicr-cross-review/SKILL.md Outdated
Comment thread .agents/skills/aicr-cross-review/SKILL.md Outdated
@yuanchen8911
yuanchen8911 force-pushed the add-cross-review-skill branch 2 times, most recently from aac74dd to 1c0be23 Compare July 24, 2026 22:34

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
.agents/skills/aicr-cross-review/SKILL.md (1)

211-251: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Phase 4 template still has no section for residualRisk, and now also drops per-candidate flags.

workflow.mjs collects and returns residualRisk at the top level (still no home in this template — previously flagged, unresolved), and separately attaches scrutiny flags to every emitted candidate (e.g. "reported this file without listing it in filesChecked — scrutinize", workflow.mjs lines 358-361, 560) — but the template has no place to surface either, so both are effectively produced and then discarded before reaching the human-readable report.

📝 Proposed fix: add sections for both fields
 ### Open Questions

 <unverifiable findings + reviewers' open questions>

+### Residual Risk
+
+<risk that remains even after fixes — e.g. known limitations reviewers flagged as acceptable but worth tracking>
+
+### Scrutiny Flags
+
+<findings flagged during merge/verification for extra scrutiny, e.g. files cited but absent from a reporter's filesChecked>
+
 ### Test Coverage (Go PRs only)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.agents/skills/aicr-cross-review/SKILL.md around lines 211 - 251, Update the
Cross-Review Summary template to add dedicated sections for top-level
residualRisk and per-candidate flags. Ensure the output schema preserves and
renders both fields in the human-readable report, alongside the existing
findings and candidate-related content.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.agents/skills/aicr-cross-review/SKILL.md:
- Around line 112-132: Use the captured baseRefName throughout baseline
selection: in .agents/skills/aicr-cross-review/SKILL.md lines 112-132, fetch and
diff against origin/<baseRefName> instead of hardcoded main and pass
<baseRefName> to the Phase 1.5 coverage-check.sh invocation; in
.agents/skills/aicr-cross-review/scripts/coverage-check.sh lines 49-61, accept
an optional base-ref argument defaulting to main and use it for the base fetch
and worktree instead of origin/main.
- Around line 182-194: Update the Codex references in the “Review” and
“Operational notes” sections to match workflow.mjs: describe Codex as running
through the general-purpose agent with the mandatory CODEX_DISPATCH protocol,
and remove the stale codex:codex-rescue and optional-dispatch claims. Preserve
the existing workflow, recovery, and reviewer-count guidance.

---

Duplicate comments:
In @.agents/skills/aicr-cross-review/SKILL.md:
- Around line 211-251: Update the Cross-Review Summary template to add dedicated
sections for top-level residualRisk and per-candidate flags. Ensure the output
schema preserves and renders both fields in the human-readable report, alongside
the existing findings and candidate-related content.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 4f077e4f-783e-4968-be39-cf91a0441fb1

📥 Commits

Reviewing files that changed from the base of the PR and between 22c062a and aac74dd.

📒 Files selected for processing (5)
  • .agents/skills/aicr-cross-review/SKILL.md
  • .agents/skills/aicr-cross-review/scripts/codex-watch.sh
  • .agents/skills/aicr-cross-review/scripts/coverage-check.sh
  • .agents/skills/aicr-cross-review/scripts/workflow.mjs
  • docs/contributor/skills.md

Comment thread .agents/skills/aicr-cross-review/SKILL.md Outdated
Comment thread .agents/skills/aicr-cross-review/SKILL.md Outdated
@yuanchen8911
yuanchen8911 force-pushed the add-cross-review-skill branch 14 times, most recently from d3e2699 to ec1aa4a Compare July 25, 2026 01:23
@yuanchen8911

Copy link
Copy Markdown
Contributor Author

Remaining "never posts" claim fixed (025fd8f)

Confirmed — I fixed the frontmatter last round but missed the same claim in the intro paragraph, which still read "The skill never executes the reviewed commit's code and never posts" with no qualifier. It now reads "never posts to the PR unless you explicitly ask (Phase 5)".

I audited the whole surface rather than just the reported line, matching across line wraps since that is why the first pass missed it. Four claims exist and all are now correct:

  • frontmatter: "never posts unless explicitly asked"
  • intro: "never posts to the PR unless you explicitly ask (Phase 5)"
  • Rules: "Never post to the PR without an explicit user request"
  • workflow.mjs: "this skill must never post by default" (a comment on the Claude lane)

One unqualified "never" is intentional and stays: "Never post Dismissed Findings or Positive Observations." That is absolute even when posting is requested — dismissed findings and praise are local report content, not PR content.

The PR description carried the same softer wording and was updated to match.

@yuanchen8911
yuanchen8911 force-pushed the add-cross-review-skill branch from 025fd8f to 5a1ff55 Compare July 26, 2026 23:44
@yuanchen8911

Copy link
Copy Markdown
Contributor Author

Path quoting completed and a posting injection fixed (5a1ff55)

Both confirmed.

Unquoted paths in SKILL.md. Correct, and the PR-body claim was an overclaim on my part: I quoted the workflow's generated prompts last round and described it as covering generated shell commands, but SKILL.md's own setup, overlay-validation and cleanup blocks still used bare git -C <repo-path> in 9 places. Setup would fail before workflow.mjs ever ran. All 9 are quoted now, and the PR description has been corrected to say which surfaces are covered.

Shell evaluation on explicit posting — this one was worth catching. Reproduced against a realistic finding body:

report text: Use `id` to check the caller, and $(whoami) is wrong here
result:      Use uid=502(yuanc) gid=20(staff) groups=... to check the caller, and yuanc is wrong here

Both the backticks and $(...) were evaluated before gh would have run. The severity comes from provenance, as you noted: finding text quotes the reviewed PR's own content, so a PR could place a command substitution in code that a reviewer then quotes verbatim into a finding — and the one operation that runs it is the one the user explicitly asked for.

Phase 5 now instructs writing the filtered summary to a file with the Write tool and posting with gh pr comment <n> --repo <owner>/<repo> --body-file "$REPORT_FILE", with the reason stated inline so it does not get "simplified" back later. Verified the payload above round-trips through a file verbatim with no evaluation.

I also swept for other unquoted path placeholders in shell blocks; the two remaining occurrences were already quoted.

Agreed on rejecting the cleanup traps, extra validators, schema tightening, repoNotes propagation, and lint configuration.

Still no live end-to-end Workflow run.

@yuanchen8911
yuanchen8911 force-pushed the add-cross-review-skill branch from 5a1ff55 to f3502b1 Compare July 27, 2026 00:00
@yuanchen8911

Copy link
Copy Markdown
Contributor Author

Symlink trust bypass and undefined variable fixed (f3502b1)

Both confirmed; the first was reproduced.

Local-overlay trust bypass. The tracked-status check applies to the link, not its target, so an untracked symlink resolving to a PR-tracked file passed as trusted. Reproduced in a scratch repo where a fork force-adds the ignored overlay:

TRUSTED AGENTS.local.md  <-- would be read
SKIP CLAUDE.local.md (tracked)
reading the 'TRUSTED' symlink yields: PR-CONTROLLED INSTRUCTIONS

The guard I added last round worked correctly on the real file and was defeated by the alias pointing at it. Your point about checkout behaviour is what makes it reachable: git overwrites ignored files during checkout without complaint, so a reviewer who checks out a fork branch to look at it silently acquires the PR's overlay.

Fixed by skipping symlinks before the tracked check, as you suggested — no target resolution. Re-ran both cases:

  • attack: SKIP AGENTS.local.md — symlink / SKIP CLAUDE.local.md (tracked) — nothing trusted
  • normal: SKIP AGENTS.local.md — symlink / TRUSTED CLAUDE.local.md — overlay still read through the real file, no content lost

Undefined $REPORT_FILE. Correct, and it would have broken every explicitly requested post — a Write-tool call cannot export a shell variable. Now --body-file "<report-file>", with a note that this is the literal path passed to Write.

Also ran bash -n over all five SKILL.md shell blocks with placeholders substituted; all parse.

Agreed on the rejected cleanup, restoration, extra head-race, prompt-duplication and lint suggestions. Still no live end-to-end Workflow run — agreed that is the final readiness check.

@yuanchen8911
yuanchen8911 force-pushed the add-cross-review-skill branch from f3502b1 to a812087 Compare July 27, 2026 01:11
@yuanchen8911

Copy link
Copy Markdown
Contributor Author

Six findings addressed (a812087)

Four verified decisively; two accepted on mechanism with a caveat noted below.

Fork remote layout (4). Confirmed the sharpest way available: the fork carries exactly one PR ref — its own PR #1 — while the canonical repo has 1915's head. So fetch origin works only in an atypical clone like mine, where origin is the canonical repo, and breaks setup for any contributor with GitHub's standard layout. Now fetched from https://github.com/<owner>/<repo>.git by URL.

Late findings bypass completeness (5). Reproduced. With both lanes returning the same newFindings entry, it was confirmed with zero cross-evaluations from source votes alone:

before: confirmed ["F1","F2"]  contested []
after:  confirmed ["F1"]       contested ["F2"]

Late candidates are absent from presented, so they stay contested. No extra round.

Allowlist position dependency (6). Confirmed by offset: in the CodeRabbit prompt CODERABBIT_RUN sits at 542 and NO_EXECUTION at 934, so the mandatory command is above the rule permitting commands "prescribed below". Same shape in the Codex lane. Changed to "prescribes anywhere".

Explore is the wrong agent (3). Agreed, and its own registry description settles it: Explore "reads excerpts rather than whole files, so it locates code; it doesn't review or audit it" — which is precisely what the integration lane does. Switched to general-purpose. That made the model argument redundant, since it existed only to override Explore's pinned model, so MODEL_OPT, the arg, and the model-handoff prose are all deleted.

Skill tool (1). Added disallowed-tools: Skill. Caveat stated plainly: I could not verify that this frontmatter field propagates to Workflow-spawned subagents, and I am not going to repeat the pattern of reporting an unverified fix as done. The prompt-level prohibition in every lane remains the primary control; treat this as defense-in-depth pending a live run.

Overlay guard is not a trust boundary (2). Accepted, and the honest fix is to stop claiming otherwise rather than to add machinery. Subagents load the checkout's CLAUDE.md hierarchy before any guard runs, so the symlink/tracked check reduces accidental exposure only. SKILL.md now says so and points to the same operational remedy as the self-review guard: for an untrusted or fork PR, start the session from a trusted checkout. repoNotes is documented as a relevance digest.

Verification: all seven scenarios pass, bash -n clean on all five shell blocks, JS parses. Agreed on excluding the second CI-head check, fence-tag, rounds, PR-state and broad-validator suggestions.

Still no live end-to-end Workflow run — and finding 1's verification specifically depends on one.

@yuanchen8911
yuanchen8911 force-pushed the add-cross-review-skill branch from a812087 to 1a7d965 Compare July 27, 2026 01:32
@yuanchen8911

Copy link
Copy Markdown
Contributor Author

Single-vote dismissal removed, contract text corrected (1a7d965)

The blocker was real and reproduced. With CodeRabbit unavailable, the reporter softening to OPEN_QUESTION, and one evidenced DISAGREE from Codex:

before: dismissed ["F1"]  contested []  consensusReached: true
after:  dismissed []      contested ["F1"]  consensusReached: false

The finding was critical, was buried by one of three slots, skipped adversarial verification entirely, and the run still advertised 2-of-3 consensus. Dismissal was asymmetric with confirmation — confirmation needed two evidenced AGREEs while dismissal could happen on one DISAGREE — which is exactly the asymmetry the contract does not permit.

Removed the agrees.length === 0 && disagrees.length > 0 branch as you suggested. Verified the legitimate path is untouched: two evidenced DISAGREEs still dismiss (dism: ["F1"] cont: []), and the verifier-REFUTED dismissal path in the full regression is unaffected.

Contract text. All four corrected, prose only:

  • The description no longer claims AskUserQuestion; the skill never calls it.
  • The workflow overview no longer says integration uses Explore — every lane is now general-purpose.
  • The Unresolved section is retitled "no settled disposition", since findings with zero valid votes land there alongside those that failed verification.
  • The Contested section is retitled "no 2-of-3 disposition", with a caption naming all three ways in: split reviewers, a lone dissent, and late findings never presented for evaluation.

Eight scenarios pass, bash -n clean on all five shell blocks, JS parses.

Thanks for chasing down the disallowed-tools semantics — that resolves the caveat I flagged last round, and a live Workflow run stays the right runtime confirmation.

@yuanchen8911
yuanchen8911 force-pushed the add-cross-review-skill branch from 1a7d965 to eddbc04 Compare July 27, 2026 01:48
@yuanchen8911

Copy link
Copy Markdown
Contributor Author

Dead branch removed and tally comment corrected (eddbc04)

Both applied. No behavioral change — nine scenarios produce identical results before and after.

Unreachable consumer backfill. Verified rather than accepted on inspection: enumerating every combination of consumer path and line, 9 key collisions are possible and the backfill condition can fire in 0 of them. Since the consumer coordinates are part of the dedupe key, a key match implies they are already equal. Branch deleted and the merge comment corrected — it claimed to preserve "consumer coordinates" that never needed merging.

tally() comment. It was staler than reported: besides "zero support = dismissed", it still described the CodeRabbit arbitration branch removed several revisions ago. Rewritten to state the actual rule — two evidenced AGREEs confirm, two evidenced DISAGREEs dismiss, anything else is contested — and to note the symmetry explicitly, since the asymmetry between those two thresholds was the blocker fixed in the previous commit.

The PR description now records that dead code in this skill is removed on proof: the consumer backfill via key-collision enumeration, the arbitration branch via the 128-state vote enumeration.

Agreed on the rejected CodeRabbit suggestions — cleanup traps, a second head-race check, candidate batching/recovery, and expanded failure reporting all sit outside the simplicity and fail-closed boundary.

With no static blocker remaining, the live Workflow run is the only outstanding gate.

@yuanchen8911
yuanchen8911 force-pushed the add-cross-review-skill branch from eddbc04 to 8f29ebd Compare July 27, 2026 02:25
@yuanchen8911

yuanchen8911 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Three findings, the dead-code sweep, and template hygiene (8f29ebd)

All verified before fixing.

1. Pinned reads treated PR data as shell source. Reproduced the glob half directly — git grep -n foo HEAD -- *.nonexistent under zsh fails with no matches found before git ever runs, while the quoted form exits 0. The injection half follows from the same root: <path> is substituted with a filename read out of the PR.

The canonical recipe now reads:

read:   git -C "<repo>" show "<sha>:<path>"
search: git -C "<repo>" grep -n -e "<pattern>" <sha> -- "<glob>"
(keep the quotes; substitute only inside them)

There is one recipe, not three: CODEX_LEAN now points at it instead of restating it, which is what let the copies drift apart.

2. Integration findings could omit consumer coordinates. Confirmed — consumerPath/consumerLine are absent from the schema's required list, and the verifier only traces a consumer when consumerPath exists, so a schema-valid finding could reach consensus with its central claim unverified.

I implemented this as an intake rejection rather than incomplete, deliberately: round-1 intake problems already drop-and-log (the blank-evidence rule), while cross-review problems return incomplete. Both are fail-closed and the finding never enters the tally either way; dropping keeps one rule for one phase instead of two. Verified: the coordinate-less finding is dropped and a well-formed one alongside it survives.

3. The 20-minute timebox was unenforceable. Confirmed against this environment: Bash defaults to 120000 ms and is capped at 600000. The lane now runs with an explicit 600000 ms timeout and the prompt states why, so it will not be "simplified" back to a larger number. Your point that this erases the third lane rather than corrupting consensus is right, which is why the fix is a smaller honest deadline rather than backgrounding machinery.

Dead-code sweep. All four removed after confirming each: if (integ) is unreachable-false after the required-lane check, !resolution[c.id] is always true at that point, the "not counted" vote rendering cannot occur now that unevidenced verdicts return incomplete, and rounds was write-only. Removing rounds also let const round = 2 go.

Template hygiene. Correct — I had deleted the unchecked options rather than leaving them. The body now carries the canonical template verbatim, with Documentation update and Build/CI/tooling added under Type of Change and Docs/examples under Components. Checklist entries that do not apply are left unchecked with a short N/A reason rather than removed.

Ten scenarios pass, including the new integration-coordinates case.


Correction (3c4a9be): the injection half was not fixed by this commit.

Double quotes stop globbing but still expand $(...) and backticks, so the fix described above only closed the zsh-glob failure — a filename like docs/$(id).md would still have executed. Demonstrated in the next round:

double-quoted:  abc:docs/COMMAND_SUBSTITUTION_RAN.md          <- executed
single-quoted:  abc:docs/$(echo COMMAND_SUBSTITUTION_RAN).md  <- literal

Fixed at 7aaecb7 by using single quotes, which are the only literal form in sh/bash/zsh, with the reason stated inline in the recipe so it is not changed back.

@yuanchen8911
yuanchen8911 force-pushed the add-cross-review-skill branch from 8f29ebd to 7aaecb7 Compare July 27, 2026 02:46
@yuanchen8911

Copy link
Copy Markdown
Contributor Author

Four findings and the cleanup sweep (7aaecb7)

1. Double quotes did not stop execution — my previous fix was wrong, and I said otherwise. Reproduced:

double-quoted:  abc:docs/COMMAND_SUBSTITUTION_RAN.md   <- executed
single-quoted:  abc:docs/$(echo COMMAND_SUBSTITUTION_RAN).md   <- literal

Last round I quoted the placeholders, verified the zsh glob case, and then claimed in both the commit message and the PR body that this prevented smuggling $(...) into the shell. It prevented globbing only. The recipe now uses single quotes and says inline why they must not be changed back, since the double-quoted version looked correct to me and would look correct to the next editor. Verified the rendered command: the shell receives $(echo PWNED) as literal text.

2. Malformed integration output produced false consensus. Confirmed: the finding was dropped, nothing was emitted, and consensusReached: true. My drop-at-intake choice last round was wrong for this lane specifically — I argued consistency with the blank-evidence rule, but integration is required, so discarding its only output leaves a run that looks clean while the required lane contributed nothing. Now returns incomplete naming the finding.

3. PR URLs were not normalized. Correct — <n> feeds temp paths, scoped ref names and gh pr checks, and a literal URL invalidates all three while the Input section advertises URL input. Phase 1 step 1 now passes $ARGUMENTS straight to gh pr view and takes .number, with an explicit instruction to use that numeric value everywhere and not write a parser.

4. Fallback pointed at a command that posts. Both places now stop with the reason. The contradiction was self-inflicted: the same file explains two hundred lines later that /code-review posts, which is why the Claude lane stopped delegating to it.

Cleanup, all folded in: three stale 20-minute descriptions corrected, the duplicate ${NO_EXECUTION} in the Codex prompt removed, agent: claude-code dropped, and the doubled "blocking blocking" fixed.

Ten scenarios pass. Two of this round's four findings were defects in the previous round's fixes, which is the fourth time that has happened — the live Workflow run remains the outstanding gate.

@yuanchen8911
yuanchen8911 force-pushed the add-cross-review-skill branch from 7aaecb7 to 183be53 Compare July 27, 2026 03:00
@yuanchen8911

yuanchen8911 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Three findings fixed (183be53)

1. Uninspectable filename produced clean consensus. Confirmed — consensusReached checks contested and unresolved only, so open questions never block it. My previous instruction routed an unreadable path into openQuestions, which meant all three lanes could report they inspected nothing and the run would still return status: ok, consensusReached: true.

Took your fix rather than adding escaping: a path that cannot be read through the single-quoted recipe is now the lane's failure outcome — unavailable for review and cross-review lanes, UNVERIFIABLE for the refuter. Verified: the same scenario now returns incomplete naming the lane. The instruction states inline that open questions do not block consensus but an unavailable required lane does, so the disposition is not re-softened later.

2. Numeric PR input failed in the fork layout. Confirmed. I fixed the fetch to use the canonical URL two rounds ago but left the normalizing gh pr view unscoped, so in the supported layout it exits 1 before reaching the fetch. REPO is now defined once as the canonical repository, and every gh pr view, gh pr checks, gh pr comment and the ref fetch are scoped to it.

3. Self-review guard consumed <n> before it existed. Confirmed, and it was worst exactly where it matters — reviewing a PR that modifies this skill, which is what this PR is. Normalization moved into Phase 0 immediately after the tool check, with the guard running against the files list from that same response. Phase 1 no longer re-fetches.

Folding 2 and 3 into one edit was right; they were the same ordering problem seen from two angles.

Eleven scenarios pass, all six Bash blocks parse, JS parses.

Two of these three were again defects in earlier fixes of mine — the unscoped gh pr view beside an already-scoped fetch, and an open-question disposition that could not block anything. The live Workflow run is still the outstanding gate.


Correction (3c4a9be): the REPO variable described above did not survive.

It was defined in one Bash invocation and referenced in three others, and each Bash call is a fresh shell — the same file says so on line 136. Reproduced as bash: line 1: REPO: unbound variable. All five call sites now name NVIDIA/aicr literally instead of carrying shell state.

The same round also removed no-argument mode: gh pr view "" --repo NVIDIA/aicr cannot find a fork PR from the local branch name, since the branch is on the fork and the PR is on NVIDIA/aicr. A PR number or URL is now required.

@yuanchen8911
yuanchen8911 force-pushed the add-cross-review-skill branch from 183be53 to 3c4a9be Compare July 27, 2026 03:14
@yuanchen8911

Copy link
Copy Markdown
Contributor Author

Two findings fixed (3c4a9be)

Both reproduced before fixing.

1. REPO did not survive between Bash calls.

bash: line 1: REPO: unbound variable

Defined in Phase 0, referenced in Batch B and Phases 3/5 — three separate Bash invocations. The same file states on line 136 that shell variables do not persist between calls, which I wrote, and then last round I introduced a variable that depends on exactly that. All five call sites now name NVIDIA/aicr literally; nothing is carried. Your reasoning for the literal form is the right one: this is an AICR-specific skill, so parameterising the repository buys nothing and cost a broken setup path.

2. No-argument mode could not find fork PRs. Reproduced from this PR's own worktree:

$ gh pr view "" --repo NVIDIA/aicr --json number
no pull requests found for branch "add-cross-review-skill"

The branch lives on the fork while the PR lives on NVIDIA/aicr, so branch-name lookup cannot bridge them. Removed rather than repaired — resolving a fork owner and branch is machinery outside the skill's purpose. A PR number or URL is now required, the pre-flight stops with a usage message, and argument-hint changed from [PR-number-or-URL] to <PR-number-or-URL> to match.

Six Bash blocks parse, JS parses, eleven scenarios pass.

Both findings were introduced by the previous round's fix for the fork layout: I scoped the calls correctly and then chose a mechanism that could not survive the tool boundary, while leaving a no-argument path that the new scoping made unreachable. The live Workflow run remains the outstanding gate.

@yuanchen8911 yuanchen8911 changed the title WIP: feat(skills): add aicr-cross-review skill for multi-agent PR review feat(skills): add aicr-cross-review skill for multi-agent PR review Jul 27, 2026
@yuanchen8911
yuanchen8911 marked this pull request as ready for review July 27, 2026 03:31
@yuanchen8911
yuanchen8911 requested a review from lockwobr July 27, 2026 03:34
@yuanchen8911

Copy link
Copy Markdown
Contributor Author

All seven review threads verified addressed at 3c4a9be

@ArangoGutierrez — each of your seven threads now has a verification reply anchored at the current head, with the evidence inline. Summary so you can close them out:

Finding Disposition at 3c4a9be
Fork gate was prose, and make test runs the PR's Makefile Coverage lane deleted; the skill executes nothing from the reviewed commit
verdictFor auto-confirms without cross-checking Reproduced; now returns incomplete — strict per-candidate evaluation, fatal blank evidence, two-vote dismissal
codex-watch.sh bounds sleep, not wall clock File deleted; the Codex companion's own bounded wait is used
gh api does not substitute {number} Inline review POST removed; summary posts via --body-file
Diff baseline vs BASE_SHA diverge Single baseline; coverage gone, CodeRabbit resolves merge-base itself
repoNotes read from the mutable working copy Base-pinned — and documented as accident-reduction, not a trust boundary
Overlay named AGENTS.local.md, not CLAUDE.local.md Both handled; symlink skipped before the tracked check

Four of the seven were resolved by deleting the thing rather than fixing it — the coverage lane and the Codex watchdog together accounted for most of what you flagged.

Three of your comments also led somewhere past what the thread said, and those are noted in the individual replies: make test running arbitrary Makefile recipes is what justified removing coverage entirely; the repoNotes thread led to establishing that subagents auto-load the CLAUDE.md hierarchy regardless, so that check can only ever be accident-reduction; and the overlay thread surfaced a symlink bypass of the guard added in response to it.

I have not resolved the threads — they are yours to close if you agree with the dispositions.

Live end-to-end run of the skill is in progress against a separate PR; that was the last outstanding readiness item.

@yuanchen8911

Copy link
Copy Markdown
Contributor Author

Ready for re-review — 3c4a9be

@ArangoGutierrez @mchmarny @njhensley @lockwobr

Since the last round of review this became a substantially smaller change: two files instead of four, and the skill no longer executes anything from the reviewed commit.

What changed structurally

  • The local Go coverage lane was deleted. It was the only code-execution surface, and the Merge Gate already enforces the project threshold on fork PRs; Phase 3 just reports gh pr checks now.
  • The Codex watchdog was deleted in favour of the companion's own bounded wait.
  • Availability is now report-incomplete-and-stop: Claude, Codex and integration are required, CodeRabbit is best-effort with a fixed NONE vote slot. No quorum arithmetic, no retries, no arbitration.
  • Consensus is symmetric: two evidenced AGREEs confirm, two evidenced DISAGREEs dismiss, everything else is contested.

Where to look if you are re-reviewing selectively

scripts/workflow.mjstally(), intake(), and the cross-review completeness gate carry the consensus guarantees. SKILL.md — Phase 0 (repo/PR resolution and the self-review guard) and Phase 1 Batch B (commit pinning) carry the setup guarantees.

All seven of @ArangoGutierrez's threads have verification replies anchored at this head; four were resolved by deleting the subject rather than fixing it. I have deliberately not resolved them.

Behaviour is covered by eleven stubbed-harness scenarios described under Testing. The skill is also being exercised end-to-end against a live PR for the first time; if that surfaces anything I will push a fix and say so here rather than let it pass silently.

@yuanchen8911
yuanchen8911 force-pushed the add-cross-review-skill branch from 3c4a9be to 1be6fc4 Compare July 27, 2026 14:21
@yuanchen8911

yuanchen8911 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Head is now e65e917 — live-run result and one fix

Current head is e65e91784; 3c4a9be25 is no longer in this PR. The re-review request and the thread verifications still stand — they describe the PR as it stands rather than any single commit.

What changed. git diff 3c4a9be25 1be6fc49c -- .agents/skills/aicr-cross-review/ is a single hunk. Everything else was a rebase onto current main.

The skill has now been run end-to-end against a live PR (#1921), and it did not complete. Result was status: incomplete — the Codex lane died in the cross-review round on a transport error (API Error: Connection closed mid-response) after two harness retries, and CodeRabbit's CLI hung at connecting_to_review_service through two 600s windows. Codex is a required lane, so the engine emitted no consensus at all rather than a partial tally.

That is the designed behavior and the part I most wanted exercised: two of six agents failed and nothing degraded into a quorum. It preserved the raw material (2 candidate findings, 6 open questions) explicitly labeled unverified.

But it is a partial result and I do not want to oversell it. The run never reached tally(), so the consensus path remains covered only by the static harness scenarios — same evidence as before. The fail-closed path is now tested against reality; the happy path is not.

The one defect it surfaced, fixed in e65e917. workflow.mjs instructed the Codex lane "Never bypass the sandbox — the companion runs fine sandboxed," and the live lane reported it needed a bypass to start, because the companion writes its job log under a sandbox-denied path. The absolute claim is replaced with the scoped exception: that one write may bypass, nothing touching the working copy or GitHub may.

Verification note. node --check cannot validate workflow.mjs — it fails identically on unmodified HEAD, because the script body legitimately uses a top-level return inside the harness's async wrapper. Syntax is checked by wrapping the source in an async function first.

The six threads that today's push marked outdated have per-thread replies re-confirming each check at the new head. All seven remain open for @ArangoGutierrez to close.

Pin updated: rebased 1be6fc49ce65e91784 onto ff0301b53 (the #1911 merge). This PR's own files are byte-identical across that rebase — git diff 1be6fc49c e65e91784 -- .agents/skills/aicr-cross-review/ is empty — so every check above still holds verbatim at the new head.

@yuanchen8911
yuanchen8911 force-pushed the add-cross-review-skill branch 2 times, most recently from e65e917 to cfda806 Compare July 27, 2026 16:58
Adds `aicr-cross-review` under `.agents/skills/` — a three-reviewer consensus
workflow (Claude Code, Codex, CodeRabbit) with integration impact analysis and
adversarial verification of every confirmed finding. Also registers the skill in
the Available Skills catalog.

Two files: SKILL.md (phases 0-5 orchestration) and scripts/workflow.mjs (the
Workflow-tool consensus engine).

The skill never executes the reviewed commit's code. There is no build, test, or
coverage step, and every reviewer prompt forbids running tests, builds, package
managers, repository scripts, or checked-in binaries from the PR. The only things
executed are trusted tools the user already installed: git, gh, the CodeRabbit CLI,
and the Codex companion. Coverage is left to CI, whose Merge Gate already enforces
the project-wide threshold from .settings.yaml on fork PRs included.

No lane may execute the reviewed commit's code. The execution rules appear in all six
prompts (both round-1 review lanes, CodeRabbit, integration, the cross-review round,
and the adversarial verifier), and CODEX_DISPATCH requires every dispatched Codex task
to carry them verbatim, since Codex runs in its own shell. The rule permits the trusted
commands a prompt explicitly prescribes — notably CodeRabbit's detached worktree — so
it no longer contradicts the lane it governs.

CI status is read with `gh pr checks` after re-confirming headRefOid still equals the
pinned HEAD_SHA; `gh pr checks` reports the PR's current head, and the head can move
during a review. `--required` is deliberately not used: before the aggregate gate job
exists it exits 1 with "no required checks reported", making a normal in-progress run
look like a failure. The head check is an explicit `if`, not `A && B || C`: chaining
would report "head moved" whenever `gh pr checks` merely exited nonzero for pending
(8) or failing checks, even with a matching SHA.

Availability contract — report incomplete and stop:
- Claude, Codex and integration analysis are required in round 1.
- Claude and Codex must each return exactly one evaluation per candidate in the
  cross-review round. A missing evaluation, a duplicate, or an unknown candidate id
  returns incomplete, so a candidate nobody evaluated cannot ride its initial source
  votes to confirmed.
- An AGREE or DISAGREE returned with blank evidence is fatal, not merely discarded:
  with no stored evaluation the reviewer falls back to its round-1 source vote, so a
  dropped verdict would still decide the tally.
- CodeRabbit runs once and is best-effort. It is not a pre-flight requirement, and it
  uses a general-purpose agent rather than the CodeRabbit plugin agent type. When it does not run its fixed vote slot
  records NONE, which raises the bar (Claude and Codex must then agree) rather than
  lowering it. No dynamic participants, no quorum arithmetic, no retries.

Consensus integrity:
- Confirmed = 2 of 3 reviewer slots AGREE with evidence; integration analysis is
  never a reviewer slot. A finding whose evidence is blank or whitespace-only is
  rejected at intake, so it never registers its reporter as a source — schema
  `required` guarantees the field exists, not that it says anything.
- One consumer per finding: the dedupe key includes the consumer coordinates, so a
  changed declaration breaking two callers stays two candidates, each verified
  separately.
- Every confirmed finding goes to a fresh adversarial refuter. UNVERIFIABLE, a dead
  verifier, or a verdict without a citation lands in `unresolved`, which is emitted
  and blocks `consensusReached`.

Pinning:
- The diff comes from `refs/pull/N/head` and the PR's actual `baseRefName` into
  session-scoped refs, asserted against the `headRefOid` captured at setup.
- Every reviewer prompt reads via `git show <headSha>:<path>` and searches via
  `git grep <pattern> <headSha>` — never the mutable working copy, which may be on
  another branch during concurrent sessions.
- `repoNotes` comes from the base-pinned CLAUDE.md; the untracked local overlay is
  read only after confirming git does not track it, since .gitignore does not stop
  `git add -f`.

Filesystem paths interpolated into generated shell commands are quoted — in the
workflow prompts and in the SKILL.md setup, overlay-validation and cleanup blocks — so
a checkout or temp path containing spaces does not split into multiple arguments and
fail a required lane.

Explicitly requested posting uses `gh pr comment --body-file <report-file>` against a
file written by the Write tool, never `--body "..."`. Finding text quotes PR content,
so backticks or $(...) in a finding would otherwise be evaluated by the shell before
gh runs.

All lanes run as general-purpose agents. The integration lane no longer uses Explore,
which reads excerpts to locate code and is explicitly not a review agent; with Explore
gone every lane inherits the session model, so the model argument and its handoff prose
are removed. The skill denies the Skill tool so a reviewer lane cannot auto-invoke a
skill that posts.

Pinned-read recipes put their placeholders in SINGLE quotes and send the pattern
through `git grep -e`. Double quotes stop globbing but still expand $(...) and
backticks, so a PR-controlled filename such as docs/$(id).md would execute; single
quotes are the only literal form in sh/bash/zsh.

An integration finding missing consumerPath or consumerLine stops the run. Integration
is a required lane, so dropping its malformed output produced a result that looked
clean and reported consensusReached: true while that lane had contributed nothing.

A path that cannot be read through the single-quoted recipe makes the lane return
unavailable rather than an open question. Open questions do not block consensus, so a
filename containing a quote could otherwise yield a clean-looking run in which nothing
was actually inspected.

Every gh call and the PR-ref fetch name NVIDIA/aicr literally rather than carrying a
shell variable: each Bash call is a fresh shell, and the local repository is the
contributor's fork in GitHub's standard layout, which has neither the PR nor
refs/pull/*.

A PR number or URL is required. There is no no-argument mode, because a fork PR cannot
be identified from the local branch name alone. The reference is normalized in Phase 0,
so the numeric .number is available to the self-review guard that runs immediately after
it and to every later temp path and scoped ref.

Pre-flight and environment failures stop with a reason instead of suggesting
/code-review, which posts its result to the PR.

The CodeRabbit lane runs with an explicit 600000 ms timeout. The Bash tool defaults to
two minutes and is capped at ten, so the previous ~20-minute timebox was not
enforceable and would have silently erased the lane.

There is no single-vote dismissal. Dismissal needs two evidenced DISAGREEs; a lone
dissent leaves the finding contested, so one slot of three cannot bury an issue that
only it ever judged.

Findings raised during the cross-review round stay contested: they were never presented
to anyone, so their reporters' source votes alone must not confirm them.

PR refs are fetched from the canonical repository by URL rather than from `origin`,
which is the contributor's fork in GitHub's standard fork layout and carries no
refs/pull/*.

The local-overlay check is documented as accident-reduction rather than a trust
boundary — reviewer subagents load the checkout's CLAUDE.md hierarchy before any guard
here runs, so an untrusted PR needs a session started from a trusted checkout.

The local-overlay trust check skips symlinks before testing tracked status. The check
applies to the link, not its target, so an untracked symlink resolving to a PR-tracked
file would otherwise be treated as a trusted overlay.

Safety:
- The Claude lane reviews directly rather than delegating to the `code-review`
  command, whose step 8 instructs its agent to `gh pr comment` the result back to
  the PR. This skill never posts by default.
- If the PR modifies this skill, the review stops and asks for a trusted checkout.

Signed-off-by: Yuan Chen <yuanchen97@gmail.com>
@yuanchen8911
yuanchen8911 force-pushed the add-cross-review-skill branch from cfda806 to 078f6d9 Compare July 27, 2026 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/docs size/XL theme/ci-dx CI pipelines, developer experience, and build tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants