diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 1e512ed..ce038c7 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -103,14 +103,25 @@ 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 && !github.event.pull_request.draft + # An edit of the title or description and a push can land on the same + # 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 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: - 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' && github.run_id || 'code' }} + cancel-in-progress: ${{ github.event.action != 'edited' }} outputs: risk: ${{ steps.verdict.outputs.risk }} complexity: ${{ steps.verdict.outputs.complexity }} @@ -188,6 +199,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 }} @@ -215,6 +227,30 @@ jobs: mode=reuse 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. + # 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' \ + | 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 50 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 @@ -284,7 +320,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 }}. @@ -462,8 +502,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 dab567a..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,6 +103,26 @@ jobs: path: .dx-central persist-credentials: false + - name: The labels and the review's comments, 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, and for the comments, where + # the review's last record says which commit it read. + run: | + 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 + 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 + - name: Check branch, title and commits env: # Through env, never interpolated into the script: a title is @@ -111,7 +135,9 @@ 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 }} + 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 57da37b..83505ad 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 @@ -37,16 +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. 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 @@ -151,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 0015243..3f70deb 100755 --- a/scripts/conventions.sh +++ b/scripts/conventions.sh @@ -9,16 +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), 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:-} + 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._/-]+\))?!?: [^ ].*[^.]$" @@ -37,10 +59,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" ] && 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 [ "$bang" = '!' ] || error "The pull request is labelled type:breaking, so its title needs the \"!\" of a breaking change: \"$token$bang:\"." @@ -79,13 +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:-} 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" - 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 + 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 @@ -105,6 +135,20 @@ 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 + # 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" 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" exit "$fail" fi