Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
37 changes: 21 additions & 16 deletions scripts/review-batch.sh
Original file line number Diff line number Diff line change
Expand Up @@ -21,28 +21,28 @@ PRS_FILE="${PRS_FILE:-prs.txt}"
MAX_PRS="${MAX_PRS:-10}"
CANDIDATE_LIMIT="${CANDIDATE_LIMIT:-100}"

# Pre-flight: validate engine availability before touching any PR.
# Sets CLAUDE_AVAILABLE, GEMINI_AVAILABLE, COPILOT_AVAILABLE and emits
# ::warning:: annotations + job-summary table for any unavailable engine.
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
# shellcheck source=validate-engines.sh
source "$SCRIPT_DIR/validate-engines.sh"
validate_engines

# ---------------------------------------------------------------------------
# Pre-flight smoke test for the Copilot engine.
# Copilot REST API smoke test (only when Copilot is the primary engine).
#
# gh copilot suggest is a shell-command suggestion tool that does not accept
# large prompts or the -p flag in modern gh CLI versions (see issue #147).
# We now use the GitHub Models REST API directly. This pre-flight step verifies
# API connectivity and auth BEFORE reviewing any PRs, so format/auth errors
# surface as a clear setup failure rather than being mis-classified as
# rate-limit hits mid-run (which caused session aborts skipping many PRs).
# Verifies GitHub Models API connectivity and auth BEFORE reviewing any PRs,
# so format/auth errors surface as a clear setup failure rather than being
# mis-classified as rate-limit hits mid-run.
# ---------------------------------------------------------------------------
if [ "${REVIEW_ENGINE:-claude}" = "copilot" ]; then
SCRIPT_DIR_BATCH="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
echo "==> Copilot engine pre-flight: testing GitHub Models API"

# Determine the configured API model (set by engine.sh; fall back to a
# known-available model for the connectivity check itself).
# shellcheck source=engine.sh
_smoke_model="${COPILOT_API_MODEL:-}"
if [ -z "$_smoke_model" ]; then
# Source engine.sh to get COPILOT_API_MODEL without running any reviews.
# Redirect stdout to discard the " engine: ..." echo from engine.sh.
source "$SCRIPT_DIR_BATCH/engine.sh" > /dev/null || {
# shellcheck source=engine.sh
source "$SCRIPT_DIR/engine.sh" > /dev/null || {
echo "::error::Copilot pre-flight: failed to source engine.sh — check REVIEW_ENGINE and file path" >&2
exit 1
}
Expand Down Expand Up @@ -137,15 +137,20 @@ while IFS= read -r pr_url; do
# Exit code 2 = engine rate-limited.
# Fallback chain: claude -> gemini -> copilot
if [ "$rc" -eq 2 ] && [ "${REVIEW_ENGINE:-claude}" = "claude" ]; then
if command -v gemini >/dev/null 2>&1 && [ -n "${GOOGLE_API_KEY:-}" ]; then
# Use the availability flag set by validate_engines() at startup.
if [ "${GEMINI_AVAILABLE:-false}" = "true" ]; then
echo "::warning::Claude rate limit hit — switching to Gemini engine for remaining PRs"
export REVIEW_ENGINE=gemini
engine_fallbacks=$((engine_fallbacks + 1))
fallback_engines="${fallback_engines:+$fallback_engines, }gemini"
rc=0
bash scripts/review-one-pr.sh "$pr_url" || rc=$?
else
echo "::warning::Claude rate limit hit but Gemini fallback unavailable (CLI not installed or GOOGLE_API_KEY missing) — falling through to Copilot"
# Derive the specific reason so the warning is actionable without docs.
_gemini_miss=""
command -v gemini >/dev/null 2>&1 || _gemini_miss="Gemini CLI not installed (fix: npm install -g @google/gemini-cli)"
[ -n "${GOOGLE_API_KEY:-}" ] || _gemini_miss="${_gemini_miss:+$_gemini_miss; }GOOGLE_API_KEY secret not set"
echo "::warning::Claude rate limit hit but Gemini fallback unavailable (${_gemini_miss}) — falling through to Copilot"
rc=2
export REVIEW_ENGINE=gemini # Set to gemini so the next block catches it
fi
Expand Down
87 changes: 87 additions & 0 deletions scripts/validate-engines.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
#!/usr/bin/env bash
# validate-engines.sh — Pre-flight availability check for review engines.
#
# Provides:
# validate_engines() — checks Claude, Gemini, and Copilot availability
#
# After validate_engines() returns the following vars are exported:
# CLAUDE_AVAILABLE — "true" if claude CLI + CLAUDE_CODE_OAUTH_TOKEN are present
# GEMINI_AVAILABLE — "true" if gemini CLI + GOOGLE_API_KEY are present
# COPILOT_AVAILABLE — "true" if gh copilot is usable with COPILOT_GITHUB_TOKEN
#
# For each unavailable fallback engine a ::warning:: annotation is emitted that
# includes the exact command an operator needs to fix the gap — so the log is
# self-contained and there is no silent skip.
#
# If GITHUB_STEP_SUMMARY is set (normal in GitHub Actions) an engine-availability
# table is appended to the job summary so the degraded state is always visible
# in the run UI without needing to dig into logs.
#
# Always exits 0. Degraded state is recorded but never aborts the run.

validate_engines() {
local claude_ok=false gemini_ok=false copilot_ok=false

# ── Claude ──────────────────────────────────────────────────────────────────
if command -v claude >/dev/null 2>&1 && [ -n "${CLAUDE_CODE_OAUTH_TOKEN:-}" ]; then
claude_ok=true
fi

# ── Gemini ──────────────────────────────────────────────────────────────────
# Collect every reason the engine is unavailable so the warning is precise.
local gemini_reasons=""
if ! command -v gemini >/dev/null 2>&1; then
gemini_reasons="Gemini CLI not installed (fix: npm install -g @google/gemini-cli)"
fi
if [ -z "${GOOGLE_API_KEY:-}" ]; then
if [ -n "$gemini_reasons" ]; then
gemini_reasons="$gemini_reasons; GOOGLE_API_KEY secret not set"
else
gemini_reasons="GOOGLE_API_KEY secret not set"
fi
fi
Comment on lines +32 to +42

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The logic for building the gemini_reasons string can be simplified using the shell-idiomatic ${var:+$var; } pattern. This is more concise and matches the style used in scripts/review-batch.sh.

Suggested change
local gemini_reasons=""
if ! command -v gemini >/dev/null 2>&1; then
gemini_reasons="Gemini CLI not installed (fix: npm install -g @google/gemini-cli)"
fi
if [ -z "${GOOGLE_API_KEY:-}" ]; then
if [ -n "$gemini_reasons" ]; then
gemini_reasons="$gemini_reasons; GOOGLE_API_KEY secret not set"
else
gemini_reasons="GOOGLE_API_KEY secret not set"
fi
fi
local gemini_reasons=""
command -v gemini >/dev/null 2>&1 || gemini_reasons="Gemini CLI not installed (fix: npm install -g @google/gemini-cli)"
if [ -z "${GOOGLE_API_KEY:-}" ]; then
gemini_reasons="${gemini_reasons:+$gemini_reasons; }GOOGLE_API_KEY secret not set"
fi
References
  1. Use shell-idiomatic variable expansion for concise string concatenation. (link)


if [ -z "$gemini_reasons" ]; then
gemini_ok=true
else
echo "::warning::Gemini fallback unavailable — ${gemini_reasons}. When Claude is rate-limited, runs will fall through directly to Copilot."
fi

# ── Copilot ─────────────────────────────────────────────────────────────────
# gh copilot is now a built-in; auth via COPILOT_GITHUB_TOKEN (user PAT with
# Copilot subscription). Fall back to GH_TOKEN for non-production test paths.
if env GH_TOKEN="${COPILOT_GITHUB_TOKEN:-${GH_TOKEN:-}}" \
gh copilot --version >/dev/null 2>&1; then
copilot_ok=true
Comment on lines +53 to +55
Comment on lines +53 to +55
fi
Comment on lines +53 to +56

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The PR description and file header mention that warnings are emitted for any unavailable fallback engine, but currently, only Gemini has a warning. Adding a warning for Copilot would ensure that configuration issues for all fallback engines are visible in the logs. This supports the engine fallback mechanism by ensuring fallbacks are correctly configured before rate limits are encountered.

Suggested change
if env GH_TOKEN="${COPILOT_GITHUB_TOKEN:-${GH_TOKEN:-}}" \
gh copilot --version >/dev/null 2>&1; then
copilot_ok=true
fi
if env GH_TOKEN="${COPILOT_GITHUB_TOKEN:-${GH_TOKEN:-}}" \
gh copilot --version >/dev/null 2>&1; then
copilot_ok=true
else
echo "::warning::Copilot fallback unavailable. When Gemini is rate-limited, runs will abort."
fi
References
  1. Ensure engine fallback mechanisms are supported by validating and reporting the status of fallback engines.


export CLAUDE_AVAILABLE="$claude_ok"
export GEMINI_AVAILABLE="$gemini_ok"
export COPILOT_AVAILABLE="$copilot_ok"

_emit_engine_summary "$claude_ok" "$gemini_ok" "$copilot_ok"
}

# _engine_badge <bool> → "ok" or "unavailable"
_engine_badge() {
[ "$1" = "true" ] && printf 'ok' || printf 'unavailable'
}

# _emit_engine_summary <claude_ok> <gemini_ok> <copilot_ok>
# Appends an engine-availability Markdown table to GITHUB_STEP_SUMMARY.
# No-ops when GITHUB_STEP_SUMMARY is unset (local runs, unit tests without
# a summary file).
_emit_engine_summary() {
local claude_ok="$1" gemini_ok="$2" copilot_ok="$3"
local dest="${GITHUB_STEP_SUMMARY:-}"
[ -z "$dest" ] && return 0
{
printf '### Engine availability (pre-flight)\n\n'
printf '| Engine | Status |\n'
printf '|---------|--------|\n'
printf '| Claude | %s |\n' "$(_engine_badge "$claude_ok")"
printf '| Gemini | %s |\n' "$(_engine_badge "$gemini_ok")"
printf '| Copilot | %s |\n' "$(_engine_badge "$copilot_ok")"
printf '\n'
} >> "$dest"
}
Comment on lines +74 to +87
194 changes: 194 additions & 0 deletions tests/test-validate-engines.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,194 @@
#!/usr/bin/env bash
# Unit tests for scripts/validate-engines.sh
#
# Tests validate_engines() by injecting mock binaries via PATH manipulation
# and controlling env vars inside subshells.
#
# Run: bash tests/test-validate-engines.sh
# Exit: 0 if all tests pass, 1 if any fail.

set -euo pipefail

REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
PASS=0
FAIL=0

# ── Helpers ──────────────────────────────────────────────────────────────────

assert_eq() {
local desc="$1" expected="$2" actual="$3"
if [ "$expected" = "$actual" ]; then
printf 'PASS: %s\n' "$desc"
PASS=$((PASS + 1))
else
printf 'FAIL: %s\n expected: %s\n actual: %s\n' "$desc" "$expected" "$actual"
FAIL=$((FAIL + 1))
fi
}

assert_contains() {
local desc="$1" needle="$2" haystack="$3"
if printf '%s' "$haystack" | grep -qF "$needle"; then
printf 'PASS: %s\n' "$desc"
PASS=$((PASS + 1))
else
printf 'FAIL: %s\n expected to contain: %s\n actual: %s\n' "$desc" "$needle" "$haystack"
FAIL=$((FAIL + 1))
fi
}

# run_validate <extra_path> [VAR=value ...]
# Runs validate_engines() in a clean subshell with:
# - PATH = <extra_path>:$MOCK_DIR:<system-bins>
# - Only the env vars listed as arguments (plus GITHUB_STEP_SUMMARY if given)
# Prints validate_engines stdout/stderr followed by three lines:
# CLAUDE_AVAILABLE=<value>
# GEMINI_AVAILABLE=<value>
# COPILOT_AVAILABLE=<value>
run_validate() {
local extra_path="${1:-}"; shift
local combined_path="${extra_path:+$extra_path:}$MOCK_DIR:/usr/local/bin:/usr/bin:/bin"
(
Comment on lines +49 to +51
# Isolate from the caller's secrets
unset GOOGLE_API_KEY CLAUDE_CODE_OAUTH_TOKEN COPILOT_GITHUB_TOKEN GH_TOKEN \
GITHUB_STEP_SUMMARY 2>/dev/null || true
export PATH="$combined_path"
# Apply caller-supplied overrides
for _kv in "$@"; do
# export "KEY=value" — bash handles the assignment form correctly.
export "$_kv"
done
# shellcheck source=../scripts/validate-engines.sh
source "$REPO_ROOT/scripts/validate-engines.sh"
validate_engines 2>&1
printf 'CLAUDE_AVAILABLE=%s\n' "$CLAUDE_AVAILABLE"
printf 'GEMINI_AVAILABLE=%s\n' "$GEMINI_AVAILABLE"
printf 'COPILOT_AVAILABLE=%s\n' "$COPILOT_AVAILABLE"
)
}

# ── Mock binary setup ─────────────────────────────────────────────────────────

MOCK_DIR=$(mktemp -d)
MOCK_NO_GEMINI_DIR=$(mktemp -d)
trap 'rm -rf "$MOCK_DIR" "$MOCK_NO_GEMINI_DIR"' EXIT

# gemini mock (succeeds)
printf '#!/usr/bin/env bash\nexit 0\n' > "$MOCK_DIR/gemini"
chmod +x "$MOCK_DIR/gemini"

# claude mock (succeeds)
printf '#!/usr/bin/env bash\nexit 0\n' > "$MOCK_DIR/claude"
chmod +x "$MOCK_DIR/claude"

# gh mock — handles 'gh copilot --version' and falls back to real gh for others
cat > "$MOCK_DIR/gh" << 'MOCK_GH'
#!/usr/bin/env bash
if [ "${1:-}" = "copilot" ] && [ "${2:-}" = "--version" ]; then
echo "GitHub Copilot extension 1.0.0"
exit 0
fi
# Fall through to real gh for non-copilot subcommands (list-prs etc. in live runs)
_real_gh="$(command -v gh 2>/dev/null || true)"
if [ -n "$_real_gh" ] && [ "$_real_gh" != "$0" ]; then
exec "$_real_gh" "$@"
fi
exit 1
MOCK_GH
chmod +x "$MOCK_DIR/gh"

# MOCK_NO_GEMINI_DIR: has claude and gh but no gemini
cp "$MOCK_DIR/claude" "$MOCK_NO_GEMINI_DIR/"
cp "$MOCK_DIR/gh" "$MOCK_NO_GEMINI_DIR/"

# ── Tests ─────────────────────────────────────────────────────────────────────
echo "Running validate_engines() unit tests..."
echo ""

# 1. GEMINI_AVAILABLE=false when GOOGLE_API_KEY is unset (CLI is present)
out=$(run_validate "$MOCK_DIR" \
"GOOGLE_API_KEY=" \
"CLAUDE_CODE_OAUTH_TOKEN=fake-token" \
"COPILOT_GITHUB_TOKEN=" \
"GH_TOKEN=")
assert_eq "GEMINI_AVAILABLE=false when GOOGLE_API_KEY unset" \
"GEMINI_AVAILABLE=false" "$(printf '%s\n' "$out" | grep '^GEMINI_AVAILABLE=')"

# 2. GEMINI_AVAILABLE=false when gemini CLI is absent (key is set)
out=$(run_validate "$MOCK_NO_GEMINI_DIR" \
"GOOGLE_API_KEY=fake-key" \
"CLAUDE_CODE_OAUTH_TOKEN=fake-token" \
"COPILOT_GITHUB_TOKEN=" \
"GH_TOKEN=")
assert_eq "GEMINI_AVAILABLE=false when CLI absent" \
"GEMINI_AVAILABLE=false" "$(printf '%s\n' "$out" | grep '^GEMINI_AVAILABLE=')"

# 3. GEMINI_AVAILABLE=true when CLI is present and GOOGLE_API_KEY is set
out=$(run_validate "$MOCK_DIR" \
"GOOGLE_API_KEY=fake-key" \
"CLAUDE_CODE_OAUTH_TOKEN=fake-token" \
"COPILOT_GITHUB_TOKEN=" \
"GH_TOKEN=")
assert_eq "GEMINI_AVAILABLE=true when CLI present and key set" \
"GEMINI_AVAILABLE=true" "$(printf '%s\n' "$out" | grep '^GEMINI_AVAILABLE=')"

# 4. Warning includes install command when CLI is missing
out=$(run_validate "$MOCK_NO_GEMINI_DIR" \
"GOOGLE_API_KEY=fake-key" \
"CLAUDE_CODE_OAUTH_TOKEN=" \
"COPILOT_GITHUB_TOKEN=" \
"GH_TOKEN=")
assert_contains "Warning includes install command when CLI absent" \
"npm install -g @google/gemini-cli" "$out"

# 5. Warning emitted (::warning:: annotation) when GOOGLE_API_KEY unset
out=$(run_validate "$MOCK_DIR" \
"GOOGLE_API_KEY=" \
"CLAUDE_CODE_OAUTH_TOKEN=" \
"COPILOT_GITHUB_TOKEN=" \
"GH_TOKEN=")
assert_contains "::warning:: annotation emitted when key absent" \
"::warning::" "$out"

# 6. No ::warning:: annotation for Gemini when both CLI and key are present
out=$(run_validate "$MOCK_DIR" \
"GOOGLE_API_KEY=fake-key" \
"CLAUDE_CODE_OAUTH_TOKEN=fake-token" \
"COPILOT_GITHUB_TOKEN=" \
"GH_TOKEN=")
if printf '%s\n' "$out" | grep -q '::warning::.*Gemini'; then
printf 'FAIL: No Gemini warning when engine is healthy\n got: %s\n' "$out"
FAIL=$((FAIL + 1))
else
printf 'PASS: No Gemini warning when engine is healthy\n'
PASS=$((PASS + 1))
fi

# 7. Job summary lists "Gemini | unavailable" when key is absent
SUMMARY_FILE=$(mktemp)
run_validate "$MOCK_DIR" \
"GOOGLE_API_KEY=" \
"CLAUDE_CODE_OAUTH_TOKEN=" \
"COPILOT_GITHUB_TOKEN=" \
"GH_TOKEN=" \
"GITHUB_STEP_SUMMARY=$SUMMARY_FILE" > /dev/null
assert_contains "Job summary: 'Gemini | unavailable' when key absent" \
"| Gemini | unavailable |" "$(cat "$SUMMARY_FILE")"
rm -f "$SUMMARY_FILE"

# 8. Job summary lists "Gemini | ok" when CLI and key are present
SUMMARY_FILE=$(mktemp)
run_validate "$MOCK_DIR" \
"GOOGLE_API_KEY=fake-key" \
"CLAUDE_CODE_OAUTH_TOKEN=" \
"COPILOT_GITHUB_TOKEN=" \
"GH_TOKEN=" \
"GITHUB_STEP_SUMMARY=$SUMMARY_FILE" > /dev/null
assert_contains "Job summary: 'Gemini | ok' when engine available" \
"| Gemini | ok |" "$(cat "$SUMMARY_FILE")"
rm -f "$SUMMARY_FILE"

# ── Results ───────────────────────────────────────────────────────────────────
echo ""
printf '%d passed, %d failed\n' "$PASS" "$FAIL"
[ "$FAIL" -eq 0 ] || exit 1