From 26205c5f9937aa14ff794d8e4547915e93f742db Mon Sep 17 00:00:00 2001 From: tend-agent <270458913+tend-agent@users.noreply.github.com> Date: Tue, 1 Sep 2026 06:49:35 +0000 Subject: [PATCH] fix(review-runs): inherit errexit so a failed gh call can't shrink the corrections window --- .../tests/test_review_runs_corrections.py | 45 ++++++++++++++++++- .../scripts/review-runs-corrections.sh | 14 +++--- 2 files changed, 52 insertions(+), 7 deletions(-) diff --git a/generator/tests/test_review_runs_corrections.py b/generator/tests/test_review_runs_corrections.py index 3af29ec3..941d2dc2 100644 --- a/generator/tests/test_review_runs_corrections.py +++ b/generator/tests/test_review_runs_corrections.py @@ -45,9 +45,17 @@ [ -n "${SEARCH_FAILS:-}" ] && { echo "API rate limit exceeded" >&2; exit 1; } emit "$(cat "$CANDIDATES_JSON")" ;; "pr list"*) emit "$(cat "$BOT_PRS_JSON")" ;; - *"/issues/comments"*) emit "$(cat "$ISSUE_COMMENTS_JSON")" ;; + *"/issues/comments"*) + [ -n "${ISSUE_COMMENTS_FAILS:-}" ] && { echo "502 Bad Gateway" >&2; exit 1; } + emit "$(cat "$ISSUE_COMMENTS_JSON")" ;; *"/pulls/comments"*) emit "$(cat "$PR_COMMENTS_JSON")" ;; - *"/pulls/"*"/reviews"*) emit "$(cat "$REVIEWS_JSON")" ;; + # Unset, the placeholder matches no numeric PR path. + *"/pulls/"*"/reviews"*) + case "$*" in + *"/pulls/${REVIEWS_FAIL_FOR:-none}/reviews"*) + echo "502 Bad Gateway" >&2; exit 1 ;; + esac + emit "$(cat "$REVIEWS_JSON")" ;; *) exit 1 ;; esac """ @@ -157,6 +165,39 @@ def test_a_failed_candidate_search_aborts_rather_than_reporting_no_reviews( assert result.stdout == "" +def test_a_failed_comment_endpoint_aborts_rather_than_reporting_the_other_half( + env: dict[str, str], +) -> None: + """Both comment endpoints are read in one loop inside a command substitution. + + Bash applies `-e` there only under `inherit_errexit`, and `pipefail` sees + just the loop's final iteration — so a dropped `issues` page would leave + every conversation comment out of a report that still exits 0. + """ + _write(env, "PR_COMMENTS_JSON", [_comment(IN_WINDOW)]) + env["ISSUE_COMMENTS_FAILS"] = "1" + + result = _run(env, SINCE) + + assert result.returncode != 0 + assert result.stdout == "" + + +def test_a_failed_review_fetch_aborts_rather_than_dropping_that_prs_reviews( + env: dict[str, str], +) -> None: + """One `gh` call per candidate PR, in that same loop: a failure on any but + the last would drop that PR's reviews and report the rest as the window.""" + _write(env, "CANDIDATES_JSON", [{"number": 1}, {"number": 2}]) + _write(env, "REVIEWS_JSON", [_review(IN_WINDOW)]) + env["REVIEWS_FAIL_FOR"] = "1" + + result = _run(env, SINCE) + + assert result.returncode != 0 + assert result.stdout == "" + + def test_a_quiet_window_reports_empty_lists(env: dict[str, str]) -> None: out = _collect(env) diff --git a/plugins/tend-ci-runner/scripts/review-runs-corrections.sh b/plugins/tend-ci-runner/scripts/review-runs-corrections.sh index be348a00..8d5e31c8 100755 --- a/plugins/tend-ci-runner/scripts/review-runs-corrections.sh +++ b/plugins/tend-ci-runner/scripts/review-runs-corrections.sh @@ -23,6 +23,11 @@ # env: GITHUB_REPOSITORY (optional; falls back to the checkout's remote) set -euo pipefail +# Bash does not apply `-e` inside a command substitution unless this is on, +# and every collection below is one. Without it a `gh` call that fails in any +# but the last iteration of a loop is dropped from the JSON and the run +# reports the remainder as the whole window. +shopt -s inherit_errexit SINCE="${1:-}" if [ -z "$SINCE" ]; then @@ -65,11 +70,10 @@ COMMENTS=$( # wraps around a standalone inline reply, which would otherwise report a # correction on every thread where anyone replied inline. # -# The candidate list is its own assignment because `set -e` checks an -# assignment's command substitution but not a `for` word list's: inline, a -# failed `--search` — the search API's 30 req/min, not the 5000/h core budget — -# would yield an empty list, skip the loop, and report `reviews: []` as an -# all-clear. +# The candidate list is its own assignment because a `for` word list is the one +# place `inherit_errexit` does not reach: inline, a failed `--search` — the +# search API's 30 req/min, not the 5000/h core budget — would yield an empty +# list, skip the loop, and report `reviews: []` as an all-clear. CANDIDATES=$(gh pr list --repo "$REPO" --state all --limit 200 \ --search "updated:>=$SINCE" --json number --jq '.[].number') REVIEWS=$(