Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
4d4143f
fix(dev-lead): intent-based model dispatch and fix copilot write fall…
May 16, 2026
8d4e2cc
fix(dev-lead): engine-agnostic model dispatch and copilot write guard
May 16, 2026
6c63d8e
Merge branch 'main' into fix/issue-195-intent-dispatch-and-copilot
don-petry May 16, 2026
6789814
Merge branch 'main' into fix/issue-195-intent-dispatch-and-copilot
don-petry May 16, 2026
28374b4
fix(dev-lead): treat engine-not-installed (exit 127) as unavailable i…
May 17, 2026
40434c3
Merge branch 'main' into fix/issue-195-intent-dispatch-and-copilot
don-petry May 17, 2026
2612f50
Merge branch 'main' into fix/issue-195-intent-dispatch-and-copilot
don-petry May 17, 2026
43b7095
Merge branch 'main' into fix/issue-195-intent-dispatch-and-copilot
don-petry May 17, 2026
d43f88f
Merge branch 'main' into fix/issue-195-intent-dispatch-and-copilot
don-petry May 17, 2026
dc6c258
Merge branch 'main' into fix/issue-195-intent-dispatch-and-copilot
don-petry May 17, 2026
3b54da1
Merge branch 'main' into fix/issue-195-intent-dispatch-and-copilot
don-petry May 17, 2026
ec023ba
Merge branch 'main' into fix/issue-195-intent-dispatch-and-copilot
don-petry May 17, 2026
03e225f
Merge branch 'main' into fix/issue-195-intent-dispatch-and-copilot
don-petry May 17, 2026
e00479b
Merge branch 'main' into fix/issue-195-intent-dispatch-and-copilot
don-petry May 17, 2026
2b29c86
Merge branch 'main' into fix/issue-195-intent-dispatch-and-copilot
don-petry May 17, 2026
b16caed
Merge branch 'main' into fix/issue-195-intent-dispatch-and-copilot
don-petry May 17, 2026
1a99ba7
Merge branch 'main' into fix/issue-195-intent-dispatch-and-copilot
don-petry May 17, 2026
922fadf
Merge branch 'main' into fix/issue-195-intent-dispatch-and-copilot
May 18, 2026
0987378
Merge branch 'main' into fix/issue-195-intent-dispatch-and-copilot
don-petry May 18, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion scripts/dev-lead-fix-ci.sh
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ set -euo pipefail

source "$(dirname "$0")/engine.sh"

INTENT_TYPE="fix-ci"
PR_NUMBER="${PR_NUMBER:-}"
HEAD_SHA="${HEAD_SHA:-}"
CHECKS_JSON="${CHECKS_JSON:-[]}"
Expand Down Expand Up @@ -202,7 +203,7 @@ main() {
echo " [fix-ci] cycle $cycle/$MAX_CI_CYCLES"

local engine_rc=0
run_writer_with_fallback "$prompt_file" || engine_rc=$?
run_writer_with_fallback "$prompt_file" "$(model_for_intent "$INTENT_TYPE")" || engine_rc=$?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit 8d4e2cc. model_for_intent now returns a tier key ("triage"/"action"/"deep") instead of a concrete model string. run_writer_with_fallback calls _setup_engine_vars($engine) before each invocation so ENGINE_*_MODEL vars are correct for each provider in the fallback chain. run_writer resolves the tier against those vars, so Gemini and Copilot always receive their own compatible model IDs.


if [ "$engine_rc" -ne 0 ]; then
if [ "$engine_rc" -eq 2 ]; then
Expand Down
3 changes: 2 additions & 1 deletion scripts/dev-lead-fix-issue.sh
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ set -euo pipefail

source "$(dirname "$0")/engine.sh"

INTENT_TYPE="fix-issue"
ISSUE_NUMBER="${ISSUE_NUMBER:-}"
REPO="${REPO:-${GITHUB_REPOSITORY:-}}"
DEV_LEAD_DRY_RUN="${DEV_LEAD_DRY_RUN:-false}"
Expand Down Expand Up @@ -54,7 +55,7 @@ main() {
local pre_engine_sha
pre_engine_sha=$(git rev-parse HEAD)

if ! run_writer_with_fallback "$prompt_file"; then
if ! run_writer_with_fallback "$prompt_file" "$(model_for_intent "$INTENT_TYPE")"; then

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit 8d4e2cc. model_for_intent now returns a tier key ("triage"/"action"/"deep") instead of a concrete model string. run_writer_with_fallback calls _setup_engine_vars($engine) before each invocation so ENGINE_*_MODEL vars are correct for each provider in the fallback chain. run_writer resolves the tier against those vars, so Gemini and Copilot always receive their own compatible model IDs.

echo "::error::Engine failed to implement issue #${ISSUE_NUMBER}"
rm -f "$prompt_file"
exit 1
Expand Down
13 changes: 7 additions & 6 deletions scripts/dev-lead-fix-reviews.sh
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ fi

build_and_run() {
local template_name="$1"
local model="${2:-$ENGINE_ACTION_MODEL}"
local prompt_file="/tmp/dev-lead-${template_name}-prompt-$$.md"
# Export required vars then envsubst
envsubst < "${PROMPTS_DIR}/${template_name}.md" > "$prompt_file"
Expand All @@ -44,7 +45,7 @@ build_and_run() {
fi

local rc=0
run_writer_with_fallback "$prompt_file" || rc=$?
run_writer_with_fallback "$prompt_file" "$model" || rc=$?
rm -f "$prompt_file"
return "$rc"
Comment on lines 46 to 50

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit 8d4e2cc. model_for_intent now returns a tier key ("triage"/"action"/"deep") instead of a concrete model string. run_writer_with_fallback calls _setup_engine_vars($engine) before each invocation so ENGINE_*_MODEL vars are correct for each provider in the fallback chain. run_writer resolves the tier against those vars, so Gemini and Copilot always receive their own compatible model IDs.

}
Expand Down Expand Up @@ -257,7 +258,7 @@ case "$INTENT_TYPE" in
--jq '.data.repository.pullRequest.reviewThreads.nodes | map(select(.isResolved == false))' 2>/dev/null || echo "[]")
export OPEN_THREADS_JSON
rc=0
build_and_run "fix-reviews" || rc=$?
build_and_run "fix-reviews" "$(model_for_intent "fix-reviews")" || rc=$?
[ "$rc" -eq 2 ] && handle_rate_limit "fix-reviews"
if [ "$rc" -eq 0 ]; then
if commit_and_push "fix-reviews"; then
Expand All @@ -272,7 +273,7 @@ case "$INTENT_TYPE" in
export PR_NUMBER PR_URL="https://github.com/${REPO}/pull/${PR_NUMBER}"
export REPO ACTOR="${ACTOR:-}" COMMENT_BODY="${COMMENT_BODY:-}" HEAD_SHA
rc=0
build_and_run "fix-bot-comment" || rc=$?
build_and_run "fix-bot-comment" "$(model_for_intent "fix-bot-comment")" || rc=$?
[ "$rc" -eq 2 ] && handle_rate_limit "fix-bot-comment"
if [ "$rc" -eq 0 ]; then
if commit_and_push "fix-bot-comment"; then
Expand All @@ -288,7 +289,7 @@ case "$INTENT_TYPE" in
export PR_URL="https://github.com/${REPO}/pull/${PR_NUMBER}"
export REPO ACTOR="${ACTOR:-}" USER_INSTRUCTION="${USER_INSTRUCTION:-}" PR_DESCRIPTION="${PR_DESCRIPTION:-}"
rc=0
build_and_run "human" || rc=$?
build_and_run "human" "$(model_for_intent "human")" || rc=$?
[ "$rc" -eq 2 ] && handle_rate_limit "human"
if [ "$rc" -eq 0 ]; then
if commit_and_push "human"; then
Expand Down Expand Up @@ -317,7 +318,7 @@ case "$INTENT_TYPE" in
--jq '.data.repository.pullRequest.reviewThreads.nodes | map(select(.isResolved == false))' 2>/dev/null || echo "[]")
export OPEN_THREADS_JSON BASE_REF="${BASE_REF:-main}"
rc=0
build_and_run "human-pr" || rc=$?
build_and_run "human-pr" "$(model_for_intent "human-pr")" || rc=$?
[ "$rc" -eq 2 ] && handle_rate_limit "human-pr"
if [ "$rc" -eq 0 ]; then
if commit_and_push "human-pr"; then
Expand All @@ -344,7 +345,7 @@ case "$INTENT_TYPE" in
CONFLICTING_FILES=$(git merge-tree "$(git merge-base HEAD "origin/${BASE_REF}")" HEAD "origin/${BASE_REF}" 2>/dev/null | grep "^changed in both" | awk '{print $NF}' || true)
export CONFLICTING_FILES
rc=0
build_and_run "rebase" || rc=$?
build_and_run "rebase" "$(model_for_intent "rebase")" || rc=$?
[ "$rc" -eq 2 ] && handle_rate_limit "rebase"
if [ "$rc" -eq 0 ]; then
if commit_and_push "rebase"; then
Expand Down
138 changes: 89 additions & 49 deletions scripts/engine.sh
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,16 @@ set -euo pipefail
# Supports: claude, gemini, copilot
#
# Sourced by review-one-pr.sh — provides:
# run_triage <prompt_file> — no-tool tier (stdout capture)
# run_agentic <prompt_file> <model> — full-tool tier (stdout)
# run_duck <prompt_file> <model> — cross-engine adversarial (stdout)
# run_triage <prompt_file> — no-tool tier (review-one-pr.sh only)
# run_agentic <prompt_file> <model> — full-tool tier (review-one-pr.sh only)
# run_duck <prompt_file> <model> — adversarial cross-engine (review-one-pr.sh only)
# ENGINE_* env vars for model names and labels
# DUCK_ENGINE / DUCK_MODEL for rubber-duck cross-engine review
#
# Sourced by dev-lead scripts — provides:
# model_for_intent <intent> — select model tier by intent complexity
# run_writer <prompt_file> [model] — write-capable agent run
# run_writer_with_fallback <prompt_file> [model] — run with engine fallback

REVIEW_ENGINE="${REVIEW_ENGINE:-claude}"
export REVIEW_ENGINE
Expand All @@ -29,8 +34,13 @@ DUCK_TIMEOUT_SEC="${DUCK_TIMEOUT_SEC:-300}"
RETRY_MAX_ATTEMPTS="${RETRY_MAX_ATTEMPTS:-2}" # total attempts including first
RETRY_BASE_DELAY_SEC="${RETRY_BASE_DELAY_SEC:-5}"

set_engine_config() {
case "$REVIEW_ENGINE" in
# _setup_engine_vars <engine>
# Sets all ENGINE_* model vars for the given engine name.
# Called once at source time and again by run_writer_with_fallback when
# switching engines mid-fallback, so each invocation gets compatible model IDs.
_setup_engine_vars() {
local eng="${1:-$REVIEW_ENGINE}"
case "$eng" in
claude)
ENGINE_TRIAGE_MODEL="claude-haiku-4-5-20251001"
ENGINE_DEEP_MODEL="claude-sonnet-4-6"
Expand All @@ -44,13 +54,13 @@ set_engine_config() {
DUCK_MODEL="o4-mini"
;;
gemini)
ENGINE_TRIAGE_MODEL="auto"
ENGINE_DEEP_MODEL="auto"
ENGINE_AUDIT_MODEL="auto"
ENGINE_ACTION_MODEL="auto"
ENGINE_SINGLE_MODEL="auto"
ENGINE_LABEL="auto (Gemini 2.5/3 balanced)"
ENGINE_SINGLE_LABEL="single-reviewer mode: auto"
ENGINE_TRIAGE_MODEL="gemini-2.0-flash"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid hard-coding the expiring Gemini triage model

For REVIEW_ENGINE=gemini, triage paths now pass --model gemini-2.0-flash; the official Gemini deprecations page lists that model's shutdown date as June 1, 2026, and says shut down endpoints are no longer available (https://ai.google.dev/gemini-api/docs/deprecations). That means every Gemini-backed triage run, including human-pr/fix-bot-comment via the new intent mapping and review-one-pr.sh triage, will start failing shortly after this lands instead of using the previous CLI-selected auto model.

Useful? React with 👍 / 👎.

ENGINE_DEEP_MODEL="gemini-1.5-pro"
ENGINE_AUDIT_MODEL="gemini-1.5-pro"
ENGINE_ACTION_MODEL="gemini-1.5-pro"
ENGINE_SINGLE_MODEL="gemini-1.5-pro"
Comment on lines +58 to +61

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Use a supported Gemini write model

When REVIEW_ENGINE=gemini, or when Claude falls back to Gemini, write/deep/audit runs now pass --model gemini-1.5-pro. The current official Gemini model list no longer includes any 1.5 models and the deprecation page says shut down models have unavailable endpoints (https://ai.google.dev/gemini-api/docs/models, https://ai.google.dev/gemini-api/docs/deprecations), so these invocations will fail as deterministic model-not-found errors rather than providing the intended write-capable fallback. Use a currently supported Gemini model such as the documented 2.5/3 replacement tier instead.

Useful? React with 👍 / 👎.

ENGINE_LABEL="triage: gemini-2.0-flash → deep: gemini-1.5-pro + duck: sonnet 4.6 → audit: gemini-1.5-pro"
ENGINE_SINGLE_LABEL="single-reviewer mode: gemini-1.5-pro"
# Cross-engine rubber duck: use Claude for diversity
DUCK_ENGINE="claude"
DUCK_MODEL="claude-sonnet-4-6"
Expand All @@ -75,19 +85,17 @@ set_engine_config() {
DUCK_MODEL="claude-sonnet-4-6"
;;
*)
echo "::error::Unknown REVIEW_ENGINE='$REVIEW_ENGINE' (expected: claude, gemini, or copilot)"
exit 1
echo "::error::Unknown REVIEW_ENGINE='$eng' (expected: claude, gemini, or copilot)"
return 1
;;
esac

export ENGINE_TRIAGE_MODEL ENGINE_DEEP_MODEL ENGINE_AUDIT_MODEL
export ENGINE_ACTION_MODEL ENGINE_SINGLE_MODEL
export ENGINE_LABEL ENGINE_SINGLE_LABEL
export DUCK_ENGINE DUCK_MODEL
}

# Initial config
set_engine_config
_setup_engine_vars "$REVIEW_ENGINE" || exit 1
echo " engine: $REVIEW_ENGINE ($ENGINE_LABEL)"

# is_rate_limited <text>
Expand Down Expand Up @@ -449,25 +457,52 @@ parse_reset_time() {
printf '%s' "${iso:-}" > /tmp/dev-lead-rate-limit-reset
}

# run_writer <prompt_file> [model]
# model_for_intent <intent>
# Returns an engine-agnostic tier key: "triage", "action", or "deep".
# run_writer resolves the key to the correct model ID for the active engine
# via ENGINE_TRIAGE_MODEL / ENGINE_ACTION_MODEL / ENGINE_DEEP_MODEL, which are
# re-initialized per engine by run_writer_with_fallback so fallback engines
# always get a compatible model ID.
# triage — human-pr, fix-bot-comment: lightweight checks, no agentic writes needed
# action — fix-reviews, fix-ci, rebase: write operations on known-scope diffs
# deep — fix-issue, human: full agentic feature work
model_for_intent() {
case "${1:-}" in
human-pr|fix-bot-comment) echo "triage" ;;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Route write intents to an action-capable tier

For INTENT_TYPE=human-pr or fix-bot-comment, this mapping now passes triage into run_writer_with_fallback, which resolves to Haiku/Flash instead of the action model. Those handlers still run prompts that explicitly instruct the agent to use Edit/Write tools and commit fixes for review threads or bot findings, so these write-remediation paths are downgraded from the intended action tier and are more likely to miss or fail requested code changes on every human review or bot-comment event.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The triage-tier mapping for human-pr and fix-bot-comment is intentional per issue #195 (see the proposed model_for_intent table in the issue body). The reasoning:

  • human-pr: triggered on any human PR review. The most common case (the one that caused the 2026-05-16 incident) is a zero-open-threads PR where the agent confirms no action is needed and exits. Haiku handles that cheaply. When threads exist, haiku still has full Write/Edit tool access — it's a smaller model but capable of straightforward fixes.
  • fix-bot-comment: responds to a single bot comment (SonarQube, CodeRabbit). The scope is narrow and well-defined; haiku is effective for targeted single-file edits.

Both are intentionally the cheapest tier first. If production data shows haiku misses fixes that sonnet would catch, the mapping is a one-line change in model_for_intent. That tuning is better done after re-enabling the workflow than speculatively before. Noted as a follow-up in #206 acceptance criteria.

fix-reviews|fix-ci|rebase) echo "action" ;;
fix-issue|human) echo "deep" ;;
*) echo "action" ;;
esac
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved

# run_writer <prompt_file> [tier_or_model]
# Full write-access mode for applying code fixes.
# When DEV_LEAD_DRY_RUN=true: builds prompt but does NOT call engine; exits 0.
# Exit codes: 0=success, 1=non-retriable failure, 2=rate-limited
# tier_or_model: a tier key ("triage", "action", "deep") or a literal model ID.
# Tier keys are resolved to the current engine's model via ENGINE_*_MODEL vars,
# which run_writer_with_fallback keeps in sync when switching engines.
# When DEV_LEAD_DRY_RUN=true: logs the prompt size but does NOT call engine; exits 0.
# Exit codes: 0=success, 1=non-retriable failure, 2=rate-limited or unavailable
# On exit 2, writes parsed reset timestamp to /tmp/dev-lead-rate-limit-reset.
run_writer() {
local prompt_file="$1"
local model="${2:-$ENGINE_ACTION_MODEL}"
local tier_or_model="${2:-action}"

# Resolve tier key → current engine's model ID.
local model
case "$tier_or_model" in
triage) model="$ENGINE_TRIAGE_MODEL" ;;
deep) model="$ENGINE_DEEP_MODEL" ;;
action) model="$ENGINE_ACTION_MODEL" ;;
*) model="$tier_or_model" ;; # pass-through for literal model strings
esac

if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then
echo " [dry-run] run_writer: would invoke $REVIEW_ENGINE with prompt $(wc -l < "$prompt_file") lines"
echo " [dry-run] run_writer: would invoke $REVIEW_ENGINE ($model) with prompt $(wc -l < "$prompt_file") lines"
return 0
fi

# Capture stdout to a temp file so is_rate_limited can inspect it, while
# still streaming output to the caller. The old approach read from
# /tmp/dev-lead-writer-stderr which was never written (claude --print outputs
# to stdout, not stderr), so is_rate_limited never fired and fallback engines
# were never tried.
# still streaming output to the caller.
local _tmp rc=0
_tmp=$(mktemp)

Expand All @@ -487,15 +522,13 @@ run_writer() {
< "$prompt_file" | tee "$_tmp" || rc=${PIPESTATUS[0]}
;;
copilot)
# Copilot (gh copilot suggest) is text-only — falls back to Claude for write ops
echo "::warning::Copilot engine is text-only; falling back to Claude for write operations" >&2
local saved="$REVIEW_ENGINE"
REVIEW_ENGINE="claude" timeout "$ACTION_TIMEOUT_SEC" claude --print \
--model "$model" \
--permission-mode acceptEdits \
--allowed-tools "Bash,Read,Write,Edit,Grep,Glob" \
< "$prompt_file" | tee "$_tmp" || rc=${PIPESTATUS[0]}
REVIEW_ENGINE="$saved"
# GitHub Models REST API is text-only — no Write/Edit tool access.
# Returning exit 2 signals "write engine unavailable" so run_writer_with_fallback
# exhausts the chain and the retry cron re-triggers when a write-capable
# engine (claude, gemini) is available again.
rm -f "$_tmp"
echo "::notice::copilot engine is text-only; write operations unavailable — triggering retry" >&2
return 2
;;
Comment on lines 524 to 532

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The copilot engine implementation in run_writer is a functional regression. The previous code correctly identified that Copilot (via the GitHub Models API) is text-only and lacks tool-use capabilities (like Write or Edit), so it fell back to Claude to perform the actual file modifications. By removing this fallback and calling copilot_chat directly, the run_writer function will now "succeed" (exit 0) without actually applying any code changes. This will lead to confusing "no changes made by engine" messages in the calling scripts even when the model provides a valid fix in its text response.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit 8d4e2cc. The copilot case in run_writer now returns exit 2 immediately (with a ::notice:: message) rather than calling copilot_chat and returning 0. Exit 2 exhausts the fallback chain and causes run_writer_with_fallback to emit ::error::All engines rate-limited or unavailable — the calling scripts then post a status=rate-limited marker and the retry cron re-triggers when a write-capable engine (claude, gemini) is available again. No false 'no changes made' or 'applied' markers.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a good solution. Waiting for Claude to become available is not an acceptable solution. If we can't use Copilot with this architecture then we should disable it until we can design a valid approach.

Should we be using GitHub Agentic Workflows? https://github.github.com/gh-aw/

esac

Expand All @@ -509,36 +542,43 @@ run_writer() {
return "$rc"
}

# run_writer_with_fallback <prompt_file> [model]
# Tries primary engine, falls back through claude → gemini → copilot on rate-limit.
# Only rate-limit (exit 2) triggers fallback; other failures propagate immediately.
# run_writer_with_fallback <prompt_file> [tier_or_model]
# Tries primary engine, falls back through claude → gemini → copilot on rate-limit (exit 2).
# Re-initializes ENGINE_* vars for each engine so model IDs are always compatible.
# Exit 2 (rate-limited) and exit 127 (engine binary not installed) both trigger fallback.
# Other non-zero exits propagate immediately as real failures.
run_writer_with_fallback() {
local prompt_file="$1"
local tier_or_model="${2:-action}"
local engines=("$REVIEW_ENGINE")

for e in claude gemini copilot; do
[ "$e" != "$REVIEW_ENGINE" ] && engines+=("$e")
done

local orig_engine="$REVIEW_ENGINE"
for engine in "${engines[@]}"; do
local saved="$REVIEW_ENGINE"
# Re-initialize ENGINE_* vars for this engine so model tier resolution
# in run_writer returns a compatible model ID for the active provider.
export REVIEW_ENGINE="$engine"
# Re-evaluate model names for the new engine
set_engine_config
_setup_engine_vars "$engine"
local rc=0
# Don't pass 'model' argument; run_writer will use the updated $ENGINE_ACTION_MODEL
run_writer "$prompt_file" || rc=$?
export REVIEW_ENGINE="$saved"
# Restore original config for subsequent PRs in the same session
set_engine_config
[ "$rc" -eq 0 ] && return 0
if [ "$rc" -eq 2 ]; then
echo "::warning::$engine rate-limited, trying next engine" >&2
run_writer "$prompt_file" "$tier_or_model" || rc=$?
if [ "$rc" -eq 0 ]; then
export REVIEW_ENGINE="$orig_engine"; _setup_engine_vars "$orig_engine"
return 0
fi
if [ "$rc" -eq 2 ] || [ "$rc" -eq 127 ]; then

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Do not retry missing engine binaries as rate limits

In environments where a fallback CLI is not installed or the install step failed, timeout --help defines exit 127 as COMMAND cannot be found, but this branch treats it the same as a provider rate limit. For example, the default dev-lead workflow always installs Claude but only installs Gemini when Gemini is the primary engine, so a Claude rate limit followed by missing gemini falls through to Copilot and posts a status=rate-limited marker instead of surfacing the deterministic setup problem; the retry cron will then keep re-dispatching the same broken environment until Claude resets rather than fixing the unavailable fallback.

Useful? React with 👍 / 👎.

# exit 2: rate-limited or text-only engine unavailable for writes
# exit 127: engine binary not installed in this environment
echo "::warning::$engine unavailable (exit $rc), trying next engine" >&2
continue
fi
export REVIEW_ENGINE="$orig_engine"; _setup_engine_vars "$orig_engine"
return "$rc"
done

export REVIEW_ENGINE="$orig_engine"; _setup_engine_vars "$orig_engine"
echo "::error::All engines rate-limited or unavailable" >&2
return 2
}
43 changes: 35 additions & 8 deletions tests/dev-lead/unit/test_engine_fallback.bats
Original file line number Diff line number Diff line change
Expand Up @@ -92,16 +92,10 @@ STUBEOF
}

@test "fallback: all rate-limited → returns 2" {
# All engines exit 2
# claude exits 2 (rate-limited), gemini exits 2 (rate-limited),
# copilot returns 2 directly in run_writer (text-only, write ops unsupported).
_make_stub "claude" 2
_make_stub "gemini" 2
# copilot falls back to claude internally, so we only need claude and gemini
# Create a copilot stub that also fails with 2
cat > "$STUB_BIN_DIR/copilot" <<'STUBEOF'
#!/usr/bin/env bash
exit 2
STUBEOF
chmod +x "$STUB_BIN_DIR/copilot"
_source_engine "claude"

run run_writer_with_fallback "$TEST_PROMPT"
Expand All @@ -122,6 +116,39 @@ STUBEOF
[ "$status" -eq 1 ]
}

@test "fallback: engine not installed (exit 127) → tries next engine" {
# Simulates gemini not installed: exit 127 should trigger fallback, not abort
_make_stub "claude" 2
_make_stub "gemini" 127
_source_engine "claude"

run run_writer_with_fallback "$TEST_PROMPT"

# claude rate-limited (2), gemini not installed (127), copilot text-only (2) → all exhausted
[ "$status" -eq 2 ]
}

@test "fallback: engine not installed (127) → next engine succeeds" {
# claude rate-limited, gemini not installed, but the next engine after gemini succeeds
# In the actual fallback order: claude → gemini → copilot. Since copilot returns 2
# (text-only) in run_writer, we verify with a scenario where gemini is "not found"
# and a recording stub shows that claude was tried after gemini failed.
_make_stub "gemini" 2
Comment on lines +131 to +136

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Rename this test to match the asserted outcome.

The title/comment says “next engine succeeds,” but the case asserts status 2 (all engines exhausted). Please align the name/comments with the actual expectation to avoid misleading failures.

Proposed rename/comment cleanup
-@test "fallback: engine not installed (127) → next engine succeeds" {
-  # claude rate-limited, gemini not installed, but the next engine after gemini succeeds
-  # In the actual fallback order: claude → gemini → copilot. Since copilot returns 2
-  # (text-only) in run_writer, we verify with a scenario where gemini is "not found"
-  # and a recording stub shows that claude was tried after gemini failed.
+@test "fallback: engine not installed (127) → continues and eventually exhausts" {
+  # In fallback order gemini (primary) → claude → copilot, verify 127 is treated
+  # as unavailable/continue and claude is still attempted before exhaustion.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@test "fallback: engine not installed (127) → next engine succeeds" {
# claude rate-limited, gemini not installed, but the next engine after gemini succeeds
# In the actual fallback order: claude → gemini → copilot. Since copilot returns 2
# (text-only) in run_writer, we verify with a scenario where gemini is "not found"
# and a recording stub shows that claude was tried after gemini failed.
_make_stub "gemini" 2
`@test` "fallback: engine not installed (127) → continues and eventually exhausts" {
# In fallback order gemini (primary) → claude → copilot, verify 127 is treated
# as unavailable/continue and claude is still attempted before exhaustion.
_make_stub "gemini" 2
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/dev-lead/unit/test_engine_fallback.bats` around lines 131 - 136, Rename
the test title and update its comments to reflect that the scenario asserts
status 2 (all engines exhausted) instead of "next engine succeeds": change the
`@test` header string currently "fallback: engine not installed (127) → next
engine succeeds" to something like "fallback: engine not installed (127) → all
engines exhausted (2)" and adjust the inline comment lines describing the
fallback order so they no longer claim a succeeding next engine; keep the test
body (including the _make_stub "gemini" 2 call) unchanged.

local record_file
record_file="$(mktemp)"
_make_recording_stub "claude" 127 "$record_file"
_source_engine "gemini"

run run_writer_with_fallback "$TEST_PROMPT"

# gemini (primary) rate-limited, claude not installed (127 → treated as unavailable),
# copilot text-only (2) → all engines exhausted
[ "$status" -eq 2 ]
# claude was tried (even though it exited 127, it was reached)
[ -s "$record_file" ]
rm -f "$record_file"
}

@test "fallback: fallback engine order is claude → gemini → copilot" {
# Primary = gemini (exits 2), then fallback order should try claude first
_make_stub "gemini" 2
Expand Down
Loading
Loading