From 2eb942a6c0a95b9941ee72b6e7959bfa40f1cd2a Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Sun, 11 Oct 2026 20:06:02 +0000 Subject: [PATCH 01/12] =?UTF-8?q?feat:=20implement=20issue=20#2235=20?= =?UTF-8?q?=E2=80=94=20CI=20Failure=20Analyst=20never=20runs=20for=20faile?= =?UTF-8?q?d=20GitHub=20Actions=20jobs:=20the=20check=5Frun=20trigger=20on?= =?UTF-8?q?ly=20fires=20for=20third-party=20checks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../workflows/ci-failure-analyst-reusable.yml | 80 ++--- .github/workflows/ci.yml | 4 +- .github/workflows/lint.yml | 1 + docs/aw/ci-failure-analyst.md | 79 ++++- scripts/ci-failure-analyst-resolve.sh | 134 ++++++++ tests/aw/ci-failure-analyst/scenarios.md | 41 +++ tests/ci_failure_analyst_resolve.bats | 313 ++++++++++++++++++ 7 files changed, 601 insertions(+), 51 deletions(-) create mode 100755 scripts/ci-failure-analyst-resolve.sh create mode 100644 tests/ci_failure_analyst_resolve.bats diff --git a/.github/workflows/ci-failure-analyst-reusable.yml b/.github/workflows/ci-failure-analyst-reusable.yml index f7c368595..a3d416c4f 100644 --- a/.github/workflows/ci-failure-analyst-reusable.yml +++ b/.github/workflows/ci-failure-analyst-reusable.yml @@ -36,52 +36,36 @@ jobs: REPO: ${{ github.repository }} steps: + # The payload resolution lives in scripts/ci-failure-analyst-resolve.sh + # (tests: tests/ci_failure_analyst_resolve.bats). Fetch it from THIS + # reusable's own commit, so a caller pinned to a channel always runs the + # script that matches the workflow it pinned. .github-private is public, + # so the caller's token can read it. + - name: Checkout analyst tooling + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + repository: petry-projects/.github-private + ref: ${{ github.job_workflow_sha }} + path: .ci-analyst-tooling + sparse-checkout: scripts/ci-failure-analyst-resolve.sh + sparse-checkout-cone-mode: false + fetch-depth: 1 + persist-credentials: false + + # Resolves the head SHA, the non-fork PR and the failing run from either a + # check_run payload (third-party checks) or a workflow_run payload (failed + # GitHub Actions runs). Anything else ends in a "skipping" notice, exit 0. - name: Resolve non-fork PR id: pr - env: - HEAD_SHA: ${{ github.event.check_run.head_sha }} - CHECK_RUN_PRS: ${{ toJson(github.event.check_run.pull_requests) }} - run: | - set -euo pipefail - # Prefer the pull_requests array embedded in the check_run payload; - # filter to non-fork PRs (head repo must equal base repo). - pr_number=$(echo "$CHECK_RUN_PRS" \ - | jq -r '.[] | select(.head.repo.id == .base.repo.id) | .number' \ - | head -n 1) - if [ -z "$pr_number" ]; then - # Fallback: commits-to-pulls API handles edge cases (e.g. a second - # push arrived before this check_run completed). - # shellcheck disable=SC2016 # $ENV.REPO is jq built-in syntax, not a shell variable - pr_number=$(gh api "repos/$REPO/commits/$HEAD_SHA/pulls" \ - --jq '.[] | select(.head.repo.full_name == $ENV.REPO) | .number' \ - 2>/dev/null | head -n 1 || true) - fi - if [ -z "$pr_number" ]; then - echo "::notice::No non-fork PR for SHA $HEAD_SHA — skipping" - echo "skip=true" >> "$GITHUB_OUTPUT" - else - echo "skip=false" >> "$GITHUB_OUTPUT" - echo "pr_number=$pr_number" >> "$GITHUB_OUTPUT" - fi + run: bash .ci-analyst-tooling/scripts/ci-failure-analyst-resolve.sh resolve - name: Check idempotency if: steps.pr.outputs.skip == 'false' id: idem env: PR_NUMBER: ${{ steps.pr.outputs.pr_number }} - HEAD_SHA: ${{ github.event.check_run.head_sha }} - run: | - set -euo pipefail - existing=$(gh api "repos/$REPO/issues/$PR_NUMBER/comments" \ - --paginate \ - --jq ".[] | select(.body | startswith(\"\")) | .id" \ - 2>/dev/null | head -1 || true) - if [ -n "$existing" ]; then - echo "::notice::Diagnostic comment already posted (id=$existing) — idempotency skip" - echo "exists=true" >> "$GITHUB_OUTPUT" - else - echo "exists=false" >> "$GITHUB_OUTPUT" - fi + HEAD_SHA: ${{ steps.pr.outputs.head_sha }} + run: bash .ci-analyst-tooling/scripts/ci-failure-analyst-resolve.sh idempotency - name: Install Claude Code if: steps.pr.outputs.skip == 'false' && steps.idem.outputs.exists == 'false' @@ -97,10 +81,11 @@ jobs: if: steps.pr.outputs.skip == 'false' && steps.idem.outputs.exists == 'false' env: PR_NUMBER: ${{ steps.pr.outputs.pr_number }} - HEAD_SHA: ${{ github.event.check_run.head_sha }} - WORKFLOW_NAME: ${{ github.event.check_run.name }} - DETAILS_URL: ${{ github.event.check_run.details_url }} - CHECK_RUN_ID: ${{ github.event.check_run.id }} + HEAD_SHA: ${{ steps.pr.outputs.head_sha }} + WORKFLOW_NAME: ${{ steps.pr.outputs.check_name }} + DETAILS_URL: ${{ steps.pr.outputs.details_url }} + RUN_ID: ${{ steps.pr.outputs.run_id }} + FAILED_JOBS: ${{ steps.pr.outputs.failed_jobs }} run: | set -euo pipefail export PATH="$HOME/.npm-global/bin:$PATH" @@ -113,16 +98,15 @@ jobs: - Head SHA: $HEAD_SHA - Workflow/check name: $WORKFLOW_NAME - Details URL: $DETAILS_URL - - Check run ID: $CHECK_RUN_ID + - GitHub Actions run ID: $RUN_ID + - Failed jobs (job / failing step): $FAILED_JOBS Follow these steps in order: ### 1. Fetch failure logs - Extract the GitHub Actions run ID from the details URL: - run_id=$(echo "$DETAILS_URL" | grep -oP '(?<=runs/)\d+' || true) - If a run_id is found, fetch the failed-step logs: - gh run view "$run_id" --log-failed --repo $REPO 2>/dev/null | tail -200 - If no run_id (e.g. external quality gate like SonarCloud), use the PR diff: + If the run ID above is not empty, fetch the failed-step logs: + gh run view $RUN_ID --log-failed --repo $REPO 2>/dev/null | tail -200 + If the run ID is empty (e.g. an external quality gate like SonarCloud), use the PR diff: gh pr diff $PR_NUMBER --repo $REPO 2>/dev/null | head -200 ### 2. Identify the failing step and error diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 09e7c0bcd..7d76c4b0e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -63,7 +63,9 @@ jobs: chmod +x actionlint rm actionlint.tar.gz shopt -s extglob - ./actionlint -ignore 'SC2129' .github/workflows/!(*.lock).yml + # `job_workflow_sha` is a valid reusable-workflow context property (the + # reusable's own commit); the actionlint schema omits it. + ./actionlint -ignore 'SC2129' -ignore '"job_workflow_sha" is not defined' .github/workflows/!(*.lock).yml shellcheck: name: ShellCheck diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index e2019a774..dd5e3c189 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -124,6 +124,7 @@ jobs: tests/fleet_monitor_run.bats \ tests/test_fleet_paused_run.bats \ tests/template_stub_drift.bats \ + tests/ci_failure_analyst_resolve.bats \ tests/canary_rollout.bats \ tests/test_initiative_canary.bats \ tests/test_initiative_driver_canary.bats \ diff --git a/docs/aw/ci-failure-analyst.md b/docs/aw/ci-failure-analyst.md index 0546479f1..ab6be8fd0 100644 --- a/docs/aw/ci-failure-analyst.md +++ b/docs/aw/ci-failure-analyst.md @@ -4,9 +4,9 @@ Agentic workflow that diagnoses CI failures and posts a root-cause comment on th ## What it does -When a check run completes with `conclusion: failure` on a non-fork PR, the workflow: +When a check run or a watched GitHub Actions workflow run completes with `conclusion: failure` on a non-fork PR, the workflow: -1. Resolves the PR number from the check run payload (with a commits-to-pulls API fallback) +1. Resolves the head SHA, the PR number and the failing run from the `check_run` or `workflow_run` payload, with a commits-to-pulls API fallback for the PR (open PRs only) 2. Checks idempotency — skips if a `` comment already exists for this SHA 3. Fetches the failed run logs from the GitHub Actions API 4. Identifies the specific step that failed and its error message @@ -88,6 +88,81 @@ if: >- !startsWith(github.event.check_run.name, 'CI Failure Analyst') ``` +## Which failures are covered + +| Failure | Event | Covered | +|---|---|---| +| A check from another app (SonarCloud and the like) | `check_run` | Yes, through the current stub | +| A failed GitHub Actions job (CI, Lint, Tests, …) | `workflow_run` | Once the stub carries the `workflow_run` trigger below | + +GitHub does not start a workflow for an event raised by GitHub Actions' own token, so a check run +created by an Actions job never delivers `check_run` to the stub. The `check_run` trigger therefore +only ever sees third-party checks. Failed Actions runs reach the analyst through `workflow_run` (#2235). + +The reusable accepts either payload. `scripts/ci-failure-analyst-resolve.sh` (tests: +`tests/ci_failure_analyst_resolve.bats`) reads it and resolves: + +- **`check_run`**: head SHA, PR (the payload's non-fork `pull_requests`), check name, details URL, and + the Actions run id when the details URL points at one. A third-party check has no run, so the + analysis falls back to the PR diff. +- **`workflow_run`**: head SHA from `workflow_run.head_sha`, PR from `workflow_run.pull_requests` + (non-fork only), run id from `workflow_run.id`, and the run's failed jobs and their failing steps. + +When the payload has no non-fork PR, the commits-to-pulls lookup is tried, counting open PRs only. +A failure on the default branch is therefore not reported on the PR already merged into it. A +payload that carries neither event, or a run with no open non-fork PR, ends in a "skipping" notice +with exit 0. + +The reusable fetches the script from its own commit (`github.job_workflow_sha`), so a caller pinned +to a channel runs the script that matches the workflow it pinned. + +### The `workflow_run` trigger and the watched-workflow list + +The stub adds `workflow_run` next to `check_run`: + +```yaml +on: + check_run: + types: [completed] + # Each repo may edit this list: name every workflow whose failures should be + # analysed. GitHub requires names (the `name:` of each workflow file); there + # is no wildcard. Never list "CI Failure Analyst" itself. + workflow_run: + workflows: ["CI", "Lint", "Tests"] + types: [completed] + +concurrency: + group: "ci-failure-analyst-${{ github.event.check_run.head_sha || github.event.workflow_run.head_sha }}" + cancel-in-progress: false + +jobs: + analyze: + if: >- + (github.event_name == 'check_run' && + github.event.check_run.conclusion == 'failure' && + !startsWith(github.event.check_run.name, 'CI Failure Analyst')) || + (github.event_name == 'workflow_run' && + github.event.workflow_run.conclusion == 'failure' && + !startsWith(github.event.workflow_run.name, 'CI Failure Analyst')) +``` + +To change which Actions failures are analysed, edit the `workflows:` list in your repo's stub. Each +entry must match the target workflow's `name:` exactly. A workflow that is not listed is not analysed. +The `workflows:` list is the one part of the stub a repo is expected to change. + +When several watched workflows fail on the same commit, each failure starts an analyst run. The +per-SHA concurrency group runs them one at a time, and the `` marker +makes every run after the first an idempotency skip, giving one comment per commit. + +**Rollout order.** The stubs pin `ci-failure-analyst/v1-stable`. A trigger takes effect as soon as +the stub merges, but the reusable it calls is whatever that channel points at. The reusable at +v1.0.0 reads only `check_run` and fails a `workflow_run` call with `jq: Cannot iterate over null`. +So the `workflow_run` trigger goes into `templates/ci-failure-analyst.yml` and this repo's +`ci-failure-analyst.lock.yml` only after the pinned channel has been promoted to a commit carrying +the dual-payload reusable. That stub change also needs `caller-stub-freeze`'s baseline regenerated, +plus matching updates to `interaction-contracts/ci-failure-analyst.yml` and the §4 row of the +[agentic interaction model](../agentic-interaction-model.md). + ## Root cause categories | Category | Indicators | diff --git a/scripts/ci-failure-analyst-resolve.sh b/scripts/ci-failure-analyst-resolve.sh new file mode 100755 index 000000000..122fd0021 --- /dev/null +++ b/scripts/ci-failure-analyst-resolve.sh @@ -0,0 +1,134 @@ +#!/usr/bin/env bash +set -euo pipefail +# ci-failure-analyst-resolve.sh — payload resolution for the CI Failure Analyst +# reusable (.github/workflows/ci-failure-analyst-reusable.yml, #2235). +# +# The reusable is called from a stub that may fire on either event: +# check_run — a check created by another app (SonarCloud and the like). +# GitHub does not deliver check_run for checks created by +# GitHub Actions' own token, so this never sees Actions jobs. +# workflow_run — a GitHub Actions run completed; this is how the analyst +# learns about failed Actions jobs. +# The event is detected from the payload itself ($GITHUB_EVENT_PATH; a reusable +# sees its caller's payload), so a payload that carries neither ends in the +# "skipping" notice with exit 0 instead of a jq iteration error. +# +# Subcommands (results go to $GITHUB_OUTPUT): +# resolve skip=true|false, and when false: event, pr_number, head_sha, +# check_name, details_url, run_id, check_run_id, failed_jobs +# idempotency exists=true|false — whether PR_NUMBER already has a comment +# starting with ``. That marker +# keeps it to one analysis per commit however many workflows fail. +# +# Env: REPO (owner/repo), GITHUB_EVENT_PATH, GITHUB_OUTPUT, GH_TOKEN for gh; +# PR_NUMBER and HEAD_SHA for `idempotency`. + +REPO="${REPO:-${GITHUB_REPOSITORY:-}}" +GITHUB_OUTPUT="${GITHUB_OUTPUT:-/dev/null}" +export REPO + +emit() { printf '%s=%s\n' "$1" "$(printf '%s' "$2" | tr -d '\r\n')" >> "$GITHUB_OUTPUT"; } + +# payload_field — read one value from the event payload; empty on a +# missing, empty or non-JSON payload. +payload_field() { + local event_path="${GITHUB_EVENT_PATH:-}" + [ -n "$event_path" ] && [ -s "$event_path" ] || return 0 + jq -r "$1 // empty" "$event_path" 2>/dev/null || true +} + +# failed_jobs — "job / failing step; job; ..." for the run's failed jobs. +failed_jobs() { + [ -n "$1" ] || return 0 + gh api "repos/$REPO/actions/runs/$1/jobs" --paginate \ + --jq '.jobs[]? | select(.conclusion == "failure") + | .name + ([(.steps // [])[] | select(.conclusion == "failure") | " / " + .name][0] // "")' \ + 2>/dev/null | paste -sd';' - | sed 's/;/; /g' || true +} + +resolve() { + local event key head_sha pr_number name details_url run_id="" check_run_id="" + event="$(payload_field 'if (.check_run | type) == "object" then "check_run" + elif (.workflow_run | type) == "object" then "workflow_run" + else null end')" + case "$event" in + check_run) key=check_run ;; + workflow_run) key=workflow_run ;; + *) + echo "::notice::Event payload carries neither a check_run nor a workflow_run — skipping" + emit skip true + return 0 + ;; + esac + + head_sha="$(payload_field ".$key.head_sha")" + if [ -z "$head_sha" ]; then + echo "::notice::No head SHA in the $event payload — skipping" + emit skip true + return 0 + fi + + # Prefer the pull_requests array embedded in the payload, filtered to + # non-fork PRs (head repo must equal base repo). + pr_number="$(payload_field "[(.$key.pull_requests // [])[] + | select(.head.repo.id == .base.repo.id) | .number][0]")" + if [ -z "$pr_number" ]; then + # Fallback: the commits-to-pulls API covers a second push that arrived + # before this event, and a workflow_run whose payload omits the PR. Only + # an open PR counts, so a failure on the default branch is not reported + # on the PR that was already merged into it. + # shellcheck disable=SC2016 # $ENV.REPO is jq built-in syntax, not a shell variable + pr_number="$(gh api "repos/$REPO/commits/$head_sha/pulls" \ + --jq '.[] | select(.state == "open" and .head.repo.full_name == $ENV.REPO) | .number' \ + 2>/dev/null | head -n 1 || true)" + fi + if [ -z "$pr_number" ]; then + echo "::notice::No non-fork PR for SHA $head_sha — skipping" + emit skip true + return 0 + fi + + name="$(payload_field ".$key.name")" + if [ "$event" = check_run ]; then + details_url="$(payload_field ".check_run.details_url // .check_run.html_url")" + check_run_id="$(payload_field ".check_run.id")" + # An Actions-hosted check links to its run; a third-party check does not. + run_id="$(printf '%s' "$details_url" | grep -oE '/actions/runs/[0-9]+' | grep -oE '[0-9]+$' || true)" + else + details_url="$(payload_field ".workflow_run.html_url")" + run_id="$(payload_field ".workflow_run.id")" + fi + + emit skip false + emit event "$event" + emit pr_number "$pr_number" + emit head_sha "$head_sha" + emit check_name "$name" + emit details_url "$details_url" + emit run_id "$run_id" + emit check_run_id "$check_run_id" + emit failed_jobs "$(failed_jobs "$run_id")" +} + +idempotency() { + local existing + existing="$(gh api "repos/$REPO/issues/${PR_NUMBER:?PR_NUMBER is required}/comments" \ + --paginate \ + --jq ".[] | select(.body | startswith(\"\")) | .id" \ + 2>/dev/null | head -n 1 || true)" + if [ -n "$existing" ]; then + echo "::notice::Diagnostic comment already posted (id=$existing) — idempotency skip" + emit exists true + else + emit exists false + fi +} + +case "${1:-}" in + resolve) resolve ;; + idempotency) idempotency ;; + *) + echo "usage: $0 resolve|idempotency" >&2 + exit 2 + ;; +esac diff --git a/tests/aw/ci-failure-analyst/scenarios.md b/tests/aw/ci-failure-analyst/scenarios.md index 7522c4231..827ff945c 100644 --- a/tests/aw/ci-failure-analyst/scenarios.md +++ b/tests/aw/ci-failure-analyst/scenarios.md @@ -120,3 +120,44 @@ Written before the workflow per the TDD cycle in issue #233. - Workflow is a **no-op** — no comment is posted anywhere. - The analyst must not comment on issues or any other thread. + +--- + +## Scenario 3: A watched GitHub Actions workflow fails on a PR (`workflow_run`, #2235) + +**Input event:** + +```json +{ + "action": "completed", + "workflow_run": { + "id": 3000, + "name": "Lint", + "conclusion": "failure", + "head_sha": "www111", + "html_url": "https://github.com/petry-projects/example/actions/runs/3000", + "pull_requests": [ + { "number": 77, "head": { "repo": { "id": 1 } }, "base": { "repo": { "id": 1 } } } + ] + } +} +``` + +**Expected output:** + +- A comment is posted to PR #77. +- Comment begins with ``. +- The failing job and step come from run `3000`'s failed jobs and `--log-failed` logs. +- Comment includes a link to `https://github.com/petry-projects/example/actions/runs/3000`. +- If another watched workflow (or a third-party check) also fails on `www111`, no second comment is posted. + +--- + +## Edge case: `workflow_run` on the default branch, or a payload with neither event + +**Input event:** a `workflow_run` for a push to `main` (`pull_requests: []`, and the commit's only +associated PR is already merged), or a payload with no `check_run` and no `workflow_run` key. + +**Expected output:** + +- Workflow is a **no-op**: a "skipping" notice and exit 0, never `jq: Cannot iterate over null`. diff --git a/tests/ci_failure_analyst_resolve.bats b/tests/ci_failure_analyst_resolve.bats new file mode 100644 index 000000000..b733cef2d --- /dev/null +++ b/tests/ci_failure_analyst_resolve.bats @@ -0,0 +1,313 @@ +#!/usr/bin/env bats +# Tests for scripts/ci-failure-analyst-resolve.sh (#2235). +# +# The CI Failure Analyst reusable is called from a stub that triggers on +# check_run (third-party checks such as SonarCloud) and, once the stubs gain it, +# on workflow_run (failed GitHub Actions runs). The resolver reads whichever +# payload the run carries and must end every unusable payload in the +# "skipping" notice with exit 0, never in `jq: Cannot iterate over null`. +# +# gh is stubbed: each `gh api ` call is answered from a fixture file and +# the call's own --jq filter is applied with real jq, so the filters the script +# passes are exercised, not mocked away. + +REPO_ROOT="$(cd "$(dirname "$BATS_TEST_FILENAME")/.." && pwd)" +SCRIPT="$REPO_ROOT/scripts/ci-failure-analyst-resolve.sh" + +SHA="0123456789abcdef0123456789abcdef01234567" + +setup() { + TMP="$(mktemp -d)" + STUB_BIN="$TMP/bin" + GH_FIXTURES="$TMP/fixtures" + GH_CALLS="$TMP/gh-calls.log" + mkdir -p "$STUB_BIN" "$GH_FIXTURES" + : > "$GH_CALLS" + export GH_FIXTURES GH_CALLS + + # gh stub: `gh api [--paginate] [--jq ]`. The fixture for + # is $GH_FIXTURES/.json; a missing fixture is a + # failed call (like a 404). + cat > "$STUB_BIN/gh" <<'GHEOF' +#!/usr/bin/env bash +echo "$*" >> "$GH_CALLS" +[ "${1:-}" = "api" ] || exit 1 +shift +path="" expr="" +while [ $# -gt 0 ]; do + case "$1" in + --jq) expr="$2"; shift 2 ;; + --paginate) shift ;; + -*) shift ;; + *) [ -z "$path" ] && path="$1"; shift ;; + esac +done +fixture="$GH_FIXTURES/$(printf '%s' "$path" | tr '/' '_').json" +[ -f "$fixture" ] || { echo "gh: Not Found (HTTP 404)" >&2; exit 1; } +if [ -n "$expr" ]; then + jq -r "$expr" "$fixture" +else + cat "$fixture" +fi +GHEOF + chmod +x "$STUB_BIN/gh" + export PATH="$STUB_BIN:$PATH" + + export REPO="petry-projects/.github-private" + export GITHUB_OUTPUT="$TMP/output" + : > "$GITHUB_OUTPUT" + export GITHUB_EVENT_PATH="$TMP/event.json" + unset PR_NUMBER HEAD_SHA +} + +teardown() { + rm -rf "$TMP" +} + +# fixture +fixture() { + printf '%s\n' "$2" > "$GH_FIXTURES/$(printf '%s' "$1" | tr '/' '_').json" +} + +# out — the last value the script wrote for to GITHUB_OUTPUT. +out() { + grep "^$1=" "$GITHUB_OUTPUT" | tail -n 1 | cut -d= -f2- +} + +check_run_event() { + cat > "$GITHUB_EVENT_PATH" < [head_branch] +workflow_run_event() { + cat > "$GITHUB_EVENT_PATH" < "$TMP/e" && mv "$TMP/e" "$GITHUB_EVENT_PATH" + failed_jobs_fixture + run bash "$SCRIPT" resolve + [ "$status" -eq 0 ] + [ "$(out run_id)" = "777" ] + [ "$(out failed_jobs)" = "shellcheck / Run tests; template-drift" ] +} + +@test "check_run: empty pull_requests falls back to the commits-to-pulls lookup" { + check_run_event + jq '.check_run.pull_requests = []' "$GITHUB_EVENT_PATH" > "$TMP/e" && mv "$TMP/e" "$GITHUB_EVENT_PATH" + fixture "repos/$REPO/commits/$SHA/pulls" '[ + {"number": 43, "state": "open", "head": {"repo": {"full_name": "petry-projects/.github-private"}}} + ]' + run bash "$SCRIPT" resolve + [ "$status" -eq 0 ] + [ "$(out skip)" = "false" ] + [ "$(out pr_number)" = "43" ] +} + +# ── workflow_run ───────────────────────────────────────────────────────────── + +@test "workflow_run: SHA, PR, run id and failed jobs come from the workflow_run payload" { + workflow_run_event '[{"number": 99, "head": {"repo": {"id": 7}}, "base": {"repo": {"id": 7}}}]' + failed_jobs_fixture + run bash "$SCRIPT" resolve + [ "$status" -eq 0 ] + [ "$(out skip)" = "false" ] + [ "$(out event)" = "workflow_run" ] + [ "$(out pr_number)" = "99" ] + [ "$(out head_sha)" = "$SHA" ] + [ "$(out check_name)" = "Lint" ] + [ "$(out details_url)" = "https://github.com/petry-projects/.github-private/actions/runs/777" ] + [ "$(out run_id)" = "777" ] + [ "$(out check_run_id)" = "" ] + [ "$(out failed_jobs)" = "shellcheck / Run tests; template-drift" ] +} + +@test "workflow_run: a fork PR in the payload is ignored and the lookup decides" { + workflow_run_event '[{"number": 5, "head": {"repo": {"id": 8}}, "base": {"repo": {"id": 7}}}]' + fixture "repos/$REPO/commits/$SHA/pulls" '[ + {"number": 5, "state": "open", "head": {"repo": {"full_name": "someone/fork"}}} + ]' + run bash "$SCRIPT" resolve + [ "$status" -eq 0 ] + [ "$(out skip)" = "true" ] + [[ "$output" == *"::notice::No non-fork PR for SHA $SHA"*"skipping"* ]] +} + +@test "workflow_run: empty pull_requests falls back to the commits-to-pulls lookup" { + workflow_run_event '[]' + fixture "repos/$REPO/commits/$SHA/pulls" '[ + {"number": 101, "state": "open", "head": {"repo": {"full_name": "petry-projects/.github-private"}}} + ]' + run bash "$SCRIPT" resolve + [ "$status" -eq 0 ] + [ "$(out skip)" = "false" ] + [ "$(out pr_number)" = "101" ] +} + +@test "workflow_run on the default branch with no PR: skipping notice, exit 0" { + workflow_run_event '[]' main + # A push to main: the commit's only associated PR is the one already merged. + fixture "repos/$REPO/commits/$SHA/pulls" '[ + {"number": 88, "state": "closed", "head": {"repo": {"full_name": "petry-projects/.github-private"}}} + ]' + run bash "$SCRIPT" resolve + [ "$status" -eq 0 ] + [ "$(out skip)" = "true" ] + [ "$(out pr_number)" = "" ] + [[ "$output" == *"::notice::No non-fork PR for SHA $SHA"*"skipping"* ]] +} + +@test "workflow_run with null pull_requests never reaches a jq iteration error" { + workflow_run_event 'null' + run bash "$SCRIPT" resolve + [ "$status" -eq 0 ] + [ "$(out skip)" = "true" ] + [[ "$output" != *"Cannot iterate"* ]] +} + +# ── unusable payloads ──────────────────────────────────────────────────────── + +@test "empty payload: skipping notice, exit 0, no jq error, no API call" { + echo '{}' > "$GITHUB_EVENT_PATH" + run bash "$SCRIPT" resolve + [ "$status" -eq 0 ] + [ "$(out skip)" = "true" ] + [[ "$output" == *"::notice::"*"skipping"* ]] + [[ "$output" != *"Cannot iterate"* ]] + [ ! -s "$GH_CALLS" ] +} + +@test "zero-byte, non-JSON and missing payload files all skip with exit 0" { + : > "$GITHUB_EVENT_PATH" + run bash "$SCRIPT" resolve + [ "$status" -eq 0 ] + [ "$(out skip)" = "true" ] + + : > "$GITHUB_OUTPUT" + echo 'not json' > "$GITHUB_EVENT_PATH" + run bash "$SCRIPT" resolve + [ "$status" -eq 0 ] + [ "$(out skip)" = "true" ] + + : > "$GITHUB_OUTPUT" + rm -f "$GITHUB_EVENT_PATH" + run bash "$SCRIPT" resolve + [ "$status" -eq 0 ] + [ "$(out skip)" = "true" ] +} + +# ── idempotency: one analysis per commit ───────────────────────────────────── + +@test "idempotency: no marker comment yet means the analysis runs" { + export PR_NUMBER=99 HEAD_SHA="$SHA" + fixture "repos/$REPO/issues/99/comments" '[{"id": 1, "body": "unrelated"}]' + run bash "$SCRIPT" idempotency + [ "$status" -eq 0 ] + [ "$(out exists)" = "false" ] +} + +@test "two failures on one SHA: the second trigger finds the marker and skips" { + fixture "repos/$REPO/issues/99/comments" '[]' + + # First failure: a workflow_run for Lint. + workflow_run_event '[{"number": 99, "head": {"repo": {"id": 7}}, "base": {"repo": {"id": 7}}}]' + run bash "$SCRIPT" resolve + [ "$status" -eq 0 ] + first_sha="$(out head_sha)" + PR_NUMBER="$(out pr_number)" HEAD_SHA="$first_sha" run bash "$SCRIPT" idempotency + [ "$status" -eq 0 ] + [ "$(out exists)" = "false" ] + + # The first analysis posts its comment. + fixture "repos/$REPO/issues/99/comments" "[{\"id\": 321, \"body\": \"\\n## CI Failure: Lint\"}]" + + # Second failure on the same commit: a different workflow (Tests), and a + # third-party check_run, both resolve to the same SHA and so the same marker. + : > "$GITHUB_OUTPUT" + workflow_run_event '[{"number": 99, "head": {"repo": {"id": 7}}, "base": {"repo": {"id": 7}}}]' + jq '.workflow_run.name = "Tests" | .workflow_run.id = 778' "$GITHUB_EVENT_PATH" > "$TMP/e" && mv "$TMP/e" "$GITHUB_EVENT_PATH" + run bash "$SCRIPT" resolve + [ "$(out head_sha)" = "$first_sha" ] + PR_NUMBER="$(out pr_number)" HEAD_SHA="$(out head_sha)" run bash "$SCRIPT" idempotency + [ "$status" -eq 0 ] + [ "$(out exists)" = "true" ] + [[ "$output" == *"already posted (id=321)"* ]] +} + +@test "idempotency: a marker for a different SHA does not suppress this one" { + export PR_NUMBER=99 HEAD_SHA="$SHA" + fixture "repos/$REPO/issues/99/comments" '[{"id": 2, "body": ""}]' + run bash "$SCRIPT" idempotency + [ "$status" -eq 0 ] + [ "$(out exists)" = "false" ] +} + +@test "unknown subcommand is a usage error" { + run bash "$SCRIPT" bogus + [ "$status" -eq 2 ] +} From 43241964c47c04ae4fb8779b21316010e1ac55bd Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Sun, 11 Oct 2026 20:10:52 +0000 Subject: [PATCH 02/12] fix(reviews): use job.workflow_sha, harden bats tmpdir handling Co-Authored-By: Claude Sonnet 5.5 --- .github/workflows/ci-failure-analyst-reusable.yml | 2 +- .github/workflows/ci.yml | 4 ++-- docs/aw/ci-failure-analyst.md | 2 +- tests/ci_failure_analyst_resolve.bats | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/ci-failure-analyst-reusable.yml b/.github/workflows/ci-failure-analyst-reusable.yml index a3d416c4f..d22810bab 100644 --- a/.github/workflows/ci-failure-analyst-reusable.yml +++ b/.github/workflows/ci-failure-analyst-reusable.yml @@ -45,7 +45,7 @@ jobs: uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: repository: petry-projects/.github-private - ref: ${{ github.job_workflow_sha }} + ref: ${{ job.workflow_sha }} path: .ci-analyst-tooling sparse-checkout: scripts/ci-failure-analyst-resolve.sh sparse-checkout-cone-mode: false diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7d76c4b0e..919b75ee0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -63,9 +63,9 @@ jobs: chmod +x actionlint rm actionlint.tar.gz shopt -s extglob - # `job_workflow_sha` is a valid reusable-workflow context property (the + # `workflow_sha` is a valid `job` context property (the # reusable's own commit); the actionlint schema omits it. - ./actionlint -ignore 'SC2129' -ignore '"job_workflow_sha" is not defined' .github/workflows/!(*.lock).yml + ./actionlint -ignore 'SC2129' -ignore '"workflow_sha" is not defined' .github/workflows/!(*.lock).yml shellcheck: name: ShellCheck diff --git a/docs/aw/ci-failure-analyst.md b/docs/aw/ci-failure-analyst.md index ab6be8fd0..3dffb66f7 100644 --- a/docs/aw/ci-failure-analyst.md +++ b/docs/aw/ci-failure-analyst.md @@ -113,7 +113,7 @@ A failure on the default branch is therefore not reported on the PR already merg payload that carries neither event, or a run with no open non-fork PR, ends in a "skipping" notice with exit 0. -The reusable fetches the script from its own commit (`github.job_workflow_sha`), so a caller pinned +The reusable fetches the script from its own commit (`job.workflow_sha`), so a caller pinned to a channel runs the script that matches the workflow it pinned. ### The `workflow_run` trigger and the watched-workflow list diff --git a/tests/ci_failure_analyst_resolve.bats b/tests/ci_failure_analyst_resolve.bats index b733cef2d..0732e6189 100644 --- a/tests/ci_failure_analyst_resolve.bats +++ b/tests/ci_failure_analyst_resolve.bats @@ -17,7 +17,7 @@ SCRIPT="$REPO_ROOT/scripts/ci-failure-analyst-resolve.sh" SHA="0123456789abcdef0123456789abcdef01234567" setup() { - TMP="$(mktemp -d)" + TMP="$(mktemp -d)" || { echo "Failed to create temp directory" >&2; exit 1; } STUB_BIN="$TMP/bin" GH_FIXTURES="$TMP/fixtures" GH_CALLS="$TMP/gh-calls.log" @@ -61,7 +61,7 @@ GHEOF } teardown() { - rm -rf "$TMP" + [ -n "${TMP:-}" ] && rm -rf "$TMP" } # fixture From 7a5ef71d521f9b3f864dbabee10347630d5490cd Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Sun, 11 Oct 2026 20:37:43 +0000 Subject: [PATCH 03/12] fix(ci-failure-analyst): address CodeAnt bot findings #2308 - Fix Issue 1: Filter payload PRs by open state, matching the fallback behavior (lines 73-74). Accepts only open PRs from the webhook payload, consistent with the commits-to-pulls API fallback which also filters by state == "open". - Fix Issue 2: Remove error suppression in idempotency function (line 118). Changed '|| true' to '|| return' so API failures propagate instead of silently returning empty, which could cause duplicate comment posts. - Update test fixtures to include PR state field matching real GitHub webhook payloads that include state information. Co-Authored-By: Claude Haiku 4.5 --- scripts/ci-failure-analyst-resolve.sh | 8 ++++---- tests/ci_failure_analyst_resolve.bats | 7 ++++--- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/scripts/ci-failure-analyst-resolve.sh b/scripts/ci-failure-analyst-resolve.sh index 122fd0021..b8545d6d0 100755 --- a/scripts/ci-failure-analyst-resolve.sh +++ b/scripts/ci-failure-analyst-resolve.sh @@ -69,9 +69,9 @@ resolve() { fi # Prefer the pull_requests array embedded in the payload, filtered to - # non-fork PRs (head repo must equal base repo). - pr_number="$(payload_field "[(.$key.pull_requests // [])[] - | select(.head.repo.id == .base.repo.id) | .number][0]")" + # non-fork, open PRs (head repo must equal base repo). + pr_number="$(payload_field '[(.'"$key"'.pull_requests // [])[] + | select(.state == "open" and .head.repo.id == .base.repo.id) | .number][0]')" if [ -z "$pr_number" ]; then # Fallback: the commits-to-pulls API covers a second push that arrived # before this event, and a workflow_run whose payload omits the PR. Only @@ -115,7 +115,7 @@ idempotency() { existing="$(gh api "repos/$REPO/issues/${PR_NUMBER:?PR_NUMBER is required}/comments" \ --paginate \ --jq ".[] | select(.body | startswith(\"\")) | .id" \ - 2>/dev/null | head -n 1 || true)" + 2>/dev/null | head -n 1)" || return if [ -n "$existing" ]; then echo "::notice::Diagnostic comment already posted (id=$existing) — idempotency skip" emit exists true diff --git a/tests/ci_failure_analyst_resolve.bats b/tests/ci_failure_analyst_resolve.bats index 0732e6189..ac6cd368c 100644 --- a/tests/ci_failure_analyst_resolve.bats +++ b/tests/ci_failure_analyst_resolve.bats @@ -86,6 +86,7 @@ check_run_event() { "details_url": "https://sonarcloud.io/dashboard?id=petry-projects_x", "pull_requests": [ {"number": 42, + "state": "open", "head": {"repo": {"id": 1}}, "base": {"repo": {"id": 1}}} ] @@ -172,7 +173,7 @@ failed_jobs_fixture() { # ── workflow_run ───────────────────────────────────────────────────────────── @test "workflow_run: SHA, PR, run id and failed jobs come from the workflow_run payload" { - workflow_run_event '[{"number": 99, "head": {"repo": {"id": 7}}, "base": {"repo": {"id": 7}}}]' + workflow_run_event '[{"number": 99, "state": "open", "head": {"repo": {"id": 7}}, "base": {"repo": {"id": 7}}}]' failed_jobs_fixture run bash "$SCRIPT" resolve [ "$status" -eq 0 ] @@ -275,7 +276,7 @@ failed_jobs_fixture() { fixture "repos/$REPO/issues/99/comments" '[]' # First failure: a workflow_run for Lint. - workflow_run_event '[{"number": 99, "head": {"repo": {"id": 7}}, "base": {"repo": {"id": 7}}}]' + workflow_run_event '[{"number": 99, "state": "open", "head": {"repo": {"id": 7}}, "base": {"repo": {"id": 7}}}]' run bash "$SCRIPT" resolve [ "$status" -eq 0 ] first_sha="$(out head_sha)" @@ -289,7 +290,7 @@ failed_jobs_fixture() { # Second failure on the same commit: a different workflow (Tests), and a # third-party check_run, both resolve to the same SHA and so the same marker. : > "$GITHUB_OUTPUT" - workflow_run_event '[{"number": 99, "head": {"repo": {"id": 7}}, "base": {"repo": {"id": 7}}}]' + workflow_run_event '[{"number": 99, "state": "open", "head": {"repo": {"id": 7}}, "base": {"repo": {"id": 7}}}]' jq '.workflow_run.name = "Tests" | .workflow_run.id = 778' "$GITHUB_EVENT_PATH" > "$TMP/e" && mv "$TMP/e" "$GITHUB_EVENT_PATH" run bash "$SCRIPT" resolve [ "$(out head_sha)" = "$first_sha" ] From cedf7522afe673aef5522b90cd7aac66e3cea6b4 Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Sun, 11 Oct 2026 21:08:07 +0000 Subject: [PATCH 04/12] fix(reviews): drop nonexistent .state filter on payload pull_requests Co-Authored-By: Claude Sonnet 5.5 --- scripts/ci-failure-analyst-resolve.sh | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/scripts/ci-failure-analyst-resolve.sh b/scripts/ci-failure-analyst-resolve.sh index b8545d6d0..ce23e675a 100755 --- a/scripts/ci-failure-analyst-resolve.sh +++ b/scripts/ci-failure-analyst-resolve.sh @@ -69,9 +69,11 @@ resolve() { fi # Prefer the pull_requests array embedded in the payload, filtered to - # non-fork, open PRs (head repo must equal base repo). + # non-fork PRs (head repo must equal base repo). The embedded entries are + # minimal objects with no `state` field, so openness is only checked on the + # API fallback below. pr_number="$(payload_field '[(.'"$key"'.pull_requests // [])[] - | select(.state == "open" and .head.repo.id == .base.repo.id) | .number][0]')" + | select(.head.repo.id == .base.repo.id) | .number][0]')" if [ -z "$pr_number" ]; then # Fallback: the commits-to-pulls API covers a second push that arrived # before this event, and a workflow_run whose payload omits the PR. Only From 701e881ec4824d40d9fce7f721f8b2a0197ca3aa Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Sun, 11 Oct 2026 23:15:55 +0000 Subject: [PATCH 05/12] fix(actionlint): add ignore rule for workflow_sha undefined in lint.yml Match ci.yml's actionlint configuration to include the 'workflow_sha' is not defined' ignore rule. The workflow_sha property is valid in the job context but not recognized by the actionlint schema. Co-Authored-By: Claude Haiku 4.5 --- .github/workflows/lint.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index dd5e3c189..0af62cbcb 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -332,7 +332,9 @@ jobs: chmod +x actionlint rm actionlint.tar.gz shopt -s extglob - ./actionlint -ignore 'SC2129' .github/workflows/!(*.lock).yml + # `workflow_sha` is a valid `job` context property (the + # reusable's own commit); the actionlint schema omits it. + ./actionlint -ignore 'SC2129' -ignore '"workflow_sha" is not defined' .github/workflows/!(*.lock).yml validate-workflow-schedules: # Off-peak scheduling compliance (#724, epic #722): fail any PR that From a33f2ccc00de23ae1513db9b35091680aee95541 Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Mon, 12 Oct 2026 05:58:03 +0000 Subject: [PATCH 06/12] fix(ci-failure-analyst): replace head with sed in idempotency to avoid SIGPIPE When gh --paginate is piped to head -n 1, head exits after reading the first line and closes the pipe, causing gh to receive SIGPIPE if it's still writing. This makes the pipeline return 141, causing the step to fail on transient errors. Replace head with sed -n '1p' so the pipe stays open and gh can drain completely. Co-Authored-By: Claude Haiku 4.5 --- scripts/ci-failure-analyst-resolve.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/ci-failure-analyst-resolve.sh b/scripts/ci-failure-analyst-resolve.sh index ce23e675a..9c85ac905 100755 --- a/scripts/ci-failure-analyst-resolve.sh +++ b/scripts/ci-failure-analyst-resolve.sh @@ -117,7 +117,7 @@ idempotency() { existing="$(gh api "repos/$REPO/issues/${PR_NUMBER:?PR_NUMBER is required}/comments" \ --paginate \ --jq ".[] | select(.body | startswith(\"\")) | .id" \ - 2>/dev/null | head -n 1)" || return + 2>/dev/null | sed -n '1p')" || true if [ -n "$existing" ]; then echo "::notice::Diagnostic comment already posted (id=$existing) — idempotency skip" emit exists true From 973b1b721b679aa997a9c671e2c6903a62b1415c Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Mon, 12 Oct 2026 06:30:16 +0000 Subject: [PATCH 07/12] fix(reviews): fail closed when the idempotency comment lookup fails Co-Authored-By: Claude Sonnet 5.5 --- scripts/ci-failure-analyst-resolve.sh | 13 ++++++++++--- tests/ci_failure_analyst_resolve.bats | 9 +++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/scripts/ci-failure-analyst-resolve.sh b/scripts/ci-failure-analyst-resolve.sh index 9c85ac905..41b60aa5e 100755 --- a/scripts/ci-failure-analyst-resolve.sh +++ b/scripts/ci-failure-analyst-resolve.sh @@ -113,11 +113,18 @@ resolve() { } idempotency() { - local existing - existing="$(gh api "repos/$REPO/issues/${PR_NUMBER:?PR_NUMBER is required}/comments" \ + local existing ids + # Fail closed: a failed lookup must not read as "no marker yet", or a second + # diagnostic gets posted for a commit that already has one. + if ! ids="$(gh api "repos/$REPO/issues/${PR_NUMBER:?PR_NUMBER is required}/comments" \ --paginate \ --jq ".[] | select(.body | startswith(\"\")) | .id" \ - 2>/dev/null | sed -n '1p')" || true + 2>/dev/null)"; then + echo "::warning::Comment lookup failed — skipping to avoid a duplicate diagnostic" + emit exists true + return 0 + fi + existing="$(printf '%s\n' "$ids" | sed -n '1p')" if [ -n "$existing" ]; then echo "::notice::Diagnostic comment already posted (id=$existing) — idempotency skip" emit exists true diff --git a/tests/ci_failure_analyst_resolve.bats b/tests/ci_failure_analyst_resolve.bats index ac6cd368c..2df0657cd 100644 --- a/tests/ci_failure_analyst_resolve.bats +++ b/tests/ci_failure_analyst_resolve.bats @@ -308,6 +308,15 @@ failed_jobs_fixture() { [ "$(out exists)" = "false" ] } +@test "idempotency: a failed comment lookup fails closed instead of posting a duplicate" { + export PR_NUMBER=99 HEAD_SHA="$SHA" + # no fixture for the comments path: the gh stub answers with a 404 failure + run bash "$SCRIPT" idempotency + [ "$status" -eq 0 ] + [ "$(out exists)" = "true" ] + [[ "$output" == *"lookup failed"* ]] +} + @test "unknown subcommand is a usage error" { run bash "$SCRIPT" bogus [ "$status" -eq 2 ] From b96e95e741e27531160f733a89e6471dafd6f516 Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Mon, 12 Oct 2026 07:01:46 +0000 Subject: [PATCH 08/12] fix(reviews): fail loudly on PR lookup errors and recheck embedded PR state Test-Change-Justification: existing tests gained pulls/ and commits-to-pulls fixtures because resolve now verifies PR state via the API and fails on lookup errors, per review threads PRRT_kwDOR9SdIs6rW0SG and PRRT_kwDOR9SdIs6rW0SN on #2308. Co-Authored-By: Claude Sonnet 5.5 --- scripts/ci-failure-analyst-resolve.sh | 27 ++++++++++++++++++-------- tests/ci_failure_analyst_resolve.bats | 28 +++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 8 deletions(-) diff --git a/scripts/ci-failure-analyst-resolve.sh b/scripts/ci-failure-analyst-resolve.sh index 41b60aa5e..a6d7cbd1f 100755 --- a/scripts/ci-failure-analyst-resolve.sh +++ b/scripts/ci-failure-analyst-resolve.sh @@ -47,7 +47,7 @@ failed_jobs() { } resolve() { - local event key head_sha pr_number name details_url run_id="" check_run_id="" + local event key head_sha pr_number pr_state pulls name details_url run_id="" check_run_id="" event="$(payload_field 'if (.check_run | type) == "object" then "check_run" elif (.workflow_run | type) == "object" then "workflow_run" else null end')" @@ -70,19 +70,30 @@ resolve() { # Prefer the pull_requests array embedded in the payload, filtered to # non-fork PRs (head repo must equal base repo). The embedded entries are - # minimal objects with no `state` field, so openness is only checked on the - # API fallback below. + # minimal objects with no `state` field, so openness is re-checked against + # the API: the run may finish after its PR merged or closed. pr_number="$(payload_field '[(.'"$key"'.pull_requests // [])[] | select(.head.repo.id == .base.repo.id) | .number][0]')" - if [ -z "$pr_number" ]; then + if [ -n "$pr_number" ]; then + # A failed lookup is not "closed": it fails the step rather than skipping. + if ! pr_state="$(gh api "repos/$REPO/pulls/$pr_number" --jq '.state')"; then + echo "::error::Could not look up the state of PR #$pr_number" + return 1 + fi + [ "$pr_state" = open ] || pr_number="" + else # Fallback: the commits-to-pulls API covers a second push that arrived # before this event, and a workflow_run whose payload omits the PR. Only # an open PR counts, so a failure on the default branch is not reported - # on the PR that was already merged into it. + # on the PR that was already merged into it. A failed lookup fails the step + # rather than reading as "no PR". # shellcheck disable=SC2016 # $ENV.REPO is jq built-in syntax, not a shell variable - pr_number="$(gh api "repos/$REPO/commits/$head_sha/pulls" \ - --jq '.[] | select(.state == "open" and .head.repo.full_name == $ENV.REPO) | .number' \ - 2>/dev/null | head -n 1 || true)" + if ! pulls="$(gh api "repos/$REPO/commits/$head_sha/pulls" \ + --jq '.[] | select(.state == "open" and .head.repo.full_name == $ENV.REPO) | .number')"; then + echo "::error::Could not look up the PRs for SHA $head_sha" + return 1 + fi + pr_number="$(printf '%s\n' "$pulls" | sed -n '1p')" fi if [ -z "$pr_number" ]; then echo "::notice::No non-fork PR for SHA $head_sha — skipping" diff --git a/tests/ci_failure_analyst_resolve.bats b/tests/ci_failure_analyst_resolve.bats index 2df0657cd..b092f99b2 100644 --- a/tests/ci_failure_analyst_resolve.bats +++ b/tests/ci_failure_analyst_resolve.bats @@ -130,6 +130,7 @@ failed_jobs_fixture() { @test "check_run: PR, SHA, name and details URL come from the check_run payload" { check_run_event + fixture "repos/$REPO/pulls/42" '{"state": "open"}' run bash "$SCRIPT" resolve [ "$status" -eq 0 ] [ "$(out skip)" = "false" ] @@ -149,6 +150,7 @@ failed_jobs_fixture() { @test "check_run: an Actions details URL yields the run id and its failed jobs" { check_run_event + fixture "repos/$REPO/pulls/42" '{"state": "open"}' jq '.check_run.details_url = "https://github.com/petry-projects/.github-private/actions/runs/777/job/9"' \ "$GITHUB_EVENT_PATH" > "$TMP/e" && mv "$TMP/e" "$GITHUB_EVENT_PATH" failed_jobs_fixture @@ -174,6 +176,7 @@ failed_jobs_fixture() { @test "workflow_run: SHA, PR, run id and failed jobs come from the workflow_run payload" { workflow_run_event '[{"number": 99, "state": "open", "head": {"repo": {"id": 7}}, "base": {"repo": {"id": 7}}}]' + fixture "repos/$REPO/pulls/99" '{"state": "open"}' failed_jobs_fixture run bash "$SCRIPT" resolve [ "$status" -eq 0 ] @@ -225,12 +228,35 @@ failed_jobs_fixture() { @test "workflow_run with null pull_requests never reaches a jq iteration error" { workflow_run_event 'null' + fixture "repos/$REPO/commits/$SHA/pulls" '[]' run bash "$SCRIPT" resolve [ "$status" -eq 0 ] [ "$(out skip)" = "true" ] [[ "$output" != *"Cannot iterate"* ]] } +@test "workflow_run: an embedded PR that has since closed is not reported" { + workflow_run_event '[{"number": 99, "head": {"repo": {"id": 7}}, "base": {"repo": {"id": 7}}}]' + fixture "repos/$REPO/pulls/99" '{"state": "closed"}' + run bash "$SCRIPT" resolve + [ "$status" -eq 0 ] + [ "$(out skip)" = "true" ] +} + +@test "workflow_run: a failed PR-state lookup fails loudly instead of skipping" { + workflow_run_event '[{"number": 99, "head": {"repo": {"id": 7}}, "base": {"repo": {"id": 7}}}]' + run bash "$SCRIPT" resolve + [ "$status" -ne 0 ] + [ "$(out skip)" = "" ] +} + +@test "workflow_run: a failed commits-to-pulls lookup fails loudly instead of skipping" { + workflow_run_event '[]' + run bash "$SCRIPT" resolve + [ "$status" -ne 0 ] + [ "$(out skip)" = "" ] +} + # ── unusable payloads ──────────────────────────────────────────────────────── @test "empty payload: skipping notice, exit 0, no jq error, no API call" { @@ -277,6 +303,7 @@ failed_jobs_fixture() { # First failure: a workflow_run for Lint. workflow_run_event '[{"number": 99, "state": "open", "head": {"repo": {"id": 7}}, "base": {"repo": {"id": 7}}}]' + fixture "repos/$REPO/pulls/99" '{"state": "open"}' run bash "$SCRIPT" resolve [ "$status" -eq 0 ] first_sha="$(out head_sha)" @@ -291,6 +318,7 @@ failed_jobs_fixture() { # third-party check_run, both resolve to the same SHA and so the same marker. : > "$GITHUB_OUTPUT" workflow_run_event '[{"number": 99, "state": "open", "head": {"repo": {"id": 7}}, "base": {"repo": {"id": 7}}}]' + fixture "repos/$REPO/pulls/99" '{"state": "open"}' jq '.workflow_run.name = "Tests" | .workflow_run.id = 778' "$GITHUB_EVENT_PATH" > "$TMP/e" && mv "$TMP/e" "$GITHUB_EVENT_PATH" run bash "$SCRIPT" resolve [ "$(out head_sha)" = "$first_sha" ] From 53c885c7ff85f45f9f71fdcea45b21706dd75e1d Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Mon, 12 Oct 2026 07:43:14 +0000 Subject: [PATCH 09/12] fix(reviews): try every embedded PR and fall back to commits-to-pulls when none is open Test-Change-Justification: a closed embedded PR now falls through to the commits-to-pulls lookup as requested in the CodeRabbit review on #2308, so the closed-PR test must stub that lookup (empty) to still assert a skip. Co-Authored-By: Claude Sonnet 5.5 --- scripts/ci-failure-analyst-resolve.sh | 24 ++++++++++++++++-------- tests/ci_failure_analyst_resolve.bats | 13 +++++++++++++ 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/scripts/ci-failure-analyst-resolve.sh b/scripts/ci-failure-analyst-resolve.sh index a6d7cbd1f..3ef5b482b 100755 --- a/scripts/ci-failure-analyst-resolve.sh +++ b/scripts/ci-failure-analyst-resolve.sh @@ -47,7 +47,7 @@ failed_jobs() { } resolve() { - local event key head_sha pr_number pr_state pulls name details_url run_id="" check_run_id="" + local event key head_sha pr_number pr_state pulls candidates candidate name details_url run_id="" check_run_id="" event="$(payload_field 'if (.check_run | type) == "object" then "check_run" elif (.workflow_run | type) == "object" then "workflow_run" else null end')" @@ -72,16 +72,24 @@ resolve() { # non-fork PRs (head repo must equal base repo). The embedded entries are # minimal objects with no `state` field, so openness is re-checked against # the API: the run may finish after its PR merged or closed. - pr_number="$(payload_field '[(.'"$key"'.pull_requests // [])[] - | select(.head.repo.id == .base.repo.id) | .number][0]')" - if [ -n "$pr_number" ]; then + # A head SHA can belong to several PRs, so every candidate is checked and the + # first open one wins; with none open, the commits-to-pulls lookup below runs. + candidates="$(payload_field '(.'"$key"'.pull_requests // [])[] + | select(.head.repo.id == .base.repo.id) | .number')" + pr_number="" + while IFS= read -r candidate; do + [ -n "$candidate" ] || continue # A failed lookup is not "closed": it fails the step rather than skipping. - if ! pr_state="$(gh api "repos/$REPO/pulls/$pr_number" --jq '.state')"; then - echo "::error::Could not look up the state of PR #$pr_number" + if ! pr_state="$(gh api "repos/$REPO/pulls/$candidate" --jq '.state')"; then + echo "::error::Could not look up the state of PR #$candidate" return 1 fi - [ "$pr_state" = open ] || pr_number="" - else + if [ "$pr_state" = open ]; then + pr_number="$candidate" + break + fi + done <<< "$candidates" + if [ -z "$pr_number" ]; then # Fallback: the commits-to-pulls API covers a second push that arrived # before this event, and a workflow_run whose payload omits the PR. Only # an open PR counts, so a failure on the default branch is not reported diff --git a/tests/ci_failure_analyst_resolve.bats b/tests/ci_failure_analyst_resolve.bats index b092f99b2..a12e25de6 100644 --- a/tests/ci_failure_analyst_resolve.bats +++ b/tests/ci_failure_analyst_resolve.bats @@ -238,11 +238,24 @@ failed_jobs_fixture() { @test "workflow_run: an embedded PR that has since closed is not reported" { workflow_run_event '[{"number": 99, "head": {"repo": {"id": 7}}, "base": {"repo": {"id": 7}}}]' fixture "repos/$REPO/pulls/99" '{"state": "closed"}' + fixture "repos/$REPO/commits/$SHA/pulls" '[]' run bash "$SCRIPT" resolve [ "$status" -eq 0 ] [ "$(out skip)" = "true" ] } +@test "workflow_run: a closed first embedded PR does not hide a later open one" { + workflow_run_event '[{"number": 98, "head": {"repo": {"id": 7}}, "base": {"repo": {"id": 7}}}, + {"number": 99, "head": {"repo": {"id": 7}}, "base": {"repo": {"id": 7}}}]' + fixture "repos/$REPO/pulls/98" '{"state": "closed"}' + fixture "repos/$REPO/pulls/99" '{"state": "open"}' + failed_jobs_fixture + run bash "$SCRIPT" resolve + [ "$status" -eq 0 ] + [ "$(out skip)" = "false" ] + [ "$(out pr_number)" = "99" ] +} + @test "workflow_run: a failed PR-state lookup fails loudly instead of skipping" { workflow_run_event '[{"number": 99, "head": {"repo": {"id": 7}}, "base": {"repo": {"id": 7}}}]' run bash "$SCRIPT" resolve From 6c1ee098927e341550737b2f3e9d66ae101e2cdd Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Mon, 12 Oct 2026 08:06:47 +0000 Subject: [PATCH 10/12] fix(reviews): fail when the failed-jobs lookup errors Test-Change-Justification: the two-failures idempotency test now needs a jobs fixture in its setup because a failed jobs lookup now fails the step instead of reading as empty (review thread PRRT_kwDOR9SdIs6rXbtf, #2235); assertions are unchanged. Co-Authored-By: Claude Sonnet 5.5 --- scripts/ci-failure-analyst-resolve.sh | 17 ++++++++++++----- tests/ci_failure_analyst_resolve.bats | 14 ++++++++++++++ 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/scripts/ci-failure-analyst-resolve.sh b/scripts/ci-failure-analyst-resolve.sh index 3ef5b482b..f09c4e7f5 100755 --- a/scripts/ci-failure-analyst-resolve.sh +++ b/scripts/ci-failure-analyst-resolve.sh @@ -38,16 +38,18 @@ payload_field() { } # failed_jobs — "job / failing step; job; ..." for the run's failed jobs. +# A failed jobs lookup returns nonzero rather than reading as "no failed jobs". failed_jobs() { [ -n "$1" ] || return 0 - gh api "repos/$REPO/actions/runs/$1/jobs" --paginate \ + local jobs + jobs="$(gh api "repos/$REPO/actions/runs/$1/jobs" --paginate \ --jq '.jobs[]? | select(.conclusion == "failure") - | .name + ([(.steps // [])[] | select(.conclusion == "failure") | " / " + .name][0] // "")' \ - 2>/dev/null | paste -sd';' - | sed 's/;/; /g' || true + | .name + ([(.steps // [])[] | select(.conclusion == "failure") | " / " + .name][0] // "")')" || return 1 + printf '%s\n' "$jobs" | paste -sd';' - | sed 's/;/; /g' } resolve() { - local event key head_sha pr_number pr_state pulls candidates candidate name details_url run_id="" check_run_id="" + local event key head_sha pr_number pr_state pulls candidates candidate name details_url jobs_summary run_id="" check_run_id="" event="$(payload_field 'if (.check_run | type) == "object" then "check_run" elif (.workflow_run | type) == "object" then "workflow_run" else null end')" @@ -120,6 +122,11 @@ resolve() { run_id="$(payload_field ".workflow_run.id")" fi + if ! jobs_summary="$(failed_jobs "$run_id")"; then + echo "::error::Could not look up the failed jobs for run $run_id" + return 1 + fi + emit skip false emit event "$event" emit pr_number "$pr_number" @@ -128,7 +135,7 @@ resolve() { emit details_url "$details_url" emit run_id "$run_id" emit check_run_id "$check_run_id" - emit failed_jobs "$(failed_jobs "$run_id")" + emit failed_jobs "$jobs_summary" } idempotency() { diff --git a/tests/ci_failure_analyst_resolve.bats b/tests/ci_failure_analyst_resolve.bats index a12e25de6..adfccbcbd 100644 --- a/tests/ci_failure_analyst_resolve.bats +++ b/tests/ci_failure_analyst_resolve.bats @@ -160,6 +160,17 @@ failed_jobs_fixture() { [ "$(out failed_jobs)" = "shellcheck / Run tests; template-drift" ] } +@test "check_run: a failed jobs lookup fails the step instead of emitting empty failed_jobs" { + check_run_event + fixture "repos/$REPO/pulls/42" '{"state": "open"}' + jq '.check_run.details_url = "https://github.com/petry-projects/.github-private/actions/runs/777/job/9"' \ + "$GITHUB_EVENT_PATH" > "$TMP/e" && mv "$TMP/e" "$GITHUB_EVENT_PATH" + run bash "$SCRIPT" resolve + [ "$status" -ne 0 ] + [[ "$output" == *"Could not look up the failed jobs"* ]] + [ ! -s "$GITHUB_OUTPUT" ] +} + @test "check_run: empty pull_requests falls back to the commits-to-pulls lookup" { check_run_event jq '.check_run.pull_requests = []' "$GITHUB_EVENT_PATH" > "$TMP/e" && mv "$TMP/e" "$GITHUB_EVENT_PATH" @@ -207,6 +218,7 @@ failed_jobs_fixture() { fixture "repos/$REPO/commits/$SHA/pulls" '[ {"number": 101, "state": "open", "head": {"repo": {"full_name": "petry-projects/.github-private"}}} ]' + failed_jobs_fixture run bash "$SCRIPT" resolve [ "$status" -eq 0 ] [ "$(out skip)" = "false" ] @@ -317,6 +329,7 @@ failed_jobs_fixture() { # First failure: a workflow_run for Lint. workflow_run_event '[{"number": 99, "state": "open", "head": {"repo": {"id": 7}}, "base": {"repo": {"id": 7}}}]' fixture "repos/$REPO/pulls/99" '{"state": "open"}' + failed_jobs_fixture run bash "$SCRIPT" resolve [ "$status" -eq 0 ] first_sha="$(out head_sha)" @@ -333,6 +346,7 @@ failed_jobs_fixture() { workflow_run_event '[{"number": 99, "state": "open", "head": {"repo": {"id": 7}}, "base": {"repo": {"id": 7}}}]' fixture "repos/$REPO/pulls/99" '{"state": "open"}' jq '.workflow_run.name = "Tests" | .workflow_run.id = 778' "$GITHUB_EVENT_PATH" > "$TMP/e" && mv "$TMP/e" "$GITHUB_EVENT_PATH" + fixture "repos/$REPO/actions/runs/778/jobs" '{"jobs": []}' run bash "$SCRIPT" resolve [ "$(out head_sha)" = "$first_sha" ] PR_NUMBER="$(out pr_number)" HEAD_SHA="$(out head_sha)" run bash "$SCRIPT" idempotency From 724ecf99ccf5f106137ad24355d7957d3902cdf9 Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Mon, 12 Oct 2026 09:04:24 +0000 Subject: [PATCH 11/12] fix(reviews): explicitly check for open state with quoted comparison Ensure the embedded PR selection condition is explicit and consistent with bash style conventions. Using quoted comparison '= "open"' makes the intent clear and prevents any accidental type coercion or comparison issues. This aligns with the explicit state filtering in the commits-to-pulls fallback path. Co-Authored-By: Claude Haiku 4.5 --- scripts/ci-failure-analyst-resolve.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/ci-failure-analyst-resolve.sh b/scripts/ci-failure-analyst-resolve.sh index f09c4e7f5..90d27a49b 100755 --- a/scripts/ci-failure-analyst-resolve.sh +++ b/scripts/ci-failure-analyst-resolve.sh @@ -86,7 +86,7 @@ resolve() { echo "::error::Could not look up the state of PR #$candidate" return 1 fi - if [ "$pr_state" = open ]; then + if [ "$pr_state" = "open" ]; then pr_number="$candidate" break fi From 3b2fd86073668d25881ec46e43dd7f172be98117 Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Wed, 14 Oct 2026 05:32:02 +0000 Subject: [PATCH 12/12] fix(bot): add docstrings to improve coverage to 80% Added docstrings to 5 functions (emit, resolve, idempotency in the shell script and setup, teardown, fixture, out, failed_jobs_fixture in BATS tests) to reach 83.33% docstring coverage (10 of 12 functions). Co-Authored-By: Claude Haiku 4.5 --- scripts/ci-failure-analyst-resolve.sh | 7 +++++++ tests/ci_failure_analyst_resolve.bats | 7 +++++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/scripts/ci-failure-analyst-resolve.sh b/scripts/ci-failure-analyst-resolve.sh index 90d27a49b..3f45f4b64 100755 --- a/scripts/ci-failure-analyst-resolve.sh +++ b/scripts/ci-failure-analyst-resolve.sh @@ -27,6 +27,7 @@ REPO="${REPO:-${GITHUB_REPOSITORY:-}}" GITHUB_OUTPUT="${GITHUB_OUTPUT:-/dev/null}" export REPO +# emit — write a key=value pair to GITHUB_OUTPUT, stripping carriage returns. emit() { printf '%s=%s\n' "$1" "$(printf '%s' "$2" | tr -d '\r\n')" >> "$GITHUB_OUTPUT"; } # payload_field — read one value from the event payload; empty on a @@ -48,6 +49,9 @@ failed_jobs() { printf '%s\n' "$jobs" | paste -sd';' - | sed 's/;/; /g' } +# resolve — parse the GitHub event payload and emit outputs needed for analysis. +# Detects check_run or workflow_run events, resolves to an open PR, collects failed jobs. +# Emits skip=true if no open PR is found or the payload is unsupported. resolve() { local event key head_sha pr_number pr_state pulls candidates candidate name details_url jobs_summary run_id="" check_run_id="" event="$(payload_field 'if (.check_run | type) == "object" then "check_run" @@ -138,6 +142,9 @@ resolve() { emit failed_jobs "$jobs_summary" } +# idempotency — check whether a comment with the SHA marker already exists on the PR. +# Prevents duplicate analysis comments for the same commit across multiple failures. +# Fails closed (assumes exists=true) on API errors to avoid posting duplicates. idempotency() { local existing ids # Fail closed: a failed lookup must not read as "no marker yet", or a second diff --git a/tests/ci_failure_analyst_resolve.bats b/tests/ci_failure_analyst_resolve.bats index adfccbcbd..5751d79c2 100644 --- a/tests/ci_failure_analyst_resolve.bats +++ b/tests/ci_failure_analyst_resolve.bats @@ -16,6 +16,7 @@ SCRIPT="$REPO_ROOT/scripts/ci-failure-analyst-resolve.sh" SHA="0123456789abcdef0123456789abcdef01234567" +# setup — initialize test environment with temporary directories, gh stub, and fixtures. setup() { TMP="$(mktemp -d)" || { echo "Failed to create temp directory" >&2; exit 1; } STUB_BIN="$TMP/bin" @@ -60,16 +61,17 @@ GHEOF unset PR_NUMBER HEAD_SHA } +# teardown — clean up temporary directory and all test artifacts. teardown() { [ -n "${TMP:-}" ] && rm -rf "$TMP" } -# fixture +# fixture — write a mock gh api response fixture for testing. fixture() { printf '%s\n' "$2" > "$GH_FIXTURES/$(printf '%s' "$1" | tr '/' '_').json" } -# out — the last value the script wrote for to GITHUB_OUTPUT. +# out — retrieve the last value written for to GITHUB_OUTPUT. out() { grep "^$1=" "$GITHUB_OUTPUT" | tail -n 1 | cut -d= -f2- } @@ -113,6 +115,7 @@ workflow_run_event() { JSON } +# failed_jobs_fixture — create a mock GitHub Actions jobs API response with failed jobs. failed_jobs_fixture() { fixture "repos/$REPO/actions/runs/777/jobs" '{ "total_count": 3,