From 4e63ca2cd429ecf55f27a431dfb2c0fd85e2c8c8 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Thu, 16 Jul 2026 17:23:24 +0200 Subject: [PATCH] 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 * fix(ci): robust review-round wait loop, require codex evidence for marker skip Co-Authored-By: Claude Fable 5 * fix(ci): require pre-marker codex evidence, fail open on marker fetch errors Co-Authored-By: Claude Fable 5 --------- Co-authored-by: Claude Fable 5 --- .agents/skills/pr/SKILL.md | 39 +++++++- .agents/skills/pr/review-round.sh | 129 ++++++++++++++++++++++++++ .github/workflows/codex-pr-review.yml | 33 +++++++ .github/workflows/pi-pr-review.yml | 33 +++++++ .github/workflows/pr-ready-review.yml | 47 ++++++++++ docs/autonomous-mode.md | 2 + 6 files changed, 281 insertions(+), 2 deletions(-) create mode 100755 .agents/skills/pr/review-round.sh diff --git a/.agents/skills/pr/SKILL.md b/.agents/skills/pr/SKILL.md index 61758550b2..51cd2305c3 100644 --- a/.agents/skills/pr/SKILL.md +++ b/.agents/skills/pr/SKILL.md @@ -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 + ``` + + 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 @ ` + + - nit-only round, nits fixed or dismissed afterwards (head may have moved past the reviewed SHA — say so): + + `✅ Review round clean @ — nit-only verdicts at ; nits addressed in / dismissed in review replies` + + ```bash + gh pr comment --body "✅ Review round clean @ $(git rev-parse HEAD)" + gh pr ready + ``` + + 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) diff --git a/.agents/skills/pr/review-round.sh b/.agents/skills/pr/review-round.sh new file mode 100755 index 0000000000..a2dfa526a0 --- /dev/null +++ b/.agents/skills/pr/review-round.sh @@ -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() { # + 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 diff --git a/.github/workflows/codex-pr-review.yml b/.github/workflows/codex-pr-review.yml index 68bea1d715..26b2d9aae8 100644 --- a/.github/workflows/codex-pr-review.yml +++ b/.github/workflows/codex-pr-review.yml @@ -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 diff --git a/.github/workflows/pi-pr-review.yml b/.github/workflows/pi-pr-review.yml index 03c9599480..2f143663c8 100644 --- a/.github/workflows/pi-pr-review.yml +++ b/.github/workflows/pi-pr-review.yml @@ -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 diff --git a/.github/workflows/pr-ready-review.yml b/.github/workflows/pr-ready-review.yml index eb634977d1..c41ccafa9a 100644 --- a/.github/workflows/pr-ready-review.yml +++ b/.github/workflows/pr-ready-review.yml @@ -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 }} diff --git a/docs/autonomous-mode.md b/docs/autonomous-mode.md index dea2aa24ee..95d68136a6 100644 --- a/docs/autonomous-mode.md +++ b/docs/autonomous-mode.md @@ -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.)