From 32c068e010e99f7e4bf1fe0c7223479e01d609a0 Mon Sep 17 00:00:00 2001 From: Don Petry <36422719+don-petry@users.noreply.github.com> Date: Fri, 6 Nov 2026 15:16:01 -0600 Subject: [PATCH 1/4] fix(dev-lead-retry): CI-retry payloads carry the check run id and URL again lookup_check_run_details passed `--arg name ...` to `gh api --jq`, which takes no --arg: gh read "--arg" as the filter and rejected the call with "accepts 1 arg(s), received 4". The fallback object then always won, so every dev-lead-ci-failure dispatch sent an empty details_url and a null check id. Fetch the raw response and filter it with `jq --arg`, which also keeps the check name out of the filter text. An API error body still falls back to the empty object, since it has no .check_runs and jq fails on it. Tests use a gh stub that parses arguments like the real gh, so the old call fails there the same way it did in production. Co-Authored-By: Claude Opus 5.5 --- scripts/dev-lead-retry.sh | 24 +++--- tests/dev-lead/unit/test_dev_lead_retry.bats | 80 ++++++++++++++++++++ 2 files changed, 93 insertions(+), 11 deletions(-) diff --git a/scripts/dev-lead-retry.sh b/scripts/dev-lead-retry.sh index 0a924817f..25ff973d9 100755 --- a/scripts/dev-lead-retry.sh +++ b/scripts/dev-lead-retry.sh @@ -359,17 +359,19 @@ post_dispatch_guard() { # Returns JSON {id, details_url} for the most recent failed check matching # check_name on head_sha, so the retry dispatch has full failure context. lookup_check_run_details() { - local repo="$1" head_sha="$2" check_name="$3" out - # On an API error gh prints the error body to stdout, so a `|| echo` fallback - # would be appended to it; replace the output instead. - if ! out=$(gh api "repos/${repo}/commits/${head_sha}/check-runs?per_page=100" \ - --jq --arg name "$check_name" \ - '.check_runs - | map(select(.name == $name and .conclusion == "failure")) - | sort_by(.completed_at) - | last - | {id: (.id // ""), details_url: (.details_url // "")}' \ - 2>/dev/null); then + local repo="$1" head_sha="$2" check_name="$3" raw out + # `gh api --jq` takes no `--arg`, so filter the raw response with jq, which + # also keeps the check name out of the filter text. On an API error gh prints + # the error body to stdout, so a `|| echo` fallback would be appended to it; + # replace the output instead (an error body has no .check_runs, so jq fails). + if ! raw=$(gh api "repos/${repo}/commits/${head_sha}/check-runs?per_page=100" 2>/dev/null) \ + || ! out=$(jq -c --arg name "$check_name" \ + '.check_runs + | map(select(.name == $name and .conclusion == "failure")) + | sort_by(.completed_at) + | last + | {id: (.id // ""), details_url: (.details_url // "")}' \ + <<< "$raw" 2>/dev/null); then out='{"id":"","details_url":""}' fi printf '%s\n' "$out" diff --git a/tests/dev-lead/unit/test_dev_lead_retry.bats b/tests/dev-lead/unit/test_dev_lead_retry.bats index dbff1ea2e..bd60f0882 100644 --- a/tests/dev-lead/unit/test_dev_lead_retry.bats +++ b/tests/dev-lead/unit/test_dev_lead_retry.bats @@ -261,6 +261,86 @@ GHEOF [ "$output" = '{"id":"","details_url":""}' ] } +# _strict_gh_stub: parses its arguments the way the real gh does — value flags +# take the next word, and `gh api` accepts exactly one positional (the endpoint), +# else "accepts 1 arg(s), received N" and exit 1. The check-runs endpoint answers +# $CHECK_RUNS_RAW through --jq; dispatches are captured to $PAYLOAD_FILE. +_strict_gh_stub() { + cat > "$MOCK_BIN/gh" <<'GHEOF' +#!/usr/bin/env bash +shift # "api" +filter="." pos=() +while [ $# -gt 0 ]; do + case "$1" in + --jq|-q) filter="$2"; shift 2 ;; + --method|-X|-f|-F|-H|--input) shift 2 ;; + --paginate|--silent|-i) shift ;; + *) pos+=("$1"); shift ;; + esac +done +if [ "${#pos[@]}" -ne 1 ]; then + echo "accepts 1 arg(s), received ${#pos[@]}" >&2 + exit 1 +fi +case "${pos[0]}" in + */dispatches) cat > "${PAYLOAD_FILE:-/dev/null}" ;; + */check-runs*) printf '%s' "${CHECK_RUNS_RAW:-{\}}" | jq -c "$filter" ;; + *) echo "[]" ;; +esac +GHEOF + chmod +x "$MOCK_BIN/gh" +} + +# Two failed runs of "CI" (the later one wins), a success and another check. +_check_runs_raw() { + jq -nc --arg name "${1:-CI}" '{check_runs: [ + {id: 11, name: $name, conclusion: "failure", completed_at: "2026-06-18T01:00:00Z", + details_url: "https://github.com/combsmith/app/actions/runs/1/job/11"}, + {id: 12, name: $name, conclusion: "failure", completed_at: "2026-06-18T02:00:00Z", + details_url: "https://github.com/combsmith/app/actions/runs/2/job/12"}, + {id: 13, name: $name, conclusion: "success", completed_at: "2026-06-18T03:00:00Z", + details_url: "https://github.com/combsmith/app/actions/runs/3/job/13"}, + {id: 14, name: "lint", conclusion: "failure", completed_at: "2026-06-18T04:00:00Z", + details_url: "https://github.com/combsmith/app/actions/runs/4/job/14"}]}' +} + +@test "lookup_check_run_details: returns the id and URL of the latest failed run of the check" { + _strict_gh_stub + CHECK_RUNS_RAW="$(_check_runs_raw)" + export CHECK_RUNS_RAW + + run lookup_check_run_details "combsmith/app" abc123 "CI" + + [ "$status" -eq 0 ] + [ "$(jq -r '.id' <<<"$output")" = "12" ] + [ "$(jq -r '.details_url' <<<"$output")" = "https://github.com/combsmith/app/actions/runs/2/job/12" ] +} + +@test "lookup_check_run_details: a check name with quotes and backslashes matches literally" { + _strict_gh_stub + CHECK_RUNS_RAW="$(_check_runs_raw 'build "x" \\ (ubuntu)')" + export CHECK_RUNS_RAW + + run lookup_check_run_details "combsmith/app" abc123 'build "x" \\ (ubuntu)' + + [ "$status" -eq 0 ] + [ "$(jq -r '.id' <<<"$output")" = "12" ] +} + +@test "dispatch_ci_retry: the payload carries the check run id and URL" { + _strict_gh_stub + CHECK_RUNS_RAW="$(_check_runs_raw)" + export CHECK_RUNS_RAW DRY_RUN="false" + rm -f "$PAYLOAD_FILE" + + run dispatch_ci_retry "combsmith/app" 42 abc123 "CI" + + [ "$status" -eq 0 ] + [ "$(jq -r '.event_type' "$PAYLOAD_FILE")" = "dev-lead-ci-failure" ] + [ "$(jq -r '.client_payload.checks[0].id' "$PAYLOAD_FILE")" = "12" ] + [ "$(jq -r '.client_payload.checks[0].details_url' "$PAYLOAD_FILE")" = "https://github.com/combsmith/app/actions/runs/2/job/12" ] +} + @test "gh_api_list: pages are joined into one array; a non-array page becomes []" { printf '#!/usr/bin/env bash\nprintf "%%s\\n" "$PAGES"\n' > "$MOCK_BIN/gh" chmod +x "$MOCK_BIN/gh" From 6b0fd3e0802033ddf0c9bb89228818a65d7380ee Mon Sep 17 00:00:00 2001 From: Don Petry <36422719+don-petry@users.noreply.github.com> Date: Fri, 6 Nov 2026 15:37:37 -0600 Subject: [PATCH 2/4] fix(dev-lead-retry): read every check-run page; cover no-match and \(...) names A commit can carry more than 100 check runs, so a failure listed past the first page left the CI-retry payload without its id and URL. Read the check runs with --paginate and filter all pages together. Tests: the strict gh stub prints only the first page without --paginate; new cases for a failure on a later page and for a check with no failed run (both fields empty, not null); the literal-name case now includes \(...). Addresses CodeRabbit review threads on #2467. Co-Authored-By: Claude Opus 5.5 --- scripts/dev-lead-retry.sh | 15 +++---- tests/dev-lead/unit/test_dev_lead_retry.bats | 41 ++++++++++++++++---- 2 files changed, 42 insertions(+), 14 deletions(-) diff --git a/scripts/dev-lead-retry.sh b/scripts/dev-lead-retry.sh index 25ff973d9..b93ed766e 100755 --- a/scripts/dev-lead-retry.sh +++ b/scripts/dev-lead-retry.sh @@ -360,13 +360,14 @@ post_dispatch_guard() { # check_name on head_sha, so the retry dispatch has full failure context. lookup_check_run_details() { local repo="$1" head_sha="$2" check_name="$3" raw out - # `gh api --jq` takes no `--arg`, so filter the raw response with jq, which - # also keeps the check name out of the filter text. On an API error gh prints - # the error body to stdout, so a `|| echo` fallback would be appended to it; - # replace the output instead (an error body has no .check_runs, so jq fails). - if ! raw=$(gh api "repos/${repo}/commits/${head_sha}/check-runs?per_page=100" 2>/dev/null) \ - || ! out=$(jq -c --arg name "$check_name" \ - '.check_runs + # `gh api --jq` takes no `--arg`, so filter the raw pages with jq, which also + # keeps the check name out of the filter text. A commit can carry more than + # 100 check runs, so every page is read. On an API error gh prints the error + # body to stdout, so a `|| echo` fallback would be appended to it; replace the + # output instead (an error body has no .check_runs, so jq fails). + if ! raw=$(gh api --paginate "repos/${repo}/commits/${head_sha}/check-runs?per_page=100" 2>/dev/null) \ + || ! out=$(jq -sc --arg name "$check_name" \ + '[.[].check_runs[]] | map(select(.name == $name and .conclusion == "failure")) | sort_by(.completed_at) | last diff --git a/tests/dev-lead/unit/test_dev_lead_retry.bats b/tests/dev-lead/unit/test_dev_lead_retry.bats index bd60f0882..d955c480f 100644 --- a/tests/dev-lead/unit/test_dev_lead_retry.bats +++ b/tests/dev-lead/unit/test_dev_lead_retry.bats @@ -264,17 +264,19 @@ GHEOF # _strict_gh_stub: parses its arguments the way the real gh does — value flags # take the next word, and `gh api` accepts exactly one positional (the endpoint), # else "accepts 1 arg(s), received N" and exit 1. The check-runs endpoint answers -# $CHECK_RUNS_RAW through --jq; dispatches are captured to $PAYLOAD_FILE. +# $CHECK_RUNS_RAW (one JSON page per line; only the first without --paginate, +# all back to back with it, as gh does) through --jq; dispatches are captured to $PAYLOAD_FILE. _strict_gh_stub() { cat > "$MOCK_BIN/gh" <<'GHEOF' #!/usr/bin/env bash shift # "api" -filter="." pos=() +filter="." pages=1 pos=() while [ $# -gt 0 ]; do case "$1" in --jq|-q) filter="$2"; shift 2 ;; --method|-X|-f|-F|-H|--input) shift 2 ;; - --paginate|--silent|-i) shift ;; + --paginate) pages=0; shift ;; + --silent|-i) shift ;; *) pos+=("$1"); shift ;; esac done @@ -284,7 +286,8 @@ if [ "${#pos[@]}" -ne 1 ]; then fi case "${pos[0]}" in */dispatches) cat > "${PAYLOAD_FILE:-/dev/null}" ;; - */check-runs*) printf '%s' "${CHECK_RUNS_RAW:-{\}}" | jq -c "$filter" ;; + */check-runs*) printf '%s\n' "${CHECK_RUNS_RAW:-{\}}" \ + | if [ "$pages" -eq 1 ]; then head -n 1; else cat; fi | jq -c "$filter" ;; *) echo "[]" ;; esac GHEOF @@ -316,17 +319,41 @@ _check_runs_raw() { [ "$(jq -r '.details_url' <<<"$output")" = "https://github.com/combsmith/app/actions/runs/2/job/12" ] } -@test "lookup_check_run_details: a check name with quotes and backslashes matches literally" { +@test "lookup_check_run_details: a check name with quotes, backslashes and \\(...) matches literally" { _strict_gh_stub - CHECK_RUNS_RAW="$(_check_runs_raw 'build "x" \\ (ubuntu)')" + local name='build "x" \\ \(env.OS) (ubuntu)' + CHECK_RUNS_RAW="$(_check_runs_raw "$name")" export CHECK_RUNS_RAW - run lookup_check_run_details "combsmith/app" abc123 'build "x" \\ (ubuntu)' + run lookup_check_run_details "combsmith/app" abc123 "$name" [ "$status" -eq 0 ] [ "$(jq -r '.id' <<<"$output")" = "12" ] } +@test "lookup_check_run_details: a failure on a later page is found (all pages are read)" { + _strict_gh_stub + CHECK_RUNS_RAW="$(jq -nc '{check_runs: [{id: 1, name: "lint", conclusion: "success"}]}')"$'\n'"$(_check_runs_raw)" + export CHECK_RUNS_RAW + + run lookup_check_run_details "combsmith/app" abc123 "CI" + + [ "$status" -eq 0 ] + [ "$(jq -r '.id' <<<"$output")" = "12" ] + [ "$(jq -r '.details_url' <<<"$output")" = "https://github.com/combsmith/app/actions/runs/2/job/12" ] +} + +@test "lookup_check_run_details: no failed run of the check prints empty fields, not null" { + _strict_gh_stub + CHECK_RUNS_RAW="$(_check_runs_raw)" + export CHECK_RUNS_RAW + + run lookup_check_run_details "combsmith/app" abc123 "deploy" + + [ "$status" -eq 0 ] + [ "$output" = '{"id":"","details_url":""}' ] +} + @test "dispatch_ci_retry: the payload carries the check run id and URL" { _strict_gh_stub CHECK_RUNS_RAW="$(_check_runs_raw)" From 904a547e780acd927960b7e1f5cbea55a142a1f9 Mon Sep 17 00:00:00 2001 From: Don Petry <36422719+don-petry@users.noreply.github.com> Date: Fri, 6 Nov 2026 15:45:22 -0600 Subject: [PATCH 3/4] fix(dev-lead-retry): warn when a CI retry goes out without its check run When lookup_check_run_details finds no failed run of the check, or cannot read the check runs, it now prints a ::warning:: on stderr naming the repo, head SHA and check. Silence is how the --jq --arg bug went unnoticed. The name is reduced to one bounded line before it lands in the workflow command, and stdout still carries only the JSON object. Addresses donpetry-bot review on #2467. Co-Authored-By: Claude Opus 5.5 --- scripts/dev-lead-retry.sh | 9 ++++++- tests/dev-lead/unit/test_dev_lead_retry.bats | 28 ++++++++++++++++---- 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/scripts/dev-lead-retry.sh b/scripts/dev-lead-retry.sh index b93ed766e..9999cbdf7 100755 --- a/scripts/dev-lead-retry.sh +++ b/scripts/dev-lead-retry.sh @@ -358,8 +358,10 @@ post_dispatch_guard() { # lookup_check_run_details # Returns JSON {id, details_url} for the most recent failed check matching # check_name on head_sha, so the retry dispatch has full failure context. +# Without one it prints {"id":"","details_url":""} and a ::warning:: on stderr +# naming the repo and check: a retry without them cannot fetch the run's logs. lookup_check_run_details() { - local repo="$1" head_sha="$2" check_name="$3" raw out + local repo="$1" head_sha="$2" check_name="$3" raw out safe_name # `gh api --jq` takes no `--arg`, so filter the raw pages with jq, which also # keeps the check name out of the filter text. A commit can carry more than # 100 check runs, so every page is read. On an API error gh prints the error @@ -375,6 +377,11 @@ lookup_check_run_details() { <<< "$raw" 2>/dev/null); then out='{"id":"","details_url":""}' fi + if [ "$out" = '{"id":"","details_url":""}' ]; then + # One line, bounded: the name lands inside a workflow command. + safe_name=$(printf '%s' "$check_name" | tr -c '[:alnum:] ._()/-' ' ' | cut -c1-120) + echo "::warning::dev-lead-retry: no failed check run '${safe_name}' found on ${repo}@${head_sha:0:8} (or check runs unreadable); the CI retry is sent without its id and details_url" >&2 + fi printf '%s\n' "$out" } diff --git a/tests/dev-lead/unit/test_dev_lead_retry.bats b/tests/dev-lead/unit/test_dev_lead_retry.bats index d955c480f..bb4e92e1a 100644 --- a/tests/dev-lead/unit/test_dev_lead_retry.bats +++ b/tests/dev-lead/unit/test_dev_lead_retry.bats @@ -255,10 +255,11 @@ GHEOF _real_jq_gh_stub export GH_FAIL_PATHS='repos/combsmith/app/commits/*' - run lookup_check_run_details "combsmith/app" abc123 "CI" + run --separate-stderr lookup_check_run_details "combsmith/app" abc123 "CI" [ "$status" -eq 0 ] [ "$output" = '{"id":"","details_url":""}' ] + [[ "$stderr" == *"::warning::"*"'CI'"*"combsmith/app@abc123"* ]] } # _strict_gh_stub: parses its arguments the way the real gh does — value flags @@ -312,11 +313,12 @@ _check_runs_raw() { CHECK_RUNS_RAW="$(_check_runs_raw)" export CHECK_RUNS_RAW - run lookup_check_run_details "combsmith/app" abc123 "CI" + run --separate-stderr lookup_check_run_details "combsmith/app" abc123 "CI" [ "$status" -eq 0 ] [ "$(jq -r '.id' <<<"$output")" = "12" ] [ "$(jq -r '.details_url' <<<"$output")" = "https://github.com/combsmith/app/actions/runs/2/job/12" ] + [ -z "$stderr" ] } @test "lookup_check_run_details: a check name with quotes, backslashes and \\(...) matches literally" { @@ -325,18 +327,32 @@ _check_runs_raw() { CHECK_RUNS_RAW="$(_check_runs_raw "$name")" export CHECK_RUNS_RAW - run lookup_check_run_details "combsmith/app" abc123 "$name" + run --separate-stderr lookup_check_run_details "combsmith/app" abc123 "$name" [ "$status" -eq 0 ] [ "$(jq -r '.id' <<<"$output")" = "12" ] } +@test "lookup_check_run_details: the warning keeps a hostile check name to one bounded line" { + _strict_gh_stub + CHECK_RUNS_RAW="$(_check_runs_raw)" + export CHECK_RUNS_RAW + + run --separate-stderr lookup_check_run_details "combsmith/app" abc123 $'x\n::error::y %0A'"$(printf 'z%.0s' {1..300})" + + [ "$status" -eq 0 ] + [ "$output" = '{"id":"","details_url":""}' ] + [ "$(wc -l <<<"$stderr")" -eq 1 ] + [[ "$stderr" != *"::error::"* ]] + [[ "$stderr" != *"%0A"* ]] +} + @test "lookup_check_run_details: a failure on a later page is found (all pages are read)" { _strict_gh_stub CHECK_RUNS_RAW="$(jq -nc '{check_runs: [{id: 1, name: "lint", conclusion: "success"}]}')"$'\n'"$(_check_runs_raw)" export CHECK_RUNS_RAW - run lookup_check_run_details "combsmith/app" abc123 "CI" + run --separate-stderr lookup_check_run_details "combsmith/app" abc123 "CI" [ "$status" -eq 0 ] [ "$(jq -r '.id' <<<"$output")" = "12" ] @@ -348,10 +364,12 @@ _check_runs_raw() { CHECK_RUNS_RAW="$(_check_runs_raw)" export CHECK_RUNS_RAW - run lookup_check_run_details "combsmith/app" abc123 "deploy" + run --separate-stderr lookup_check_run_details "combsmith/app" abc123 "deploy (prod)" [ "$status" -eq 0 ] [ "$output" = '{"id":"","details_url":""}' ] + [[ "$stderr" == *"::warning::"*"'deploy (prod)'"*"combsmith/app@abc123"* ]] + [ "$(grep -c '::warning::' <<<"$stderr")" -eq 1 ] } @test "dispatch_ci_retry: the payload carries the check run id and URL" { From 11929668fede1b15458100abc4347bb494eff259 Mon Sep 17 00:00:00 2001 From: Don Petry <36422719+don-petry@users.noreply.github.com> Date: Fri, 6 Nov 2026 16:12:40 -0600 Subject: [PATCH 4/4] style(dev-lead-retry): use [[ ]] for the fallback check (Sonar S7688) Co-Authored-By: Claude Opus 5.5 --- scripts/dev-lead-retry.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/dev-lead-retry.sh b/scripts/dev-lead-retry.sh index 9999cbdf7..cb4aa3011 100755 --- a/scripts/dev-lead-retry.sh +++ b/scripts/dev-lead-retry.sh @@ -377,7 +377,7 @@ lookup_check_run_details() { <<< "$raw" 2>/dev/null); then out='{"id":"","details_url":""}' fi - if [ "$out" = '{"id":"","details_url":""}' ]; then + if [[ "$out" == '{"id":"","details_url":""}' ]]; then # One line, bounded: the name lands inside a workflow command. safe_name=$(printf '%s' "$check_name" | tr -c '[:alnum:] ._()/-' ' ' | cut -c1-120) echo "::warning::dev-lead-retry: no failed check run '${safe_name}' found on ${repo}@${head_sha:0:8} (or check runs unreadable); the CI retry is sent without its id and details_url" >&2