From 64c7acbd8228fe961739ce4a93a139ca76ebcfd0 Mon Sep 17 00:00:00 2001 From: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Date: Thu, 2 Jul 2026 16:29:24 +0000 Subject: [PATCH 1/2] =?UTF-8?q?feat:=20implement=20issue=20#947=20?= =?UTF-8?q?=E2=80=94=20P1=20(#860=20follow-up):=20don't=20re-invoke=20rate?= =?UTF-8?q?-limited=20advisory=20engines=20=E2=80=94=20one=20exhaustion=20?= =?UTF-8?q?notice=20per=20engine=20per=20run?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- scripts/engine.sh | 59 +++++++++++- tests/dev-lead/unit/test_engine_fallback.bats | 91 +++++++++++++++++++ 2 files changed, 148 insertions(+), 2 deletions(-) diff --git a/scripts/engine.sh b/scripts/engine.sh index f24a9a67c..395024f3c 100644 --- a/scripts/engine.sh +++ b/scripts/engine.sh @@ -1255,12 +1255,48 @@ run_writer() { return "$rc" } +# ── Run-scoped engine-exhaustion registry (issue #947) ──────────────────────── +# Once an engine is found rate-limited / unavailable during this run (process), +# it is recorded here so later run_writer_with_fallback calls in the SAME run do +# NOT re-invoke it. The #860 runaway re-walked claude → copilot → gemini on every +# dev-lead-fix-ci cycle, re-burning quota on an already-exhausted engine and +# re-posting the same usage-limit notice. Keyed by engine name → reason +# (rate-limited | missing-binary). Guarded so a re-source of engine.sh in the +# same process (e.g. review-batch's copilot pre-flight) does not wipe live state. +if ! declare -p _ENGINE_EXHAUSTED_REASON >/dev/null 2>&1; then + declare -gA _ENGINE_EXHAUSTED_REASON +fi + +# reset_engine_exhaustion — clear the registry. Exhaustion is intentionally +# run-scoped, so production code never calls this; it exists for test isolation. +reset_engine_exhaustion() { + _ENGINE_EXHAUSTED_REASON=() +} + +# _engine_is_exhausted — 0 if already marked exhausted this run. +_engine_is_exhausted() { + [ -n "${_ENGINE_EXHAUSTED_REASON[$1]:-}" ] +} + +# _mark_engine_exhausted +# Record the engine as exhausted for the rest of the run and emit EXACTLY ONE +# notice (at most one notice per engine per run). No-op on repeat marks so a +# multi-cycle caller cannot produce a notice storm. +_mark_engine_exhausted() { + local engine="$1" reason="${2:-rate-limited}" + [ -n "${_ENGINE_EXHAUSTED_REASON[$engine]:-}" ] && return 0 + _ENGINE_EXHAUSTED_REASON[$engine]="$reason" + echo "::warning::[engine-exhaustion] ${engine} marked exhausted for this run (${reason}) — it will not be re-invoked" >&2 +} + # run_writer_with_fallback [intent_type] # Tries primary engine, falls back through claude → copilot → gemini on rate-limit. # intent_type is passed to model_for_intent() so each engine uses the appropriate # tier model for the given task complexity (e.g. haiku for triage, sonnet for writes). # Only rate-limit (exit 2) and missing-binary (exit 127) trigger fallback; # other failures propagate immediately. +# An engine found rate-limited/unavailable in an earlier call this run is skipped +# (not re-invoked) — see the exhaustion registry above (#947). run_writer_with_fallback() { local prompt_file="$1" local intent="${2:-}" @@ -1290,9 +1326,23 @@ run_writer_with_fallback() { continue fi + # Run-scoped exhaustion (#947): an engine already found rate-limited/ + # unavailable earlier in this run is not re-invoked. Reflect its recorded + # reason into the aggregate so the final return code is unchanged, and emit + # no repeat notice (the one-time notice fired when it was first marked). + if _engine_is_exhausted "$engine"; then + case "${_ENGINE_EXHAUSTED_REASON[$engine]}" in + missing-binary) any_missing=1 ;; + *) any_rate_limited=1 ;; + esac + echo " [engine] $engine already exhausted this run — skipping (not re-invoked)" >&2 + continue + fi + if ! check_provider_headroom "$engine"; then echo "::warning::$engine at/above usage threshold — trying next engine" >&2 any_rate_limited=1 + _mark_engine_exhausted "$engine" "rate-limited" continue fi @@ -1316,8 +1366,13 @@ run_writer_with_fallback() { # binary rather than a retryable quota error so that infra failures # surface loudly instead of being masked as rate-limit retries). echo "::warning::$engine unavailable (exit $rc), trying next engine" >&2 - [ "$rc" -eq 2 ] && any_rate_limited=1 - [ "$rc" -eq 127 ] && any_missing=1 + if [ "$rc" -eq 2 ]; then + any_rate_limited=1 + _mark_engine_exhausted "$engine" "rate-limited" + else + any_missing=1 + _mark_engine_exhausted "$engine" "missing-binary" + fi continue fi # A non-fallback engine error (e.g. 124=timeout kill, 137/143=signal, diff --git a/tests/dev-lead/unit/test_engine_fallback.bats b/tests/dev-lead/unit/test_engine_fallback.bats index a5aa735ea..0205b013a 100644 --- a/tests/dev-lead/unit/test_engine_fallback.bats +++ b/tests/dev-lead/unit/test_engine_fallback.bats @@ -335,3 +335,94 @@ STUB rm -f "$GITHUB_STEP_SUMMARY" } + +# ── run-scoped engine exhaustion (#947) ─────────────────────────────────────── +# Once an engine is found rate-limited/unavailable during a run, it must NOT be +# re-invoked on later run_writer_with_fallback calls in the same run (the #860 +# fix-ci-cycle re-burn), and it must emit AT MOST ONE exhaustion notice per +# engine per run. These tests call the function twice+ in the SAME shell (no +# bats `run`, which forks a subshell) so the process-scoped registry persists. + +@test "exhaustion: rate-limited engine is not re-invoked on a later call in the same run" { + local claude_record gemini_record + claude_record="$(mktemp)" + gemini_record="$(mktemp)" + _make_recording_stub "claude" 2 "$claude_record" # rate-limited (exit 2) + _make_recording_stub "gemini" 0 "$gemini_record" # succeeds + export GEMINI_API_KEY="test-key" + export COPILOT_GITHUB_TOKEN="ghp_stub" # ghp_* → copilot skipped + _source_engine "claude" + + local rc1=0 rc2=0 + run_writer_with_fallback "$TEST_PROMPT" || rc1=$? + run_writer_with_fallback "$TEST_PROMPT" || rc2=$? + + [ "$rc1" -eq 0 ] + [ "$rc2" -eq 0 ] + # claude invoked once (call 1), then skipped as exhausted on call 2 + [ "$(wc -l < "$claude_record")" -eq 1 ] + # gemini served both calls + [ "$(wc -l < "$gemini_record")" -eq 2 ] + rm -f "$claude_record" "$gemini_record" +} + +@test "exhaustion: at most one exhaustion notice per engine across calls in a run" { + _make_stub "claude" 2 + _make_stub "gemini" 0 + export GEMINI_API_KEY="test-key" + export COPILOT_GITHUB_TOKEN="ghp_stub" + _source_engine "claude" + + local errlog + errlog="$(mktemp)" + run_writer_with_fallback "$TEST_PROMPT" 2>>"$errlog" || true + run_writer_with_fallback "$TEST_PROMPT" 2>>"$errlog" || true + run_writer_with_fallback "$TEST_PROMPT" 2>>"$errlog" || true + + # Exactly one "marked exhausted" notice for claude across all three calls. + local n + n=$(grep -c 'claude marked exhausted' "$errlog" || true) + [ "$n" -eq 1 ] + rm -f "$errlog" +} + +@test "exhaustion: all-engines-exhausted second call still returns 2 (rate-limited)" { + _make_stub "claude" 2 + _make_stub "gemini" 2 + export GEMINI_API_KEY="test-key" + export COPILOT_GITHUB_TOKEN="stub-token" + cat > "$STUB_BIN_DIR/gh" <<'GHEOF' +#!/usr/bin/env bash +case "$*" in + *"copilot"*) echo "rate limit exceeded"; exit 1 ;; + *) echo "{}" ;; +esac +GHEOF + chmod +x "$STUB_BIN_DIR/gh" + _source_engine "claude" + + local rc1=0 rc2=0 + run_writer_with_fallback "$TEST_PROMPT" || rc1=$? + run_writer_with_fallback "$TEST_PROMPT" || rc2=$? + + [ "$rc1" -eq 2 ] + [ "$rc2" -eq 2 ] + [ "$(cat /tmp/dev-lead-failure-reason)" = "rate-limited" ] +} + +@test "exhaustion: reset_engine_exhaustion clears the registry" { + local claude_record + claude_record="$(mktemp)" + _make_recording_stub "claude" 2 "$claude_record" + _make_stub "gemini" 0 + export GEMINI_API_KEY="test-key" + export COPILOT_GITHUB_TOKEN="ghp_stub" + _source_engine "claude" + + run_writer_with_fallback "$TEST_PROMPT" || true # claude recorded (1), exhausted + reset_engine_exhaustion + run_writer_with_fallback "$TEST_PROMPT" || true # claude re-invoked after reset + + [ "$(wc -l < "$claude_record")" -eq 2 ] + rm -f "$claude_record" +} From b311ee4ccb0a042c2a4507e2334d8d8581ea4675 Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Thu, 2 Jul 2026 19:21:11 +0000 Subject: [PATCH 2/2] fix(reviews): address review comments [skip ci-relay] --- scripts/engine.sh | 11 +++--- tests/dev-lead/unit/test_engine_fallback.bats | 34 +++++++++++++++---- 2 files changed, 33 insertions(+), 12 deletions(-) diff --git a/scripts/engine.sh b/scripts/engine.sh index 0902e3f95..4d0ccf28a 100644 --- a/scripts/engine.sh +++ b/scripts/engine.sh @@ -1275,7 +1275,7 @@ reset_engine_exhaustion() { # _engine_is_exhausted — 0 if already marked exhausted this run. _engine_is_exhausted() { - [ -n "${_ENGINE_EXHAUSTED_REASON[$1]:-}" ] + [ -n "${1:-}" ] && [ -n "${_ENGINE_EXHAUSTED_REASON["$1"]:-}" ] } # _mark_engine_exhausted @@ -1283,9 +1283,10 @@ _engine_is_exhausted() { # notice (at most one notice per engine per run). No-op on repeat marks so a # multi-cycle caller cannot produce a notice storm. _mark_engine_exhausted() { - local engine="$1" reason="${2:-rate-limited}" - [ -n "${_ENGINE_EXHAUSTED_REASON[$engine]:-}" ] && return 0 - _ENGINE_EXHAUSTED_REASON[$engine]="$reason" + local engine="${1:-}" reason="${2:-rate-limited}" + [ -z "$engine" ] && return 0 + [ -n "${_ENGINE_EXHAUSTED_REASON["$engine"]:-}" ] && return 0 + _ENGINE_EXHAUSTED_REASON["$engine"]="$reason" echo "::warning::[engine-exhaustion] ${engine} marked exhausted for this run (${reason}) — it will not be re-invoked" >&2 } @@ -1337,7 +1338,7 @@ run_writer_with_fallback() { # reason into the aggregate so the final return code is unchanged, and emit # no repeat notice (the one-time notice fired when it was first marked). if _engine_is_exhausted "$engine"; then - case "${_ENGINE_EXHAUSTED_REASON[$engine]}" in + case "${_ENGINE_EXHAUSTED_REASON["$engine"]}" in missing-binary) any_missing=1 ;; *) any_rate_limited=1 ;; esac diff --git a/tests/dev-lead/unit/test_engine_fallback.bats b/tests/dev-lead/unit/test_engine_fallback.bats index 72d0f8fe8..804d66bc3 100644 --- a/tests/dev-lead/unit/test_engine_fallback.bats +++ b/tests/dev-lead/unit/test_engine_fallback.bats @@ -397,8 +397,8 @@ STUB @test "exhaustion: rate-limited engine is not re-invoked on a later call in the same run" { local claude_record gemini_record - claude_record="$(mktemp)" - gemini_record="$(mktemp)" + claude_record="$(mktemp "$STUB_BIN_DIR/claude_record.XXXXXX")" + gemini_record="$(mktemp "$STUB_BIN_DIR/gemini_record.XXXXXX")" _make_recording_stub "claude" 2 "$claude_record" # rate-limited (exit 2) _make_recording_stub "gemini" 0 "$gemini_record" # succeeds export GEMINI_API_KEY="test-key" @@ -415,7 +415,6 @@ STUB [ "$(wc -l < "$claude_record")" -eq 1 ] # gemini served both calls [ "$(wc -l < "$gemini_record")" -eq 2 ] - rm -f "$claude_record" "$gemini_record" } @test "exhaustion: at most one exhaustion notice per engine across calls in a run" { @@ -426,7 +425,7 @@ STUB _source_engine "claude" local errlog - errlog="$(mktemp)" + errlog="$(mktemp "$STUB_BIN_DIR/errlog.XXXXXX")" run_writer_with_fallback "$TEST_PROMPT" 2>>"$errlog" || true run_writer_with_fallback "$TEST_PROMPT" 2>>"$errlog" || true run_writer_with_fallback "$TEST_PROMPT" 2>>"$errlog" || true @@ -435,7 +434,6 @@ STUB local n n=$(grep -c 'claude marked exhausted' "$errlog" || true) [ "$n" -eq 1 ] - rm -f "$errlog" } @test "exhaustion: all-engines-exhausted second call still returns 2 (rate-limited)" { @@ -464,7 +462,7 @@ GHEOF @test "exhaustion: reset_engine_exhaustion clears the registry" { local claude_record - claude_record="$(mktemp)" + claude_record="$(mktemp "$STUB_BIN_DIR/claude_record.XXXXXX")" _make_recording_stub "claude" 2 "$claude_record" _make_stub "gemini" 0 export GEMINI_API_KEY="test-key" @@ -476,5 +474,27 @@ GHEOF run_writer_with_fallback "$TEST_PROMPT" || true # claude re-invoked after reset [ "$(wc -l < "$claude_record")" -eq 2 ] - rm -f "$claude_record" +} + +@test "exhaustion: headroom-threshold breach exhausts engine permanently" { + _make_stub "gemini" 0 + export GEMINI_API_KEY="test-key" + export COPILOT_GITHUB_TOKEN="ghp_stub" + _source_engine "claude" + # Override headroom check: claude is always at/above threshold; others have headroom + check_provider_headroom() { [ "$1" = "claude" ] && return 1; return 0; } + + local errlog + errlog="$(mktemp "$STUB_BIN_DIR/errlog.XXXXXX")" + local rc1=0 rc2=0 + run_writer_with_fallback "$TEST_PROMPT" 2>>"$errlog" || rc1=$? + run_writer_with_fallback "$TEST_PROMPT" 2>>"$errlog" || rc2=$? + + # gemini serves both calls despite claude's headroom failure + [ "$rc1" -eq 0 ] + [ "$rc2" -eq 0 ] + # Exactly one exhaustion notice for claude across both calls + local n + n=$(grep -c 'claude marked exhausted' "$errlog" || true) + [ "$n" -eq 1 ] }