diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 3a34195b9..6f785e094 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -141,6 +141,7 @@ jobs: tests/auto_rebase_health.bats \ tests/pr_review_outcomes.bats \ tests/pr_review_sweep_metrics.bats \ + tests/pr_review_health.bats \ tests/model_pricing.bats \ tests/test_check_model_pins.bats \ tests/test_check_bats_negation.bats \ diff --git a/scripts/pr_review_health.sh b/scripts/pr_review_health.sh index e2a54de28..7f1adf787 100644 --- a/scripts/pr_review_health.sh +++ b/scripts/pr_review_health.sh @@ -226,6 +226,7 @@ read -r dur_n dur_min dur_p50 dur_p95 dur_max < <(echo "$runs_json" | jq -r ' # --------------------------------------------------------------------------- # 3. Helpers # --------------------------------------------------------------------------- +# Format seconds as a human-readable duration (e.g., "5m30s" or "45s"). fmt_dur() { local s=$1 if [ "$s" -ge 60 ]; then @@ -235,6 +236,24 @@ fmt_dur() { fi } +# llm_quota_refused ... — 0 when a FAILED Claude call's captured output +# reads as a quota / usage-limit refusal (#2446), e.g. "You've hit your weekly +# limit · resets 4pm (UTC)" or an HTTP 429. Only consulted after a non-zero exit, +# so a successful analysis that merely discusses rate limits is never matched. +# Narrower than engine.sh's _rate_limit_pattern on purpose: this script does not +# source engine.sh, and anything not matched here still fails the step. +llm_quota_refused() { + # Detect if a failed Claude call was refused due to quota or rate-limiting. + local f files=() + for f in "$@"; do + [[ -s "$f" ]] && files+=("$f") + done + [[ "${#files[@]}" -gt 0 ]] || return 1 + grep -qiE "hit your ([a-z]+ )?limit|reached your ([a-z]+ )?limit|[0-9]+-[a-z]+ limit reached|usage limit|too many requests|quota (exceeded|exhausted|reached)|out of (credits|tokens)|overloaded_error|([^0-9]|^)(429|529)([^0-9]|$)" \ + "${files[@]}" +} + +# Return an emoji icon representing the workflow conclusion status. conclusion_icon() { case "$1" in success) echo "✅" ;; @@ -246,7 +265,6 @@ conclusion_icon() { } # overall is the pre-computed status fed as a hint to the LLM prompt below. -# shellcheck disable=SC2034 if [ "$failed_runs" -eq 0 ]; then overall="HEALTHY" elif [ "$(echo "$failure_rate > 50" | bc)" -eq 1 ]; then @@ -275,6 +293,12 @@ RUNS_SUMMARY=$(echo "$runs_json" | jq -r ' ') LOG_DIR="${LOG_DIR:-}" +logs_file="" +llm_out="" +llm_err="" +# Registered before the first mktemp so every exit path (including a failed +# second mktemp or a failed report append) removes whatever was created. +trap 'rm -f "$logs_file" "$llm_out" "$llm_err"' EXIT logs_file=$(mktemp) # One jq pass over failed runs; append each log file in the same order while IFS=$'\t' read -r run_id run_meta; do @@ -346,16 +370,20 @@ if [ "${total_sweep_ticks:-0}" -gt 0 ] && [ "$sweep_ticks" -lt "$total_sweep_tic fi echo "Invoking Claude for log analysis..." -# Claude writes errors to stdout (not stderr), so they'd silently land in -# $REPORT_FILE with the stdout redirect below. Wrap in `if !` so a non-zero -# exit surfaces the file contents to the Actions log before aborting. -if ! claude --print --model "$(ai_model_for_family sonnet)" --no-session-persistence >> "$REPORT_FILE" < "$llm_out" 2> "$llm_err" <\` Output ONLY the markdown report — no preamble or commentary outside the report sections. PROMPT then + cat "$llm_err" >&2 + cat "$llm_out" >> "$REPORT_FILE" +elif llm_quota_refused "$llm_out" "$llm_err"; then + # Quota / usage-limit refusal (#2446): the provider declined the call, the + # health check itself is fine. Keep the deterministic sections, say why the + # analysis is missing, and exit 0 so this step does not fail the job and skip + # the runaway / stall / merge-ready scans that follow (they make no model call). + echo "::warning::Claude log analysis skipped — the call was refused for quota / usage limit. Deterministic report sections are still published. CLI output: $(cat "$llm_out" "$llm_err" | head -c 300 | tr '\n' ' ')" + { + printf '## Log analysis skipped\n\n' + printf '_The Claude log-analysis call was refused for quota / usage limit, so this report carries only the deterministic sections above. The model analysis returns on the next run with budget._\n' + } >> "$REPORT_FILE" +else echo "::error::Claude invocation failed. CLI output follows:" - cat "$REPORT_FILE" >&2 - rm -f "$logs_file" + cat "$llm_out" "$llm_err" >&2 exit 1 fi -rm -f "$logs_file" [ -n "${GITHUB_STEP_SUMMARY:-}" ] && [ -f "$REPORT_FILE" ] && cat "$REPORT_FILE" >> "$GITHUB_STEP_SUMMARY" diff --git a/tests/pr_review_health.bats b/tests/pr_review_health.bats new file mode 100644 index 000000000..5dc7305ac --- /dev/null +++ b/tests/pr_review_health.bats @@ -0,0 +1,111 @@ +#!/usr/bin/env bats +# Tests for scripts/pr_review_health.sh — how the Claude log-analysis call's +# failure is handled (#2446). `gh` and `claude` are stubbed on PATH, so no +# network I/O happens; the stubbed gh returns an empty run window. +# Run locally: bats tests/pr_review_health.bats + +# Set up test environment: create stubs for gh and claude CLIs. +setup() { + SCRIPT="${BATS_TEST_DIRNAME}/../scripts/pr_review_health.sh" + WORK="$(mktemp -d)" + STUB_BIN="${WORK}/bin" + mkdir -p "$STUB_BIN" + + # gh stub: every run list is empty, workflow discovery finds no ingress + # workflow, and the workflow-source read fails (the script tolerates it). + cat > "${STUB_BIN}/gh" <<'GH' +#!/usr/bin/env bash +case "$*" in + *"/contents/"*) exit 1 ;; + *"actions/workflows?per_page"*) exit 0 ;; + *"created="*) echo '[]' ;; + *) echo '{}' ;; +esac +GH + chmod +x "${STUB_BIN}/gh" + + export PATH="${STUB_BIN}:${PATH}" + export GITHUB_ENV="${WORK}/github_env" + export GITHUB_STEP_SUMMARY="${WORK}/step_summary" + : > "$GITHUB_ENV" + : > "$GITHUB_STEP_SUMMARY" + cd "$WORK" || return 1 +} + +# Clean up temporary test directories. +teardown() { + rm -rf "$WORK" +} + +# make_claude [stderr_text] +make_claude() { + { + printf '#!/usr/bin/env bash\ncat >/dev/null\n' + printf 'printf %%s\\\\n %q\n' "$2" + [ -n "${3:-}" ] && printf 'printf %%s\\\\n %q >&2\n' "$3" + printf 'exit %s\n' "$1" + } > "${STUB_BIN}/claude" + chmod +x "${STUB_BIN}/claude" +} + +@test "quota refusal from Claude: warns, keeps the deterministic report, exits 0" { + make_claude 1 "You've hit your weekly limit · resets 4pm (UTC)" + run bash "$SCRIPT" + [ "$status" -eq 0 ] + [[ "$output" == *"::warning::"*"quota"* ]] + grep -q '## Convergence latency (deterministic)' pr_review_health_report.md + grep -q 'Log analysis skipped' pr_review_health_report.md + # The CLI's refusal line is not pasted into the report as if it were analysis. + run grep -c "hit your weekly limit" pr_review_health_report.md + [ "$output" = "0" ] + grep -q 'Log analysis skipped' "$GITHUB_STEP_SUMMARY" +} + +@test "quota refusal reported on stderr is also recognised" { + make_claude 1 "" "API Error: 429 Too Many Requests" + run bash "$SCRIPT" + [ "$status" -eq 0 ] + grep -q 'Log analysis skipped' pr_review_health_report.md + run grep -c "429 Too Many Requests" pr_review_health_report.md + [ "$output" = "0" ] +} + +@test "documented 'reached your weekly limit' wording is recognised" { + make_claude 1 "You've reached your weekly limit · resets 4pm (UTC)" + run bash "$SCRIPT" + [ "$status" -eq 0 ] + grep -q 'Log analysis skipped' pr_review_health_report.md +} + +@test "'5-hour limit reached' wording is recognised" { + make_claude 1 "5-hour limit reached - resets 4pm" + run bash "$SCRIPT" + [ "$status" -eq 0 ] + grep -q 'Log analysis skipped' pr_review_health_report.md +} + +@test "non-quota Claude failure still fails the step" { + make_claude 1 "Error: model not found" + run bash "$SCRIPT" + [ "$status" -eq 1 ] + [[ "$output" == *"::error::Claude invocation failed"* ]] + [[ "$output" == *"Error: model not found"* ]] +} + +@test "successful Claude call appends the analysis to the report" { + make_claude 0 "### 1. Executive Summary" + run bash "$SCRIPT" + [ "$status" -eq 0 ] + grep -q '## Convergence latency (deterministic)' pr_review_health_report.md + grep -q '### 1. Executive Summary' pr_review_health_report.md + run grep -c 'Log analysis skipped' pr_review_health_report.md + [ "$output" = "0" ] +} + +@test "a successful analysis that discusses rate limits is not treated as a refusal" { + make_claude 0 "| Engine rate limit (Claude or Copilot quota) | 0 | — |" + run bash "$SCRIPT" + [ "$status" -eq 0 ] + grep -q 'Engine rate limit' pr_review_health_report.md + [[ "$output" != *"::warning::"*"quota"* ]] +}