diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index d97ea5328..3be0f3aaa 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -481,15 +481,19 @@ has_tier1_blockers() { [ "${failing_checks:-0}" -gt 0 ] || [ "${changes_requested:-0}" -gt 0 ] || [ "${unresolved_bot_threads:-0}" -gt 0 ] } -# try_enable_auto_merge: enables auto-merge (squash) on the PR if reviewDecision is -# APPROVED and auto-merge is not already set. Safe to call speculatively — checks -# eligibility first and is idempotent if auto-merge is already on. +# try_enable_auto_merge: enables auto-merge (squash) on the PR when the engine run +# succeeds (rc==0). We do NOT gate on reviewDecision here: GitHub holds the merge +# until branch protection is satisfied (required reviews approved, review threads +# resolved, required checks green), so enabling early is safe and means a PR merges +# the moment it becomes mergeable. If the engine run fails the call is skipped, so a +# new dev-lead event is required in that case. +# Safe to call speculatively: it is idempotent when auto-merge is already on. # Pass "true" as first arg for strict mode: API errors propagate and a merge failure # exits non-zero rather than emitting a warning (use for the enable-auto-merge intent). try_enable_auto_merge() { local strict="${1:-false}" - if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then - echo "[dry-run] would enable auto-merge if PR #${PR_NUMBER} is APPROVED" + if [[ "${DEV_LEAD_DRY_RUN:-false}" == "true" ]]; then + echo "[dry-run] would enable auto-merge (squash) on PR #${PR_NUMBER}" return 0 fi # Refresh HEAD_SHA to the commit that is now the PR head. commit_and_push may @@ -497,53 +501,28 @@ try_enable_auto_merge() { # --match-head-commit would fail with the stale value. local current_head current_head=$(git rev-parse HEAD 2>/dev/null || true) - [ -n "$current_head" ] && HEAD_SHA="$current_head" - - local auto_merge_state review_decision + [[ -n "$current_head" ]] && HEAD_SHA="$current_head" - if [ "$strict" = "true" ]; then + local auto_merge_state + if [[ "$strict" == "true" ]]; then auto_merge_state=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.auto_merge // empty') - if [ -n "$auto_merge_state" ]; then - echo "::notice::PR #${PR_NUMBER} auto-merge already enabled" - return 0 - fi - review_decision=$(gh api graphql -f query=' - query($owner:String!,$repo:String!,$pr:Int!){ - repository(owner:$owner,name:$repo){ - pullRequest(number:$pr){reviewDecision} - } - }' \ - -F owner="${REPO%%/*}" -F repo="${REPO##*/}" -F pr="$PR_NUMBER" \ - --jq '.data.repository.pullRequest.reviewDecision') else auto_merge_state=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" \ --jq '.auto_merge // empty' 2>/dev/null || true) - if [ -n "$auto_merge_state" ]; then - echo "::notice::PR #${PR_NUMBER} auto-merge already enabled" - return 0 - fi - review_decision=$(gh api graphql -f query=' - query($owner:String!,$repo:String!,$pr:Int!){ - repository(owner:$owner,name:$repo){ - pullRequest(number:$pr){reviewDecision} - } - }' \ - -F owner="${REPO%%/*}" -F repo="${REPO##*/}" -F pr="$PR_NUMBER" \ - --jq '.data.repository.pullRequest.reviewDecision' 2>/dev/null || true) + fi + if [[ -n "$auto_merge_state" ]]; then + echo "::notice::PR #${PR_NUMBER} auto-merge already enabled" + return 0 fi - if [ "${review_decision:-}" = "APPROVED" ]; then - echo "::notice::PR #${PR_NUMBER} is APPROVED — enabling auto-merge (squash)" - set -- --auto --squash - [ -n "${HEAD_SHA:-}" ] && set -- "$@" --match-head-commit "$HEAD_SHA" - if [ "$strict" = "true" ]; then - gh pr merge "$PR_NUMBER" --repo "$REPO" "$@" - else - gh pr merge "$PR_NUMBER" --repo "$REPO" "$@" 2>/dev/null || \ - echo "::warning::auto-merge could not be enabled on PR #${PR_NUMBER} — check repository settings and token permissions" - fi + echo "::notice::PR #${PR_NUMBER} — enabling auto-merge (squash); GitHub will merge once branch protection is satisfied" + local merge_args=("--auto" "--squash") + [[ -n "${HEAD_SHA:-}" ]] && merge_args+=("--match-head-commit" "$HEAD_SHA") + if [[ "$strict" == "true" ]]; then + gh pr merge "$PR_NUMBER" --repo "$REPO" "${merge_args[@]}" else - echo "::notice::PR #${PR_NUMBER} reviewDecision=${review_decision:-unknown} — not yet eligible for auto-merge" + gh pr merge "$PR_NUMBER" --repo "$REPO" "${merge_args[@]}" 2>/dev/null || \ + echo "::warning::auto-merge could not be enabled on PR #${PR_NUMBER} — check repository settings and token permissions" fi } @@ -802,6 +781,7 @@ handle_rate_limit() { local intent="$1" echo "::warning::All engines rate-limited for intent=${intent} — posting rate-limited marker" post_reviews_rate_limited "$intent" + [[ -n "${PR_NUMBER:-}" ]] && try_enable_auto_merge exit 2 } @@ -905,6 +885,10 @@ case "$INTENT_TYPE" in else post_reviews_terminal "on-mention" "no-changes" "Engine ran but made no changes." fi + # Enable auto-merge by default when the mention targets a PR (issue mentions + # carry no PR_NUMBER and are skipped). GitHub holds the merge until branch + # protection is satisfied. + [[ -n "${PR_NUMBER:-}" ]] && try_enable_auto_merge fi exit "$rc" ;; diff --git a/tests/dev-lead/unit/test_fix_reviews.bats b/tests/dev-lead/unit/test_fix_reviews.bats index 1c886c0ce..df2e2a420 100644 --- a/tests/dev-lead/unit/test_fix_reviews.bats +++ b/tests/dev-lead/unit/test_fix_reviews.bats @@ -420,6 +420,80 @@ STUB [[ "$output" == *"would enable auto-merge"* ]] } +@test "fix-reviews: try_enable_auto_merge dry-run output present for on-mention" { + export INTENT_TYPE="on-mention" + export DEV_LEAD_DRY_RUN="true" + export USER_INSTRUCTION="Please fix the tests" + export PR_DESCRIPTION="A test pull request" + + run bash "$FIX_REVIEWS_SCRIPT" + + [ "$status" -eq 0 ] + [[ "$output" == *"would enable auto-merge"* ]] +} + +@test "fix-reviews: auto-merge enabled by default (not gated on APPROVED reviewDecision)" { + # Regression guard: dev-lead enables auto-merge whenever it works on a PR; + # GitHub holds the merge until branch protection is satisfied. The dry-run + # message must not reintroduce an "if APPROVED" eligibility gate. + export INTENT_TYPE="fix-reviews" + export DEV_LEAD_DRY_RUN="true" + + run bash "$FIX_REVIEWS_SCRIPT" + + [ "$status" -eq 0 ] + [[ "$output" == *"would enable auto-merge"* ]] + [[ "$output" != *"if PR #"*"is APPROVED"* ]] +} + +@test "fix-reviews: rate-limited run still attempts auto-merge" { + # Regression guard: handle_rate_limit() must call try_enable_auto_merge so that + # a rate-limited dev-lead run still enables auto-merge before exiting. + export INTENT_TYPE="on-mention" + export DEV_LEAD_DRY_RUN="false" + export USER_INSTRUCTION="Please review" + export COPILOT_GITHUB_TOKEN="stub-token" + + for engine in claude gemini; do + cat > "$STUB_BIN_DIR/$engine" <<'STUB' +#!/usr/bin/env bash +echo "rate limit exceeded" +exit 1 +STUB + chmod +x "$STUB_BIN_DIR/$engine" + done + + cat > "$STUB_BIN_DIR/gh" <<'GHEOF' +#!/usr/bin/env bash +case "$1" in + copilot) echo "rate limit exceeded"; exit 1 ;; +esac +ARGS="$*" +case "$ARGS" in + *"graphql"*) + echo '{"data":{"repository":{"pullRequest":{"reviewThreads":{"nodes":[]}}}}}' ;; + *"api"*"repos/"*"issues/"*) + echo "[]" ;; + *"api"*"repos/"*"pulls/"*) + echo '{}' ;; + *"pr merge"*) + echo "AUTO_MERGE_CALLED: $ARGS"; exit 0 ;; + *"pr comment"*) + echo "COMMENT_POSTED: $ARGS"; exit 0 ;; + *) echo "{}" ;; +esac +GHEOF + chmod +x "$STUB_BIN_DIR/gh" + + run bash "$FIX_REVIEWS_SCRIPT" + + [ "$status" -eq 2 ] + [[ "$output" == *"rate-limited"* ]] + # try_enable_auto_merge is called by handle_rate_limit — either "already enabled" + # or "enabling auto-merge" appears depending on stub state; both prove the call happened. + [[ "$output" == *"auto-merge"* ]] +} + @test "fix-reviews: terminal marker written after successful fix-reviews run" { export INTENT_TYPE="fix-reviews" export DEV_LEAD_DRY_RUN="true"