From ccc6ac17770e4811cbf76e149db8453ab5ac5328 Mon Sep 17 00:00:00 2001 From: Pablo Ariel Di Loreto Date: Wed, 30 Sep 2026 03:09:15 +0000 Subject: [PATCH 1/5] fix(review): bot pull requests are reviewed, edits cancel no review, conventions read live labels Three things the last week of Offload pull requests ran into. The review of a pull request the organisation's App opened (the weekly review-learnings proposals) ended without a verdict: the Claude action refuses any bot actor not in allowed_bots, which listed only Dependabot; it now lists the App's slug too. An edit of the title or description and a push landing on one commit shared the review's concurrency group with cancel-in-progress, so one cancelled the other and the cancelled check blocked the merge though the other passed; an edit now reviews in its own group and cancels nothing. The conventions read the labels from the event, so a re-run replayed the old type:* label; they now read the labels as they are, and compare the title's type with the label only once the review has read the commit, so a push that changes the title waits for the review's new reading instead of failing on the old label. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/claude-review.yml | 16 +++++++++++++--- .github/workflows/conventions.yml | 22 +++++++++++++++++++++- README.md | 12 ++++++++++-- scripts/conventions.sh | 19 ++++++++++++++----- 4 files changed, 58 insertions(+), 11 deletions(-) diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 1e512ed..29dbcf5 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -108,9 +108,15 @@ jobs: github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository && !github.event.pull_request.draft + # An edit of the title or description and a push can land on the same + # commit a second apart. Each gets its own group, so neither cancels the + # other: a cancelled run leaves a cancelled check on the commit, which + # blocks the merge even when the other run passed. A push still cancels + # the review of the push before it (a check on an older commit blocks + # nothing); an edit waits instead of cancelling. concurrency: - group: claude-review-${{ github.repository }}-${{ github.event.pull_request.number }} - cancel-in-progress: true + group: claude-review-${{ github.repository }}-${{ github.event.pull_request.number }}-${{ github.event.action == 'edited' && 'edited' || 'code' }} + cancel-in-progress: ${{ github.event.action != 'edited' }} outputs: risk: ${{ steps.verdict.outputs.risk }} complexity: ${{ steps.verdict.outputs.complexity }} @@ -284,7 +290,11 @@ jobs: with: anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }} github_token: ${{ steps.bot.outputs.token }} - allowed_bots: 'dependabot[bot]' + # Dependabot's pull requests, and the organisation's own App's (the + # weekly review-learnings proposals, the reproduction drafts): the + # action refuses any other bot as the actor, and the review of those + # ended without a verdict. + allowed_bots: dependabot[bot],${{ steps.bot.outputs.app-slug }} prompt: | You are the code reviewer for ${{ github.repository }}, pull request #${{ github.event.pull_request.number }}. diff --git a/.github/workflows/conventions.yml b/.github/workflows/conventions.yml index dab567a..720a13e 100644 --- a/.github/workflows/conventions.yml +++ b/.github/workflows/conventions.yml @@ -99,6 +99,25 @@ jobs: path: .dx-central persist-credentials: false + - name: The labels and the last review, as they are now + id: live + env: + GH_TOKEN: ${{ github.token }} + PR: ${{ github.event.pull_request.number }} + EVENT_LABELS: ${{ join(github.event.pull_request.labels.*.name, ',') }} + # The event carries the labels of the moment it fired, and a re-run + # replays that event: a type:* label the review changed since would + # read as the old one. Ask for them now, with the commit the last + # review read; without read access to the pull request, the event's. + run: | + if pr=$(gh pr view "$PR" --repo "$GITHUB_REPOSITORY" --json labels,comments 2>/dev/null); then + labels=$(jq -r '[.labels[].name] | join(",")' <<<"$pr") + reviewed=$(jq -r '[.comments[].body | capture("").r | fromjson? | .sha // empty] | last // ""' <<<"$pr") + else + labels=$EVENT_LABELS; reviewed="" + fi + { echo "labels=$labels"; echo "reviewed=$reviewed"; } >> "$GITHUB_OUTPUT" + - name: Check branch, title and commits env: # Through env, never interpolated into the script: a title is @@ -111,7 +130,8 @@ jobs: BODY: ${{ github.event.pull_request.body }} AUTHOR_TYPE: ${{ github.event.pull_request.user.type }} SECTIONS: ${{ inputs.required-sections }} - LABELS: ${{ join(github.event.pull_request.labels.*.name, ',') }} + LABELS: ${{ steps.live.outputs.labels }} + REVIEWED_SHA: ${{ steps.live.outputs.reviewed }} run: bash .dx-central/scripts/conventions.sh docs: diff --git a/README.md b/README.md index 57da37b..27dc676 100644 --- a/README.md +++ b/README.md @@ -26,7 +26,8 @@ request template, issue forms) unless it has its own. The rules for contributors base branch, so a change cannot pick its own reviewer), skips the repository's code rules for text-only changes, and after `max-auto-reviews` (5) keeps the last verdict until the `review:full` label asks for more. - Drafts and forks are not reviewed. + Drafts and forks are not reviewed; pull requests Dependabot or the + organisation's App open are (`allowed_bots`). 3. **Merge.** A human, with [`scripts/squash-merge.sh`](scripts/squash-merge.sh) ` ` (description verbatim, co-authors kept), or GitHub's auto-merge when the floor is low, the verdict is low risk and low @@ -44,7 +45,14 @@ merge of a pull request of this repository, that its tree is the one that pull request's checks ran on and that every check there passed; any doubt runs everything. A pull request whose base branch changed after its last push fails the conventions on every edit until a push runs the suites -against the new base. Once a week everything runs against today's +against the new base. The conventions read the labels as they are when +they run, not as the event carried them, so a re-run sees the `type:*` +label the review set since; and they compare the title's type with it only +once the review has read the commit (its last record names the commit), so +a push that changes the title waits for the review's new reading instead of +failing on the old label. An edit and a push that land on one commit review +in separate concurrency groups: neither cancels the other, since a +cancelled run leaves a cancelled check that blocks the merge. Once a week everything runs against today's WordPress and tools, and a failure opens one `ci:weekly` issue; *Run workflow* runs everything by hand, and its failure is in the run alone. diff --git a/scripts/conventions.sh b/scripts/conventions.sh index 0015243..e4ec935 100755 --- a/scripts/conventions.sh +++ b/scripts/conventions.sh @@ -9,7 +9,8 @@ # Environment: BRANCH, TITLE, BODY, BASE and HEAD_REF (the commits between # them are checked), MAX_HEADER (default 100), SECTIONS (comma-separated # headings, default "What changes,Why"; empty skips), LABELS (comma-separated, -# optional), AUTHOR_TYPE (Bot skips the sections). Run from the repository. +# optional), REVIEWED_SHA (the commit the last review read, optional), +# AUTHOR_TYPE (Bot skips the sections). Run from the repository. # # conventions.sh check (exit 1 on any broken rule) # conventions.sh --test self-test against a scratch repository @@ -19,6 +20,7 @@ check() { MAX_HEADER=${MAX_HEADER:-100} SECTIONS=${SECTIONS-What changes,Why} LABELS=${LABELS:-} + REVIEWED_SHA=${REVIEWED_SHA:-} AUTHOR_TYPE=${AUTHOR_TYPE:-User} TYPES='feat|fix|docs|style|refactor|perf|test|build|ci|chore|revert' HEADER_RE="^(${TYPES})(\([a-z0-9._/-]+\))?!?: [^ ].*[^.]$" @@ -37,10 +39,14 @@ check() { # The type:* label is the review's reading of the diff (or a person's # override); the title, which becomes the commit on main, must say the # same. The review corrects the title itself; this catches a later - # hand edit that undoes it. + # hand edit that undoes it. A label the review set on an earlier commit + # is not its reading of this one: until the review has read this commit + # (it runs after this check and relabels), the comparison waits. typelabel=$(tr ',' '\n' <<<"$LABELS" | grep -m1 '^type:' | sed 's/^type://' || true) TYPE_RE='^([a-z]+)(\([^)]*\))?(!?): ' - if [ -n "$typelabel" ] && [[ "$TITLE" =~ $TYPE_RE ]]; then + if [ -n "$typelabel" ] && [ -n "$REVIEWED_SHA" ] && [ "$REVIEWED_SHA" != "$HEAD_REF" ]; then + echo "The review has not read ${HEAD_REF:0:7} yet (last: ${REVIEWED_SHA:0:7}); the title's type is checked against its reading once it has." + elif [ -n "$typelabel" ] && [[ "$TITLE" =~ $TYPE_RE ]]; then token=${BASH_REMATCH[1]}; bang=${BASH_REMATCH[3]} if [ "$typelabel" = breaking ]; then [ "$bang" = '!' ] || error "The pull request is labelled type:breaking, so its title needs the \"!\" of a breaking change: \"$token$bang:\"." @@ -79,13 +85,14 @@ check() { # One case: a scratch repository whose branch adds one commit with this # message; expect 0 (passes) or 1 (fails). test_case() { - local name=$1 want=$2 branch=$3 title=$4 message=$5 body=$6 labels=${7:-} got dir + local name=$1 want=$2 branch=$3 title=$4 message=$5 body=$6 labels=${7:-} reviewed=${8:-} got dir dir=$(mktemp -d) ( cd "$dir" && git init -q && git config user.email t@t && git config user.name t git commit -q --allow-empty -m "chore: base" && git checkout -q -b topic git commit -q --allow-empty -m "$message" - BRANCH=$branch TITLE=$title BODY=$body LABELS=$labels BASE=$(git rev-parse HEAD~1) HEAD_REF=$(git rev-parse HEAD) check >/dev/null 2>&1 + [ "$reviewed" = head ] && reviewed=$(git rev-parse HEAD) + BRANCH=$branch TITLE=$title BODY=$body LABELS=$labels REVIEWED_SHA=$reviewed BASE=$(git rev-parse HEAD~1) HEAD_REF=$(git rev-parse HEAD) check >/dev/null 2>&1 ) && got=0 || got=1 rm -rf "$dir" if [ "$got" = "$want" ]; then echo "ok $name"; else echo "FAIL $name (want $want, got $got)"; return 1; fi @@ -105,6 +112,8 @@ if [ "${1:-}" = "--test" ]; then test_case "fails: an empty Why" 1 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" $'## ๐Ÿ“ What changes\n\nA thing.\n\n## ๐Ÿ’ก Why\n\n' || fail=1 test_case "fails: a Generated with footer" 1 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good"$'\n๐Ÿค– Generated with [Claude Code](https://claude.com/claude-code)' || fail=1 test_case "fails: title type unlike the label" 1 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good" "type:feat" || fail=1 + test_case "fails: unlike the label the review set on this commit" 1 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good" "type:feat" head || fail=1 + test_case "passes: a label from a commit the review has not read" 0 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good" "type:feat" 0123456789abcdef0123456789abcdef01234567 || fail=1 [ "$fail" -eq 0 ] && echo "all tests passed" exit "$fail" fi From e59e0604cdf74ad7acbfd6573b9016dc78face88 Mon Sep 17 00:00:00 2001 From: Pablo Ariel Di Loreto Date: Wed, 30 Sep 2026 03:16:36 +0000 Subject: [PATCH 2/5] fix(review): only the App's record gates the type check; capped reviews compare; edits spend nothing The local review of the first version found that the conventions trusted a review record in anyone's comment (a fake one switched the title check off, and a sha with a newline could write into GITHUB_OUTPUT), and that a capped review never writes a new record, which would switch the check off for good. The record is now read in conventions.sh from the REST comments, only from the review App's login, only with a 40-hex commit; the review writes its cap (max) into the record, and a capped review is compared against at once. An edit's review runs in a group of its own run, so a newer edit cannot cancel a queued one, and on a commit the review has not read it spends nothing (pending), so an edit and a push on one commit pay for one review. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/claude-review.yml | 30 +++++++++++++----- .github/workflows/conventions.yml | 24 +++++++++------ README.md | 14 +++++---- scripts/conventions.sh | 48 +++++++++++++++++++++++------ 4 files changed, 83 insertions(+), 33 deletions(-) diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 29dbcf5..e55b822 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -109,13 +109,15 @@ jobs: github.event.pull_request.head.repo.full_name == github.repository && !github.event.pull_request.draft # An edit of the title or description and a push can land on the same - # commit a second apart. Each gets its own group, so neither cancels the - # other: a cancelled run leaves a cancelled check on the commit, which - # blocks the merge even when the other run passed. A push still cancels - # the review of the push before it (a check on an older commit blocks - # nothing); an edit waits instead of cancelling. + # commit a second apart, and a cancelled run leaves a cancelled check on + # the commit that blocks the merge even when the other run passed. So an + # edit runs in a group of its own run: it cancels nothing and nothing + # cancels it (it spends nothing: it reuses the verdict on a commit the + # review read, and waits for the push's review on one it has not). A push + # still cancels the review of the push before it, whose check sits on an + # older commit and blocks nothing. concurrency: - group: claude-review-${{ github.repository }}-${{ github.event.pull_request.number }}-${{ github.event.action == 'edited' && 'edited' || 'code' }} + group: claude-review-${{ github.repository }}-${{ github.event.pull_request.number }}-${{ github.event.action == 'edited' && github.run_id || 'code' }} cancel-in-progress: ${{ github.event.action != 'edited' }} outputs: risk: ${{ steps.verdict.outputs.risk }} @@ -194,6 +196,7 @@ jobs: PR: ${{ github.event.pull_request.number }} HEAD: ${{ github.event.pull_request.head.sha }} BASE: ${{ github.event.pull_request.base.sha }} + ACTION: ${{ github.event.action }} FLOOR: ${{ steps.policy.outputs.floor }} MAX_AUTO: ${{ inputs.max-auto-reviews }} MODEL: ${{ steps.policy.outputs.model }} @@ -219,6 +222,10 @@ jobs: mode=full elif [ "$last" = "$HEAD" ] && [ "$has_verdict" = yes ]; then mode=reuse + elif [ "$ACTION" = edited ]; then + # The push's own run reviews this commit (or its review was + # capped): an edit does not pay for a second one. + mode=pending elif [ -n "$last" ] && [ "$count" -ge "$MAX_AUTO" ] && [ "$has_verdict" = yes ]; then mode=capped elif [ -n "$last" ] && git cat-file -e "$last^{commit}" 2>/dev/null \ @@ -370,6 +377,11 @@ jobs: } previous="$RUNNER_TEMP/previous.json" + if [ "$MODE" = pending ]; then + echo "::notice::The review of ${SHA:0:7} runs from its push; this edit is read there, or by the next review. Nothing spent." + exit 0 + fi + rank() { case "$1" in low) echo 0;; medium) echo 1;; *) echo 2;; esac; } if [ "$MODE" = reuse ] || [ "$MODE" = capped ]; then risk=$(jq -r .risk "$previous"); complexity=$(jq -r .complexity "$previous"); blocking=$(jq -r .blocking "$previous") @@ -472,8 +484,10 @@ jobs: # The changed files travel with the record: the weekly learning job # uses them as evidence for a repository's safe and risky paths. files=$(gh pr view "$PR" --repo "$GITHUB_REPOSITORY" --json files --jq '[.files[].path] | .[:300]' 2>/dev/null || echo '[]') - record=$(jq -c --arg risk "$risk" --arg floor "$FLOOR" --arg sha "$SHA" --arg model "$MODEL" --arg mode "$MODE" --argjson count "$count" --arg cost "${cost:-}" --argjson files "$files" --arg type "$type" --arg text "$text" \ - '{risk:$risk, model_risk:.risk, floor:$floor, complexity, blocking, type:$type, description_matches:.description_matches, text:$text, lessons, findings, sha:$sha, model:$model, mode:$mode, count:$count, cost:$cost, files:$files}' <<<"$OUT") + # `max` lets the conventions tell a capped review (no new reading + # will come) from one still to run on a new commit. + record=$(jq -c --arg risk "$risk" --arg floor "$FLOOR" --arg sha "$SHA" --arg model "$MODEL" --arg mode "$MODE" --argjson count "$count" --argjson max "${MAX_AUTO:-5}" --arg cost "${cost:-}" --argjson files "$files" --arg type "$type" --arg text "$text" \ + '{risk:$risk, model_risk:.risk, floor:$floor, complexity, blocking, type:$type, description_matches:.description_matches, text:$text, lessons, findings, sha:$sha, model:$model, mode:$mode, count:$count, max:$max, cost:$cost, files:$files}' <<<"$OUT") body=$(cat < ### Claude review ยท risk **$risk** ยท complexity **$complexity** ยท type **$type**$blk diff --git a/.github/workflows/conventions.yml b/.github/workflows/conventions.yml index 720a13e..a9fd694 100644 --- a/.github/workflows/conventions.yml +++ b/.github/workflows/conventions.yml @@ -38,6 +38,10 @@ on: description: Maximum length of a PR title or commit header. type: number default: 100 + review-bot: + description: The login of the App that posts the Claude review; only its comments carry a record the conventions trust. + type: string + default: 'dilux-bot[bot]' central-ref: description: Ref of DiluxOne/.github to read the conventions script from. type: string @@ -99,7 +103,7 @@ jobs: path: .dx-central persist-credentials: false - - name: The labels and the last review, as they are now + - name: The labels and the review's comments, as they are now id: live env: GH_TOKEN: ${{ github.token }} @@ -107,16 +111,17 @@ jobs: EVENT_LABELS: ${{ join(github.event.pull_request.labels.*.name, ',') }} # The event carries the labels of the moment it fired, and a re-run # replays that event: a type:* label the review changed since would - # read as the old one. Ask for them now, with the commit the last - # review read; without read access to the pull request, the event's. + # read as the old one. Ask for them now, and for the comments, where + # the review's last record says which commit it read. run: | - if pr=$(gh pr view "$PR" --repo "$GITHUB_REPOSITORY" --json labels,comments 2>/dev/null); then - labels=$(jq -r '[.labels[].name] | join(",")' <<<"$pr") - reviewed=$(jq -r '[.comments[].body | capture("").r | fromjson? | .sha // empty] | last // ""' <<<"$pr") + if labels=$(gh api "repos/$GITHUB_REPOSITORY/issues/$PR/labels" --jq '[.[].name] | join(",")') \ + && gh api "repos/$GITHUB_REPOSITORY/issues/$PR/comments" --paginate > "$RUNNER_TEMP/comments.json"; then + echo "labels=$labels" >> "$GITHUB_OUTPUT" else - labels=$EVENT_LABELS; reviewed="" + echo "::warning::Could not read the pull request's labels and comments (the caller's job needs pull-requests: read); the event's labels are used, and a re-run may see a type:* label the review has changed since." + echo "labels=$EVENT_LABELS" >> "$GITHUB_OUTPUT" + echo '[]' > "$RUNNER_TEMP/comments.json" fi - { echo "labels=$labels"; echo "reviewed=$reviewed"; } >> "$GITHUB_OUTPUT" - name: Check branch, title and commits env: @@ -131,7 +136,8 @@ jobs: AUTHOR_TYPE: ${{ github.event.pull_request.user.type }} SECTIONS: ${{ inputs.required-sections }} LABELS: ${{ steps.live.outputs.labels }} - REVIEWED_SHA: ${{ steps.live.outputs.reviewed }} + COMMENTS_FILE: ${{ runner.temp }}/comments.json + REVIEW_BOT: ${{ inputs.review-bot }} run: bash .dx-central/scripts/conventions.sh docs: diff --git a/README.md b/README.md index 27dc676..ca31b66 100644 --- a/README.md +++ b/README.md @@ -47,12 +47,14 @@ runs everything. A pull request whose base branch changed after its last push fails the conventions on every edit until a push runs the suites against the new base. The conventions read the labels as they are when they run, not as the event carried them, so a re-run sees the `type:*` -label the review set since; and they compare the title's type with it only -once the review has read the commit (its last record names the commit), so -a push that changes the title waits for the review's new reading instead of -failing on the old label. An edit and a push that land on one commit review -in separate concurrency groups: neither cancels the other, since a -cancelled run leaves a cancelled check that blocks the merge. Once a week everything runs against today's +label the review set since; they compare the title's type with it once the +review has read the commit (the review App's last record names it) or has +reached its cap, so a push that changes the title waits for the review's +new reading instead of failing on the old label. An edit's review runs in a +concurrency group of its own: it cancels nothing and nothing cancels it (a +cancelled run leaves a cancelled check that blocks the merge), and it +spends nothing, reusing the verdict on a commit the review read and +leaving a commit it has not read to its push's run. Once a week everything runs against today's WordPress and tools, and a failure opens one `ci:weekly` issue; *Run workflow* runs everything by hand, and its failure is in the run alone. diff --git a/scripts/conventions.sh b/scripts/conventions.sh index e4ec935..46b4941 100755 --- a/scripts/conventions.sh +++ b/scripts/conventions.sh @@ -9,18 +9,38 @@ # Environment: BRANCH, TITLE, BODY, BASE and HEAD_REF (the commits between # them are checked), MAX_HEADER (default 100), SECTIONS (comma-separated # headings, default "What changes,Why"; empty skips), LABELS (comma-separated, -# optional), REVIEWED_SHA (the commit the last review read, optional), -# AUTHOR_TYPE (Bot skips the sections). Run from the repository. +# optional), COMMENTS_FILE and REVIEW_BOT (the pull request's comments as the +# REST API lists them, and the login of the App whose review records count; +# optional), AUTHOR_TYPE (Bot skips the sections). Run from the repository. # # conventions.sh check (exit 1 on any broken rule) # conventions.sh --test self-test against a scratch repository set -euo pipefail +# Whether the review is still to read HEAD_REF: its last record (only the +# review App's own comment counts, and only a 40-hex commit) names another +# commit, and the review is not capped (count below max, 5 by default: a +# capped review reads no new commit, so its label stands). Without a +# readable record the comparison runs: nothing here can switch it off. +review_pending() { + local record sha count max + [ -n "$COMMENTS_FILE" ] && [ -s "$COMMENTS_FILE" ] || return 1 + # shellcheck disable=SC2016 # jq variables, not shell ones. + record=$(jq -r --arg bot "$REVIEW_BOT" '[.[] | select(.user.login == $bot and (.body | startswith(""))) | .body] | last // ""' "$COMMENTS_FILE" 2>/dev/null \ + | sed -n 's/^.*.*$/\1/p' | tail -1 || true) + sha=$(jq -r '.sha // ""' <<<"${record:-null}" 2>/dev/null || true) + count=$(jq -r '.count // 0' <<<"${record:-null}" 2>/dev/null || echo 0) + max=$(jq -r '.max // 5' <<<"${record:-null}" 2>/dev/null || echo 5) + [[ "$sha" =~ ^[0-9a-f]{40}$ ]] && [[ "$count" =~ ^[0-9]+$ ]] && [[ "$max" =~ ^[0-9]+$ ]] || return 1 + [ "$sha" != "$HEAD_REF" ] && [ "$count" -lt "$max" ] +} + check() { MAX_HEADER=${MAX_HEADER:-100} SECTIONS=${SECTIONS-What changes,Why} LABELS=${LABELS:-} - REVIEWED_SHA=${REVIEWED_SHA:-} + COMMENTS_FILE=${COMMENTS_FILE:-} + REVIEW_BOT=${REVIEW_BOT:-dilux-bot[bot]} AUTHOR_TYPE=${AUTHOR_TYPE:-User} TYPES='feat|fix|docs|style|refactor|perf|test|build|ci|chore|revert' HEADER_RE="^(${TYPES})(\([a-z0-9._/-]+\))?!?: [^ ].*[^.]$" @@ -44,8 +64,8 @@ check() { # (it runs after this check and relabels), the comparison waits. typelabel=$(tr ',' '\n' <<<"$LABELS" | grep -m1 '^type:' | sed 's/^type://' || true) TYPE_RE='^([a-z]+)(\([^)]*\))?(!?): ' - if [ -n "$typelabel" ] && [ -n "$REVIEWED_SHA" ] && [ "$REVIEWED_SHA" != "$HEAD_REF" ]; then - echo "The review has not read ${HEAD_REF:0:7} yet (last: ${REVIEWED_SHA:0:7}); the title's type is checked against its reading once it has." + if [ -n "$typelabel" ] && review_pending; then + echo "The review has not read ${HEAD_REF:0:7} yet; the title's type is checked against its reading once it has." elif [ -n "$typelabel" ] && [[ "$TITLE" =~ $TYPE_RE ]]; then token=${BASH_REMATCH[1]}; bang=${BASH_REMATCH[3]} if [ "$typelabel" = breaking ]; then @@ -85,14 +105,17 @@ check() { # One case: a scratch repository whose branch adds one commit with this # message; expect 0 (passes) or 1 (fails). test_case() { - local name=$1 want=$2 branch=$3 title=$4 message=$5 body=$6 labels=${7:-} reviewed=${8:-} got dir + local name=$1 want=$2 branch=$3 title=$4 message=$5 body=$6 labels=${7:-} comments=${8:-} got dir dir=$(mktemp -d) ( cd "$dir" && git init -q && git config user.email t@t && git config user.name t git commit -q --allow-empty -m "chore: base" && git checkout -q -b topic git commit -q --allow-empty -m "$message" - [ "$reviewed" = head ] && reviewed=$(git rev-parse HEAD) - BRANCH=$branch TITLE=$title BODY=$body LABELS=$labels REVIEWED_SHA=$reviewed BASE=$(git rev-parse HEAD~1) HEAD_REF=$(git rev-parse HEAD) check >/dev/null 2>&1 + file="" + if [ -n "$comments" ]; then + file=$(mktemp); printf '%s' "${comments//HEADSHA/$(git rev-parse HEAD)}" > "$file" + fi + BRANCH=$branch TITLE=$title BODY=$body LABELS=$labels COMMENTS_FILE=$file REVIEW_BOT='dilux-bot[bot]' BASE=$(git rev-parse HEAD~1) HEAD_REF=$(git rev-parse HEAD) check >/dev/null 2>&1 ) && got=0 || got=1 rm -rf "$dir" if [ "$got" = "$want" ]; then echo "ok $name"; else echo "FAIL $name (want $want, got $got)"; return 1; fi @@ -112,8 +135,13 @@ if [ "${1:-}" = "--test" ]; then test_case "fails: an empty Why" 1 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" $'## ๐Ÿ“ What changes\n\nA thing.\n\n## ๐Ÿ’ก Why\n\n' || fail=1 test_case "fails: a Generated with footer" 1 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good"$'\n๐Ÿค– Generated with [Claude Code](https://claude.com/claude-code)' || fail=1 test_case "fails: title type unlike the label" 1 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good" "type:feat" || fail=1 - test_case "fails: unlike the label the review set on this commit" 1 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good" "type:feat" head || fail=1 - test_case "passes: a label from a commit the review has not read" 0 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good" "type:feat" 0123456789abcdef0123456789abcdef01234567 || fail=1 + rec() { printf '[{"user":{"login":"%s"},"body":"\\n"}]' "$1" "$2" "$3" "$4"; } + other=0123456789abcdef0123456789abcdef01234567 + test_case "fails: unlike the label the review set on this commit" 1 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good" "type:feat" "$(rec 'dilux-bot[bot]' HEADSHA 1 '')" || fail=1 + test_case "passes: a label from a commit the review has not read" 0 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good" "type:feat" "$(rec 'dilux-bot[bot]' "$other" 1 '')" || fail=1 + test_case "fails: a record anyone else posted counts for nothing" 1 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good" "type:feat" "$(rec 'someone' "$other" 1 '')" || fail=1 + test_case "fails: a capped review reads no new commit" 1 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good" "type:feat" "$(rec 'dilux-bot[bot]' "$other" 5 ',\\"max\\":5')" || fail=1 + test_case "fails: a record whose sha is not a commit" 1 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good" "type:feat" "$(rec 'dilux-bot[bot]' 'abc\\nx' 1 '')" || fail=1 [ "$fail" -eq 0 ] && echo "all tests passed" exit "$fail" fi From 83d63369c6df7fbfbe14cbfae9438402580a3995 Mon Sep 17 00:00:00 2001 From: Pablo Ariel Di Loreto Date: Wed, 30 Sep 2026 03:19:05 +0000 Subject: [PATCH 3/5] fix(review): an edit on an unread commit waits for its push's verdict instead of passing The local review found that the pending mode ended green without a verdict, under the same check name as the push's review: editing the title after a blocking or unfinished push review would have turned the required check green. An edit on a commit its push's run is still reviewing now waits for that run's record and reuses its verdict (blocking included, and the edited description compared with the one the review read), and fails when none comes; a capped review is tried first and replays its verdict. The README paragraph is re-wrapped, and the conventions test the last page of paginated comments. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/claude-review.yml | 35 +++++++++++++++++-------- README.md | 40 +++++++++++++++-------------- scripts/conventions.sh | 1 + 3 files changed, 47 insertions(+), 29 deletions(-) diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index e55b822..5f2d197 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -113,7 +113,8 @@ jobs: # the commit that blocks the merge even when the other run passed. So an # edit runs in a group of its own run: it cancels nothing and nothing # cancels it (it spends nothing: it reuses the verdict on a commit the - # review read, and waits for the push's review on one it has not). A push + # review read, and waits for the push's review of one it has not, then + # reuses that; with no such review it fails). A push # still cancels the review of the push before it, whose check sits on an # older commit and blocks nothing. concurrency: @@ -222,12 +223,31 @@ jobs: mode=full elif [ "$last" = "$HEAD" ] && [ "$has_verdict" = yes ]; then mode=reuse - elif [ "$ACTION" = edited ]; then - # The push's own run reviews this commit (or its review was - # capped): an edit does not pay for a second one. - mode=pending elif [ -n "$last" ] && [ "$count" -ge "$MAX_AUTO" ] && [ "$has_verdict" = yes ]; then mode=capped + elif [ "$ACTION" = edited ]; then + # An edit on a commit its push's run is still reviewing: it pays + # for no second review. It waits for that run's record of this + # commit and reuses its verdict (the edited description is then + # compared with the one the review read), and fails when none + # comes: a green check without a verdict would replace the push + # run's own under the same name. + for _ in $(seq 1 50); do + sleep 30 + record=$(gh api "repos/$GITHUB_REPOSITORY/issues/$PR/comments" --paginate \ + | jq -r --arg bot "$BOT" '.[] | select(.user.login == $bot and (.body | startswith(""))) | .body' \ + | grep -oP ')' | tail -1 || true) + printf '%s' "${record:-null}" | jq . > "$RUNNER_TEMP/previous.json" 2>/dev/null || echo null > "$RUNNER_TEMP/previous.json" + if [ "$(jq -r '.sha // empty' "$RUNNER_TEMP/previous.json")" = "$HEAD" ] \ + && [ "$(jq -r 'if .risk and .complexity and (.blocking != null) then "yes" else "no" end' "$RUNNER_TEMP/previous.json")" = yes ]; then + mode=reuse; break + fi + done + if [ "$mode" != reuse ]; then + echo "::error::No review of ${HEAD:0:7} came from its push within 25 minutes; push again, or re-run that review." + exit 1 + fi + last=$HEAD; count=$(jq -r '.count // 0' "$RUNNER_TEMP/previous.json") elif [ -n "$last" ] && git cat-file -e "$last^{commit}" 2>/dev/null \ && git merge-base --is-ancestor "$last" "$HEAD" \ && [ -z "$(git rev-list --merges "$last..$HEAD")" ]; then @@ -377,11 +397,6 @@ jobs: } previous="$RUNNER_TEMP/previous.json" - if [ "$MODE" = pending ]; then - echo "::notice::The review of ${SHA:0:7} runs from its push; this edit is read there, or by the next review. Nothing spent." - exit 0 - fi - rank() { case "$1" in low) echo 0;; medium) echo 1;; *) echo 2;; esac; } if [ "$MODE" = reuse ] || [ "$MODE" = capped ]; then risk=$(jq -r .risk "$previous"); complexity=$(jq -r .complexity "$previous"); blocking=$(jq -r .blocking "$previous") diff --git a/README.md b/README.md index ca31b66..883d470 100644 --- a/README.md +++ b/README.md @@ -38,25 +38,27 @@ Mention `@dilux-bot` in a review thread or in the conversation and Claude answers there; it resolves its own thread when the point is settled. Nothing runs twice for one change. An edit of the title or the description -re-runs only the conventions, the review (free on a commit it already read) -and the auto-merge decision, never the suites. A push to `main` runs the slow -suites only when it must: the job verifies that the commit is the squash -merge of a pull request of this repository, that its tree is the one that -pull request's checks ran on and that every check there passed; any doubt -runs everything. A pull request whose base branch changed after its last -push fails the conventions on every edit until a push runs the suites -against the new base. The conventions read the labels as they are when -they run, not as the event carried them, so a re-run sees the `type:*` -label the review set since; they compare the title's type with it once the -review has read the commit (the review App's last record names it) or has -reached its cap, so a push that changes the title waits for the review's -new reading instead of failing on the old label. An edit's review runs in a -concurrency group of its own: it cancels nothing and nothing cancels it (a -cancelled run leaves a cancelled check that blocks the merge), and it -spends nothing, reusing the verdict on a commit the review read and -leaving a commit it has not read to its push's run. Once a week everything runs against today's -WordPress and tools, and a failure opens one `ci:weekly` issue; *Run -workflow* runs everything by hand, and its failure is in the run alone. +re-runs only the conventions, the review (free on a commit it already +read) and the auto-merge decision, never the suites. A push to `main` runs +the slow suites only when it must: the job verifies that the commit is the +squash merge of a pull request of this repository, that its tree is the +one that pull request's checks ran on and that every check there passed; +any doubt runs everything. A pull request whose base branch changed after +its last push fails the conventions on every edit until a push runs the +suites against the new base. The conventions read the labels as they are +when they run, not as the event carried them, so a re-run sees the +`type:*` label the review set since; they compare the title's type with it +once the review has read the commit (the review App's last record names +it) or has reached its cap, so a push that changes the title waits for the +review's new reading instead of failing on the old label. An edit's review +runs in a concurrency group of its own: it cancels nothing and nothing +cancels it, since a cancelled run leaves a cancelled check that blocks the +merge. It spends nothing: on a commit the review read it reuses the +verdict, and on one its push's run is still reviewing it waits for that +verdict and reuses it, or fails when none comes. Once a week everything +runs against today's WordPress and tools, and a failure opens one +`ci:weekly` issue; *Run workflow* runs everything by hand, and its failure +is in the run alone. ## Adopt it in a new repository diff --git a/scripts/conventions.sh b/scripts/conventions.sh index 46b4941..a273c48 100755 --- a/scripts/conventions.sh +++ b/scripts/conventions.sh @@ -142,6 +142,7 @@ if [ "${1:-}" = "--test" ]; then test_case "fails: a record anyone else posted counts for nothing" 1 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good" "type:feat" "$(rec 'someone' "$other" 1 '')" || fail=1 test_case "fails: a capped review reads no new commit" 1 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good" "type:feat" "$(rec 'dilux-bot[bot]' "$other" 5 ',\\"max\\":5')" || fail=1 test_case "fails: a record whose sha is not a commit" 1 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good" "type:feat" "$(rec 'dilux-bot[bot]' 'abc\\nx' 1 '')" || fail=1 + test_case "passes: the last page's record (paginated arrays)" 0 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good" "type:feat" "$(rec 'dilux-bot[bot]' HEADSHA 1 '')$(rec 'dilux-bot[bot]' "$other" 2 '')" || fail=1 [ "$fail" -eq 0 ] && echo "all tests passed" exit "$fail" fi From a7779d55ce0e023e66b9e343d57f55f4b16e828d Mon Sep 17 00:00:00 2001 From: Pablo Ariel Di Loreto Date: Wed, 30 Sep 2026 03:22:14 +0000 Subject: [PATCH 4/5] test(conventions): the capped-review case builds valid JSON and has a counterpart under the cap The case passed a cap through printf's %s with its backslashes doubled, so the record was invalid JSON and the comparison ran for that reason, not because of the cap. The cap is now built inside rec, and a record under a cap of 8 waits where one at 3 of 3 compares. The README tables list review-bot, COMMENTS_FILE and REVIEW_BOT. Co-Authored-By: Claude Opus 5.5 --- README.md | 4 ++-- scripts/conventions.sh | 10 ++++++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 883d470..83505ad 100644 --- a/README.md +++ b/README.md @@ -163,11 +163,11 @@ Call them pinned to `@v2`; a breaking change ships as `v2`. A stack suffix | Workflow | Does | Inputs | | --- | --- | --- | -| [`conventions.yml`](.github/workflows/conventions.yml) | Branch, title, commits, description sections, no "Generated with" footer (all in `scripts/conventions.sh`), relative doc links, retired names. | `retired-names`, `required-sections`, `max-header`, `central-ref` | +| [`conventions.yml`](.github/workflows/conventions.yml) | Branch, title, commits, description sections, no "Generated with" footer (all in `scripts/conventions.sh`), relative doc links, retired names. Reads the labels and the review App's last record live, so a re-run sees today's `type:*` label. | `retired-names`, `required-sections`, `max-header`, `central-ref`, `review-bot` | | [`claude-review.yml`](.github/workflows/claude-review.yml) | The review described above. Outputs `risk`, `complexity`, `floor`, `trusted`, `blocking`. | `profile`, `central-ref`, `max-auto-reviews` | | [`review-reply.yml`](.github/workflows/review-reply.yml) | Answers `@dilux-bot` mentions from members and collaborators, with the strong model. | `profile`, `central-ref` | | [`auto-merge.yml`](.github/workflows/auto-merge.yml) | Turns GitHub's auto-merge on or off from the review's outputs and commits the description verbatim. `pull-request-edited.yml` runs it on `edited` too. | the five review outputs | -| [`scripts/conventions.sh`](scripts/conventions.sh) | Not a workflow: the conventions a pull request is held to (branch, title, every commit header, no session trailer, description sections, no "Generated with" footer). `conventions.yml` runs it on a pull request, `local-review.sh` before one; `--test` for its own tests. | env: `BRANCH`, `TITLE`, `BODY`, `BASE`, `HEAD_REF`, `MAX_HEADER`, `SECTIONS`, `LABELS`, `AUTHOR_TYPE` | +| [`scripts/conventions.sh`](scripts/conventions.sh) | Not a workflow: the conventions a pull request is held to (branch, title, every commit header, no session trailer, description sections, no "Generated with" footer). `conventions.yml` runs it on a pull request, `local-review.sh` before one; `--test` for its own tests. | env: `BRANCH`, `TITLE`, `BODY`, `BASE`, `HEAD_REF`, `COMMENTS_FILE`, `REVIEW_BOT`, `MAX_HEADER`, `SECTIONS`, `LABELS`, `AUTHOR_TYPE` | | [`scripts/review-brief.sh`](scripts/review-brief.sh) | Not a workflow: the review brief, the one file the Claude review reads (profiles, `AGENTS.md`, `docs/architecture.md`, policy floor, description, diff). `claude-review.yml` and `local-review.sh` build it with the same script; `--test` for its own tests. | env: see the script's header | | [`scripts/local-review.sh`](scripts/local-review.sh) | Not a workflow: the pull request's checks before it exists, on a contributor's machine: conventions, policy floor, brief, and the review through the local Claude Code CLI; the findings go to `.git/dx-review/findings.md` and the next run is incremental. See CONTRIBUTING.md, "Review before the pull request"; `--test` for its own tests (never runs the review). | `--base`, `--title`, `--body-file`, `--profile`, `--no-claude`, `--full`, `--model` | | [`scripts/release-ready.sh`](scripts/release-ready.sh) | Not a workflow: whether a readme's newest changelog entry is ready (exit 0) or held by a first line `Unreleased.` (exit 1); `--test` for its own tests. The release job's hold. | the readme | diff --git a/scripts/conventions.sh b/scripts/conventions.sh index a273c48..3f70deb 100755 --- a/scripts/conventions.sh +++ b/scripts/conventions.sh @@ -135,12 +135,18 @@ if [ "${1:-}" = "--test" ]; then test_case "fails: an empty Why" 1 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" $'## ๐Ÿ“ What changes\n\nA thing.\n\n## ๐Ÿ’ก Why\n\n' || fail=1 test_case "fails: a Generated with footer" 1 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good"$'\n๐Ÿค– Generated with [Claude Code](https://claude.com/claude-code)' || fail=1 test_case "fails: title type unlike the label" 1 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good" "type:feat" || fail=1 - rec() { printf '[{"user":{"login":"%s"},"body":"\\n"}]' "$1" "$2" "$3" "$4"; } + # One page of comments holding the review App's record: login, sha, count + # and, optionally, the cap. + rec() { + local max=""; [ -n "${4:-}" ] && max=",\\\"max\\\":$4" + printf '[{"user":{"login":"%s"},"body":"\\n"}]' "$1" "$2" "$3" "$max" + } other=0123456789abcdef0123456789abcdef01234567 test_case "fails: unlike the label the review set on this commit" 1 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good" "type:feat" "$(rec 'dilux-bot[bot]' HEADSHA 1 '')" || fail=1 test_case "passes: a label from a commit the review has not read" 0 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good" "type:feat" "$(rec 'dilux-bot[bot]' "$other" 1 '')" || fail=1 test_case "fails: a record anyone else posted counts for nothing" 1 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good" "type:feat" "$(rec 'someone' "$other" 1 '')" || fail=1 - test_case "fails: a capped review reads no new commit" 1 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good" "type:feat" "$(rec 'dilux-bot[bot]' "$other" 5 ',\\"max\\":5')" || fail=1 + test_case "fails: a capped review reads no new commit" 1 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good" "type:feat" "$(rec 'dilux-bot[bot]' "$other" 3 3)" || fail=1 + test_case "passes: under a cap of its own, the review is still to come" 0 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good" "type:feat" "$(rec 'dilux-bot[bot]' "$other" 3 8)" || fail=1 test_case "fails: a record whose sha is not a commit" 1 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good" "type:feat" "$(rec 'dilux-bot[bot]' 'abc\\nx' 1 '')" || fail=1 test_case "passes: the last page's record (paginated arrays)" 0 fix/a-thing "fix(sync): a thing" "fix(sync): a thing" "$good" "type:feat" "$(rec 'dilux-bot[bot]' HEADSHA 1 '')$(rec 'dilux-bot[bot]' "$other" 2 '')" || fail=1 [ "$fail" -eq 0 ] && echo "all tests passed" From 3454d08e8dd7ba438032d3e25a0522646e80b76c Mon Sep 17 00:00:00 2001 From: Pablo Ariel Di Loreto Date: Wed, 30 Sep 2026 03:40:49 +0000 Subject: [PATCH 5/5] fix(review): an edit waits up to 50 minutes for its push's review, longer than that review can run The review on GitHub found that the edit's 25-minute wait was no longer than a push's review job (30 minutes, its Claude step alone 25), so an edit could fail with nothing wrong. The edit now waits up to 50 minutes, and an edit's job gets 60. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/claude-review.yml | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 5f2d197..ce038c7 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -103,7 +103,9 @@ jobs: review: name: Claude review runs-on: ubuntu-latest - timeout-minutes: 30 + # An edit may wait for its push's review, which itself can take the 30 + # minutes a review job has: an edit's job gets longer to outlast it. + timeout-minutes: ${{ github.event.action == 'edited' && 60 || 30 }} if: >- github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository && @@ -232,7 +234,8 @@ jobs: # compared with the one the review read), and fails when none # comes: a green check without a verdict would replace the push # run's own under the same name. - for _ in $(seq 1 50); do + # Up to 50 minutes: longer than a push's review job (30) can run. + for _ in $(seq 1 100); do sleep 30 record=$(gh api "repos/$GITHUB_REPOSITORY/issues/$PR/comments" --paginate \ | jq -r --arg bot "$BOT" '.[] | select(.user.login == $bot and (.body | startswith(""))) | .body' \ @@ -244,7 +247,7 @@ jobs: fi done if [ "$mode" != reuse ]; then - echo "::error::No review of ${HEAD:0:7} came from its push within 25 minutes; push again, or re-run that review." + echo "::error::No review of ${HEAD:0:7} came from its push within 50 minutes; push again, or re-run that review." exit 1 fi last=$HEAD; count=$(jq -r '.count // 0' "$RUNNER_TEMP/previous.json")