From 70dae11b9eb14ccc41c1f020bab00e6f7d332672 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:51:52 -0400 Subject: [PATCH 1/2] fix(source-control): pass over skipped-validation reds in review-venue A pull request that edits a review caller workflow ends both review lanes red by design (failure-class=skipped-validation). review-venue took that run as the lane's verdict and marked both lanes local failing for every other pull request. A failed run whose failed jobs all carry that annotation now reaches no verdict; other failures still count. Closes #8867 Co-Authored-By: Claude Opus 5.5 --- ...eview-venue-skipped-validation-5c1e7a20.md | 7 ++ .../github/pull-request/review-venue.md | 21 ++++ .../pull-request/reference/review-venue.md | 6 +- .../pull-request/scripts/review-venue.sh | 58 +++++++--- .../pull-request/scripts/review-venue.test.sh | 109 ++++++++++++++++-- 5 files changed, 177 insertions(+), 24 deletions(-) create mode 100644 .changes/source-control/fix-8867-review-venue-skipped-validation-5c1e7a20.md diff --git a/.changes/source-control/fix-8867-review-venue-skipped-validation-5c1e7a20.md b/.changes/source-control/fix-8867-review-venue-skipped-validation-5c1e7a20.md new file mode 100644 index 0000000000..9ecf8990e3 --- /dev/null +++ b/.changes/source-control/fix-8867-review-venue-skipped-validation-5c1e7a20.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- `review-venue` no longer reads a lane as `local failing` because one pull request edited the lane's caller workflow: a run whose failed jobs all report `failure-class=skipped-validation` reaches no verdict, so the lane's health comes from the newest run before it. Any other failure, a usage-limit stop included, still reads failing ([review-venue.md](https://github.com/melodic-software/claude-code-plugins/blob/main/plugins/source-control/skills/pull-request/reference/review-venue.md)). diff --git a/plugins/source-control/reference/providers/github/pull-request/review-venue.md b/plugins/source-control/reference/providers/github/pull-request/review-venue.md index 8247d83672..84f28f0b16 100644 --- a/plugins/source-control/reference/providers/github/pull-request/review-venue.md +++ b/plugins/source-control/reference/providers/github/pull-request/review-venue.md @@ -50,3 +50,24 @@ Runs come newest first. The script takes the first with `status` `completed` and other than `skipped` (a draft, or an actor the lane does not admit) and `cancelled` (a newer push superseded it), then compares its `created_at` with the window. A usage-limit stop exits the lane job non-zero, so its run concludes `failure`. + +A run that did not succeed gets two more reads before it counts: + +```bash +gh api "repos/{owner}/{repo}/actions/runs//jobs?per_page=100" +gh api "repos/{owner}/{repo}/check-runs//annotations?per_page=100" +``` + +A job's `id` is its check run's id. When every failed or timed-out job carries a `failure` +annotation whose message starts `failure-class=skipped-validation`, the pull request edited the +caller workflow, claude-code-action skipped itself, and the run is passed over like a skipped one. +Any other class, `rate-limit` included, or a failed run with no failed job, keeps its verdict. +After 10 such runs in a row the lane reads `local stale`. Pointer: the annotation is the reusable +workflow's last step, `echo "::error title=$LANE failed::failure-class=..."`, in +[ci-workflows `pr-review.yml`](https://github.com/melodic-software/ci-workflows/blob/main/.github/workflows/pr-review.yml) +and `pr-review-security.yml`; the reads are at +[REST API: list jobs for a workflow run](https://docs.github.com/en/rest/actions/workflow-jobs#list-jobs-for-a-workflow-run) +and [REST API: list check run annotations](https://docs.github.com/en/rest/checks/runs#list-check-run-annotations). +As of: 2026-10-21, read from runs 39610040287 and 39610040502. Recheck trigger: the reusable +workflows rename `failure-class` or `skipped-validation`, or stop writing the class as an +annotation. diff --git a/plugins/source-control/skills/pull-request/reference/review-venue.md b/plugins/source-control/skills/pull-request/reference/review-venue.md index 77238433fc..e8ce3497a5 100644 --- a/plugins/source-control/skills/pull-request/reference/review-venue.md +++ b/plugins/source-control/skills/pull-request/reference/review-venue.md @@ -20,8 +20,10 @@ Refuse a `--repo` value holding a character outside `A-Za-z0-9._/-`. It prints o `code-review` first, then `security-review`: - **` ci healthy: `**: the lane's workflow is enabled and its newest pull request - run that reached a verdict (finished, and neither skipped nor cancelled) succeeded within the - window. CI covers this lane. + run that reached a verdict succeeded within the window. CI covers this lane. A run reaches no + verdict when it was skipped, cancelled, or failed only because its pull request edits the + lane's caller workflow (`skipped-validation`): that red says something about one pull request, + not the lane. - **` local absent: `**: no recent pull request run in the repository calls the lane, or the caller workflow is disabled. - **` local failing: `**: that newest run failed, timed out or did not start. A diff --git a/plugins/source-control/skills/pull-request/scripts/review-venue.sh b/plugins/source-control/skills/pull-request/scripts/review-venue.sh index 3973700566..3cd82a6aa5 100755 --- a/plugins/source-control/skills/pull-request/scripts/review-venue.sh +++ b/plugins/source-control/skills/pull-request/scripts/review-venue.sh @@ -6,9 +6,11 @@ # workflow: ci-workflows pr-review.yml (code-review) or pr-review-security.yml # (security-review). It is healthy when its workflow is active and its newest # pull_request run that reached a verdict (completed, and neither skipped nor -# cancelled) succeeded and was created within --max-age-days. A disabled -# workflow, a failed run (a usage-limit stop included), a timed-out one, or none -# in the window is not healthy. +# cancelled) succeeded and was created within --max-age-days. A failed run whose +# every failed job says failure-class=skipped-validation reached no verdict: the +# pull request edits the caller workflow, so the review skipped itself by +# design. A disabled workflow, any other failed run (a usage-limit stop +# included), a timed-out one, or none in the window is not healthy. # # Usage: # review-venue.sh [--repo ] [--max-age-days ] @@ -68,6 +70,25 @@ REUSABLE=( ) WORKFLOW_ID=("" "") WORKFLOW_PATH=("" "") PAGES=3 +# Skipped-validation reds passed over per lane before it reads stale. +MAX_PASSED=10 + +# skipped_validation : true when the run failed only because the pull +# request edits the caller workflow. A run with no failed job keeps its verdict. +skipped_validation() { + local jobs ids id ann hit + jobs=$(gh api "repos/$REPO/actions/runs/$1/jobs?per_page=100") || die 2 "cannot read the jobs of run $1" + ids=$(jq -r '.jobs[]? | select(.conclusion | IN("failure", "timed_out")) | .id' <<<"$jobs") || + die 2 "cannot parse the jobs of run $1" + [[ -n "$ids" ]] || return 1 + for id in $ids; do + [[ "$id" =~ ^[0-9]+$ ]] || die 2 "unexpected job id in run $1" + ann=$(gh api "repos/$REPO/check-runs/$id/annotations?per_page=100") || die 2 "cannot read the annotations of job $id" + hit=$(jq 'any(.[]?; .annotation_level == "failure" and (.message | test("^failure-class=skipped-validation(:|$)")))' <<<"$ann") || + die 2 "cannot parse the annotations of job $id" + [[ "$hit" == true ]] || return 1 + done +} # Find each lane's caller workflow among the newest pull_request runs. Every PR # event starts each lane, a draft's run included (it concludes skipped), so a @@ -106,15 +127,24 @@ for i in "${!LANES[@]}"; do fi lane_runs=$(gh api "repos/$REPO/actions/workflows/$id/runs?event=pull_request&per_page=100") || die 2 "cannot list the runs of $path" - jq -r --arg lane "$lane" --arg path "$path" --argjson days "$MAX_AGE_DAYS" ' - [.workflow_runs[]? | select(.status == "completed" and (.conclusion | IN("skipped", "cancelled") | not))][0] as $r - | if $r == null then - "\($lane) local stale: \($path) has no run with a verdict among its newest 100" - elif ($r.created_at | fromdateiso8601) < (now - $days * 86400) then - "\($lane) local stale: the newest run with a verdict, \($r.id), is from \($r.created_at), older than \($days) days (\($path))" - elif $r.conclusion == "success" then - "\($lane) ci healthy: newest run with a verdict, \($r.id), succeeded at \($r.created_at) (\($path))" - else - "\($lane) local failing: newest run with a verdict, \($r.id), concluded \($r.conclusion) at \($r.created_at) (\($path))" - end' <<<"$lane_runs" || die 2 "cannot parse the runs of $path" + verdicts=$(jq -r '.workflow_runs[]? | select(.status == "completed" and (.conclusion | IN("skipped", "cancelled") | not)) + | "\(.id)\t\(.conclusion)\t\(.created_at)"' <<<"$lane_runs") || die 2 "cannot parse the runs of $path" + rid="" passed=0 + while IFS=$'\t' read -r rid conclusion created; do + [[ -n "$rid" ]] || continue + [[ "$rid" =~ ^[0-9]+$ ]] || die 2 "unexpected run id in $path" + if [[ "$conclusion" == success ]] || ! skipped_validation "$rid"; then break; fi + rid="" passed=$((passed + 1)) + ((passed < MAX_PASSED)) || break + done <<<"$verdicts" + [[ -z "$rid" ]] || made=$(jq -nr --arg t "$created" '$t | fromdateiso8601') || die 2 "cannot parse the time of run $rid" + if [[ -z "$rid" ]]; then + printf '%s local stale: %s has no run with a verdict among its newest 100\n' "$lane" "$path" + elif ((made < $(date +%s) - MAX_AGE_DAYS * 86400)); then + printf '%s local stale: the newest run with a verdict, %s, is from %s, older than %s days (%s)\n' "$lane" "$rid" "$created" "$MAX_AGE_DAYS" "$path" + elif [[ "$conclusion" == success ]]; then + printf '%s ci healthy: newest run with a verdict, %s, succeeded at %s (%s)\n' "$lane" "$rid" "$created" "$path" + else + printf '%s local failing: newest run with a verdict, %s, concluded %s at %s (%s)\n' "$lane" "$rid" "$conclusion" "$created" "$path" + fi done diff --git a/plugins/source-control/skills/pull-request/scripts/review-venue.test.sh b/plugins/source-control/skills/pull-request/scripts/review-venue.test.sh index 0b06a4c442..fe02bc2833 100755 --- a/plugins/source-control/skills/pull-request/scripts/review-venue.test.sh +++ b/plugins/source-control/skills/pull-request/scripts/review-venue.test.sh @@ -2,7 +2,8 @@ # Regression tests for review-venue.sh, black-box with a stubbed `gh` on PATH. # Expected verdicts come from the rule the script header and # reference/review-venue.md state: healthy only when the newest run with a -# verdict (completed, not skipped or cancelled) succeeded within the window. +# verdict (completed, not skipped or cancelled, and not a skipped-validation +# red) succeeded within the window. set -uo pipefail @@ -19,12 +20,24 @@ command -v jq >/dev/null 2>&1 || skip_suite "jq not installed" STUB_DIR="$TEST_TMPDIR/stubs" mkdir -p "$STUB_DIR" # The repository's run listing page N comes from $TEST_TMPDIR/page-N.json, a -# lane workflow's runs from $TEST_TMPDIR/wf-.json; a missing file fails. +# lane workflow's runs from $TEST_TMPDIR/wf-.json, a run's jobs from +# jobs-.json and a job's annotations from ann-.json; a missing +# file fails. cat >"$STUB_DIR/gh" <>"$TEST_TMPDIR/calls" [[ "\$1" == api ]] || { echo "gh-stub: unrouted \$*" >&2; exit 1; } case "\$2" in + */actions/runs/*/jobs\?*) + id="\${2#*/actions/runs/}"; id="\${id%%/*}" + [[ -f "$TEST_TMPDIR/jobs-\$id.json" ]] || exit 1 + cat "$TEST_TMPDIR/jobs-\$id.json" + ;; + */check-runs/*/annotations*) + id="\${2#*/check-runs/}"; id="\${id%%/*}" + [[ -f "$TEST_TMPDIR/ann-\$id.json" ]] || exit 1 + cat "$TEST_TMPDIR/ann-\$id.json" + ;; */actions/runs\?*) page="\${2##*page=}" [[ -f "$TEST_TMPDIR/page-\$page.json" ]] || exit 1 @@ -69,15 +82,32 @@ page() { jq -sc '{workflow_runs: .}' <<<"$(printf '%s\n' "$@")" >"$TEST_TMPDIR/page-$n.json" } -# wf ... -> wf-id.json, newest first, -# event pull_request unless given; the workflow itself reads active. +# wf ... -> wf-id.json, +# newest first, event pull_request unless given; the workflow itself reads +# active. A failed run gets one failed lane job whose annotations carry +# `failure-class=` as the reusable workflow's last step writes it +# (ci-workflows .github/workflows/pr-review.yml), class `other` unless given; +# a startup_failure run has no jobs. +NEXT_RUN=1000 wf() { - local id="$1" spec runs=() + local id="$1" spec runs=() rid shift for spec in "$@"; do - IFS=: read -r st co h ev <<<"$spec" - runs+=("$(jq -nc --arg s "$st" --arg c "$co" --arg t "$(ago "$h")" --arg e "${ev:-pull_request}" --argjson i "$RANDOM" \ + IFS=: read -r st co h ev cl <<<"$spec" + rid=$((NEXT_RUN++)) + runs+=("$(jq -nc --arg s "$st" --arg c "$co" --arg t "$(ago "$h")" --arg e "${ev:-pull_request}" --argjson i "$rid" \ '{id: $i, event: $e, status: $s, conclusion: (if $c == "" then null else $c end), created_at: $t}')") + case "$co" in + startup_failure) echo '{"jobs": []}' >"$TEST_TMPDIR/jobs-$rid.json" ;; + failure | timed_out) + jq -nc --argjson j "$((rid * 10))" --arg c "$co" \ + '{jobs: [{id: 1, conclusion: "success"}, {id: $j, conclusion: $c}]}' >"$TEST_TMPDIR/jobs-$rid.json" + jq -nc --arg c "${cl:-other}" '[ + {annotation_level: "failure", message: "Process completed with exit code 1."}, + {annotation_level: "failure", message: "failure-class=\($c): why"}]' >"$TEST_TMPDIR/ann-$((rid * 10)).json" + ;; + *) ;; + esac done jq -sc '{workflow_runs: .}' <<<"$(printf '%s\n' "${runs[@]}")" >"$TEST_TMPDIR/wf-$id.json" wf_state "$id" active @@ -86,7 +116,7 @@ wf() { # wf_state -> the workflow's state, as GET .../actions/workflows/ returns it wf_state() { jq -nc --arg s "$2" '{state: $s}' >"$TEST_TMPDIR/wfmeta-$1.json"; } -reset() { rm -f "$TEST_TMPDIR"/page-*.json "$TEST_TMPDIR"/wf-*.json "$TEST_TMPDIR"/wfmeta-*.json; } +reset() { rm -f "$TEST_TMPDIR"/page-*.json "$TEST_TMPDIR"/wf-*.json "$TEST_TMPDIR"/wfmeta-*.json "$TEST_TMPDIR"/jobs-*.json "$TEST_TMPDIR"/ann-*.json; } run() { : >"$TEST_TMPDIR/calls" @@ -123,6 +153,69 @@ assert_contains "a lane whose newest verdict failed runs locally" "$out" "code-r assert_contains "the failing line names the conclusion" "$out" "concluded failure" assert_contains "the other lane keeps its own verdict" "$out" "security-review ci healthy" +# #8867: a pull request that edits a review caller workflow ends both lanes red +# by design (failure-class=skipped-validation); that says nothing about the lane. +reset +page 1 "${BOTH_PAGE[@]}" +wf 11 completed:failure:1::skipped-validation completed:success:3 +wf 22 completed:failure:1::skipped-validation completed:success:3 +out=$(run) +assert_exit "a skipped-validation red is still a read" 0 "$?" +assert_contains "a workflow-editing PR's red leaves the code lane healthy" "$out" "code-review ci healthy" +assert_contains "a workflow-editing PR's red leaves the security lane healthy" "$out" "security-review ci healthy" + +reset +page 1 "${BOTH_PAGE[@]}" +wf 11 completed:failure:1::skipped-validation completed:failure:2::auth completed:success:3 +wf 22 completed:success:1 +out=$(run) +assert_contains "a real failure behind a skipped-validation red still fails" "$out" "code-review local failing" + +reset +page 1 "${BOTH_PAGE[@]}" +wf 11 completed:failure:1 completed:failure:2 completed:timed_out:3 completed:success:4 +wf 22 completed:failure:1::rate-limit completed:success:2 +out=$(run) +assert_contains "a lane whose recent runs all fail reads failing" "$out" "code-review local failing" +assert_contains "a usage-limit stop is not a skipped-validation red" "$out" "security-review local failing" + +reset +page 1 "${BOTH_PAGE[@]}" +wf 11 completed:failure:1::skipped-validation completed:failure:2::skipped-validation +wf 22 completed:success:1 +out=$(run) +assert_contains "a lane with only skipped-validation reds is stale" "$out" "code-review local stale" + +reset +page 1 "${BOTH_PAGE[@]}" +wf 11 completed:failure:1::skipped-validation completed:success:200 +wf 22 completed:success:1 +out=$(run) +assert_contains "the window applies to the run behind a skipped-validation red" "$out" "code-review local stale" + +reset +page 1 "${BOTH_PAGE[@]}" +wf 11 completed:failure:1::skipped-validation completed:success:3 +wf 22 completed:success:1 +rm -f "$TEST_TMPDIR"/ann-*.json +run >/dev/null 2>&1 +assert_exit "unreadable annotations fail closed" 2 "$?" + +reset +page 1 "${BOTH_PAGE[@]}" +wf 11 completed:failure:1::skipped-validation completed:success:3 +wf 22 completed:success:1 +rm -f "$TEST_TMPDIR"/jobs-*.json +run >/dev/null 2>&1 +assert_exit "unreadable jobs fail closed" 2 "$?" + +reset +page 1 "${BOTH_PAGE[@]}" +wf 11 completed:success:1 +wf 22 completed:success:1 +run >/dev/null +assert_not_contains "a healthy lane reads no jobs" "$(cat "$TEST_TMPDIR/calls")" "/jobs" + reset page 1 "${BOTH_PAGE[@]}" wf 11 completed:timed_out:1 From 1a58c0e14e6b1712ec806bba322a61660e9434d7 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Wed, 21 Oct 2026 11:18:14 -0400 Subject: [PATCH 2/2] chore(ci): resync the pull request head Co-Authored-By: Claude Opus 5.5