gate PR ready on clean agent-driven review rounds (#10157)

* feat(ci): gate PR ready on clean review rounds driven from draft Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(ci): robust review-round wait loop, require codex evidence for marker skip Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(ci): require pre-marker codex evidence, fail open on marker fetch errors Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-08-18 16:02:10 +00:00 · 2026-07-16 17:23:24 +02:00
parent 0e547adf23
commit 4e63ca2cd4
6 changed files with 281 additions and 2 deletions
@@ -1,18 +1,19 @@
 ---
 name: pr
 user_invocable: true
-description: Open a draft pull request on GitHub. MUST use when you want to create/open a PR.
+description: Open a draft pull request on GitHub and drive CI review rounds until it is ready. MUST use when you want to create/open a PR.
 ---

 # Pull Request Skill

-Create a draft pull request with a clear title and explicit description of changes.
+Create a draft pull request with a clear title and explicit description of changes, then drive it through CI review rounds to ready.

 ## Instructions

 1. **Analyze branch changes**: Understand all commits since diverging from main
 2. **Push to remote**: Ensure all commits are pushed
 3. **Create draft PR**: Always open as draft for review before merging
+4. **Drive review rounds**: trigger CI reviews on the draft and only flip to ready once every verdict is a go (see "Review rounds" below)

 ## PR Title Format

@@ -124,6 +125,40 @@ and continue once they confirm it's done.
   )"
   ```
 9. Return the PR URL to the user
+10. Drive the PR through CI review rounds to ready (see "Review rounds" below)
+
+## Review rounds (draft → ready)
+
+A PR leaves draft **only after a clean CI review round**. Never run `gh pr ready` before that.
+
+1. **Trigger a round and wait for it**: launch the waiter as a background Bash task (a round takes 10–30 min; you are woken when it exits — do not stop the session or poll in the foreground while it runs):
+
+   ```bash
+   bash .agents/skills/pr/review-round.sh <PR_NUMBER>
+   ```
+
+   It comments `/review` on the PR — which runs the Codex, Claude and Pi CI reviewers even on a draft — waits for the spawned `PR Review Commands` workflow run(s) to complete, then prints one verdict line per reviewer and saves the full review comments to files.
+
+2. **Judge the round.** Codex is mandatory; Claude, Pi and cubic count whenever they posted. Every review starts with one of the three `REVIEW.md` verdicts:
+   - Codex verdict missing → the round is void: comment `/codex` on the PR, wait for it the same way, and judge again.
+   - Any **"Should address issues before merging"** → fix the P0/P1 findings (and the nits while you're there), commit, push, and start a new round (step 1).
+   - Only **"Mergeable, but should ideally address nits"** and/or **"Good to merge"** → fix the nits too; a nit that is wrong or genuinely not worth fixing may instead be dismissed by replying to the review comment with your reasoning. Push nit-only fixes without starting another full round.
+
+3. **Flip to ready with the marker comment.** The review workflows skip the redundant `ready_for_review`-triggered round when the PR author has posted a marker naming the current head SHA **and** the PR's latest Codex review *posted before the marker* has a non-blocking verdict (reviewer evidence — a bare marker with no round behind it, or one whose last pre-marker Codex verdict is "Should address issues", skips nothing). Keep the prefix exact and use the full 40-char SHA of the head you are flipping:
+   - every verdict was "Good to merge" (head unchanged since the round):
+
+     `✅ Review round clean @ <head-sha>`
+
+   - nit-only round, nits fixed or dismissed afterwards (head may have moved past the reviewed SHA — say so):
+
+     `✅ Review round clean @ <head-sha> — nit-only verdicts at <round-sha>; nits addressed in <commit sha(s)> / dismissed in review replies`
+
+   ```bash
+   gh pr comment <PR_NUMBER> --body "✅ Review round clean @ $(git rev-parse HEAD)"
+   gh pr ready <PR_NUMBER>
+   ```
+
+   If any P0/P1 finding is unaddressed or the head moved for reasons other than nit fixes, do **not** post the marker or flip — run another round instead.

 ## EE Companion PR (when `*_ee.rs` files were modified)

@@ -0,0 +1,129 @@
+#!/usr/bin/env bash
+# Trigger a CI review round on a PR and wait for it to finish.
+#
+# Usage: bash .agents/skills/pr/review-round.sh [PR_NUMBER]
+#   PR_NUMBER defaults to the current branch's PR.
+#
+# Comments `/review` on the PR (works on drafts), waits for the spawned
+# "PR Review Commands" workflow run(s) to complete, then prints one verdict
+# line per reviewer and saves each full review comment to a file. A round
+# takes 10-30 minutes: run this in the background and act on its output when
+# it exits, per the pr skill ("Review rounds").
+set -euo pipefail
+
+REPO=${REPO:-$(gh repo view --json nameWithOwner --jq .nameWithOwner)}
+PR=${1:-$(gh pr view --json number --jq .number)}
+
+# Timestamp of the trigger comment, straight from GitHub, so local clock skew
+# can't make the run/comment filters below miss part of the round.
+TRIGGER_TIME=$(gh api "repos/$REPO/issues/$PR/comments" -f body='/review' --jq .created_at)
+echo "Review round triggered on $REPO#$PR at $TRIGGER_TIME"
+
+# Retry wrapper for one-off gh/API hiccups: a 45-minute wait must not die on
+# a single transient failure.
+retry() {
+  local attempt
+  for attempt in 1 2 3; do
+    if "$@"; then return 0; fi
+    sleep 10
+  done
+  return 1
+}
+
+# The /review comment spawns one "PR Review Commands" run holding the
+# claude/codex/pi jobs. Runs aren't linked to a PR, so wait on every run of
+# that workflow created after the trigger: a concurrent round on another PR
+# can only delay the answer, never truncate it. Every issue comment on any PR
+# spawns a fast-completing parse run of the same workflow, so the round's own
+# run may briefly lag the listing while unrelated runs already show completed:
+# require the all-completed state to hold past a floor and across two
+# consecutive polls before trusting it.
+DEADLINE=$(( $(date +%s) + 45 * 60 ))
+NO_RUN_DEADLINE=$(( $(date +%s) + 5 * 60 ))
+MIN_WAIT_UNTIL=$(( $(date +%s) + 3 * 60 ))
+STABLE=0
+FAILURES=0
+while :; do
+  if RUNS=$(gh run list --repo "$REPO" --workflow=pr-review-commands.yml \
+      --created ">=$TRIGGER_TIME" --limit 100 --json status); then
+    FAILURES=0
+  else
+    FAILURES=$(( FAILURES + 1 ))
+    if [ "$FAILURES" -ge 5 ]; then
+      echo "ERROR: listing workflow runs failed $FAILURES times in a row; aborting the wait." >&2
+      exit 1
+    fi
+    echo "WARNING: listing workflow runs failed (attempt $FAILURES/5); retrying in 60s." >&2
+    sleep 60
+    continue
+  fi
+  TOTAL=$(jq length <<<"$RUNS")
+  PENDING=$(jq '[.[] | select(.status != "completed")] | length' <<<"$RUNS")
+  NOW=$(date +%s)
+  if [ "$TOTAL" -gt 0 ] && [ "$PENDING" -eq 0 ] && [ "$NOW" -gt "$MIN_WAIT_UNTIL" ]; then
+    STABLE=$(( STABLE + 1 ))
+    if [ "$STABLE" -ge 2 ]; then
+      break
+    fi
+  else
+    STABLE=0
+  fi
+  if [ "$TOTAL" -eq 0 ] && [ "$NOW" -gt "$NO_RUN_DEADLINE" ]; then
+    echo "ERROR: no 'PR Review Commands' run appeared within 5 minutes of the /review comment; check that the comment author has write access and the workflow is enabled." >&2
+    exit 1
+  fi
+  if [ "$NOW" -gt "$DEADLINE" ]; then
+    echo "WARNING: review round still pending after 45 minutes; reporting whatever has been posted so far." >&2
+    break
+  fi
+  sleep 60
+done
+
+OUT_DIR=$(mktemp -d -t review-round-XXXXXX)
+COMMENTS_RAW=$(retry gh api "repos/$REPO/issues/$PR/comments?per_page=100" --paginate)
+jq -s --arg t "$TRIGGER_TIME" '[.[][] | select(.created_at > $t)]' \
+  <<<"$COMMENTS_RAW" > "$OUT_DIR/comments.json"
+# cubic posts through the PR reviews API, not issue comments.
+REVIEWS_RAW=$(retry gh api "repos/$REPO/pulls/$PR/reviews?per_page=100" --paginate)
+jq -s --arg t "$TRIGGER_TIME" '[.[][] | select((.submitted_at // "") > $t)]' \
+  <<<"$REVIEWS_RAW" > "$OUT_DIR/pr-reviews.json"
+
+VERDICT_RE='(Good to merge|Mergeable, but should ideally address nits|Should address issues before merging)'
+
+body_by_header() {
+  jq -r --arg h "$1" '[.[] | select(.body // "" | contains($h))] | last | .body // empty' \
+    "$OUT_DIR/comments.json"
+}
+body_by_login() {
+  jq -r --arg l "$1" '[.[] | select(.user.login == $l)] | last | .body // empty' \
+    "$OUT_DIR/comments.json"
+}
+report() { # <reviewer-name> <comment-body>
+  local name=$1 body=$2 verdict
+  if [ -z "$body" ]; then
+    echo "$name: (no review posted this round)"
+    return
+  fi
+  printf '%s\n' "$body" > "$OUT_DIR/$name.md"
+  verdict=$(printf '%s\n' "$body" | grep -m1 -oE "${VERDICT_RE}.*" | sed 's/\*\*//g' || true)
+  echo "$name: ${verdict:-(review posted but no verdict line; read $OUT_DIR/$name.md)}"
+}
+
+echo
+echo "=== Review round verdicts for $REPO#$PR (posted after $TRIGGER_TIME) ==="
+CODEX_BODY=$(body_by_header '## Codex Review')
+report codex "$CODEX_BODY"
+report claude "$(body_by_login 'claude[bot]')"
+report pi "$(body_by_header '## Pi Review')"
+CUBIC_BODY=$(jq -r '[.[] | select(.user.login | test("^cubic(-dev-ai)?(\\[bot\\])?$"; "i"))] | last | .body // empty' \
+  "$OUT_DIR/pr-reviews.json")
+if [ -z "$CUBIC_BODY" ]; then
+  CUBIC_BODY=$(jq -r '[.[] | select(.user.login | test("^cubic(-dev-ai)?(\\[bot\\])?$"; "i"))] | last | .body // empty' \
+    "$OUT_DIR/comments.json")
+fi
+report cubic "$CUBIC_BODY"
+echo
+echo "Full round output: $OUT_DIR (comments.json, pr-reviews.json, one .md per reviewer)"
+if [ -z "$CODEX_BODY" ]; then
+  echo "WARNING: Codex verdict missing - the round is incomplete. Re-trigger with a '/codex' PR comment and wait again." >&2
+fi
@@ -82,6 +82,7 @@ jobs:
          EVENT_BODY: ${{ github.event.pull_request.body }}
          EVENT_FORK: ${{ github.event.pull_request.head.repo.fork }}
          EVENT_AUTHOR: ${{ github.event.pull_request.user.login }}
+          EVENT_ACTION: ${{ github.event.action }}
        run: |
          if [ -n "$INPUT_PR_NUMBER" ]; then
            PR_JSON=$(gh pr view "$INPUT_PR_NUMBER" --repo "${{ github.repository }}" \
@@ -113,6 +114,38 @@ jobs:
            echo "skip=true" >> "$GITHUB_OUTPUT"
            exit 0
          fi
+          # An agent-driven PR flips to ready only after a clean /review round on
+          # a draft, marked by an author comment naming the head SHA (pr skill,
+          # "Review rounds"). Re-reviewing that same head on ready_for_review is
+          # redundant. The marker alone is author attestation, so also require
+          # reviewer evidence: a Codex review (posted by github-actions[bot], not
+          # forgeable by the author) that predates the marker and carries a
+          # non-blocking verdict. Comment-triggered and synchronize runs never
+          # skip. Keep the three copies of this check in sync (pr-ready-review /
+          # codex-pr-review / pi-pr-review); a shared local action would need the
+          # repo checked out before the check, which the fork paths here
+          # deliberately avoid.
+          if [ "$EVENT_ACTION" = "ready_for_review" ] && [ -z "$INPUT_PR_NUMBER" ]; then
+            # Fetch failures fail open (no skip): an API hiccup must run the
+            # review, never skip it or fail the job.
+            COMMENTS=$(gh api "repos/${{ github.repository }}/issues/$PR_NUMBER/comments?per_page=100" --paginate | jq -s '[.[][]]') || COMMENTS='[]'
+            MARKER_TIME=$(jq -r --arg author "$PR_AUTHOR" --arg marker "✅ Review round clean @ $HEAD_SHA" \
+              '[.[] | select(.user.login == $author) | select(.body | contains($marker)) | .created_at] | min // empty' <<<"$COMMENTS")
+            CODEX_VERDICT=''
+            if [ -n "$MARKER_TIME" ]; then
+              # Only Codex evidence that predates the marker counts: the ready-
+              # triggered Codex run itself posts after the flip and must not
+              # vouch for a sibling reviewer's skip.
+              CODEX_VERDICT=$(jq -r --arg mt "$MARKER_TIME" \
+                '[.[] | select(.user.login == "github-actions[bot]") | select(.body | contains("## Codex Review")) | select(.created_at < $mt)] | last | .body // ""' <<<"$COMMENTS" \
+                | grep -m1 -oE '(Good to merge|Mergeable, but should ideally address nits|Should address issues before merging)' || true)
+            fi
+            if [ -n "$MARKER_TIME" ] && [ -n "$CODEX_VERDICT" ] && [ "$CODEX_VERDICT" != "Should address issues before merging" ]; then
+              echo "Clean review round marker found for $HEAD_SHA with pre-marker non-blocking Codex verdict; skipping redundant review."
+              echo "skip=true" >> "$GITHUB_OUTPUT"
+              exit 0
+            fi
+          fi
          # PR title/body are attacker-controlled free text. Use an unguessable
          # per-run delimiter so a fork can't embed a fixed heredoc terminator to
          # inject extra outputs — e.g. is_fork=false (last-write-wins), which
@@ -75,6 +75,7 @@ jobs:
          EVENT_BODY: ${{ github.event.pull_request.body }}
          EVENT_FORK: ${{ github.event.pull_request.head.repo.fork }}
          EVENT_AUTHOR: ${{ github.event.pull_request.user.login }}
+          EVENT_ACTION: ${{ github.event.action }}
        run: |
          if [ -n "$INPUT_PR_NUMBER" ]; then
            PR_JSON=$(gh pr view "$INPUT_PR_NUMBER" --repo "${{ github.repository }}" \
@@ -106,6 +107,38 @@ jobs:
            echo "skip=true" >> "$GITHUB_OUTPUT"
            exit 0
          fi
+          # An agent-driven PR flips to ready only after a clean /review round on
+          # a draft, marked by an author comment naming the head SHA (pr skill,
+          # "Review rounds"). Re-reviewing that same head on ready_for_review is
+          # redundant. The marker alone is author attestation, so also require
+          # reviewer evidence: a Codex review (posted by github-actions[bot], not
+          # forgeable by the author) that predates the marker and carries a
+          # non-blocking verdict. Comment-triggered and synchronize runs never
+          # skip. Keep the three copies of this check in sync (pr-ready-review /
+          # codex-pr-review / pi-pr-review); a shared local action would need the
+          # repo checked out before the check, which the fork paths here
+          # deliberately avoid.
+          if [ "$EVENT_ACTION" = "ready_for_review" ] && [ -z "$INPUT_PR_NUMBER" ]; then
+            # Fetch failures fail open (no skip): an API hiccup must run the
+            # review, never skip it or fail the job.
+            COMMENTS=$(gh api "repos/${{ github.repository }}/issues/$PR_NUMBER/comments?per_page=100" --paginate | jq -s '[.[][]]') || COMMENTS='[]'
+            MARKER_TIME=$(jq -r --arg author "$PR_AUTHOR" --arg marker "✅ Review round clean @ $HEAD_SHA" \
+              '[.[] | select(.user.login == $author) | select(.body | contains($marker)) | .created_at] | min // empty' <<<"$COMMENTS")
+            CODEX_VERDICT=''
+            if [ -n "$MARKER_TIME" ]; then
+              # Only Codex evidence that predates the marker counts: the ready-
+              # triggered Codex run itself posts after the flip and must not
+              # vouch for a sibling reviewer's skip.
+              CODEX_VERDICT=$(jq -r --arg mt "$MARKER_TIME" \
+                '[.[] | select(.user.login == "github-actions[bot]") | select(.body | contains("## Codex Review")) | select(.created_at < $mt)] | last | .body // ""' <<<"$COMMENTS" \
+                | grep -m1 -oE '(Good to merge|Mergeable, but should ideally address nits|Should address issues before merging)' || true)
+            fi
+            if [ -n "$MARKER_TIME" ] && [ -n "$CODEX_VERDICT" ] && [ "$CODEX_VERDICT" != "Should address issues before merging" ]; then
+              echo "Clean review round marker found for $HEAD_SHA with pre-marker non-blocking Codex verdict; skipping redundant review."
+              echo "skip=true" >> "$GITHUB_OUTPUT"
+              exit 0
+            fi
+          fi
          # PR title/body are attacker-controlled free text. Use an unguessable
          # per-run delimiter so a fork can't embed a fixed heredoc terminator to
          # inject extra outputs — e.g. is_fork=false (last-write-wins), which
@@ -48,12 +48,55 @@ jobs:
      pull-requests: read
      id-token: write
    steps:
+      # An agent-driven PR flips to ready only after a clean /review round on a
+      # draft, marked by an author comment naming the head SHA (pr skill, "Review
+      # rounds"). Re-reviewing that same head on ready_for_review is redundant.
+      # The marker alone is author attestation, so also require reviewer evidence:
+      # a Codex review (posted by github-actions[bot], not forgeable by the author)
+      # that predates the marker and carries a non-blocking verdict. Comment-
+      # triggered (workflow_call) and opened runs never skip. Keep the three
+      # copies of this check in sync (pr-ready-review / codex-pr-review /
+      # pi-pr-review); a shared local action would need the repo checked out
+      # before the check, which the codex/pi fork paths deliberately avoid.
+      - name: Check clean-round marker
+        id: marker
+        if: github.event_name == 'pull_request' && github.event.action == 'ready_for_review'
+        env:
+          GH_TOKEN: ${{ github.token }}
+          REPO: ${{ github.repository }}
+          PR_NUMBER: ${{ github.event.pull_request.number }}
+          HEAD_SHA: ${{ github.event.pull_request.head.sha }}
+          PR_AUTHOR: ${{ github.event.pull_request.user.login }}
+        run: |
+          # Fetch failures fail open (skip=false): an API hiccup must run the
+          # review, never skip it or fail the job.
+          COMMENTS=$(gh api "repos/$REPO/issues/$PR_NUMBER/comments?per_page=100" --paginate | jq -s '[.[][]]') || COMMENTS='[]'
+          MARKER_TIME=$(jq -r --arg author "$PR_AUTHOR" --arg marker "✅ Review round clean @ $HEAD_SHA" \
+            '[.[] | select(.user.login == $author) | select(.body | contains($marker)) | .created_at] | min // empty' <<<"$COMMENTS")
+          CODEX_VERDICT=''
+          if [ -n "$MARKER_TIME" ]; then
+            # Only Codex evidence that predates the marker counts: the ready-
+            # triggered Codex run itself posts after the flip and must not vouch
+            # for a sibling reviewer's skip.
+            CODEX_VERDICT=$(jq -r --arg mt "$MARKER_TIME" \
+              '[.[] | select(.user.login == "github-actions[bot]") | select(.body | contains("## Codex Review")) | select(.created_at < $mt)] | last | .body // ""' <<<"$COMMENTS" \
+              | grep -m1 -oE '(Good to merge|Mergeable, but should ideally address nits|Should address issues before merging)' || true)
+          fi
+          if [ -n "$MARKER_TIME" ] && [ -n "$CODEX_VERDICT" ] && [ "$CODEX_VERDICT" != "Should address issues before merging" ]; then
+            echo "Clean review round marker found for $HEAD_SHA with pre-marker non-blocking Codex verdict; skipping redundant review."
+            echo "skip=true" >> "$GITHUB_OUTPUT"
+          else
+            echo "skip=false" >> "$GITHUB_OUTPUT"
+          fi
+
      - name: Checkout repository
+        if: steps.marker.outputs.skip != 'true'
        uses: actions/checkout@v5
        with:
          fetch-depth: 1

      - name: Check EE access
+        if: steps.marker.outputs.skip != 'true'
        id: ee
        env:
          EE_TOKEN: ${{ secrets.WINDMILL_EE_PRIVATE_ACCESS }}
@@ -80,6 +123,7 @@ jobs:
        run: ./backend/substitute_ee_code.sh --copy --dir ./windmill-ee-private

      - name: Resolve PR number
+        if: steps.marker.outputs.skip != 'true'
        id: resolve
        env:
          GH_TOKEN: ${{ github.token }}
@@ -99,6 +143,7 @@ jobs:
          echo "pr_author=$PR_AUTHOR" >> "$GITHUB_OUTPUT"

      - name: Fetch prior PR discussion
+        if: steps.marker.outputs.skip != 'true'
        id: prior
        env:
          GH_TOKEN: ${{ github.token }}
@@ -117,6 +162,7 @@ jobs:
          ' prior-comments.json > prior-comments.md

      - name: Read review prompt
+        if: steps.marker.outputs.skip != 'true'
        id: review-prompt
        env:
          EXTRA_PROMPT: ${{ inputs.extra_prompt }}
@@ -140,6 +186,7 @@ jobs:
          } >> "$GITHUB_ENV"

      - name: Automatic PR Review
+        if: steps.marker.outputs.skip != 'true'
        uses: anthropics/claude-code-action@v1
        with:
          claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }}
@@ -65,6 +65,8 @@ After code changes compile and type-check, verify the feature works:

 When done, directly open or update a **draft PR** against `main` once the work and validation are complete. Do not stop at a local summary or ask a human to create the PR manually.

+Then drive the PR through CI review rounds to ready, per the `pr` skill ("Review rounds"): run `.agents/skills/pr/review-round.sh` as a background task, let its completion wake you, address the findings, and repeat until every reviewer verdict is a go — then post the clean-round marker comment and `gh pr ready`. Do not end the session at an unreviewed draft or while a round is pending; waiting on the background waiter costs nothing.
+
 In your final summary, provide:
 - What was changed and why (files modified, approach taken)
 - What checks passed (cargo check, npm run check, etc.)