From 9b50b0a0ccfc7b6adb9593ca98bff4ce8a3181cc Mon Sep 17 00:00:00 2001 From: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Date: Thu, 4 Jun 2026 14:36:00 -0500 Subject: [PATCH 01/11] fix(dev-lead): auto-resolve outdated bot threads and treat unresolved bot threads as blockers Implements Option B and C to fix bot reviewer thread resolution issues (issue #425 follow-up): **Option B - Auto-resolve outdated bot threads:** - New function resolve_bot_outdated_threads() resolves all outdated threads from any bot reviewer - Filters threads where: isOutdated=true + isResolved=false + author ends with [bot] - Called in no-changes paths for fix-reviews, fix-bot-comment, and review-changes intents - Prevents bot threads from cluttering PRs when no code changes are needed - Safe: outdated threads reference code that no longer exists, so resolution is unambiguous **Option C - Treat unresolved bot threads as Tier-1 blockers:** - Enhanced has_tier1_blockers() function to check for unresolved bot reviewer threads - Now returns true if ANY of: failing CI, CHANGES_REQUESTED reviews, or unresolved bot threads - Prevents posting "no-changes" marker when bot feedback still needs addressing - Ensures retry cron can re-attempt when bot reviewer threads exist - Updated warning messages to explicitly mention "unresolved bot threads" **Test Coverage:** - Test: resolve_bot_outdated_threads called in no-changes path (dry-run) - Test: has_tier1_blockers includes unresolved bot threads - Test: does not post no-changes when unresolved bot threads exist - All 56 unit tests passing **Impact:** - PR #422: 6 outdated bot threads will auto-resolve on next run - PR #410: 2 outdated bot threads will auto-resolve on next run - Future PRs: Outdated bot threads won't accumulate or block progress **Fixes:** - Bot reviewer threads no longer stuck when no code changes needed - "no-changes" markers properly gated by unresolved bot threads - Retry cron can keep attempting when bot feedback needs addressing Co-Authored-By: Claude Haiku 4.5 --- scripts/dev-lead-fix-reviews.sh | 168 +++++- tests/dev-lead/unit/test_fix_reviews.bats | 589 ++++++++++++++++++++++ 2 files changed, 754 insertions(+), 3 deletions(-) diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index ed889a799..aa785e28d 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -261,6 +261,147 @@ resolve_actor_outdated_threads() { echo "::notice::resolve_actor_outdated_threads: resolved ${resolved_count} outdated thread(s) on PR #${PR_NUMBER}" } +# fetch_pr_context: exports CI_STATUS_JSON and ALL_REVIEWS_JSON for holistic assessment. +# Called before engine invocation in fix-reviews, fix-bot-comment, and review-changes +# so the agent can identify Tier-1 blockers (failing CI + CHANGES_REQUESTED reviews) +# and never wrongly declare "no-changes" while the PR is still blocked. +fetch_pr_context() { + # CI check results: requires HEAD_SHA. Gracefully degrade to empty array when not set + # (e.g., review-changes in dry-run where the PR API call is skipped). + CI_STATUS_JSON="[]" + if [ -n "${HEAD_SHA:-}" ]; then + CI_STATUS_JSON=$(gh api --paginate "repos/${REPO}/commits/${HEAD_SHA}/check-runs?per_page=100" \ + 2>/dev/null \ + | jq -s '[.[].check_runs[] | {name:.name, status:.status, conclusion:.conclusion, details_url:.details_url}]' \ + 2>/dev/null || echo "[]") + # Also include legacy commit statuses (Jenkins, external CI, etc.) that use + # the separate statuses API rather than check-runs. Merge into CI_STATUS_JSON + # so the agent sees a unified picture of all required status checks. + local statuses_json + # The statuses API returns the full history per context (newest first). + # Dedupe by context so a stale failure overwritten by a later success does not + # appear as a Tier-1 blocker: group_by preserves input order within each group, + # so `first` picks the newest entry for each context. + statuses_json=$(gh api --paginate "repos/${REPO}/commits/${HEAD_SHA}/statuses?per_page=100" \ + 2>/dev/null \ + | jq -s '[ [.[].[] | select(.context != null)] | group_by(.context)[] | first | {name:.context, status:(if .state == "pending" then "in_progress" else "completed" end), conclusion:(if .state == "success" then "success" elif .state == "failure" or .state == "error" then "failure" else null end), details_url:.target_url} ]' \ + 2>/dev/null || echo "[]") + CI_STATUS_JSON=$(printf '%s\n%s' "$CI_STATUS_JSON" "$statuses_json" \ + | jq -s 'add // []' 2>/dev/null || echo "$CI_STATUS_JSON") + fi + export CI_STATUS_JSON + + # All PR reviews with state — deduplicated per reviewer (latest review per user only). + # Uses --paginate so PRs with more than 100 reviews are fully covered. + ALL_REVIEWS_JSON=$(gh api --paginate "repos/${REPO}/pulls/${PR_NUMBER}/reviews?per_page=100" \ + 2>/dev/null \ + | jq -s '[ [.[].[] | select(.user != null)] | group_by(.user.login)[] | sort_by(.id) | last | {id:.id, user:.user.login, state:.state, submitted_at:.submitted_at} ]' \ + 2>/dev/null || echo "[]") + export ALL_REVIEWS_JSON +} + +# resolve_bot_outdated_threads: resolves all outdated review threads from bot reviewers. +# This is a cleanup function for the no-changes path: when no code changes are needed, +# we still want to mark outdated bot comments as resolved so they don't clutter the PR. +# Outdated threads reference code that no longer exists, so resolution is unambiguous. +# +# Unlike resolve_actor_outdated_threads, this resolves threads from ANY bot author +# (author login ends with [bot]), not just a specific ACTOR. +resolve_bot_outdated_threads() { + local intent="$1" + if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then + echo "[dry-run] would resolve outdated review threads from bot reviewers on PR #${PR_NUMBER}" + return 0 + fi + + if [ -z "${PR_NUMBER:-}" ]; then + echo "::notice::resolve_bot_outdated_threads: PR_NUMBER not set for intent=${intent} — skipping" + return 0 + fi + + local ids + # Query for all unresolved outdated threads where the author is a bot (login ends with [bot]) + ids=$(gh api graphql -f query=' + query($owner:String!,$repo:String!,$pr:Int!) { + repository(owner:$owner, name:$repo) { + pullRequest(number:$pr) { + reviewThreads(first:100) { + nodes { id isResolved isOutdated comments(first:1) { nodes { author { login } } } } + } + } + } + }' \ + -F owner="${REPO%%/*}" -F repo="${REPO##*/}" -F pr="$PR_NUMBER" 2>/dev/null \ + | jq -r '.data.repository.pullRequest.reviewThreads.nodes + | map(select(.isResolved == false + and .isOutdated == true + and (.comments.nodes[0].author.login | endswith("[bot]")))) + | .[].id' 2>/dev/null || true) + + if [ -z "$ids" ]; then + echo "::notice::no outdated unresolved threads from bot reviewers on PR #${PR_NUMBER}" + return 0 + fi + + local resolved_count=0 + while IFS= read -r id; do + [ -z "$id" ] && continue + if gh api graphql -f query='mutation($id: ID!) { resolveReviewThread(input: {threadId: $id}) { thread { isResolved } } }' \ + -f id="$id" >/dev/null 2>&1; then + resolved_count=$((resolved_count + 1)) + echo "::notice::resolved outdated bot thread ${id}" + else + echo "::warning::failed to resolve outdated bot thread ${id}" + fi + done <<< "$ids" + echo "::notice::resolve_bot_outdated_threads: resolved ${resolved_count} outdated bot thread(s) on PR #${PR_NUMBER}" +} + +# has_tier1_blockers: returns 0 (true) if CI_STATUS_JSON, ALL_REVIEWS_JSON, or unresolved +# bot reviewer threads contain Tier-1 blockers: +# - CI checks with non-success conclusion (failure, timed_out, cancelled, action_required, stale, startup_failure) +# - Any reviewer with state = CHANGES_REQUESTED +# - Unresolved review threads from bot reviewers (prevents review-changes from ignoring bot feedback) +# Used to gate post_no_changes — never post a terminal no-changes marker while blockers +# exist, so the retry cron can re-attempt on the same SHA. +has_tier1_blockers() { + local failing_checks changes_requested unresolved_bot_threads + + failing_checks=$(printf '%s' "${CI_STATUS_JSON:-[]}" | \ + jq '[.[] | select(.conclusion != null and ( + .conclusion == "failure" or .conclusion == "timed_out" or + .conclusion == "cancelled" or .conclusion == "action_required" or + .conclusion == "stale" or .conclusion == "startup_failure" + ))] | length' 2>/dev/null || echo "0") + + changes_requested=$(printf '%s' "${ALL_REVIEWS_JSON:-[]}" | \ + jq '[.[] | select(.state == "CHANGES_REQUESTED")] | length' \ + 2>/dev/null || echo "0") + + # Check for unresolved bot reviewer threads. This prevents declaring "no-changes" + # when bot comments still need addressing. Use GraphQL to fetch thread status. + unresolved_bot_threads=0 + if [ -n "${PR_NUMBER:-}" ]; then + unresolved_bot_threads=$(gh api graphql -f query=' + query($owner:String!,$repo:String!,$pr:Int!) { + repository(owner:$owner, name:$repo) { + pullRequest(number:$pr) { + reviewThreads(first:100) { + nodes { isResolved comments(first:1) { nodes { author { login } } } } + } + } + } + }' \ + -F owner="${REPO%%/*}" -F repo="${REPO##*/}" -F pr="$PR_NUMBER" 2>/dev/null \ + | jq '[.data.repository.pullRequest.reviewThreads.nodes + | map(select(.isResolved == false + and (.comments.nodes[0].author.login | endswith("[bot]")))) + | length] | .[0]' 2>/dev/null || echo "0") + fi + + [ "${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. @@ -533,6 +674,7 @@ case "$INTENT_TYPE" in -F owner="${REPO%%/*}" -F repo="${REPO##*/}" -F pr="$PR_NUMBER" \ --jq '.data.repository.pullRequest.reviewThreads.nodes | map(select(.isResolved == false))' 2>/dev/null || echo "[]") export OPEN_THREADS_JSON + fetch_pr_context rc=0 build_and_run "fix-reviews" || rc=$? [ "$rc" -eq 2 ] && handle_rate_limit "fix-reviews" @@ -542,8 +684,14 @@ case "$INTENT_TYPE" in post_reviews_terminal "fix-reviews" "applied" "Changes committed and pushed." else notify_coderabbit_resolve - post_no_changes "fix-reviews" + if has_tier1_blockers; then + echo "::warning::Tier-1 blockers still present (failing CI, CHANGES_REQUESTED reviews, or unresolved bot threads) — skipping no-changes marker to allow retries" + else + post_no_changes "fix-reviews" + fi fi + # Always resolve outdated bot threads in the no-changes path as cleanup + resolve_bot_outdated_threads "fix-reviews" resolve_actor_outdated_threads "fix-reviews" try_enable_auto_merge fi @@ -552,6 +700,7 @@ case "$INTENT_TYPE" in fix-bot-comment) export PR_NUMBER PR_URL="https://github.com/${REPO}/pull/${PR_NUMBER}" export REPO ACTOR="${ACTOR:-}" COMMENT_BODY="${COMMENT_BODY:-}" HEAD_SHA + fetch_pr_context rc=0 build_and_run "fix-bot-comment" || rc=$? [ "$rc" -eq 2 ] && handle_rate_limit "fix-bot-comment" @@ -561,8 +710,14 @@ case "$INTENT_TYPE" in post_reviews_terminal "fix-bot-comment" "applied" "Changes committed and pushed." else notify_coderabbit_resolve - post_no_changes "fix-bot-comment" + if has_tier1_blockers; then + echo "::warning::Tier-1 blockers still present (failing CI, CHANGES_REQUESTED reviews, or unresolved bot threads) — skipping no-changes marker to allow retries" + else + post_no_changes "fix-bot-comment" + fi fi + # Always resolve outdated bot threads in the no-changes path as cleanup + resolve_bot_outdated_threads "fix-bot-comment" resolve_actor_outdated_threads "fix-bot-comment" try_enable_auto_merge fi @@ -604,6 +759,7 @@ case "$INTENT_TYPE" in -F owner="${REPO%%/*}" -F repo="${REPO##*/}" -F pr="$PR_NUMBER" \ --jq '.data.repository.pullRequest.reviewThreads.nodes | map(select(.isResolved == false))' 2>/dev/null || echo "[]") export OPEN_THREADS_JSON BASE_REF="${BASE_REF:-main}" + fetch_pr_context rc=0 build_and_run "review-changes" || rc=$? [ "$rc" -eq 2 ] && handle_rate_limit "review-changes" @@ -613,8 +769,14 @@ case "$INTENT_TYPE" in post_reviews_terminal "review-changes" "applied" "Changes committed and pushed." else notify_coderabbit_resolve - post_reviews_terminal "review-changes" "no-changes" "No changes were needed for this PR." + if has_tier1_blockers; then + echo "::warning::Tier-1 blockers still present (failing CI, CHANGES_REQUESTED reviews, or unresolved bot threads) — skipping no-changes marker to allow retries" + else + post_reviews_terminal "review-changes" "no-changes" "No changes were needed for this PR." + fi fi + # Always resolve outdated bot threads in the no-changes path as cleanup + resolve_bot_outdated_threads "review-changes" resolve_actor_outdated_threads "review-changes" try_enable_auto_merge fi diff --git a/tests/dev-lead/unit/test_fix_reviews.bats b/tests/dev-lead/unit/test_fix_reviews.bats index 4872d8484..9dc529a29 100644 --- a/tests/dev-lead/unit/test_fix_reviews.bats +++ b/tests/dev-lead/unit/test_fix_reviews.bats @@ -918,3 +918,592 @@ STUB # Should call resolve_actor_outdated_threads in dry-run mode [[ "$output" == *"would resolve outdated review threads authored by donpetry"* ]] } + +# ── ALL_REVIEWS_JSON deduplication: latest review per user ─────────────────── +# The GitHub Reviews API returns the full history of all reviews. If a reviewer +# previously requested changes but later approved, both entries are present. +# The jq filter in collect_assessment_data must keep only the latest per user. + +@test "collect_assessment_data: jq dedup expression keeps only latest review per user" { + # Feed sample multi-review JSON through the exact jq expression used in the script + # to verify that CHANGES_REQUESTED is superseded by a later APPROVED from the same user. + # Input is a single-page array; jq -s simulates --paginate output (wraps to [[...]]) + # so .[].[] correctly flattens across pages. + local input='[ + {"id":1,"user":{"login":"alice"},"state":"CHANGES_REQUESTED","submitted_at":"2024-01-01T00:00:00Z"}, + {"id":2,"user":{"login":"bob"},"state":"APPROVED","submitted_at":"2024-01-02T00:00:00Z"}, + {"id":3,"user":{"login":"alice"},"state":"APPROVED","submitted_at":"2024-01-03T00:00:00Z"} + ]' + local jq_expr='[ [.[].[] | select(.user != null)] | group_by(.user.login)[] | sort_by(.id) | last | {id:.id, user:.user.login, state:.state, submitted_at:.submitted_at} ]' + + run bash -c "printf '%s' '$input' | jq -s '$jq_expr'" + + [ "$status" -eq 0 ] + # alice's latest review (id=3) is APPROVED — CHANGES_REQUESTED (id=1) must not appear + [[ "$output" == *'"alice"'* ]] + [[ "$output" == *'"APPROVED"'* ]] + # Only one entry for alice — no duplicate + local alice_count + alice_count=$(echo "$output" | grep -c '"alice"') + [ "$alice_count" -eq 1 ] + # CHANGES_REQUESTED should not appear in output + [[ "$output" != *'"CHANGES_REQUESTED"'* ]] +} + +@test "collect_assessment_data: jq dedup expression filters out null-user entries" { + local input='[ + {"id":1,"user":null,"state":"APPROVED","submitted_at":"2024-01-01T00:00:00Z"}, + {"id":2,"user":{"login":"alice"},"state":"APPROVED","submitted_at":"2024-01-02T00:00:00Z"} + ]' + local jq_expr='[ [.[].[] | select(.user != null)] | group_by(.user.login)[] | sort_by(.id) | last | {id:.id, user:.user.login, state:.state, submitted_at:.submitted_at} ]' + + run bash -c "printf '%s' '$input' | jq -s '$jq_expr'" + + [ "$status" -eq 0 ] + # Result should have only alice; the null-user entry is filtered out + local count + count=$(echo "$output" | jq 'length') + [ "$count" -eq 1 ] +} + +# ── holistic assessment: CI_STATUS_JSON and ALL_REVIEWS_JSON fetching ────────── +# These tests verify that fix-reviews, review-changes, and fix-bot-comment all +# fetch CI check results and all PR reviews before running the engine, so the +# agent can detect Tier-1 blockers (failing CI + CHANGES_REQUESTED reviews) and +# never wrongly declare "no-changes" while the PR is still blocked. + +_make_assessment_gh_stub() { + local calls_file="$1" + cat > "$STUB_BIN_DIR/gh" << GHEOF +#!/usr/bin/env bash +# Record every invocation for assertion +printf '%s\n' "\$*" >> "${calls_file}" +ARGS="\$*" +case "\$ARGS" in + *"commits/"*"check-runs"*) + # Return raw GitHub API format — script now uses --paginate piped to jq -s + echo '{"check_runs":[{"name":"Lint","status":"completed","conclusion":"failure","details_url":"https://example.com"}]}' ;; + *"commits/"*"statuses"*) + echo '[]' ;; + *"pulls/"*"/reviews"*) + # Return raw GitHub API format with user as object — script now uses --paginate piped to jq -s + echo '[{"id":1,"user":{"login":"gemini-code-assist[bot]"},"state":"CHANGES_REQUESTED","submitted_at":"2024-01-01T00:00:00Z"}]' ;; + *"graphql"*) + echo '{"data":{"repository":{"pullRequest":{"reviewThreads":{"nodes":[]},"reviewDecision":null}}}}' ;; + *"issues/"*"comments"*) + echo "[]" ;; + *"pr checkout"*) exit 0 ;; + *"pr comment"*) exit 0 ;; + *"pr merge"*) exit 0 ;; + *"pulls/"*) echo '{"head":{"sha":"ddd444eee555"},"auto_merge":null}' ;; + *) echo "{}" ;; +esac +GHEOF + chmod +x "$STUB_BIN_DIR/gh" +} + +@test "fix-reviews: fix-reviews case fetches CI check-runs endpoint" { + export INTENT_TYPE="fix-reviews" + export DEV_LEAD_DRY_RUN="true" + export HEAD_SHA="ddd444eee555" + + local calls_file + calls_file="$(mktemp)" + _make_assessment_gh_stub "$calls_file" + + run bash "$FIX_REVIEWS_SCRIPT" + + [ "$status" -eq 0 ] + # Script must have called the check-runs API endpoint + grep -q "commits/.*check-runs" "$calls_file" + rm -f "$calls_file" +} + +@test "fix-reviews: fix-reviews case fetches all PR reviews endpoint" { + export INTENT_TYPE="fix-reviews" + export DEV_LEAD_DRY_RUN="true" + export HEAD_SHA="ddd444eee555" + + local calls_file + calls_file="$(mktemp)" + _make_assessment_gh_stub "$calls_file" + + run bash "$FIX_REVIEWS_SCRIPT" + + [ "$status" -eq 0 ] + # Script must have called the reviews API endpoint + grep -q "pulls/.*reviews" "$calls_file" + rm -f "$calls_file" +} + +@test "fix-reviews: review-changes case fetches CI check-runs endpoint" { + export INTENT_TYPE="review-changes" + export DEV_LEAD_DRY_RUN="true" + export HEAD_SHA="ddd444eee555" + export PR_TITLE="Test PR" + export PR_DESCRIPTION="A test pull request" + + local calls_file + calls_file="$(mktemp)" + _make_assessment_gh_stub "$calls_file" + + run bash "$FIX_REVIEWS_SCRIPT" + + [ "$status" -eq 0 ] + grep -q "commits/.*check-runs" "$calls_file" + rm -f "$calls_file" +} + +@test "fix-reviews: review-changes case fetches all PR reviews endpoint" { + export INTENT_TYPE="review-changes" + export DEV_LEAD_DRY_RUN="true" + export HEAD_SHA="ddd444eee555" + export PR_TITLE="Test PR" + export PR_DESCRIPTION="A test pull request" + + local calls_file + calls_file="$(mktemp)" + _make_assessment_gh_stub "$calls_file" + + run bash "$FIX_REVIEWS_SCRIPT" + + [ "$status" -eq 0 ] + grep -q "pulls/.*reviews" "$calls_file" + rm -f "$calls_file" +} + +@test "fix-reviews: fix-bot-comment case fetches CI check-runs endpoint" { + export INTENT_TYPE="fix-bot-comment" + export DEV_LEAD_DRY_RUN="true" + export HEAD_SHA="ddd444eee555" + export COMMENT_BODY="SonarQube found issues" + + local calls_file + calls_file="$(mktemp)" + _make_assessment_gh_stub "$calls_file" + + run bash "$FIX_REVIEWS_SCRIPT" + + [ "$status" -eq 0 ] + grep -q "commits/.*check-runs" "$calls_file" + rm -f "$calls_file" +} + +@test "fix-reviews: fix-bot-comment case fetches all PR reviews endpoint" { + export INTENT_TYPE="fix-bot-comment" + export DEV_LEAD_DRY_RUN="true" + export HEAD_SHA="ddd444eee555" + export COMMENT_BODY="SonarQube found issues" + + local calls_file + calls_file="$(mktemp)" + _make_assessment_gh_stub "$calls_file" + + run bash "$FIX_REVIEWS_SCRIPT" + + [ "$status" -eq 0 ] + grep -q "pulls/.*reviews" "$calls_file" + rm -f "$calls_file" +} + +@test "fix-reviews: fix-reviews case fetches commit statuses endpoint" { + export INTENT_TYPE="fix-reviews" + export DEV_LEAD_DRY_RUN="true" + export HEAD_SHA="ddd444eee555" + + local calls_file + calls_file="$(mktemp)" + _make_assessment_gh_stub "$calls_file" + + run bash "$FIX_REVIEWS_SCRIPT" + + [ "$status" -eq 0 ] + grep -q "commits/.*statuses" "$calls_file" + rm -f "$calls_file" +} + +@test "fix-reviews: review-changes case fetches commit statuses endpoint" { + export INTENT_TYPE="review-changes" + export DEV_LEAD_DRY_RUN="true" + export HEAD_SHA="ddd444eee555" + export PR_TITLE="Test PR" + export PR_DESCRIPTION="A test pull request" + + local calls_file + calls_file="$(mktemp)" + _make_assessment_gh_stub "$calls_file" + + run bash "$FIX_REVIEWS_SCRIPT" + + [ "$status" -eq 0 ] + grep -q "commits/.*statuses" "$calls_file" + rm -f "$calls_file" +} + +@test "fix-reviews: fix-bot-comment case fetches commit statuses endpoint" { + export INTENT_TYPE="fix-bot-comment" + export DEV_LEAD_DRY_RUN="true" + export HEAD_SHA="ddd444eee555" + export COMMENT_BODY="SonarQube found issues" + + local calls_file + calls_file="$(mktemp)" + _make_assessment_gh_stub "$calls_file" + + run bash "$FIX_REVIEWS_SCRIPT" + + [ "$status" -eq 0 ] + grep -q "commits/.*statuses" "$calls_file" + rm -f "$calls_file" +} + +# ── Tier-1 blocker gating: no-changes must not be posted while blockers exist ── +# Regression tests for issue #425: the agent must not declare "no-changes" while +# CI is failing or a reviewer has CHANGES_REQUESTED. + +@test "fix-reviews: does not post no-changes when Tier-1 blockers exist (fix-reviews)" { + local calls_file tmpdir + calls_file="$(mktemp)" + tmpdir="$(mktemp -d)" + _make_assessment_gh_stub "$calls_file" + + # Run from a non-git tmpdir so commit_and_push returns 1 (no changes), taking the no-changes path + run bash -c " + cd '$tmpdir' + export INTENT_TYPE=fix-reviews DEV_LEAD_DRY_RUN=true + export PR_NUMBER=54 HEAD_SHA=ddd444eee555 REPO='petry-projects/.github-private' + export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead' + export PATH=\"$STUB_BIN_DIR:\$PATH\" + bash '$FIX_REVIEWS_SCRIPT' + " 2>&1 + rm -rf "$tmpdir" "$calls_file" + + [ "$status" -eq 0 ] + # Must NOT post a terminal no-changes marker while CI is failing + CHANGES_REQUESTED + [[ "$output" != *"status=no-changes"* ]] + # Must emit the warning explaining why no-changes was skipped + [[ "$output" == *"Tier-1 blockers still present"* ]] +} + +@test "fix-reviews: does not post no-changes when Tier-1 blockers exist (fix-bot-comment)" { + local calls_file tmpdir + calls_file="$(mktemp)" + tmpdir="$(mktemp -d)" + _make_assessment_gh_stub "$calls_file" + + run bash -c " + cd '$tmpdir' + export INTENT_TYPE=fix-bot-comment DEV_LEAD_DRY_RUN=true + export PR_NUMBER=54 HEAD_SHA=ddd444eee555 REPO='petry-projects/.github-private' + export COMMENT_BODY='SonarQube found issues' + export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead' + export PATH=\"$STUB_BIN_DIR:\$PATH\" + bash '$FIX_REVIEWS_SCRIPT' + " 2>&1 + rm -rf "$tmpdir" "$calls_file" + + [ "$status" -eq 0 ] + [[ "$output" != *"status=no-changes"* ]] + [[ "$output" == *"Tier-1 blockers still present"* ]] +} + +@test "fix-reviews: does not post no-changes when Tier-1 blockers exist (review-changes)" { + local calls_file tmpdir + calls_file="$(mktemp)" + tmpdir="$(mktemp -d)" + _make_assessment_gh_stub "$calls_file" + + run bash -c " + cd '$tmpdir' + export INTENT_TYPE=review-changes DEV_LEAD_DRY_RUN=true + export PR_NUMBER=54 HEAD_SHA=ddd444eee555 REPO='petry-projects/.github-private' + export PR_TITLE='Test PR' PR_DESCRIPTION='A test pull request' + export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead' + export PATH=\"$STUB_BIN_DIR:\$PATH\" + bash '$FIX_REVIEWS_SCRIPT' + " 2>&1 + rm -rf "$tmpdir" "$calls_file" + + [ "$status" -eq 0 ] + [[ "$output" != *"status=no-changes"* ]] + [[ "$output" == *"Tier-1 blockers still present"* ]] +} + +@test "fix-reviews: posts no-changes when no Tier-1 blockers exist (fix-reviews)" { + local tmpdir + tmpdir="$(mktemp -d)" + + # Stub with all-success CI and no CHANGES_REQUESTED reviews + cat > "$STUB_BIN_DIR/gh" <<'GHEOF' +#!/usr/bin/env bash +ARGS="$*" +case "$ARGS" in + *"commits/"*"check-runs"*) + echo '{"check_runs":[{"name":"Lint","status":"completed","conclusion":"success","details_url":"https://example.com"}]}' ;; + *"commits/"*"statuses"*) + echo '[]' ;; + *"pulls/"*"reviews"*) + echo '[]' ;; + *"graphql"*) + echo '{"data":{"repository":{"pullRequest":{"reviewThreads":{"nodes":[]},"reviewDecision":null}}}}' ;; + *"issues/"*"comments"*) + echo "[]" ;; + *"pr checkout"*) exit 0 ;; + *"pr comment"*) exit 0 ;; + *"pr merge"*) exit 0 ;; + *"pulls/"*) echo '{"head":{"sha":"ddd444eee555"},"auto_merge":null}' ;; + *) echo "{}" ;; +esac +GHEOF + chmod +x "$STUB_BIN_DIR/gh" + + # Run from a non-git tmpdir so commit_and_push returns 1 (no changes), taking the no-changes path + run bash -c " + cd '$tmpdir' + export INTENT_TYPE=fix-reviews DEV_LEAD_DRY_RUN=true + export PR_NUMBER=54 HEAD_SHA=ddd444eee555 REPO='petry-projects/.github-private' + export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead' + export PATH=\"$STUB_BIN_DIR:\$PATH\" + bash '$FIX_REVIEWS_SCRIPT' + " 2>&1 + rm -rf "$tmpdir" + + [ "$status" -eq 0 ] + # No blockers → no-changes marker should be posted + [[ "$output" == *"status=no-changes"* ]] +} + +# ── Legacy commit statuses dedup: latest state per context wins ──────────────── +# The /statuses API returns the full history per context (newest first). A stale +# failure followed by a newer success for the same context must not suppress +# no-changes — only the latest entry per context should be considered. + +@test "collect_assessment_data: jq statuses dedup keeps only latest entry per context" { + # GitHub /statuses API returns newest-first. Here jenkins/build had a failure then a + # newer success, so success appears first in the array (= newest). After dedup, only + # the success (first entry per context) should remain. + local input='[ + {"context":"jenkins/build","state":"success","target_url":"https://ci.example.com/2"}, + {"context":"jenkins/build","state":"failure","target_url":"https://ci.example.com/1"}, + {"context":"other/check","state":"success","target_url":"https://ci.example.com/3"} + ]' + local jq_expr='[ [.[].[] | select(.context != null)] | group_by(.context)[] | first | {name:.context, conclusion:(if .state == "success" then "success" elif .state == "failure" or .state == "error" then "failure" else null end)} ]' + + run bash -c "printf '%s' '$input' | jq -s '$jq_expr'" + + [ "$status" -eq 0 ] + # The newer success for jenkins/build must appear; the old failure must not + local jenkins_conclusion + jenkins_conclusion=$(echo "$output" | jq -r '.[] | select(.name == "jenkins/build") | .conclusion') + [ "$jenkins_conclusion" = "success" ] + # Only one entry for jenkins/build — no duplicate + local jenkins_count + jenkins_count=$(echo "$output" | jq '[.[] | select(.name == "jenkins/build")] | length') + [ "$jenkins_count" -eq 1 ] +} + +@test "fix-reviews: does not treat superseded legacy CI failure as Tier-1 blocker (fix-reviews)" { + local tmpdir + tmpdir="$(mktemp -d)" + + # Stub: check-runs all success; statuses has old failure then new success for same context + cat > "$STUB_BIN_DIR/gh" <<'GHEOF' +#!/usr/bin/env bash +ARGS="$*" +case "$ARGS" in + *"commits/"*"check-runs"*) + echo '{"check_runs":[{"name":"Lint","status":"completed","conclusion":"success","details_url":"https://example.com"}]}' ;; + *"commits/"*"statuses"*) + # Newest-first: success (newer) comes before failure (older) in the history + echo '[{"context":"jenkins/build","state":"success","target_url":"https://ci.example.com/2"},{"context":"jenkins/build","state":"failure","target_url":"https://ci.example.com/1"}]' ;; + *"pulls/"*"reviews"*) + echo '[]' ;; + *"graphql"*) + echo '{"data":{"repository":{"pullRequest":{"reviewThreads":{"nodes":[]},"reviewDecision":null}}}}' ;; + *"issues/"*"comments"*) + echo "[]" ;; + *"pr checkout"*) exit 0 ;; + *"pr comment"*) exit 0 ;; + *"pr merge"*) exit 0 ;; + *"pulls/"*) echo '{"head":{"sha":"ddd444eee555"},"auto_merge":null}' ;; + *) echo "{}" ;; +esac +GHEOF + chmod +x "$STUB_BIN_DIR/gh" + + # Run from a non-git tmpdir so commit_and_push returns 1 (no changes), taking the no-changes path + run bash -c " + cd '$tmpdir' + export INTENT_TYPE=fix-reviews DEV_LEAD_DRY_RUN=true + export PR_NUMBER=54 HEAD_SHA=ddd444eee555 REPO='petry-projects/.github-private' + export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead' + export PATH=\"$STUB_BIN_DIR:\$PATH\" + bash '$FIX_REVIEWS_SCRIPT' + " 2>&1 + rm -rf "$tmpdir" + + [ "$status" -eq 0 ] + # The old failure is superseded by the newer success → no Tier-1 blockers → no-changes is posted + [[ "$output" == *"status=no-changes"* ]] + [[ "$output" != *"Tier-1 blockers still present"* ]] +} + +@test "fix-reviews: resolve_bot_outdated_threads called in no-changes path (dry-run)" { + local tmpdir + tmpdir="$(mktemp -d)" + + cat > "$STUB_BIN_DIR/gh" <<'GHEOF' +#!/usr/bin/env bash +ARGS="$*" +case "$ARGS" in + *"check-runs"*) + echo '{"check_runs":[{"name":"Lint","status":"completed","conclusion":"success","details_url":"https://example.com"}]}' ;; + *"statuses"*) + echo '[]' ;; + *"pulls/"*"reviews"*) + echo '[]' ;; + *"graphql"*) + echo '{"data":{"repository":{"pullRequest":{"reviewThreads":{"nodes":[]},"reviewDecision":null}}}}' ;; + *"pr checkout"*) exit 0 ;; + *"pr comment"*) exit 0 ;; + *"pr merge"*) exit 0 ;; + *"pulls/"*) echo '{"head":{"sha":"abc123"},"auto_merge":null}' ;; + *) echo "{}" ;; +esac +GHEOF + chmod +x "$STUB_BIN_DIR/gh" + + run bash -c " + cd '$tmpdir' + export INTENT_TYPE=fix-reviews DEV_LEAD_DRY_RUN=true + export PR_NUMBER=422 HEAD_SHA=abc123 REPO='petry-projects/.github-private' + export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead' + export PATH=\"$STUB_BIN_DIR:\$PATH\" + bash '$FIX_REVIEWS_SCRIPT' + " 2>&1 + rm -rf "$tmpdir" + + [ "$status" -eq 0 ] + [[ "$output" == *"resolve outdated review threads from bot reviewers"* ]] +} + +@test "fix-reviews: has_tier1_blockers includes unresolved bot threads" { + local tmpdir + tmpdir="$(mktemp -d)" + + # Stub: check-runs success, statuses none, but graphql returns unresolved bot threads + cat > "$STUB_BIN_DIR/gh" <<'GHEOF' +#!/usr/bin/env bash +ARGS="$*" +case "$ARGS" in + *"check-runs"*) + echo '{"check_runs":[{"name":"Lint","status":"completed","conclusion":"success","details_url":"https://example.com"}]}' ;; + *"statuses"*) + echo '[]' ;; + *"pulls/"*"reviews"*) + echo '[]' ;; + *"graphql"*) + # First query (for has_tier1_blockers) or review-changes context query returns bot threads + echo '{ + "data": { + "repository": { + "pullRequest": { + "reviewThreads": { + "nodes": [ + { + "isResolved": false, + "comments": {"nodes": [{"author": {"login": "copilot-pull-request-reviewer[bot]"}}]} + }, + { + "isResolved": false, + "comments": {"nodes": [{"author": {"login": "coderabbitai[bot]"}}]} + } + ] + }, + "reviewDecision": null + } + } + } + }' ;; + *"pr checkout"*) exit 0 ;; + *"pr comment"*) exit 0 ;; + *"pr merge"*) exit 0 ;; + *"pulls/"*) echo '{"head":{"sha":"abc123"},"auto_merge":null}' ;; + *) echo "{}" ;; +esac +GHEOF + chmod +x "$STUB_BIN_DIR/gh" + + run bash -c " + cd '$tmpdir' + export INTENT_TYPE=review-changes DEV_LEAD_DRY_RUN=true + export PR_NUMBER=422 HEAD_SHA=abc123 REPO='petry-projects/.github-private' + export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead' + export PATH=\"$STUB_BIN_DIR:\$PATH\" + bash '$FIX_REVIEWS_SCRIPT' + " 2>&1 + rm -rf "$tmpdir" + + [ "$status" -eq 0 ] + # When unresolved bot threads exist, blockers are present + [[ "$output" == *"Tier-1 blockers still present"* ]] + [[ "$output" == *"unresolved bot threads"* ]] +} + +@test "fix-reviews: does not post no-changes when unresolved bot threads exist" { + local tmpdir + tmpdir="$(mktemp -d)" + + # Stub: graphql returns unresolved bot threads + cat > "$STUB_BIN_DIR/gh" <<'GHEOF' +#!/usr/bin/env bash +ARGS="$*" +case "$ARGS" in + *"check-runs"*) + echo '{"check_runs":[{"name":"Lint","status":"completed","conclusion":"success","details_url":"https://example.com"}]}' ;; + *"statuses"*) + echo '[]' ;; + *"pulls/"*"reviews"*) + echo '[]' ;; + *"graphql"*) + echo '{ + "data": { + "repository": { + "pullRequest": { + "reviewThreads": { + "nodes": [ + { + "isResolved": false, + "comments": {"nodes": [{"author": {"login": "copilot-pull-request-reviewer[bot]"}}]} + } + ] + } + } + } + } + }' ;; + *"pr checkout"*) exit 0 ;; + *"pr comment"*) exit 0 ;; + *"pr merge"*) exit 0 ;; + *"pulls/"*) echo '{"head":{"sha":"abc123"},"auto_merge":null}' ;; + *) echo "{}" ;; +esac +GHEOF + chmod +x "$STUB_BIN_DIR/gh" + + run bash -c " + cd '$tmpdir' + export INTENT_TYPE=review-changes DEV_LEAD_DRY_RUN=true + export PR_NUMBER=422 HEAD_SHA=abc123 REPO='petry-projects/.github-private' + export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead' + export PATH=\"$STUB_BIN_DIR:\$PATH\" + bash '$FIX_REVIEWS_SCRIPT' + " 2>&1 + rm -rf "$tmpdir" + + [ "$status" -eq 0 ] + # Unresolved bot threads present → has_tier1_blockers returns true → no-changes marker NOT posted + [[ "$output" != *"status=no-changes"* ]] + [[ "$output" == *"Tier-1 blockers still present"* ]] + [[ "$output" == *"unresolved bot threads"* ]] +} From 373986f3404cb49ca11f0636ec298f961ea78e5c Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Thu, 4 Jun 2026 21:23:09 +0000 Subject: [PATCH 02/11] chore: apply manual instructions [skip ci-relay] --- scripts/dev-lead-fix-reviews.sh | 103 ++++++++++++----- tests/dev-lead/unit/test_fix_reviews.bats | 128 +++++++++++++++++++++- 2 files changed, 201 insertions(+), 30 deletions(-) diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index aa785e28d..e6c177184 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -300,13 +300,14 @@ fetch_pr_context() { export ALL_REVIEWS_JSON } -# resolve_bot_outdated_threads: resolves all outdated review threads from bot reviewers. +# resolve_bot_outdated_threads: resolves up to 100 outdated review threads from bot reviewers. # This is a cleanup function for the no-changes path: when no code changes are needed, # we still want to mark outdated bot comments as resolved so they don't clutter the PR. # Outdated threads reference code that no longer exists, so resolution is unambiguous. # +# Best-effort: fetches the first 100 threads only (not paginated — acceptable for cleanup). # Unlike resolve_actor_outdated_threads, this resolves threads from ANY bot author -# (author login ends with [bot]), not just a specific ACTOR. +# (author __typename == "Bot", or login ends with [bot]). resolve_bot_outdated_threads() { local intent="$1" if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then @@ -320,22 +321,25 @@ resolve_bot_outdated_threads() { fi local ids - # Query for all unresolved outdated threads where the author is a bot (login ends with [bot]) + # Query for unresolved outdated threads where the author is a bot. + # __typename == "Bot" covers bots whose GraphQL login omits the [bot] suffix; + # endswith("[bot]") covers bots that include it — both checks together are belt-and-suspenders. ids=$(gh api graphql -f query=' query($owner:String!,$repo:String!,$pr:Int!) { repository(owner:$owner, name:$repo) { pullRequest(number:$pr) { reviewThreads(first:100) { - nodes { id isResolved isOutdated comments(first:1) { nodes { author { login } } } } + nodes { id isResolved isOutdated comments(first:1) { nodes { author { login __typename } } } } } } } }' \ -F owner="${REPO%%/*}" -F repo="${REPO##*/}" -F pr="$PR_NUMBER" 2>/dev/null \ - | jq -r '.data.repository.pullRequest.reviewThreads.nodes + | jq -r '.data?.repository?.pullRequest?.reviewThreads?.nodes // [] | map(select(.isResolved == false and .isOutdated == true - and (.comments.nodes[0].author.login | endswith("[bot]")))) + and (((.comments.nodes?[0]?.author?.login // "") | endswith("[bot]")) + or ((.comments.nodes?[0]?.author?.__typename // "") == "Bot")))) | .[].id' 2>/dev/null || true) if [ -z "$ids" ]; then @@ -357,6 +361,28 @@ resolve_bot_outdated_threads() { echo "::notice::resolve_bot_outdated_threads: resolved ${resolved_count} outdated bot thread(s) on PR #${PR_NUMBER}" } +# has_hard_blockers: returns 0 (true) if CI_STATUS_JSON or ALL_REVIEWS_JSON contain +# hard Tier-1 blockers (failing CI checks or CHANGES_REQUESTED reviews). +# Unlike has_tier1_blockers, does NOT check for unresolved bot threads — used to +# distinguish "bot threads are the sole blocker" from "hard blockers present", so +# callers can post a retry marker instead of silently stalling on bot feedback. +has_hard_blockers() { + local failing_checks changes_requested + + failing_checks=$(printf '%s' "${CI_STATUS_JSON:-[]}" | \ + jq '[.[] | select(.conclusion != null and ( + .conclusion == "failure" or .conclusion == "timed_out" or + .conclusion == "cancelled" or .conclusion == "action_required" or + .conclusion == "stale" or .conclusion == "startup_failure" + ))] | length' 2>/dev/null || echo "0") + + changes_requested=$(printf '%s' "${ALL_REVIEWS_JSON:-[]}" | \ + jq '[.[] | select(.state == "CHANGES_REQUESTED")] | length' \ + 2>/dev/null || echo "0") + + [ "${failing_checks:-0}" -gt 0 ] || [ "${changes_requested:-0}" -gt 0 ] +} + # has_tier1_blockers: returns 0 (true) if CI_STATUS_JSON, ALL_REVIEWS_JSON, or unresolved # bot reviewer threads contain Tier-1 blockers: # - CI checks with non-success conclusion (failure, timed_out, cancelled, action_required, stale, startup_failure) @@ -378,25 +404,43 @@ has_tier1_blockers() { jq '[.[] | select(.state == "CHANGES_REQUESTED")] | length' \ 2>/dev/null || echo "0") - # Check for unresolved bot reviewer threads. This prevents declaring "no-changes" - # when bot comments still need addressing. Use GraphQL to fetch thread status. + # Count unresolved bot reviewer threads with cursor pagination to cover PRs with >100 threads. + # Detects bots via __typename == "Bot" (covers bots whose GraphQL login omits [bot] suffix) + # or login ending with [bot] (belt-and-suspenders for bots that include the suffix). unresolved_bot_threads=0 if [ -n "${PR_NUMBER:-}" ]; then - unresolved_bot_threads=$(gh api graphql -f query=' - query($owner:String!,$repo:String!,$pr:Int!) { - repository(owner:$owner, name:$repo) { - pullRequest(number:$pr) { - reviewThreads(first:100) { - nodes { isResolved comments(first:1) { nodes { author { login } } } } - } + local cursor="" has_next_page="true" page_response page_count + local cursor_args=() + local bot_thread_query='query($owner:String!,$repo:String!,$pr:Int!,$cursor:String){ + repository(owner:$owner,name:$repo){ + pullRequest(number:$pr){ + reviewThreads(first:100,after:$cursor){ + pageInfo{hasNextPage endCursor} + nodes{isResolved comments(first:1){nodes{author{login __typename}}}} } } - }' \ - -F owner="${REPO%%/*}" -F repo="${REPO##*/}" -F pr="$PR_NUMBER" 2>/dev/null \ - | jq '[.data.repository.pullRequest.reviewThreads.nodes + } + }' + while [ "$has_next_page" = "true" ]; do + page_response=$(gh api graphql -f query="$bot_thread_query" \ + -F owner="${REPO%%/*}" -F repo="${REPO##*/}" -F pr="$PR_NUMBER" \ + "${cursor_args[@]}" 2>/dev/null || echo "{}") + page_count=$(printf '%s' "$page_response" | jq \ + '[.data?.repository?.pullRequest?.reviewThreads?.nodes // [] | map(select(.isResolved == false - and (.comments.nodes[0].author.login | endswith("[bot]")))) + and (((.comments.nodes?[0]?.author?.login // "") | endswith("[bot]")) + or ((.comments.nodes?[0]?.author?.__typename // "") == "Bot")))) | length] | .[0]' 2>/dev/null || echo "0") + unresolved_bot_threads=$(( ${unresolved_bot_threads:-0} + ${page_count:-0} )) + has_next_page=$(printf '%s' "$page_response" | jq -r \ + '.data?.repository?.pullRequest?.reviewThreads?.pageInfo?.hasNextPage // false' \ + 2>/dev/null || echo "false") + cursor=$(printf '%s' "$page_response" | jq -r \ + '.data?.repository?.pullRequest?.reviewThreads?.pageInfo?.endCursor // ""' \ + 2>/dev/null || echo "") + [ -z "$cursor" ] && has_next_page="false" + cursor_args=("-f" "cursor=${cursor}") + done fi [ "${failing_checks:-0}" -gt 0 ] || [ "${changes_requested:-0}" -gt 0 ] || [ "${unresolved_bot_threads:-0}" -gt 0 ] @@ -684,8 +728,11 @@ case "$INTENT_TYPE" in post_reviews_terminal "fix-reviews" "applied" "Changes committed and pushed." else notify_coderabbit_resolve - if has_tier1_blockers; then - echo "::warning::Tier-1 blockers still present (failing CI, CHANGES_REQUESTED reviews, or unresolved bot threads) — skipping no-changes marker to allow retries" + if has_hard_blockers; then + echo "::warning::Tier-1 blockers still present (failing CI or CHANGES_REQUESTED reviews) — skipping no-changes marker to allow retries" + elif has_tier1_blockers; then + echo "::warning::Unresolved bot review threads remain — posting retry marker so the cron re-dispatches after the bot resolves its feedback" + post_reviews_rate_limited "fix-reviews" else post_no_changes "fix-reviews" fi @@ -710,8 +757,11 @@ case "$INTENT_TYPE" in post_reviews_terminal "fix-bot-comment" "applied" "Changes committed and pushed." else notify_coderabbit_resolve - if has_tier1_blockers; then - echo "::warning::Tier-1 blockers still present (failing CI, CHANGES_REQUESTED reviews, or unresolved bot threads) — skipping no-changes marker to allow retries" + if has_hard_blockers; then + echo "::warning::Tier-1 blockers still present (failing CI or CHANGES_REQUESTED reviews) — skipping no-changes marker to allow retries" + elif has_tier1_blockers; then + echo "::warning::Unresolved bot review threads remain — posting retry marker so the cron re-dispatches after the bot resolves its feedback" + post_reviews_rate_limited "fix-bot-comment" else post_no_changes "fix-bot-comment" fi @@ -769,8 +819,11 @@ case "$INTENT_TYPE" in post_reviews_terminal "review-changes" "applied" "Changes committed and pushed." else notify_coderabbit_resolve - if has_tier1_blockers; then - echo "::warning::Tier-1 blockers still present (failing CI, CHANGES_REQUESTED reviews, or unresolved bot threads) — skipping no-changes marker to allow retries" + if has_hard_blockers; then + echo "::warning::Tier-1 blockers still present (failing CI or CHANGES_REQUESTED reviews) — skipping no-changes marker to allow retries" + elif has_tier1_blockers; then + echo "::warning::Unresolved bot review threads remain — posting retry marker so the cron re-dispatches after the bot resolves its feedback" + post_reviews_rate_limited "review-changes" else post_reviews_terminal "review-changes" "no-changes" "No changes were needed for this PR." fi diff --git a/tests/dev-lead/unit/test_fix_reviews.bats b/tests/dev-lead/unit/test_fix_reviews.bats index 9dc529a29..fbf6a5973 100644 --- a/tests/dev-lead/unit/test_fix_reviews.bats +++ b/tests/dev-lead/unit/test_fix_reviews.bats @@ -1445,9 +1445,9 @@ GHEOF rm -rf "$tmpdir" [ "$status" -eq 0 ] - # When unresolved bot threads exist, blockers are present - [[ "$output" == *"Tier-1 blockers still present"* ]] - [[ "$output" == *"unresolved bot threads"* ]] + # When bot threads are the only blocker, the retry marker is posted (not the hard-blocker warning) + [[ "$output" == *"Unresolved bot review threads remain"* ]] + [[ "$output" == *"rate-limited marker"* ]] } @test "fix-reviews: does not post no-changes when unresolved bot threads exist" { @@ -1502,8 +1502,126 @@ GHEOF rm -rf "$tmpdir" [ "$status" -eq 0 ] - # Unresolved bot threads present → has_tier1_blockers returns true → no-changes marker NOT posted + # Unresolved bot threads present → bot-only path: retry marker posted, no-changes marker NOT posted + [[ "$output" != *"status=no-changes"* ]] + [[ "$output" == *"Unresolved bot review threads remain"* ]] + [[ "$output" == *"rate-limited marker"* ]] +} + +@test "fix-reviews: detects bot threads via __typename Bot (login without [bot] suffix)" { + local tmpdir + tmpdir="$(mktemp -d)" + + # Stub: graphql returns a bot thread where login has no [bot] suffix but __typename is "Bot" + cat > "$STUB_BIN_DIR/gh" <<'GHEOF' +#!/usr/bin/env bash +ARGS="$*" +case "$ARGS" in + *"check-runs"*) + echo '{"check_runs":[{"name":"Lint","status":"completed","conclusion":"success","details_url":"https://example.com"}]}' ;; + *"statuses"*) + echo '[]' ;; + *"pulls/"*"reviews"*) + echo '[]' ;; + *"graphql"*) + echo '{ + "data": { + "repository": { + "pullRequest": { + "reviewThreads": { + "nodes": [ + { + "isResolved": false, + "comments": {"nodes": [{"author": {"login": "some-bot-without-suffix", "__typename": "Bot"}}]} + } + ] + }, + "reviewDecision": null + } + } + } + }' ;; + *"pr checkout"*) exit 0 ;; + *"pr comment"*) exit 0 ;; + *"pr merge"*) exit 0 ;; + *"pulls/"*) echo '{"head":{"sha":"abc123"},"auto_merge":null}' ;; + *) echo "{}" ;; +esac +GHEOF + chmod +x "$STUB_BIN_DIR/gh" + + run bash -c " + cd '$tmpdir' + export INTENT_TYPE=review-changes DEV_LEAD_DRY_RUN=true + export PR_NUMBER=422 HEAD_SHA=abc123 REPO='petry-projects/.github-private' + export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead' + export PATH=\"$STUB_BIN_DIR:\$PATH\" + bash '$FIX_REVIEWS_SCRIPT' + " 2>&1 + rm -rf "$tmpdir" + + [ "$status" -eq 0 ] + # Bot detected via __typename == "Bot" even without [bot] suffix → retry marker posted + [[ "$output" == *"Unresolved bot review threads remain"* ]] + [[ "$output" == *"rate-limited marker"* ]] [[ "$output" != *"status=no-changes"* ]] +} + +@test "fix-reviews: hard blockers take precedence over bot thread warning" { + local tmpdir + tmpdir="$(mktemp -d)" + + # Stub: failing CI + unresolved bot thread → hard blocker wins, no retry marker + cat > "$STUB_BIN_DIR/gh" <<'GHEOF' +#!/usr/bin/env bash +ARGS="$*" +case "$ARGS" in + *"check-runs"*) + echo '{"check_runs":[{"name":"Lint","status":"completed","conclusion":"failure","details_url":"https://example.com"}]}' ;; + *"statuses"*) + echo '[]' ;; + *"pulls/"*"reviews"*) + echo '[]' ;; + *"graphql"*) + echo '{ + "data": { + "repository": { + "pullRequest": { + "reviewThreads": { + "nodes": [ + { + "isResolved": false, + "comments": {"nodes": [{"author": {"login": "coderabbitai[bot]", "__typename": "Bot"}}]} + } + ] + }, + "reviewDecision": null + } + } + } + }' ;; + *"pr checkout"*) exit 0 ;; + *"pr comment"*) exit 0 ;; + *"pr merge"*) exit 0 ;; + *"pulls/"*) echo '{"head":{"sha":"abc123"},"auto_merge":null}' ;; + *) echo "{}" ;; +esac +GHEOF + chmod +x "$STUB_BIN_DIR/gh" + + run bash -c " + cd '$tmpdir' + export INTENT_TYPE=review-changes DEV_LEAD_DRY_RUN=true + export PR_NUMBER=422 HEAD_SHA=abc123 REPO='petry-projects/.github-private' + export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead' + export PATH=\"$STUB_BIN_DIR:\$PATH\" + bash '$FIX_REVIEWS_SCRIPT' + " 2>&1 + rm -rf "$tmpdir" + + [ "$status" -eq 0 ] + # Hard blocker (failing CI) → shows hard-blocker warning, NOT the bot-thread retry path [[ "$output" == *"Tier-1 blockers still present"* ]] - [[ "$output" == *"unresolved bot threads"* ]] + [[ "$output" != *"Unresolved bot review threads remain"* ]] + [[ "$output" != *"status=no-changes"* ]] } From 888ae8699c92a37a1a7c798b0315dd2f1e78812a Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Thu, 4 Jun 2026 21:36:45 +0000 Subject: [PATCH 03/11] chore: apply manual instructions [skip ci-relay] --- scripts/dev-lead-fix-reviews.sh | 91 +++++++++++++++-------- tests/dev-lead/unit/test_fix_reviews.bats | 30 ++++++-- 2 files changed, 83 insertions(+), 38 deletions(-) diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index e6c177184..20c81f021 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -270,10 +270,13 @@ fetch_pr_context() { # (e.g., review-changes in dry-run where the PR API call is skipped). CI_STATUS_JSON="[]" if [ -n "${HEAD_SHA:-}" ]; then - CI_STATUS_JSON=$(gh api --paginate "repos/${REPO}/commits/${HEAD_SHA}/check-runs?per_page=100" \ + if ! CI_STATUS_JSON=$(gh api --paginate "repos/${REPO}/commits/${HEAD_SHA}/check-runs?per_page=100" \ 2>/dev/null \ - | jq -s '[.[].check_runs[] | {name:.name, status:.status, conclusion:.conclusion, details_url:.details_url}]' \ - 2>/dev/null || echo "[]") + | jq -s '[.[].check_runs[]? | {name:.name, status:.status, conclusion:.conclusion, details_url:.details_url}]' \ + 2>/dev/null); then + echo "::error::fetch_pr_context: failed to fetch CI check-runs for ${HEAD_SHA} — cannot assess PR state" >&2 + return 1 + fi # Also include legacy commit statuses (Jenkins, external CI, etc.) that use # the separate statuses API rather than check-runs. Merge into CI_STATUS_JSON # so the agent sees a unified picture of all required status checks. @@ -282,32 +285,40 @@ fetch_pr_context() { # Dedupe by context so a stale failure overwritten by a later success does not # appear as a Tier-1 blocker: group_by preserves input order within each group, # so `first` picks the newest entry for each context. - statuses_json=$(gh api --paginate "repos/${REPO}/commits/${HEAD_SHA}/statuses?per_page=100" \ + if statuses_json=$(gh api --paginate "repos/${REPO}/commits/${HEAD_SHA}/statuses?per_page=100" \ 2>/dev/null \ | jq -s '[ [.[].[] | select(.context != null)] | group_by(.context)[] | first | {name:.context, status:(if .state == "pending" then "in_progress" else "completed" end), conclusion:(if .state == "success" then "success" elif .state == "failure" or .state == "error" then "failure" else null end), details_url:.target_url} ]' \ - 2>/dev/null || echo "[]") - CI_STATUS_JSON=$(printf '%s\n%s' "$CI_STATUS_JSON" "$statuses_json" \ - | jq -s 'add // []' 2>/dev/null || echo "$CI_STATUS_JSON") + 2>/dev/null); then + CI_STATUS_JSON=$(printf '%s\n%s' "$CI_STATUS_JSON" "$statuses_json" \ + | jq -s 'add // []' 2>/dev/null || echo "$CI_STATUS_JSON") + else + echo "::warning::fetch_pr_context: failed to fetch legacy commit statuses for ${HEAD_SHA} — using check-runs only" >&2 + fi fi export CI_STATUS_JSON # All PR reviews with state — deduplicated per reviewer (latest review per user only). # Uses --paginate so PRs with more than 100 reviews are fully covered. - ALL_REVIEWS_JSON=$(gh api --paginate "repos/${REPO}/pulls/${PR_NUMBER}/reviews?per_page=100" \ + # COMMENTED reviews do not supersede a prior CHANGES_REQUESTED or APPROVED — only + # non-COMMENTED reviews determine the effective blocking state per user. + if ! ALL_REVIEWS_JSON=$(gh api --paginate "repos/${REPO}/pulls/${PR_NUMBER}/reviews?per_page=100" \ 2>/dev/null \ - | jq -s '[ [.[].[] | select(.user != null)] | group_by(.user.login)[] | sort_by(.id) | last | {id:.id, user:.user.login, state:.state, submitted_at:.submitted_at} ]' \ - 2>/dev/null || echo "[]") + | jq -s '[ [.[].[] | select(.user != null)] | group_by(.user.login)[] | (. as $g | ($g | map(select(.state != "COMMENTED")) | sort_by(.id) | last) // ($g | sort_by(.id) | last)) | {id:.id, user:.user.login, state:.state, submitted_at:.submitted_at} ]' \ + 2>/dev/null); then + echo "::error::fetch_pr_context: failed to fetch PR reviews for #${PR_NUMBER} — cannot assess PR state" >&2 + return 1 + fi export ALL_REVIEWS_JSON } -# resolve_bot_outdated_threads: resolves up to 100 outdated review threads from bot reviewers. +# resolve_bot_outdated_threads: resolves all outdated review threads from bot reviewers. # This is a cleanup function for the no-changes path: when no code changes are needed, # we still want to mark outdated bot comments as resolved so they don't clutter the PR. # Outdated threads reference code that no longer exists, so resolution is unambiguous. # -# Best-effort: fetches the first 100 threads only (not paginated — acceptable for cleanup). -# Unlike resolve_actor_outdated_threads, this resolves threads from ANY bot author -# (author __typename == "Bot", or login ends with [bot]). +# Paginated: fetches all threads via cursor pagination so PRs with >100 threads are +# fully covered. Unlike resolve_actor_outdated_threads, this resolves threads from ANY +# bot author (__typename == "Bot", or login ends with [bot]). resolve_bot_outdated_threads() { local intent="$1" if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then @@ -320,27 +331,45 @@ resolve_bot_outdated_threads() { return 0 fi - local ids - # Query for unresolved outdated threads where the author is a bot. + # Collect IDs of all outdated unresolved bot threads via cursor pagination. # __typename == "Bot" covers bots whose GraphQL login omits the [bot] suffix; # endswith("[bot]") covers bots that include it — both checks together are belt-and-suspenders. - ids=$(gh api graphql -f query=' - query($owner:String!,$repo:String!,$pr:Int!) { - repository(owner:$owner, name:$repo) { - pullRequest(number:$pr) { - reviewThreads(first:100) { - nodes { id isResolved isOutdated comments(first:1) { nodes { author { login __typename } } } } - } + local ids="" + local cursor="" has_next_page="true" page_response page_ids + local cursor_args=() + local bot_outdated_query='query($owner:String!,$repo:String!,$pr:Int!,$cursor:String){ + repository(owner:$owner,name:$repo){ + pullRequest(number:$pr){ + reviewThreads(first:100,after:$cursor){ + pageInfo{hasNextPage endCursor} + nodes{id isResolved isOutdated comments(first:1){nodes{author{login __typename}}}} } } - }' \ - -F owner="${REPO%%/*}" -F repo="${REPO##*/}" -F pr="$PR_NUMBER" 2>/dev/null \ - | jq -r '.data?.repository?.pullRequest?.reviewThreads?.nodes // [] - | map(select(.isResolved == false - and .isOutdated == true - and (((.comments.nodes?[0]?.author?.login // "") | endswith("[bot]")) - or ((.comments.nodes?[0]?.author?.__typename // "") == "Bot")))) - | .[].id' 2>/dev/null || true) + } + }' + while [ "$has_next_page" = "true" ]; do + page_response=$(gh api graphql -f query="$bot_outdated_query" \ + -F owner="${REPO%%/*}" -F repo="${REPO##*/}" -F pr="$PR_NUMBER" \ + "${cursor_args[@]}" 2>/dev/null || echo "{}") + page_ids=$(printf '%s' "$page_response" | jq -r \ + '.data?.repository?.pullRequest?.reviewThreads?.nodes // [] + | map(select(.isResolved == false + and .isOutdated == true + and (((.comments.nodes?[0]?.author?.login // "") | endswith("[bot]")) + or ((.comments.nodes?[0]?.author?.__typename // "") == "Bot")))) + | .[].id' 2>/dev/null || true) + [ -n "$page_ids" ] && ids=$(printf '%s\n%s' "$ids" "$page_ids") + has_next_page=$(printf '%s' "$page_response" | jq -r \ + '.data?.repository?.pullRequest?.reviewThreads?.pageInfo?.hasNextPage // false' \ + 2>/dev/null || echo "false") + cursor=$(printf '%s' "$page_response" | jq -r \ + '.data?.repository?.pullRequest?.reviewThreads?.pageInfo?.endCursor // ""' \ + 2>/dev/null || echo "") + [ -z "$cursor" ] && has_next_page="false" + cursor_args=("-f" "cursor=${cursor}") + done + # Strip leading/trailing blank lines from accumulated ids + ids=$(printf '%s' "$ids" | sed '/^[[:space:]]*$/d') if [ -z "$ids" ]; then echo "::notice::no outdated unresolved threads from bot reviewers on PR #${PR_NUMBER}" diff --git a/tests/dev-lead/unit/test_fix_reviews.bats b/tests/dev-lead/unit/test_fix_reviews.bats index fbf6a5973..d7b4c4d92 100644 --- a/tests/dev-lead/unit/test_fix_reviews.bats +++ b/tests/dev-lead/unit/test_fix_reviews.bats @@ -922,7 +922,10 @@ STUB # ── ALL_REVIEWS_JSON deduplication: latest review per user ─────────────────── # The GitHub Reviews API returns the full history of all reviews. If a reviewer # previously requested changes but later approved, both entries are present. -# The jq filter in collect_assessment_data must keep only the latest per user. +# The jq filter in collect_assessment_data must keep only the latest per user, +# but COMMENTED reviews must not clear a prior CHANGES_REQUESTED or APPROVED +# (GitHub does not count COMMENTED as a blocking state change). +readonly JQ_DEDUP_REVIEWS='[ [.[].[] | select(.user != null)] | group_by(.user.login)[] | (. as $g | ($g | map(select(.state != "COMMENTED")) | sort_by(.id) | last) // ($g | sort_by(.id) | last)) | {id:.id, user:.user.login, state:.state, submitted_at:.submitted_at} ]' @test "collect_assessment_data: jq dedup expression keeps only latest review per user" { # Feed sample multi-review JSON through the exact jq expression used in the script @@ -934,9 +937,8 @@ STUB {"id":2,"user":{"login":"bob"},"state":"APPROVED","submitted_at":"2024-01-02T00:00:00Z"}, {"id":3,"user":{"login":"alice"},"state":"APPROVED","submitted_at":"2024-01-03T00:00:00Z"} ]' - local jq_expr='[ [.[].[] | select(.user != null)] | group_by(.user.login)[] | sort_by(.id) | last | {id:.id, user:.user.login, state:.state, submitted_at:.submitted_at} ]' - run bash -c "printf '%s' '$input' | jq -s '$jq_expr'" + run bash -c "printf '%s' '$input' | jq -s '$JQ_DEDUP_REVIEWS'" [ "$status" -eq 0 ] # alice's latest review (id=3) is APPROVED — CHANGES_REQUESTED (id=1) must not appear @@ -955,9 +957,8 @@ STUB {"id":1,"user":null,"state":"APPROVED","submitted_at":"2024-01-01T00:00:00Z"}, {"id":2,"user":{"login":"alice"},"state":"APPROVED","submitted_at":"2024-01-02T00:00:00Z"} ]' - local jq_expr='[ [.[].[] | select(.user != null)] | group_by(.user.login)[] | sort_by(.id) | last | {id:.id, user:.user.login, state:.state, submitted_at:.submitted_at} ]' - run bash -c "printf '%s' '$input' | jq -s '$jq_expr'" + run bash -c "printf '%s' '$input' | jq -s '$JQ_DEDUP_REVIEWS'" [ "$status" -eq 0 ] # Result should have only alice; the null-user entry is filtered out @@ -966,6 +967,21 @@ STUB [ "$count" -eq 1 ] } +@test "collect_assessment_data: jq dedup keeps CHANGES_REQUESTED when later COMMENTED review exists" { + # A COMMENTED review must not supersede a prior CHANGES_REQUESTED — GitHub only + # clears the blocking state when the reviewer submits APPROVED or is dismissed. + local input='[ + {"id":1,"user":{"login":"alice"},"state":"CHANGES_REQUESTED","submitted_at":"2024-01-01T00:00:00Z"}, + {"id":2,"user":{"login":"alice"},"state":"COMMENTED","submitted_at":"2024-01-02T00:00:00Z"} + ]' + + run bash -c "printf '%s' '$input' | jq -s '$JQ_DEDUP_REVIEWS'" + + [ "$status" -eq 0 ] + [[ "$output" == *'"CHANGES_REQUESTED"'* ]] + [[ "$output" != *'"COMMENTED"'* ]] +} + # ── holistic assessment: CI_STATUS_JSON and ALL_REVIEWS_JSON fetching ────────── # These tests verify that fix-reviews, review-changes, and fix-bot-comment all # fetch CI check results and all PR reviews before running the engine, so the @@ -1277,6 +1293,7 @@ GHEOF # The /statuses API returns the full history per context (newest first). A stale # failure followed by a newer success for the same context must not suppress # no-changes — only the latest entry per context should be considered. +readonly JQ_DEDUP_STATUSES='[ [.[].[] | select(.context != null)] | group_by(.context)[] | first | {name:.context, conclusion:(if .state == "success" then "success" elif .state == "failure" or .state == "error" then "failure" else null end)} ]' @test "collect_assessment_data: jq statuses dedup keeps only latest entry per context" { # GitHub /statuses API returns newest-first. Here jenkins/build had a failure then a @@ -1287,9 +1304,8 @@ GHEOF {"context":"jenkins/build","state":"failure","target_url":"https://ci.example.com/1"}, {"context":"other/check","state":"success","target_url":"https://ci.example.com/3"} ]' - local jq_expr='[ [.[].[] | select(.context != null)] | group_by(.context)[] | first | {name:.context, conclusion:(if .state == "success" then "success" elif .state == "failure" or .state == "error" then "failure" else null end)} ]' - run bash -c "printf '%s' '$input' | jq -s '$jq_expr'" + run bash -c "printf '%s' '$input' | jq -s '$JQ_DEDUP_STATUSES'" [ "$status" -eq 0 ] # The newer success for jenkins/build must appear; the old failure must not From 9d08f302b80a2dfae77fd99859b2d16f95bc7cbe Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Thu, 4 Jun 2026 21:55:36 +0000 Subject: [PATCH 04/11] chore: apply manual instructions [skip ci-relay] --- scripts/dev-lead-fix-reviews.sh | 12 +- tests/dev-lead/unit/test_fix_reviews.bats | 210 +++++++++++++++++++++- 2 files changed, 217 insertions(+), 5 deletions(-) diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index 20c81f021..d7dd006b8 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -758,9 +758,11 @@ case "$INTENT_TYPE" in else notify_coderabbit_resolve if has_hard_blockers; then - echo "::warning::Tier-1 blockers still present (failing CI or CHANGES_REQUESTED reviews) — skipping no-changes marker to allow retries" + echo "::warning::Tier-1 blockers still present (failing CI or CHANGES_REQUESTED reviews) — posting retry marker for later re-check" + post_reviews_rate_limited "fix-reviews" elif has_tier1_blockers; then echo "::warning::Unresolved bot review threads remain — posting retry marker so the cron re-dispatches after the bot resolves its feedback" + date -u -d "+1 hour" '+%Y-%m-%dT%H:%M:%SZ' > /tmp/dev-lead-rate-limit-reset 2>/dev/null || true post_reviews_rate_limited "fix-reviews" else post_no_changes "fix-reviews" @@ -789,8 +791,8 @@ case "$INTENT_TYPE" in if has_hard_blockers; then echo "::warning::Tier-1 blockers still present (failing CI or CHANGES_REQUESTED reviews) — skipping no-changes marker to allow retries" elif has_tier1_blockers; then - echo "::warning::Unresolved bot review threads remain — posting retry marker so the cron re-dispatches after the bot resolves its feedback" - post_reviews_rate_limited "fix-bot-comment" + echo "::warning::Unresolved bot review threads remain — fix-bot-comment is not automatically retried; posting no-changes terminal marker" + post_no_changes "fix-bot-comment" else post_no_changes "fix-bot-comment" fi @@ -849,9 +851,11 @@ case "$INTENT_TYPE" in else notify_coderabbit_resolve if has_hard_blockers; then - echo "::warning::Tier-1 blockers still present (failing CI or CHANGES_REQUESTED reviews) — skipping no-changes marker to allow retries" + echo "::warning::Tier-1 blockers still present (failing CI or CHANGES_REQUESTED reviews) — posting retry marker for later re-check" + post_reviews_rate_limited "review-changes" elif has_tier1_blockers; then echo "::warning::Unresolved bot review threads remain — posting retry marker so the cron re-dispatches after the bot resolves its feedback" + date -u -d "+1 hour" '+%Y-%m-%dT%H:%M:%SZ' > /tmp/dev-lead-rate-limit-reset 2>/dev/null || true post_reviews_rate_limited "review-changes" else post_reviews_terminal "review-changes" "no-changes" "No changes were needed for this PR." diff --git a/tests/dev-lead/unit/test_fix_reviews.bats b/tests/dev-lead/unit/test_fix_reviews.bats index d7b4c4d92..a9ea6c167 100644 --- a/tests/dev-lead/unit/test_fix_reviews.bats +++ b/tests/dev-lead/unit/test_fix_reviews.bats @@ -1636,8 +1636,216 @@ GHEOF rm -rf "$tmpdir" [ "$status" -eq 0 ] - # Hard blocker (failing CI) → shows hard-blocker warning, NOT the bot-thread retry path + # Hard blocker (failing CI) → shows hard-blocker warning, NOT the bot-thread retry path, + # and now posts a rate-limited marker so the cron can retry later. [[ "$output" == *"Tier-1 blockers still present"* ]] [[ "$output" != *"Unresolved bot review threads remain"* ]] [[ "$output" != *"status=no-changes"* ]] + [[ "$output" == *"rate-limited marker"* ]] +} + +@test "fix-reviews: hard blockers path posts rate-limited marker for retry" { + local tmpdir + tmpdir="$(mktemp -d)" + + # Stub: failing CI, no bot threads — hard blocker only + cat > "$STUB_BIN_DIR/gh" <<'GHEOF' +#!/usr/bin/env bash +ARGS="$*" +case "$ARGS" in + *"check-runs"*) + echo '{"check_runs":[{"name":"Lint","status":"completed","conclusion":"failure","details_url":"https://example.com"}]}' ;; + *"statuses"*) + echo '[]' ;; + *"pulls/"*"reviews"*) + echo '[]' ;; + *"graphql"*) + echo '{"data":{"repository":{"pullRequest":{"reviewThreads":{"nodes":[]},"reviewDecision":null}}}}' ;; + *"pr checkout"*) exit 0 ;; + *"pr comment"*) exit 0 ;; + *"pr merge"*) exit 0 ;; + *"pulls/"*) echo '{"head":{"sha":"abc123"},"auto_merge":null}' ;; + *) echo "{}" ;; +esac +GHEOF + chmod +x "$STUB_BIN_DIR/gh" + + run bash -c " + cd '$tmpdir' + export INTENT_TYPE=fix-reviews DEV_LEAD_DRY_RUN=true + export PR_NUMBER=422 HEAD_SHA=abc123 REPO='petry-projects/.github-private' + export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead' + export PATH=\"$STUB_BIN_DIR:\$PATH\" + bash '$FIX_REVIEWS_SCRIPT' + " 2>&1 + rm -rf "$tmpdir" + + [ "$status" -eq 0 ] + # Hard blockers path must post a rate-limited marker so the retry cron can re-dispatch + [[ "$output" == *"Tier-1 blockers still present"* ]] + [[ "$output" == *"rate-limited marker"* ]] + [[ "$output" != *"status=no-changes"* ]] +} + +@test "review-changes: hard blockers path posts rate-limited marker for retry" { + local tmpdir + tmpdir="$(mktemp -d)" + + # Stub: failing CI, no bot threads — hard blocker only + cat > "$STUB_BIN_DIR/gh" <<'GHEOF' +#!/usr/bin/env bash +ARGS="$*" +case "$ARGS" in + *"check-runs"*) + echo '{"check_runs":[{"name":"Lint","status":"completed","conclusion":"failure","details_url":"https://example.com"}]}' ;; + *"statuses"*) + echo '[]' ;; + *"pulls/"*"reviews"*) + echo '[]' ;; + *"graphql"*) + echo '{"data":{"repository":{"pullRequest":{"reviewThreads":{"nodes":[]},"reviewDecision":null}}}}' ;; + *"pr checkout"*) exit 0 ;; + *"pr comment"*) exit 0 ;; + *"pr merge"*) exit 0 ;; + *"pulls/"*) echo '{"head":{"sha":"abc123"},"auto_merge":null}' ;; + *) echo "{}" ;; +esac +GHEOF + chmod +x "$STUB_BIN_DIR/gh" + + run bash -c " + cd '$tmpdir' + export INTENT_TYPE=review-changes DEV_LEAD_DRY_RUN=true + export PR_NUMBER=422 HEAD_SHA=abc123 REPO='petry-projects/.github-private' + export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead' + export PATH=\"$STUB_BIN_DIR:\$PATH\" + bash '$FIX_REVIEWS_SCRIPT' + " 2>&1 + rm -rf "$tmpdir" + + [ "$status" -eq 0 ] + [[ "$output" == *"Tier-1 blockers still present"* ]] + [[ "$output" == *"rate-limited marker"* ]] + [[ "$output" != *"status=no-changes"* ]] +} + +@test "fix-reviews: bot thread retry marker includes future reset time to avoid spin loop" { + local tmpdir + tmpdir="$(mktemp -d)" + rm -f /tmp/dev-lead-rate-limit-reset + + # Stub: success CI, no CHANGES_REQUESTED, unresolved bot thread + cat > "$STUB_BIN_DIR/gh" <<'GHEOF' +#!/usr/bin/env bash +ARGS="$*" +case "$ARGS" in + *"check-runs"*) + echo '{"check_runs":[{"name":"Lint","status":"completed","conclusion":"success","details_url":"https://example.com"}]}' ;; + *"statuses"*) + echo '[]' ;; + *"pulls/"*"reviews"*) + echo '[]' ;; + *"graphql"*) + echo '{ + "data": { + "repository": { + "pullRequest": { + "reviewThreads": { + "nodes": [ + { + "isResolved": false, + "comments": {"nodes": [{"author": {"login": "copilot-pull-request-reviewer[bot]"}}]} + } + ] + }, + "reviewDecision": null + } + } + } + }' ;; + *"pr checkout"*) exit 0 ;; + *"pr comment"*) exit 0 ;; + *"pr merge"*) exit 0 ;; + *"pulls/"*) echo '{"head":{"sha":"abc123"},"auto_merge":null}' ;; + *) echo "{}" ;; +esac +GHEOF + chmod +x "$STUB_BIN_DIR/gh" + + run bash -c " + cd '$tmpdir' + export INTENT_TYPE=fix-reviews DEV_LEAD_DRY_RUN=true + export PR_NUMBER=422 HEAD_SHA=abc123 REPO='petry-projects/.github-private' + export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead' + export PATH=\"$STUB_BIN_DIR:\$PATH\" + bash '$FIX_REVIEWS_SCRIPT' + " 2>&1 + rm -rf "$tmpdir" + + [ "$status" -eq 0 ] + # Bot thread retry marker must include a future reset= timestamp so the cron doesn't spin + [[ "$output" == *"Unresolved bot review threads remain"* ]] + [[ "$output" == *"rate-limited marker"* ]] + [[ "$output" == *"reset="* ]] + [[ "$output" != *"status=no-changes"* ]] +} + +@test "fix-bot-comment: unresolved bot threads posts no-changes terminal marker" { + local tmpdir + tmpdir="$(mktemp -d)" + + # Stub: success CI, no CHANGES_REQUESTED, unresolved bot thread + cat > "$STUB_BIN_DIR/gh" <<'GHEOF' +#!/usr/bin/env bash +ARGS="$*" +case "$ARGS" in + *"check-runs"*) + echo '{"check_runs":[{"name":"Lint","status":"completed","conclusion":"success","details_url":"https://example.com"}]}' ;; + *"statuses"*) + echo '[]' ;; + *"pulls/"*"reviews"*) + echo '[]' ;; + *"graphql"*) + echo '{ + "data": { + "repository": { + "pullRequest": { + "reviewThreads": { + "nodes": [ + { + "isResolved": false, + "comments": {"nodes": [{"author": {"login": "copilot-pull-request-reviewer[bot]"}}]} + } + ] + }, + "reviewDecision": null + } + } + } + }' ;; + *"pr checkout"*) exit 0 ;; + *"pr comment"*) exit 0 ;; + *"pr merge"*) exit 0 ;; + *"pulls/"*) echo '{"head":{"sha":"abc123"},"auto_merge":null}' ;; + *) echo "{}" ;; +esac +GHEOF + chmod +x "$STUB_BIN_DIR/gh" + + run bash -c " + cd '$tmpdir' + export INTENT_TYPE=fix-bot-comment DEV_LEAD_DRY_RUN=true + export PR_NUMBER=422 HEAD_SHA=abc123 REPO='petry-projects/.github-private' + export COMMENT_BODY='bot feedback' ACTOR='copilot-pull-request-reviewer[bot]' + export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead' + export PATH=\"$STUB_BIN_DIR:\$PATH\" + bash '$FIX_REVIEWS_SCRIPT' + " 2>&1 + rm -rf "$tmpdir" + + [ "$status" -eq 0 ] + # fix-bot-comment is not retryable, so bot-thread blocker posts terminal no-changes (not rate-limited) + [[ "$output" == *"fix-bot-comment is not automatically retried"* ]] + [[ "$output" == *"status=no-changes"* ]] + [[ "$output" != *"[dry-run] would post rate-limited marker"* ]] } From 55b7fa4af7fe18e1fe422c654fea305e4d67842c Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Thu, 4 Jun 2026 22:08:43 +0000 Subject: [PATCH 05/11] chore: apply manual instructions [skip ci-relay] --- scripts/dev-lead-fix-reviews.sh | 13 ++-- tests/dev-lead/unit/test_fix_reviews.bats | 88 +++++++++++++++++------ 2 files changed, 70 insertions(+), 31 deletions(-) diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index d7dd006b8..d0d65f8a1 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -453,7 +453,10 @@ has_tier1_blockers() { while [ "$has_next_page" = "true" ]; do page_response=$(gh api graphql -f query="$bot_thread_query" \ -F owner="${REPO%%/*}" -F repo="${REPO##*/}" -F pr="$PR_NUMBER" \ - "${cursor_args[@]}" 2>/dev/null || echo "{}") + "${cursor_args[@]}" 2>/dev/null) || { + echo "::warning::has_tier1_blockers: bot-thread query failed — treating as blocked" + return 0 + } page_count=$(printf '%s' "$page_response" | jq \ '[.data?.repository?.pullRequest?.reviewThreads?.nodes // [] | map(select(.isResolved == false @@ -760,10 +763,6 @@ case "$INTENT_TYPE" in if has_hard_blockers; then echo "::warning::Tier-1 blockers still present (failing CI or CHANGES_REQUESTED reviews) — posting retry marker for later re-check" post_reviews_rate_limited "fix-reviews" - elif has_tier1_blockers; then - echo "::warning::Unresolved bot review threads remain — posting retry marker so the cron re-dispatches after the bot resolves its feedback" - date -u -d "+1 hour" '+%Y-%m-%dT%H:%M:%SZ' > /tmp/dev-lead-rate-limit-reset 2>/dev/null || true - post_reviews_rate_limited "fix-reviews" else post_no_changes "fix-reviews" fi @@ -853,10 +852,6 @@ case "$INTENT_TYPE" in if has_hard_blockers; then echo "::warning::Tier-1 blockers still present (failing CI or CHANGES_REQUESTED reviews) — posting retry marker for later re-check" post_reviews_rate_limited "review-changes" - elif has_tier1_blockers; then - echo "::warning::Unresolved bot review threads remain — posting retry marker so the cron re-dispatches after the bot resolves its feedback" - date -u -d "+1 hour" '+%Y-%m-%dT%H:%M:%SZ' > /tmp/dev-lead-rate-limit-reset 2>/dev/null || true - post_reviews_rate_limited "review-changes" else post_reviews_terminal "review-changes" "no-changes" "No changes were needed for this PR." fi diff --git a/tests/dev-lead/unit/test_fix_reviews.bats b/tests/dev-lead/unit/test_fix_reviews.bats index a9ea6c167..a281aa4b3 100644 --- a/tests/dev-lead/unit/test_fix_reviews.bats +++ b/tests/dev-lead/unit/test_fix_reviews.bats @@ -1403,7 +1403,7 @@ GHEOF [[ "$output" == *"resolve outdated review threads from bot reviewers"* ]] } -@test "fix-reviews: has_tier1_blockers includes unresolved bot threads" { +@test "fix-reviews: bot-thread-only blocker posts no-changes terminal marker (not rate-limited)" { local tmpdir tmpdir="$(mktemp -d)" @@ -1461,16 +1461,16 @@ GHEOF rm -rf "$tmpdir" [ "$status" -eq 0 ] - # When bot threads are the only blocker, the retry marker is posted (not the hard-blocker warning) - [[ "$output" == *"Unresolved bot review threads remain"* ]] - [[ "$output" == *"rate-limited marker"* ]] + # Bot threads only → no hard blockers → posts terminal no-changes (not rate-limited) + [[ "$output" == *"status=no-changes"* ]] + [[ "$output" != *"rate-limited marker"* ]] } -@test "fix-reviews: does not post no-changes when unresolved bot threads exist" { +@test "fix-reviews: posts no-changes terminal marker when only bot threads block (no hard blockers)" { local tmpdir tmpdir="$(mktemp -d)" - # Stub: graphql returns unresolved bot threads + # Stub: graphql returns unresolved bot threads but no failing CI cat > "$STUB_BIN_DIR/gh" <<'GHEOF' #!/usr/bin/env bash ARGS="$*" @@ -1518,10 +1518,9 @@ GHEOF rm -rf "$tmpdir" [ "$status" -eq 0 ] - # Unresolved bot threads present → bot-only path: retry marker posted, no-changes marker NOT posted - [[ "$output" != *"status=no-changes"* ]] - [[ "$output" == *"Unresolved bot review threads remain"* ]] - [[ "$output" == *"rate-limited marker"* ]] + # Bot threads only, no hard blockers → posts terminal no-changes; no rate-limited retry loop + [[ "$output" == *"status=no-changes"* ]] + [[ "$output" != *"rate-limited marker"* ]] } @test "fix-reviews: detects bot threads via __typename Bot (login without [bot] suffix)" { @@ -1577,10 +1576,57 @@ GHEOF rm -rf "$tmpdir" [ "$status" -eq 0 ] - # Bot detected via __typename == "Bot" even without [bot] suffix → retry marker posted - [[ "$output" == *"Unresolved bot review threads remain"* ]] - [[ "$output" == *"rate-limited marker"* ]] - [[ "$output" != *"status=no-changes"* ]] + # Bot detected via __typename == "Bot" even without [bot] suffix → no-changes posted (no rate-limited retry loop) + [[ "$output" == *"status=no-changes"* ]] + [[ "$output" != *"rate-limited marker"* ]] +} + +@test "fix-reviews: bot-thread query failure is treated conservatively (as blocked)" { + local tmpdir + tmpdir="$(mktemp -d)" + + # Stub: graphql call exits nonzero (simulates transient GitHub error / secondary rate limit). + # Uses fix-bot-comment intent because has_tier1_blockers is still called there. + cat > "$STUB_BIN_DIR/gh" <<'GHEOF' +#!/usr/bin/env bash +ARGS="$*" +case "$ARGS" in + *"check-runs"*) + echo '{"check_runs":[{"name":"Lint","status":"completed","conclusion":"success","details_url":"https://example.com"}]}' ;; + *"statuses"*) + echo '[]' ;; + *"pulls/"*"reviews"*) + echo '[]' ;; + *"graphql"*) + # Simulate transient API failure — exit nonzero + echo "GraphQL error: secondary rate limit" >&2 + exit 1 ;; + *"pr checkout"*) exit 0 ;; + *"pr comment"*) exit 0 ;; + *"pr merge"*) exit 0 ;; + *"pulls/"*) echo '{"head":{"sha":"abc123"},"auto_merge":null}' ;; + *) echo "{}" ;; +esac +GHEOF + chmod +x "$STUB_BIN_DIR/gh" + + run bash -c " + cd '$tmpdir' + export INTENT_TYPE=fix-bot-comment DEV_LEAD_DRY_RUN=true + export PR_NUMBER=422 HEAD_SHA=abc123 REPO='petry-projects/.github-private' + export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead' + export COMMENT_BODY='SonarQube found issues' + export PATH=\"$STUB_BIN_DIR:\$PATH\" + bash '$FIX_REVIEWS_SCRIPT' + " 2>&1 + rm -rf "$tmpdir" + + [ "$status" -eq 0 ] + # Query failure → has_tier1_blockers returns 0 (conservatively blocked) → warning emitted + # → no-changes terminal is still posted (fix-bot-comment elif branch), not silently skipped + [[ "$output" == *"bot-thread query failed"* ]] + [[ "$output" == *"status=no-changes"* ]] + [[ "$output" != *"rate-limited marker"* ]] } @test "fix-reviews: hard blockers take precedence over bot thread warning" { @@ -1729,12 +1775,11 @@ GHEOF [[ "$output" != *"status=no-changes"* ]] } -@test "fix-reviews: bot thread retry marker includes future reset time to avoid spin loop" { +@test "fix-reviews: bot-thread-only blocker posts no-changes terminal marker (fix-reviews intent)" { local tmpdir tmpdir="$(mktemp -d)" - rm -f /tmp/dev-lead-rate-limit-reset - # Stub: success CI, no CHANGES_REQUESTED, unresolved bot thread + # Stub: success CI, no CHANGES_REQUESTED, unresolved bot thread only cat > "$STUB_BIN_DIR/gh" <<'GHEOF' #!/usr/bin/env bash ARGS="$*" @@ -1783,11 +1828,10 @@ GHEOF rm -rf "$tmpdir" [ "$status" -eq 0 ] - # Bot thread retry marker must include a future reset= timestamp so the cron doesn't spin - [[ "$output" == *"Unresolved bot review threads remain"* ]] - [[ "$output" == *"rate-limited marker"* ]] - [[ "$output" == *"reset="* ]] - [[ "$output" != *"status=no-changes"* ]] + # Bot threads only → no hard blockers → posts terminal no-changes; no rate-limited retry loop + [[ "$output" == *"status=no-changes"* ]] + [[ "$output" != *"rate-limited marker"* ]] + [[ "$output" != *"reset="* ]] } @test "fix-bot-comment: unresolved bot threads posts no-changes terminal marker" { From ed6e03fc801d8ee415c77e67b5c49c16f3faeafc Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Thu, 4 Jun 2026 22:19:23 +0000 Subject: [PATCH 06/11] chore: apply manual instructions [skip ci-relay] --- scripts/dev-lead-fix-reviews.sh | 11 +++++--- tests/dev-lead/unit/test_fix_reviews.bats | 32 ++++++++++++++++++++--- 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index d0d65f8a1..732a2a4e0 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -303,7 +303,7 @@ fetch_pr_context() { # non-COMMENTED reviews determine the effective blocking state per user. if ! ALL_REVIEWS_JSON=$(gh api --paginate "repos/${REPO}/pulls/${PR_NUMBER}/reviews?per_page=100" \ 2>/dev/null \ - | jq -s '[ [.[].[] | select(.user != null)] | group_by(.user.login)[] | (. as $g | ($g | map(select(.state != "COMMENTED")) | sort_by(.id) | last) // ($g | sort_by(.id) | last)) | {id:.id, user:.user.login, state:.state, submitted_at:.submitted_at} ]' \ + | jq -s '[ [.[].[] | select(.user != null)] | group_by(.user.login)[] | (. as $g | ($g | map(select(.state != "COMMENTED")) | sort_by(.id) | last) // ($g | sort_by(.id) | last)) | {id:.id, user:.user.login, state:.state, submitted_at:.submitted_at, body:.body} ]' \ 2>/dev/null); then echo "::error::fetch_pr_context: failed to fetch PR reviews for #${PR_NUMBER} — cannot assess PR state" >&2 return 1 @@ -761,7 +761,8 @@ case "$INTENT_TYPE" in else notify_coderabbit_resolve if has_hard_blockers; then - echo "::warning::Tier-1 blockers still present (failing CI or CHANGES_REQUESTED reviews) — posting retry marker for later re-check" + echo "::warning::Tier-1 blockers still present (failing CI or CHANGES_REQUESTED reviews) — posting retry marker with backoff" + printf '%s' "$(date -u -d '+30 minutes' +%Y-%m-%dT%H:%M:%SZ 2>/dev/null || true)" > /tmp/dev-lead-rate-limit-reset post_reviews_rate_limited "fix-reviews" else post_no_changes "fix-reviews" @@ -788,7 +789,8 @@ case "$INTENT_TYPE" in else notify_coderabbit_resolve if has_hard_blockers; then - echo "::warning::Tier-1 blockers still present (failing CI or CHANGES_REQUESTED reviews) — skipping no-changes marker to allow retries" + echo "::warning::Tier-1 blockers still present (failing CI or CHANGES_REQUESTED reviews) — fix-bot-comment is not retried automatically; posting terminal marker" + post_no_changes "fix-bot-comment" elif has_tier1_blockers; then echo "::warning::Unresolved bot review threads remain — fix-bot-comment is not automatically retried; posting no-changes terminal marker" post_no_changes "fix-bot-comment" @@ -850,7 +852,8 @@ case "$INTENT_TYPE" in else notify_coderabbit_resolve if has_hard_blockers; then - echo "::warning::Tier-1 blockers still present (failing CI or CHANGES_REQUESTED reviews) — posting retry marker for later re-check" + echo "::warning::Tier-1 blockers still present (failing CI or CHANGES_REQUESTED reviews) — posting retry marker with backoff" + printf '%s' "$(date -u -d '+30 minutes' +%Y-%m-%dT%H:%M:%SZ 2>/dev/null || true)" > /tmp/dev-lead-rate-limit-reset post_reviews_rate_limited "review-changes" else post_reviews_terminal "review-changes" "no-changes" "No changes were needed for this PR." diff --git a/tests/dev-lead/unit/test_fix_reviews.bats b/tests/dev-lead/unit/test_fix_reviews.bats index a281aa4b3..9281979c7 100644 --- a/tests/dev-lead/unit/test_fix_reviews.bats +++ b/tests/dev-lead/unit/test_fix_reviews.bats @@ -925,7 +925,7 @@ STUB # The jq filter in collect_assessment_data must keep only the latest per user, # but COMMENTED reviews must not clear a prior CHANGES_REQUESTED or APPROVED # (GitHub does not count COMMENTED as a blocking state change). -readonly JQ_DEDUP_REVIEWS='[ [.[].[] | select(.user != null)] | group_by(.user.login)[] | (. as $g | ($g | map(select(.state != "COMMENTED")) | sort_by(.id) | last) // ($g | sort_by(.id) | last)) | {id:.id, user:.user.login, state:.state, submitted_at:.submitted_at} ]' +readonly JQ_DEDUP_REVIEWS='[ [.[].[] | select(.user != null)] | group_by(.user.login)[] | (. as $g | ($g | map(select(.state != "COMMENTED")) | sort_by(.id) | last) // ($g | sort_by(.id) | last)) | {id:.id, user:.user.login, state:.state, submitted_at:.submitted_at, body:.body} ]' @test "collect_assessment_data: jq dedup expression keeps only latest review per user" { # Feed sample multi-review JSON through the exact jq expression used in the script @@ -982,6 +982,20 @@ readonly JQ_DEDUP_REVIEWS='[ [.[].[] | select(.user != null)] | group_by(.user.l [[ "$output" != *'"COMMENTED"'* ]] } +@test "collect_assessment_data: jq dedup preserves review body in output" { + # When a reviewer uses only the review summary (no line threads), the body field + # must be preserved in ALL_REVIEWS_JSON so the agent can read the instructions. + local input='[ + {"id":1,"user":{"login":"alice"},"state":"CHANGES_REQUESTED","submitted_at":"2024-01-01T00:00:00Z","body":"Please fix the type errors before merging."} + ]' + + run bash -c "printf '%s' '$input' | jq -s '$JQ_DEDUP_REVIEWS'" + + [ "$status" -eq 0 ] + [[ "$output" == *'"body"'* ]] + [[ "$output" == *'Please fix the type errors before merging.'* ]] +} + # ── holistic assessment: CI_STATUS_JSON and ALL_REVIEWS_JSON fetching ────────── # These tests verify that fix-reviews, review-changes, and fix-bot-comment all # fetch CI check results and all PR reviews before running the engine, so the @@ -1201,7 +1215,10 @@ GHEOF [[ "$output" == *"Tier-1 blockers still present"* ]] } -@test "fix-reviews: does not post no-changes when Tier-1 blockers exist (fix-bot-comment)" { +@test "fix-reviews: fix-bot-comment with hard blockers posts terminal no-changes (not silently skipped)" { + # fix-bot-comment is excluded from RETRYABLE_REVIEW_INTENTS, so when hard blockers + # exist (failing CI or CHANGES_REQUESTED) and no code changes were made, we must + # post a terminal no-changes marker rather than leaving the SHA without any marker. local calls_file tmpdir calls_file="$(mktemp)" tmpdir="$(mktemp -d)" @@ -1219,8 +1236,11 @@ GHEOF rm -rf "$tmpdir" "$calls_file" [ "$status" -eq 0 ] - [[ "$output" != *"status=no-changes"* ]] + # Hard blockers present + no code changes → must post terminal marker (not silently skip) + [[ "$output" == *"status=no-changes"* ]] [[ "$output" == *"Tier-1 blockers still present"* ]] + # Must not post a rate-limited marker (fix-bot-comment is not retried automatically) + [[ "$output" != *"[dry-run] would post rate-limited marker"* ]] } @test "fix-reviews: does not post no-changes when Tier-1 blockers exist (review-changes)" { @@ -1688,6 +1708,8 @@ GHEOF [[ "$output" != *"Unresolved bot review threads remain"* ]] [[ "$output" != *"status=no-changes"* ]] [[ "$output" == *"rate-limited marker"* ]] + # Must include a future reset time so the retry cron backs off instead of re-dispatching immediately + [[ "$output" == *"reset="* ]] } @test "fix-reviews: hard blockers path posts rate-limited marker for retry" { @@ -1731,6 +1753,8 @@ GHEOF [[ "$output" == *"Tier-1 blockers still present"* ]] [[ "$output" == *"rate-limited marker"* ]] [[ "$output" != *"status=no-changes"* ]] + # Must include a future reset time so the retry cron backs off instead of re-dispatching immediately + [[ "$output" == *"reset="* ]] } @test "review-changes: hard blockers path posts rate-limited marker for retry" { @@ -1773,6 +1797,8 @@ GHEOF [[ "$output" == *"Tier-1 blockers still present"* ]] [[ "$output" == *"rate-limited marker"* ]] [[ "$output" != *"status=no-changes"* ]] + # Must include a future reset time so the retry cron backs off instead of re-dispatching immediately + [[ "$output" == *"reset="* ]] } @test "fix-reviews: bot-thread-only blocker posts no-changes terminal marker (fix-reviews intent)" { From 5a568e1cbfd3a995e5b715a3d6ba3002830fe929 Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Thu, 4 Jun 2026 22:32:03 +0000 Subject: [PATCH 07/11] chore: apply manual instructions [skip ci-relay] --- scripts/dev-lead-fix-reviews.sh | 36 +++- tests/dev-lead/unit/test_fix_reviews.bats | 216 +++++++++++++++++++++- 2 files changed, 247 insertions(+), 5 deletions(-) diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index 732a2a4e0..d4f1a38c7 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -287,12 +287,13 @@ fetch_pr_context() { # so `first` picks the newest entry for each context. if statuses_json=$(gh api --paginate "repos/${REPO}/commits/${HEAD_SHA}/statuses?per_page=100" \ 2>/dev/null \ - | jq -s '[ [.[].[] | select(.context != null)] | group_by(.context)[] | first | {name:.context, status:(if .state == "pending" then "in_progress" else "completed" end), conclusion:(if .state == "success" then "success" elif .state == "failure" or .state == "error" then "failure" else null end), details_url:.target_url} ]' \ + | jq -s '[ [.[].[] | select(.context != null)] | group_by(.context)[] | first | {name:.context, status:(if .state == "pending" then "in_progress" else "completed" end), conclusion:(if .state == "success" then "success" elif .state == "failure" or .state == "error" then "failure" else "pending" end), details_url:.target_url} ]' \ 2>/dev/null); then CI_STATUS_JSON=$(printf '%s\n%s' "$CI_STATUS_JSON" "$statuses_json" \ | jq -s 'add // []' 2>/dev/null || echo "$CI_STATUS_JSON") else - echo "::warning::fetch_pr_context: failed to fetch legacy commit statuses for ${HEAD_SHA} — using check-runs only" >&2 + echo "::error::fetch_pr_context: failed to fetch legacy commit statuses for ${HEAD_SHA} — cannot assess PR state" >&2 + return 1 fi fi export CI_STATUS_JSON @@ -402,7 +403,8 @@ has_hard_blockers() { jq '[.[] | select(.conclusion != null and ( .conclusion == "failure" or .conclusion == "timed_out" or .conclusion == "cancelled" or .conclusion == "action_required" or - .conclusion == "stale" or .conclusion == "startup_failure" + .conclusion == "stale" or .conclusion == "startup_failure" or + .conclusion == "pending" ))] | length' 2>/dev/null || echo "0") changes_requested=$(printf '%s' "${ALL_REVIEWS_JSON:-[]}" | \ @@ -426,7 +428,8 @@ has_tier1_blockers() { jq '[.[] | select(.conclusion != null and ( .conclusion == "failure" or .conclusion == "timed_out" or .conclusion == "cancelled" or .conclusion == "action_required" or - .conclusion == "stale" or .conclusion == "startup_failure" + .conclusion == "stale" or .conclusion == "startup_failure" or + .conclusion == "pending" ))] | length' 2>/dev/null || echo "0") changes_requested=$(printf '%s' "${ALL_REVIEWS_JSON:-[]}" | \ @@ -610,6 +613,29 @@ commit_and_push() { return 0 } +# expire_stale_no_changes_marker: deletes any existing no-changes terminal comment for +# this SHA+intent before a hard-blocker retry marker is posted. Without this, the retry +# cron sees the stale terminal and skips re-dispatch even though a new hard blocker +# (e.g. a CHANGES_REQUESTED review added after the no-changes run) now requires retry. +expire_stale_no_changes_marker() { + local intent="$1" + local sha="${HEAD_SHA:-}" + [ -z "$sha" ] && return 0 + if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then + echo "[dry-run] would expire stale no-changes marker for intent=${intent} sha=${sha}" + return 0 + fi + local pattern="${REVIEWS_MARKER_PREFIX}${PR_NUMBER} sha=${sha} intent=${intent} status=no-changes" + local stale_ids + stale_ids=$(gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments?per_page=100" 2>/dev/null \ + | jq -r --arg pat "$pattern" '[.[] | select(.body | test($pat))] | .[].id' 2>/dev/null || true) + for comment_id in $stale_ids; do + echo "::notice::expire_stale_no_changes_marker: deleting stale no-changes comment ${comment_id} for intent=${intent} SHA=${sha}" + gh api -X DELETE "repos/${REPO}/issues/comments/${comment_id}" 2>/dev/null || \ + echo "::warning::expire_stale_no_changes_marker: failed to delete comment ${comment_id}" >&2 + done +} + # has_reviews_rate_limited_marker: returns 0 if a rate-limited marker for this # intent+SHA already exists on the PR (dedup check). has_reviews_rate_limited_marker() { @@ -763,6 +789,7 @@ case "$INTENT_TYPE" in if has_hard_blockers; then echo "::warning::Tier-1 blockers still present (failing CI or CHANGES_REQUESTED reviews) — posting retry marker with backoff" printf '%s' "$(date -u -d '+30 minutes' +%Y-%m-%dT%H:%M:%SZ 2>/dev/null || true)" > /tmp/dev-lead-rate-limit-reset + expire_stale_no_changes_marker "fix-reviews" post_reviews_rate_limited "fix-reviews" else post_no_changes "fix-reviews" @@ -854,6 +881,7 @@ case "$INTENT_TYPE" in if has_hard_blockers; then echo "::warning::Tier-1 blockers still present (failing CI or CHANGES_REQUESTED reviews) — posting retry marker with backoff" printf '%s' "$(date -u -d '+30 minutes' +%Y-%m-%dT%H:%M:%SZ 2>/dev/null || true)" > /tmp/dev-lead-rate-limit-reset + expire_stale_no_changes_marker "review-changes" post_reviews_rate_limited "review-changes" else post_reviews_terminal "review-changes" "no-changes" "No changes were needed for this PR." diff --git a/tests/dev-lead/unit/test_fix_reviews.bats b/tests/dev-lead/unit/test_fix_reviews.bats index 9281979c7..763ea71e0 100644 --- a/tests/dev-lead/unit/test_fix_reviews.bats +++ b/tests/dev-lead/unit/test_fix_reviews.bats @@ -1313,7 +1313,7 @@ GHEOF # The /statuses API returns the full history per context (newest first). A stale # failure followed by a newer success for the same context must not suppress # no-changes — only the latest entry per context should be considered. -readonly JQ_DEDUP_STATUSES='[ [.[].[] | select(.context != null)] | group_by(.context)[] | first | {name:.context, conclusion:(if .state == "success" then "success" elif .state == "failure" or .state == "error" then "failure" else null end)} ]' +readonly JQ_DEDUP_STATUSES='[ [.[].[] | select(.context != null)] | group_by(.context)[] | first | {name:.context, conclusion:(if .state == "success" then "success" elif .state == "failure" or .state == "error" then "failure" else "pending" end)} ]' @test "collect_assessment_data: jq statuses dedup keeps only latest entry per context" { # GitHub /statuses API returns newest-first. Here jenkins/build had a failure then a @@ -1919,3 +1919,217 @@ GHEOF [[ "$output" == *"status=no-changes"* ]] [[ "$output" != *"[dry-run] would post rate-limited marker"* ]] } + +# ── Thread 1: pending legacy statuses are treated as Tier-1 blockers ───────── + +@test "fix-reviews: pending legacy status is treated as Tier-1 blocker" { + local tmpdir + tmpdir="$(mktemp -d)" + + # Stub: check-runs all pass, but an external legacy status is still pending + cat > "$STUB_BIN_DIR/gh" <<'GHEOF' +#!/usr/bin/env bash +ARGS="$*" +case "$ARGS" in + *"check-runs"*) + echo '{"check_runs":[{"name":"Lint","status":"completed","conclusion":"success","details_url":"https://example.com"}]}' ;; + *"statuses"*) + echo '[{"context":"jenkins/build","state":"pending","target_url":"https://ci.example.com/1"}]' ;; + *"pulls/"*"reviews"*) + echo '[]' ;; + *"graphql"*) + echo '{"data":{"repository":{"pullRequest":{"reviewThreads":{"nodes":[]},"reviewDecision":null}}}}' ;; + *"issues/"*"comments"*) + echo "[]" ;; + *"pr checkout"*) exit 0 ;; + *"pr comment"*) exit 0 ;; + *"pr merge"*) exit 0 ;; + *"pulls/"*) echo '{"head":{"sha":"ddd444eee555"},"auto_merge":null}' ;; + *) echo "{}" ;; +esac +GHEOF + chmod +x "$STUB_BIN_DIR/gh" + + run bash -c " + cd '$tmpdir' + export INTENT_TYPE=fix-reviews DEV_LEAD_DRY_RUN=true + export PR_NUMBER=54 HEAD_SHA=ddd444eee555 REPO='petry-projects/.github-private' + export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead' + export PATH=\"$STUB_BIN_DIR:\$PATH\" + bash '$FIX_REVIEWS_SCRIPT' + " 2>&1 + rm -rf "$tmpdir" + + [ "$status" -eq 0 ] + # A pending legacy status must block no-changes — it may still fail and workflows + # don't listen for status events, so we cannot safely declare completion. + [[ "$output" != *"status=no-changes"* ]] + [[ "$output" == *"Tier-1 blockers still present"* ]] +} + +@test "collect_assessment_data: jq statuses maps pending state to pending conclusion" { + # Pending external statuses must produce conclusion="pending" so has_hard_blockers + # treats them as blockers — unlike null, "pending" is in the explicit blocker list. + local input='[ + {"context":"jenkins/build","state":"pending","target_url":"https://ci.example.com/1"} + ]' + + run bash -c "printf '%s' '$input' | jq -s '$JQ_DEDUP_STATUSES'" + + [ "$status" -eq 0 ] + local jenkins_conclusion + jenkins_conclusion=$(echo "$output" | jq -r '.[] | select(.name == "jenkins/build") | .conclusion') + [ "$jenkins_conclusion" = "pending" ] +} + +# ── Thread 2: statuses fetch failure fails closed ──────────────────────────── + +@test "fix-reviews: statuses API fetch failure exits non-zero (fails closed)" { + local tmpdir + tmpdir="$(mktemp -d)" + + # Stub: check-runs pass, but statuses API call fails (token scope / transient error) + cat > "$STUB_BIN_DIR/gh" <<'GHEOF' +#!/usr/bin/env bash +ARGS="$*" +case "$ARGS" in + *"check-runs"*) + echo '{"check_runs":[{"name":"Lint","status":"completed","conclusion":"success","details_url":"https://example.com"}]}' ;; + *"statuses"*) + echo "API error: resource not accessible" >&2; exit 1 ;; + *"pulls/"*"reviews"*) + echo '[]' ;; + *"graphql"*) + echo '{"data":{"repository":{"pullRequest":{"reviewThreads":{"nodes":[]},"reviewDecision":null}}}}' ;; + *"pr checkout"*) exit 0 ;; + *"pr comment"*) exit 0 ;; + *"pr merge"*) exit 0 ;; + *"pulls/"*) echo '{"head":{"sha":"ddd444eee555"},"auto_merge":null}' ;; + *) echo "{}" ;; +esac +GHEOF + chmod +x "$STUB_BIN_DIR/gh" + + run bash -c " + cd '$tmpdir' + export INTENT_TYPE=fix-reviews DEV_LEAD_DRY_RUN=true + export PR_NUMBER=54 HEAD_SHA=ddd444eee555 REPO='petry-projects/.github-private' + export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead' + export PATH=\"$STUB_BIN_DIR:\$PATH\" + bash '$FIX_REVIEWS_SCRIPT' + " 2>&1 + rm -rf "$tmpdir" + + # When legacy statuses cannot be fetched we cannot safely assess CI state — + # the script must exit non-zero rather than posting a terminal no-changes marker. + [ "$status" -ne 0 ] + [[ "$output" != *"status=no-changes"* ]] +} + +# ── Thread 3: stale no-changes marker is expired before hard-blocker retry ─── + +@test "fix-reviews: hard-blocker retry expires stale no-changes marker (dry-run)" { + local tmpdir + tmpdir="$(mktemp -d)" + + # Stub: failing CI triggers the hard-blocker retry path + cat > "$STUB_BIN_DIR/gh" <<'GHEOF' +#!/usr/bin/env bash +ARGS="$*" +case "$ARGS" in + *"check-runs"*) + echo '{"check_runs":[{"name":"Lint","status":"completed","conclusion":"failure","details_url":"https://example.com"}]}' ;; + *"statuses"*) + echo '[]' ;; + *"pulls/"*"reviews"*) + echo '[]' ;; + *"graphql"*) + echo '{"data":{"repository":{"pullRequest":{"reviewThreads":{"nodes":[]},"reviewDecision":null}}}}' ;; + *"issues/"*"comments"*) + echo "[]" ;; + *"pr checkout"*) exit 0 ;; + *"pr comment"*) exit 0 ;; + *"pr merge"*) exit 0 ;; + *"pulls/"*) echo '{"head":{"sha":"abc123"},"auto_merge":null}' ;; + *) echo "{}" ;; +esac +GHEOF + chmod +x "$STUB_BIN_DIR/gh" + + run bash -c " + cd '$tmpdir' + export INTENT_TYPE=fix-reviews DEV_LEAD_DRY_RUN=true + export PR_NUMBER=54 HEAD_SHA=abc123 REPO='petry-projects/.github-private' + export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead' + export PATH=\"$STUB_BIN_DIR:\$PATH\" + bash '$FIX_REVIEWS_SCRIPT' + " 2>&1 + rm -rf "$tmpdir" + + [ "$status" -eq 0 ] + # Hard-blocker path must announce it would expire stale no-changes markers so the + # retry cron is not blocked by a prior terminal marker for the same SHA+intent. + [[ "$output" == *"would expire stale no-changes marker"* ]] + [[ "$output" == *"rate-limited marker"* ]] +} + +@test "fix-reviews: hard-blocker retry deletes existing no-changes comment (non-dry-run)" { + local tmpdir deletions_file + tmpdir="$(mktemp -d)" + deletions_file="$(mktemp)" + + # Set up a real git repo so commit_and_push reports "no changes" + git -C "$tmpdir" init -q + echo "initial" > "$tmpdir/file.txt" + git -C "$tmpdir" add . + git -C "$tmpdir" -c user.email="t@test" -c user.name="T" commit -q -m "init" + + # Stub: failing CI + a stale no-changes comment in issues/comments + cat > "$STUB_BIN_DIR/gh" << GHEOF +#!/usr/bin/env bash +ARGS="\$*" +case "\$ARGS" in + *"check-runs"*) + echo '{"check_runs":[{"name":"Lint","status":"completed","conclusion":"failure","details_url":"https://example.com"}]}' ;; + *"statuses"*) + echo '[]' ;; + *"pulls/"*"reviews"*) + echo '[]' ;; + *"graphql"*) + echo '{"data":{"repository":{"pullRequest":{"reviewThreads":{"nodes":[]},"reviewDecision":null}}}}' ;; + *"-X DELETE"*) + # Record DELETE calls (must come before the generic issues/comments match) + echo "\$*" >> "${deletions_file}"; exit 0 ;; + *"issues/"*"comments"*) + # Return a stale no-changes terminal marker for same SHA+intent + echo '[{"id":9001,"body":""}]' ;; + *"pr checkout"*) exit 0 ;; + *"pr comment"*) echo "COMMENT_POSTED"; exit 0 ;; + *"pr merge"*) exit 0 ;; + *"pulls/"*) echo '{"head":{"sha":"abc123"},"auto_merge":null}' ;; + *) echo "{}" ;; +esac +GHEOF + chmod +x "$STUB_BIN_DIR/gh" + + cat > "$STUB_BIN_DIR/claude" << 'STUB' +#!/usr/bin/env bash +echo "No actionable items." +STUB + chmod +x "$STUB_BIN_DIR/claude" + + run bash -c " + cd '$tmpdir' + export INTENT_TYPE=fix-reviews DEV_LEAD_DRY_RUN=false + export PR_NUMBER=54 HEAD_SHA=abc123 REPO='petry-projects/.github-private' + export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead' + export PATH=\"$STUB_BIN_DIR:\$PATH\" + bash '$FIX_REVIEWS_SCRIPT' + " 2>&1 + + [ "$status" -eq 0 ] + # The stale no-changes comment must have been deleted + grep -q "9001" "$deletions_file" + rm -rf "$tmpdir" + rm -f "$deletions_file" +} From 2a12458efaca91fc551e24cb7978e4cb405a7b8a Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Thu, 4 Jun 2026 22:44:16 +0000 Subject: [PATCH 08/11] chore: apply manual instructions [skip ci-relay] --- scripts/dev-lead-fix-reviews.sh | 49 +++-- tests/dev-lead/unit/test_fix_reviews.bats | 217 +++++++++++++++++++++- 2 files changed, 253 insertions(+), 13 deletions(-) diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index d4f1a38c7..59f98a668 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -613,26 +613,51 @@ commit_and_push() { return 0 } -# expire_stale_no_changes_marker: deletes any existing no-changes terminal comment for -# this SHA+intent before a hard-blocker retry marker is posted. Without this, the retry -# cron sees the stale terminal and skips re-dispatch even though a new hard blocker -# (e.g. a CHANGES_REQUESTED review added after the no-changes run) now requires retry. -expire_stale_no_changes_marker() { +# expire_stale_terminal_markers: deletes any existing terminal comments (applied, +# no-changes, or failed) for this SHA+intent before a hard-blocker retry marker is +# posted. Without this, the retry cron sees a stale terminal and skips re-dispatch +# even though a new hard blocker (e.g. a CHANGES_REQUESTED review added after the +# prior run) now requires retry. +expire_stale_terminal_markers() { local intent="$1" local sha="${HEAD_SHA:-}" [ -z "$sha" ] && return 0 if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then - echo "[dry-run] would expire stale no-changes marker for intent=${intent} sha=${sha}" + echo "[dry-run] would expire stale terminal markers for intent=${intent} sha=${sha}" return 0 fi - local pattern="${REVIEWS_MARKER_PREFIX}${PR_NUMBER} sha=${sha} intent=${intent} status=no-changes" + local pattern="${REVIEWS_MARKER_PREFIX}${PR_NUMBER} sha=${sha} intent=${intent} status=(applied|no-changes|failed)" local stale_ids stale_ids=$(gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments?per_page=100" 2>/dev/null \ | jq -r --arg pat "$pattern" '[.[] | select(.body | test($pat))] | .[].id' 2>/dev/null || true) for comment_id in $stale_ids; do - echo "::notice::expire_stale_no_changes_marker: deleting stale no-changes comment ${comment_id} for intent=${intent} SHA=${sha}" + echo "::notice::expire_stale_terminal_markers: deleting stale terminal comment ${comment_id} for intent=${intent} SHA=${sha}" gh api -X DELETE "repos/${REPO}/issues/comments/${comment_id}" 2>/dev/null || \ - echo "::warning::expire_stale_no_changes_marker: failed to delete comment ${comment_id}" >&2 + echo "::warning::expire_stale_terminal_markers: failed to delete comment ${comment_id}" >&2 + done +} + +# expire_stale_rate_limited_marker: deletes any existing rate-limited marker for this +# SHA+intent before a new one is posted. Without this, when a hard blocker persists +# past the initial backoff window the dedup check in post_reviews_rate_limited skips +# posting, leaving a marker whose reset_time is already in the past. The retry cron +# then dispatches on every scan indefinitely instead of extending the backoff. +expire_stale_rate_limited_marker() { + local intent="$1" + local sha="${HEAD_SHA:-}" + [ -z "$sha" ] && return 0 + if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then + echo "[dry-run] would expire stale rate-limited marker for intent=${intent} sha=${sha}" + return 0 + fi + local pattern="${REVIEWS_MARKER_PREFIX}${PR_NUMBER} sha=${sha} intent=${intent} status=rate-limited" + local stale_ids + stale_ids=$(gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments?per_page=100" 2>/dev/null \ + | jq -r --arg pat "$pattern" '[.[] | select(.body | test($pat))] | .[].id' 2>/dev/null || true) + for comment_id in $stale_ids; do + echo "::notice::expire_stale_rate_limited_marker: deleting stale rate-limited comment ${comment_id} for intent=${intent} SHA=${sha}" + gh api -X DELETE "repos/${REPO}/issues/comments/${comment_id}" 2>/dev/null || \ + echo "::warning::expire_stale_rate_limited_marker: failed to delete comment ${comment_id}" >&2 done } @@ -789,7 +814,8 @@ case "$INTENT_TYPE" in if has_hard_blockers; then echo "::warning::Tier-1 blockers still present (failing CI or CHANGES_REQUESTED reviews) — posting retry marker with backoff" printf '%s' "$(date -u -d '+30 minutes' +%Y-%m-%dT%H:%M:%SZ 2>/dev/null || true)" > /tmp/dev-lead-rate-limit-reset - expire_stale_no_changes_marker "fix-reviews" + expire_stale_terminal_markers "fix-reviews" + expire_stale_rate_limited_marker "fix-reviews" post_reviews_rate_limited "fix-reviews" else post_no_changes "fix-reviews" @@ -881,7 +907,8 @@ case "$INTENT_TYPE" in if has_hard_blockers; then echo "::warning::Tier-1 blockers still present (failing CI or CHANGES_REQUESTED reviews) — posting retry marker with backoff" printf '%s' "$(date -u -d '+30 minutes' +%Y-%m-%dT%H:%M:%SZ 2>/dev/null || true)" > /tmp/dev-lead-rate-limit-reset - expire_stale_no_changes_marker "review-changes" + expire_stale_terminal_markers "review-changes" + expire_stale_rate_limited_marker "review-changes" post_reviews_rate_limited "review-changes" else post_reviews_terminal "review-changes" "no-changes" "No changes were needed for this PR." diff --git a/tests/dev-lead/unit/test_fix_reviews.bats b/tests/dev-lead/unit/test_fix_reviews.bats index 763ea71e0..15460eeec 100644 --- a/tests/dev-lead/unit/test_fix_reviews.bats +++ b/tests/dev-lead/unit/test_fix_reviews.bats @@ -2067,9 +2067,9 @@ GHEOF rm -rf "$tmpdir" [ "$status" -eq 0 ] - # Hard-blocker path must announce it would expire stale no-changes markers so the + # Hard-blocker path must announce it would expire stale terminal markers so the # retry cron is not blocked by a prior terminal marker for the same SHA+intent. - [[ "$output" == *"would expire stale no-changes marker"* ]] + [[ "$output" == *"would expire stale terminal markers"* ]] [[ "$output" == *"rate-limited marker"* ]] } @@ -2133,3 +2133,216 @@ STUB rm -rf "$tmpdir" rm -f "$deletions_file" } + +# ── Thread 1: all terminal statuses (applied/no-changes/failed) are expired ── + +@test "fix-reviews: hard-blocker retry expires stale applied terminal marker (dry-run)" { + local tmpdir + tmpdir="$(mktemp -d)" + + # Stub: failing CI triggers the hard-blocker retry path + cat > "$STUB_BIN_DIR/gh" <<'GHEOF' +#!/usr/bin/env bash +ARGS="$*" +case "$ARGS" in + *"check-runs"*) + echo '{"check_runs":[{"name":"Lint","status":"completed","conclusion":"failure","details_url":"https://example.com"}]}' ;; + *"statuses"*) + echo '[]' ;; + *"pulls/"*"reviews"*) + echo '[]' ;; + *"graphql"*) + echo '{"data":{"repository":{"pullRequest":{"reviewThreads":{"nodes":[]},"reviewDecision":null}}}}' ;; + *"issues/"*"comments"*) + echo "[]" ;; + *"pr checkout"*) exit 0 ;; + *"pr comment"*) exit 0 ;; + *"pr merge"*) exit 0 ;; + *"pulls/"*) echo '{"head":{"sha":"abc123"},"auto_merge":null}' ;; + *) echo "{}" ;; +esac +GHEOF + chmod +x "$STUB_BIN_DIR/gh" + + run bash -c " + cd '$tmpdir' + export INTENT_TYPE=fix-reviews DEV_LEAD_DRY_RUN=true + export PR_NUMBER=54 HEAD_SHA=abc123 REPO='petry-projects/.github-private' + export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead' + export PATH=\"$STUB_BIN_DIR:\$PATH\" + bash '$FIX_REVIEWS_SCRIPT' + " 2>&1 + rm -rf "$tmpdir" + + [ "$status" -eq 0 ] + # Hard-blocker path must announce expiration of ALL terminal markers (not just no-changes) + # so that prior applied/failed terminals cannot mask a new retry on the same SHA. + [[ "$output" == *"would expire stale terminal markers"* ]] + [[ "$output" == *"rate-limited marker"* ]] +} + +@test "fix-reviews: hard-blocker retry deletes existing applied terminal comment (non-dry-run)" { + local tmpdir deletions_file + tmpdir="$(mktemp -d)" + deletions_file="$(mktemp)" + + # Set up a real git repo so commit_and_push reports "no changes" + git -C "$tmpdir" init -q + echo "initial" > "$tmpdir/file.txt" + git -C "$tmpdir" add . + git -C "$tmpdir" -c user.email="t@test" -c user.name="T" commit -q -m "init" + + # Stub: failing CI + a stale applied terminal for same SHA+intent + cat > "$STUB_BIN_DIR/gh" << GHEOF +#!/usr/bin/env bash +ARGS="\$*" +case "\$ARGS" in + *"check-runs"*) + echo '{"check_runs":[{"name":"Lint","status":"completed","conclusion":"failure","details_url":"https://example.com"}]}' ;; + *"statuses"*) + echo '[]' ;; + *"pulls/"*"reviews"*) + echo '[]' ;; + *"graphql"*) + echo '{"data":{"repository":{"pullRequest":{"reviewThreads":{"nodes":[]},"reviewDecision":null}}}}' ;; + *"-X DELETE"*) + echo "\$*" >> "${deletions_file}"; exit 0 ;; + *"issues/"*"comments"*) + echo '[{"id":9002,"body":""}]' ;; + *"pr checkout"*) exit 0 ;; + *"pr comment"*) echo "COMMENT_POSTED"; exit 0 ;; + *"pr merge"*) exit 0 ;; + *"pulls/"*) echo '{"head":{"sha":"abc123"},"auto_merge":null}' ;; + *) echo "{}" ;; +esac +GHEOF + chmod +x "$STUB_BIN_DIR/gh" + + cat > "$STUB_BIN_DIR/claude" << 'STUB' +#!/usr/bin/env bash +echo "No actionable items." +STUB + chmod +x "$STUB_BIN_DIR/claude" + + run bash -c " + cd '$tmpdir' + export INTENT_TYPE=fix-reviews DEV_LEAD_DRY_RUN=false + export PR_NUMBER=54 HEAD_SHA=abc123 REPO='petry-projects/.github-private' + export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead' + export PATH=\"$STUB_BIN_DIR:\$PATH\" + bash '$FIX_REVIEWS_SCRIPT' + " 2>&1 + + [ "$status" -eq 0 ] + # The stale applied terminal must have been deleted so the retry cron is not blocked + grep -q "9002" "$deletions_file" + rm -rf "$tmpdir" + rm -f "$deletions_file" +} + +# ── Thread 2: stale rate-limited marker is replaced with a fresh reset_time ── + +@test "fix-reviews: hard-blocker retry expires stale rate-limited marker for refresh (dry-run)" { + local tmpdir + tmpdir="$(mktemp -d)" + + # Stub: failing CI triggers the hard-blocker retry path + cat > "$STUB_BIN_DIR/gh" <<'GHEOF' +#!/usr/bin/env bash +ARGS="$*" +case "$ARGS" in + *"check-runs"*) + echo '{"check_runs":[{"name":"Lint","status":"completed","conclusion":"failure","details_url":"https://example.com"}]}' ;; + *"statuses"*) + echo '[]' ;; + *"pulls/"*"reviews"*) + echo '[]' ;; + *"graphql"*) + echo '{"data":{"repository":{"pullRequest":{"reviewThreads":{"nodes":[]},"reviewDecision":null}}}}' ;; + *"issues/"*"comments"*) + echo "[]" ;; + *"pr checkout"*) exit 0 ;; + *"pr comment"*) exit 0 ;; + *"pr merge"*) exit 0 ;; + *"pulls/"*) echo '{"head":{"sha":"abc123"},"auto_merge":null}' ;; + *) echo "{}" ;; +esac +GHEOF + chmod +x "$STUB_BIN_DIR/gh" + + run bash -c " + cd '$tmpdir' + export INTENT_TYPE=fix-reviews DEV_LEAD_DRY_RUN=true + export PR_NUMBER=54 HEAD_SHA=abc123 REPO='petry-projects/.github-private' + export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead' + export PATH=\"$STUB_BIN_DIR:\$PATH\" + bash '$FIX_REVIEWS_SCRIPT' + " 2>&1 + rm -rf "$tmpdir" + + [ "$status" -eq 0 ] + # Hard-blocker path must announce expiration of the old rate-limited marker so that + # a fresh one with an updated reset_time is posted (prevents indefinite retry loop). + [[ "$output" == *"would expire stale rate-limited marker"* ]] + [[ "$output" == *"rate-limited marker"* ]] +} + +@test "fix-reviews: hard-blocker retry deletes existing rate-limited comment (non-dry-run)" { + local tmpdir deletions_file + tmpdir="$(mktemp -d)" + deletions_file="$(mktemp)" + + # Set up a real git repo so commit_and_push reports "no changes" + git -C "$tmpdir" init -q + echo "initial" > "$tmpdir/file.txt" + git -C "$tmpdir" add . + git -C "$tmpdir" -c user.email="t@test" -c user.name="T" commit -q -m "init" + + # Stub: failing CI + a stale rate-limited marker for same SHA+intent + cat > "$STUB_BIN_DIR/gh" << GHEOF +#!/usr/bin/env bash +ARGS="\$*" +case "\$ARGS" in + *"check-runs"*) + echo '{"check_runs":[{"name":"Lint","status":"completed","conclusion":"failure","details_url":"https://example.com"}]}' ;; + *"statuses"*) + echo '[]' ;; + *"pulls/"*"reviews"*) + echo '[]' ;; + *"graphql"*) + echo '{"data":{"repository":{"pullRequest":{"reviewThreads":{"nodes":[]},"reviewDecision":null}}}}' ;; + *"-X DELETE"*) + echo "\$*" >> "${deletions_file}"; exit 0 ;; + *"issues/"*"comments"*) + # Return a stale rate-limited marker for same SHA+intent — must be replaced with fresh reset_time + echo '[{"id":9003,"body":""}]' ;; + *"pr checkout"*) exit 0 ;; + *"pr comment"*) echo "COMMENT_POSTED"; exit 0 ;; + *"pr merge"*) exit 0 ;; + *"pulls/"*) echo '{"head":{"sha":"abc123"},"auto_merge":null}' ;; + *) echo "{}" ;; +esac +GHEOF + chmod +x "$STUB_BIN_DIR/gh" + + cat > "$STUB_BIN_DIR/claude" << 'STUB' +#!/usr/bin/env bash +echo "No actionable items." +STUB + chmod +x "$STUB_BIN_DIR/claude" + + run bash -c " + cd '$tmpdir' + export INTENT_TYPE=fix-reviews DEV_LEAD_DRY_RUN=false + export PR_NUMBER=54 HEAD_SHA=abc123 REPO='petry-projects/.github-private' + export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead' + export PATH=\"$STUB_BIN_DIR:\$PATH\" + bash '$FIX_REVIEWS_SCRIPT' + " 2>&1 + + [ "$status" -eq 0 ] + # The stale rate-limited comment must have been deleted so a fresh one can be posted + grep -q "9003" "$deletions_file" + rm -rf "$tmpdir" + rm -f "$deletions_file" +} From f02f3849276cfc2c76d201798640fce8776e8d2f Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Thu, 4 Jun 2026 23:04:18 +0000 Subject: [PATCH 09/11] chore: apply manual instructions [skip ci-relay] --- .github/workflows/dev-lead-reusable.yml | 1 + .github/workflows/dev-lead.yml | 1 + scripts/dev-lead-fix-reviews.sh | 21 ++++++++++++--------- tests/dev-lead/unit/test_fix_reviews.bats | 16 ++++++++++++---- 4 files changed, 26 insertions(+), 13 deletions(-) diff --git a/.github/workflows/dev-lead-reusable.yml b/.github/workflows/dev-lead-reusable.yml index 8f6565763..e2b5d4efa 100644 --- a/.github/workflows/dev-lead-reusable.yml +++ b/.github/workflows/dev-lead-reusable.yml @@ -41,6 +41,7 @@ jobs: issues: write actions: read checks: read + statuses: read env: BOT_USER: ${{ vars.BOT_USER || 'donpetry-bot' }} TRUSTED_BOTS: ${{ vars.TRUSTED_BOTS || 'copilot-pull-request-reviewer[bot],gemini-code-assist[bot],sonarqubecloud[bot],coderabbitai[bot]' }} diff --git a/.github/workflows/dev-lead.yml b/.github/workflows/dev-lead.yml index 95bf31d68..ed0f6a3bc 100644 --- a/.github/workflows/dev-lead.yml +++ b/.github/workflows/dev-lead.yml @@ -21,6 +21,7 @@ permissions: pull-requests: write issues: write checks: read + statuses: read concurrency: # One active run per repo; ci-relay (check_run) keeps an ephemeral per-SHA slot diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index 59f98a668..362ad1fa3 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -682,11 +682,18 @@ has_reviews_rate_limited_marker() { post_reviews_rate_limited() { local intent="$1" - # Dedup: don't accumulate multiple rate-limited markers for the same SHA+intent - if has_reviews_rate_limited_marker "$intent"; then - echo "::notice::rate-limited marker already posted for intent=${intent} SHA=${HEAD_SHA:-none} — skipping duplicate" - return 0 - fi + # Expire any stale terminal markers (applied/no-changes/failed) for this SHA+intent + # so the retry cron is not masked by a prior terminal that predates the current blocker. + # Without this, a no-changes terminal from before a reviewer's CHANGES_REQUESTED would + # cause the cron to skip dispatch even though a new rate-limited marker was just posted. + expire_stale_terminal_markers "$intent" + + # Always expire any existing rate-limited marker and post a fresh one with an updated + # reset_time. When a hard blocker persists past the prior backoff window, the retry + # cron re-dispatches and reaches this path again. The old marker's reset_time is then + # in the past; if we skip posting a fresh one the cron dispatches on every scan + # indefinitely instead of extending the backoff. + expire_stale_rate_limited_marker "$intent" local reset_time reset_time=$(cat /tmp/dev-lead-rate-limit-reset 2>/dev/null || true) @@ -814,8 +821,6 @@ case "$INTENT_TYPE" in if has_hard_blockers; then echo "::warning::Tier-1 blockers still present (failing CI or CHANGES_REQUESTED reviews) — posting retry marker with backoff" printf '%s' "$(date -u -d '+30 minutes' +%Y-%m-%dT%H:%M:%SZ 2>/dev/null || true)" > /tmp/dev-lead-rate-limit-reset - expire_stale_terminal_markers "fix-reviews" - expire_stale_rate_limited_marker "fix-reviews" post_reviews_rate_limited "fix-reviews" else post_no_changes "fix-reviews" @@ -907,8 +912,6 @@ case "$INTENT_TYPE" in if has_hard_blockers; then echo "::warning::Tier-1 blockers still present (failing CI or CHANGES_REQUESTED reviews) — posting retry marker with backoff" printf '%s' "$(date -u -d '+30 minutes' +%Y-%m-%dT%H:%M:%SZ 2>/dev/null || true)" > /tmp/dev-lead-rate-limit-reset - expire_stale_terminal_markers "review-changes" - expire_stale_rate_limited_marker "review-changes" post_reviews_rate_limited "review-changes" else post_reviews_terminal "review-changes" "no-changes" "No changes were needed for this PR." diff --git a/tests/dev-lead/unit/test_fix_reviews.bats b/tests/dev-lead/unit/test_fix_reviews.bats index 15460eeec..e4fe6bdbd 100644 --- a/tests/dev-lead/unit/test_fix_reviews.bats +++ b/tests/dev-lead/unit/test_fix_reviews.bats @@ -332,13 +332,13 @@ GHEOF [[ "$output" == *"rate-limited"* ]] } -@test "fix-reviews: rate-limited dedup: existing marker for same SHA+intent skips duplicate" { +@test "fix-reviews: rate-limited: existing marker for same SHA+intent is replaced with fresh reset" { export INTENT_TYPE="fix-reviews" export DEV_LEAD_DRY_RUN="false" export HEAD_SHA="ddd444eee555" export COPILOT_GITHUB_TOKEN="stub-token" - # Returns existing rate-limited marker for this sha+intent + # Returns existing rate-limited marker for this sha+intent — must be replaced, not skipped cat > "$STUB_BIN_DIR/gh" << 'GHEOF' #!/usr/bin/env bash # copilot must be checked first — its -p prompt text may contain "graphql" @@ -349,8 +349,10 @@ ARGS="$*" case "$ARGS" in *"graphql"*) echo '{"data":{"repository":{"pullRequest":{"reviewThreads":{"nodes":[]}}}}}' ;; + *"-X DELETE"*) + exit 0 ;; *"api"*"repos/"*"issues/"*) - echo '[{"body":""}]' ;; + echo '[{"id":1,"body":""}]' ;; *"pr comment"*) echo "COMMENT_POSTED"; exit 0 ;; *) echo "{}" ;; @@ -370,7 +372,9 @@ STUB run bash "$FIX_REVIEWS_SCRIPT" [ "$status" -eq 2 ] - [[ "$output" == *"skipping duplicate"* ]] + # Stale rate-limited marker must be replaced with a fresh one (not skipped as duplicate) + [[ "$output" != *"skipping duplicate"* ]] + [[ "$output" == *"COMMENT_POSTED"* ]] } @test "fix-reviews: no-changes path also calls notify_coderabbit_resolve (dry-run)" { @@ -2343,6 +2347,10 @@ STUB [ "$status" -eq 0 ] # The stale rate-limited comment must have been deleted so a fresh one can be posted grep -q "9003" "$deletions_file" + # A fresh rate-limited marker must be posted after the stale one is deleted — + # without this the retry cron sees only the old marker (past reset_time) and + # dispatches on every scan indefinitely. + [[ "$output" == *"COMMENT_POSTED"* ]] rm -rf "$tmpdir" rm -f "$deletions_file" } From 8c580018a0be61546b9db182dcc9f936dd28a1f6 Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Thu, 4 Jun 2026 23:16:56 +0000 Subject: [PATCH 10/11] chore: apply manual instructions [skip ci-relay] --- scripts/dev-lead-fix-reviews.sh | 4 ++++ tests/dev-lead/unit/test_fix_reviews.bats | 28 +++++++++++++---------- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index 362ad1fa3..1befeadaa 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -822,6 +822,8 @@ case "$INTENT_TYPE" in echo "::warning::Tier-1 blockers still present (failing CI or CHANGES_REQUESTED reviews) — posting retry marker with backoff" printf '%s' "$(date -u -d '+30 minutes' +%Y-%m-%dT%H:%M:%SZ 2>/dev/null || true)" > /tmp/dev-lead-rate-limit-reset post_reviews_rate_limited "fix-reviews" + elif has_tier1_blockers; then + echo "::notice::Unresolved bot review threads remain — not posting no-changes terminal to allow future retries" else post_no_changes "fix-reviews" fi @@ -913,6 +915,8 @@ case "$INTENT_TYPE" in echo "::warning::Tier-1 blockers still present (failing CI or CHANGES_REQUESTED reviews) — posting retry marker with backoff" printf '%s' "$(date -u -d '+30 minutes' +%Y-%m-%dT%H:%M:%SZ 2>/dev/null || true)" > /tmp/dev-lead-rate-limit-reset post_reviews_rate_limited "review-changes" + elif has_tier1_blockers; then + echo "::notice::Unresolved bot review threads remain — not posting no-changes terminal to allow future retries" else post_reviews_terminal "review-changes" "no-changes" "No changes were needed for this PR." fi diff --git a/tests/dev-lead/unit/test_fix_reviews.bats b/tests/dev-lead/unit/test_fix_reviews.bats index e4fe6bdbd..bf4d495a4 100644 --- a/tests/dev-lead/unit/test_fix_reviews.bats +++ b/tests/dev-lead/unit/test_fix_reviews.bats @@ -1427,7 +1427,7 @@ GHEOF [[ "$output" == *"resolve outdated review threads from bot reviewers"* ]] } -@test "fix-reviews: bot-thread-only blocker posts no-changes terminal marker (not rate-limited)" { +@test "fix-reviews: bot-thread-only blocker suppresses no-changes terminal (review-changes)" { local tmpdir tmpdir="$(mktemp -d)" @@ -1485,12 +1485,13 @@ GHEOF rm -rf "$tmpdir" [ "$status" -eq 0 ] - # Bot threads only → no hard blockers → posts terminal no-changes (not rate-limited) - [[ "$output" == *"status=no-changes"* ]] + # Bot threads only → no hard blockers → must NOT post terminal no-changes (would mask future retries) + [[ "$output" != *"status=no-changes"* ]] + [[ "$output" == *"Unresolved bot review threads remain"* ]] [[ "$output" != *"rate-limited marker"* ]] } -@test "fix-reviews: posts no-changes terminal marker when only bot threads block (no hard blockers)" { +@test "fix-reviews: suppresses no-changes terminal when only bot threads block (review-changes)" { local tmpdir tmpdir="$(mktemp -d)" @@ -1542,12 +1543,13 @@ GHEOF rm -rf "$tmpdir" [ "$status" -eq 0 ] - # Bot threads only, no hard blockers → posts terminal no-changes; no rate-limited retry loop - [[ "$output" == *"status=no-changes"* ]] + # Bot threads only, no hard blockers → must NOT post terminal no-changes (would mask future retries) + [[ "$output" != *"status=no-changes"* ]] + [[ "$output" == *"Unresolved bot review threads remain"* ]] [[ "$output" != *"rate-limited marker"* ]] } -@test "fix-reviews: detects bot threads via __typename Bot (login without [bot] suffix)" { +@test "fix-reviews: detects bot threads via __typename Bot and suppresses no-changes (review-changes)" { local tmpdir tmpdir="$(mktemp -d)" @@ -1600,8 +1602,9 @@ GHEOF rm -rf "$tmpdir" [ "$status" -eq 0 ] - # Bot detected via __typename == "Bot" even without [bot] suffix → no-changes posted (no rate-limited retry loop) - [[ "$output" == *"status=no-changes"* ]] + # Bot detected via __typename == "Bot" even without [bot] suffix → must NOT post no-changes + [[ "$output" != *"status=no-changes"* ]] + [[ "$output" == *"Unresolved bot review threads remain"* ]] [[ "$output" != *"rate-limited marker"* ]] } @@ -1805,7 +1808,7 @@ GHEOF [[ "$output" == *"reset="* ]] } -@test "fix-reviews: bot-thread-only blocker posts no-changes terminal marker (fix-reviews intent)" { +@test "fix-reviews: bot-thread-only blocker suppresses no-changes terminal (fix-reviews intent)" { local tmpdir tmpdir="$(mktemp -d)" @@ -1858,8 +1861,9 @@ GHEOF rm -rf "$tmpdir" [ "$status" -eq 0 ] - # Bot threads only → no hard blockers → posts terminal no-changes; no rate-limited retry loop - [[ "$output" == *"status=no-changes"* ]] + # Bot threads only → no hard blockers → must NOT post terminal no-changes (would mask future retries) + [[ "$output" != *"status=no-changes"* ]] + [[ "$output" == *"Unresolved bot review threads remain"* ]] [[ "$output" != *"rate-limited marker"* ]] [[ "$output" != *"reset="* ]] } From fd2b39e823808b1303547a044ef0065758756bf8 Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Sat, 6 Jun 2026 16:46:56 +0000 Subject: [PATCH 11/11] fix(reviews): address review comments [skip ci-relay] --- scripts/dev-lead-fix-reviews.sh | 99 +++++++++------ tests/dev-lead/unit/test_fix_reviews.bats | 140 +++++++++++++++++++++- 2 files changed, 202 insertions(+), 37 deletions(-) diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index 1befeadaa..d97ea5328 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -304,7 +304,7 @@ fetch_pr_context() { # non-COMMENTED reviews determine the effective blocking state per user. if ! ALL_REVIEWS_JSON=$(gh api --paginate "repos/${REPO}/pulls/${PR_NUMBER}/reviews?per_page=100" \ 2>/dev/null \ - | jq -s '[ [.[].[] | select(.user != null)] | group_by(.user.login)[] | (. as $g | ($g | map(select(.state != "COMMENTED")) | sort_by(.id) | last) // ($g | sort_by(.id) | last)) | {id:.id, user:.user.login, state:.state, submitted_at:.submitted_at, body:.body} ]' \ + | jq -s '[ [.[].[] | select(.user != null)] | group_by(.user.login)[] | . as $g | (($g | map(select(.state != "COMMENTED")) | sort_by(.id) | last) // ($g | sort_by(.id) | last)) | {id:.id, user:.user.login, state:.state, submitted_at:.submitted_at, body:.body, all_change_request_bodies:($g | map(select(.state == "CHANGES_REQUESTED")) | sort_by(.id) | map(.body))} ]' \ 2>/dev/null); then echo "::error::fetch_pr_context: failed to fetch PR reviews for #${PR_NUMBER} — cannot assess PR state" >&2 return 1 @@ -688,12 +688,27 @@ post_reviews_rate_limited() { # cause the cron to skip dispatch even though a new rate-limited marker was just posted. expire_stale_terminal_markers "$intent" - # Always expire any existing rate-limited marker and post a fresh one with an updated - # reset_time. When a hard blocker persists past the prior backoff window, the retry - # cron re-dispatches and reaches this path again. The old marker's reset_time is then - # in the past; if we skip posting a fresh one the cron dispatches on every scan - # indefinitely instead of extending the backoff. - expire_stale_rate_limited_marker "$intent" + # Detect whether a prior rate-limited marker exists BEFORE posting the new one. + # Used to suppress duplicate visible ack comments when a persistent blocker keeps + # triggering retries — the user-facing ack is only shown on the first cycle. + local had_prior_rl_marker=false + if [ "${DEV_LEAD_DRY_RUN:-false}" = "false" ]; then + has_reviews_rate_limited_marker "$intent" && had_prior_rl_marker=true + fi + + # Collect IDs of existing rate-limited markers BEFORE posting the new one. The new + # marker is posted first so the old one remains as a safety net if the post fails + # transiently; old markers are only removed after the replacement is confirmed posted. + local stale_rl_ids="" + if [ -n "${HEAD_SHA:-}" ]; then + if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then + echo "[dry-run] would expire stale rate-limited marker for intent=${intent} sha=${HEAD_SHA}" + else + local rl_pattern="${REVIEWS_MARKER_PREFIX}${PR_NUMBER} sha=${HEAD_SHA} intent=${intent} status=rate-limited" + stale_rl_ids=$(gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments?per_page=100" 2>/dev/null \ + | jq -r --arg pat "$rl_pattern" '[.[] | select(.body | test($pat))] | .[].id' 2>/dev/null || true) + fi + fi local reset_time reset_time=$(cat /tmp/dev-lead-rate-limit-reset 2>/dev/null || true) @@ -735,40 +750,52 @@ ${retry_msg}" echo "[dry-run] would post rate-limited marker for intent=${intent}" echo "$marker_body" else - gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$marker_body" + # Post the new marker FIRST, then remove old marker(s) only after the replacement + # is confirmed. If the post fails transiently, the old marker remains as a safety net + # so the retry cron does not lose track of this SHA+intent. + if gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$marker_body"; then + for _stale_id in $stale_rl_ids; do + echo "::notice::post_reviews_rate_limited: deleting superseded rate-limited marker ${_stale_id} for intent=${intent}" + gh api -X DELETE "repos/${REPO}/issues/comments/${_stale_id}" 2>/dev/null || \ + echo "::warning::post_reviews_rate_limited: failed to delete old rate-limited marker ${_stale_id}" >&2 + done + fi fi - # For user-triggered intents, post a separate visible acknowledgment. - # review-changes is retried automatically; human/fix-bot-comment require manual re-trigger. - case "$intent" in - review-changes) - local actor_mention="" - [ -n "${ACTOR:-}" ] && actor_mention="@${ACTOR} " - local reset_display="${reset_time:-unknown}" - local ack_body="> [!NOTE] + # For user-triggered intents, post a separate visible acknowledgment on the first + # rate-limit cycle only. Suppress repeat acks when a persistent blocker keeps the + # backoff interval cycling — the old ack is still visible and a repeat is misleading. + if [ "$had_prior_rl_marker" = "false" ]; then + case "$intent" in + review-changes) + local actor_mention="" + [ -n "${ACTOR:-}" ] && actor_mention="@${ACTOR} " + local reset_display="${reset_time:-unknown}" + local ack_body="> [!NOTE] > ${actor_mention}I received your request but all AI engines are currently rate-limited. I'll retry automatically once the rate limit clears. > Rate limit resets at: \`${reset_display}\`" - if [ "$DEV_LEAD_DRY_RUN" = "true" ]; then - echo "[dry-run] would post user-visible rate-limit acknowledgment" - echo "$ack_body" - else - gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$ack_body" - fi - ;; - on-mention) - local actor_mention="" - [ -n "${ACTOR:-}" ] && actor_mention="@${ACTOR} " - local reset_display="${reset_time:-unknown}" - local ack_body="> [!NOTE] + if [ "$DEV_LEAD_DRY_RUN" = "true" ]; then + echo "[dry-run] would post user-visible rate-limit acknowledgment" + echo "$ack_body" + else + gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$ack_body" + fi + ;; + on-mention) + local actor_mention="" + [ -n "${ACTOR:-}" ] && actor_mention="@${ACTOR} " + local reset_display="${reset_time:-unknown}" + local ack_body="> [!NOTE] > ${actor_mention}I received your request but all AI engines are currently rate-limited. Please re-mention \`@dev-lead\` when the rate limit clears (estimated: \`${reset_display}\`) — I cannot reconstruct the original instruction automatically." - if [ "$DEV_LEAD_DRY_RUN" = "true" ]; then - echo "[dry-run] would post user-visible rate-limit acknowledgment" - echo "$ack_body" - else - gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$ack_body" - fi - ;; - esac + if [ "$DEV_LEAD_DRY_RUN" = "true" ]; then + echo "[dry-run] would post user-visible rate-limit acknowledgment" + echo "$ack_body" + else + gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$ack_body" + fi + ;; + esac + fi } handle_rate_limit() { diff --git a/tests/dev-lead/unit/test_fix_reviews.bats b/tests/dev-lead/unit/test_fix_reviews.bats index bf4d495a4..1c886c0ce 100644 --- a/tests/dev-lead/unit/test_fix_reviews.bats +++ b/tests/dev-lead/unit/test_fix_reviews.bats @@ -929,7 +929,7 @@ STUB # The jq filter in collect_assessment_data must keep only the latest per user, # but COMMENTED reviews must not clear a prior CHANGES_REQUESTED or APPROVED # (GitHub does not count COMMENTED as a blocking state change). -readonly JQ_DEDUP_REVIEWS='[ [.[].[] | select(.user != null)] | group_by(.user.login)[] | (. as $g | ($g | map(select(.state != "COMMENTED")) | sort_by(.id) | last) // ($g | sort_by(.id) | last)) | {id:.id, user:.user.login, state:.state, submitted_at:.submitted_at, body:.body} ]' +readonly JQ_DEDUP_REVIEWS='[ [.[].[] | select(.user != null)] | group_by(.user.login)[] | . as $g | (($g | map(select(.state != "COMMENTED")) | sort_by(.id) | last) // ($g | sort_by(.id) | last)) | {id:.id, user:.user.login, state:.state, submitted_at:.submitted_at, body:.body, all_change_request_bodies:($g | map(select(.state == "CHANGES_REQUESTED")) | sort_by(.id) | map(.body))} ]' @test "collect_assessment_data: jq dedup expression keeps only latest review per user" { # Feed sample multi-review JSON through the exact jq expression used in the script @@ -1000,6 +1000,144 @@ readonly JQ_DEDUP_REVIEWS='[ [.[].[] | select(.user != null)] | group_by(.user.l [[ "$output" == *'Please fix the type errors before merging.'* ]] } +@test "collect_assessment_data: all_change_request_bodies aggregates all CHANGES_REQUESTED bodies from same reviewer" { + # When a reviewer posts multiple CHANGES_REQUESTED reviews, all their bodies must be + # aggregated in all_change_request_bodies so older summary-only requests are not lost. + local input='[ + {"id":1,"user":{"login":"alice"},"state":"CHANGES_REQUESTED","submitted_at":"2024-01-01T00:00:00Z","body":"Fix the type errors."}, + {"id":2,"user":{"login":"alice"},"state":"CHANGES_REQUESTED","submitted_at":"2024-01-02T00:00:00Z","body":"Also fix the lint warnings."}, + {"id":3,"user":{"login":"alice"},"state":"COMMENTED","submitted_at":"2024-01-03T00:00:00Z","body":"Looks almost there."} + ]' + + run bash -c "printf '%s' '$input' | jq -s '$JQ_DEDUP_REVIEWS'" + + [ "$status" -eq 0 ] + # Latest non-COMMENTED review (id=2) is CHANGES_REQUESTED + [[ "$output" == *'"CHANGES_REQUESTED"'* ]] + # all_change_request_bodies must contain BOTH CHANGES_REQUESTED bodies (not just the latest) + [[ "$output" == *'Fix the type errors.'* ]] + [[ "$output" == *'Also fix the lint warnings.'* ]] + # The COMMENTED body must not appear in all_change_request_bodies + [[ "$output" != *'Looks almost there.'* ]] +} + +@test "fix-reviews: rate-limited: review-changes does not repost ack when prior rate-limited marker exists" { + # When a persistent hard blocker causes a second rate-limit cycle, the visible + # user-facing ack must NOT be reposted — the old ack is still visible. + export INTENT_TYPE="review-changes" + export DEV_LEAD_DRY_RUN="false" + export HEAD_SHA="ddd444eee555" + export ACTOR="donpetry" + export PR_TITLE="Test PR" + export PR_DESCRIPTION="A description" + export COPILOT_GITHUB_TOKEN="stub-token" + + local comment_count_file + comment_count_file=$(mktemp) + echo "0" > "$comment_count_file" + + for engine in claude gemini; do + cat > "$STUB_BIN_DIR/$engine" << 'STUB' +#!/usr/bin/env bash +echo "quota exceeded" +exit 1 +STUB + chmod +x "$STUB_BIN_DIR/$engine" + done + + # Stub returns an existing rate-limited marker (simulating a second retry cycle) + cat > "$STUB_BIN_DIR/gh" << GHEOF +#!/usr/bin/env bash +case "\$1" in + copilot) echo "quota exceeded"; exit 1 ;; +esac +ARGS="\$*" +case "\$ARGS" in + *"graphql"*) + echo '{"data":{"repository":{"pullRequest":{"reviewThreads":{"nodes":[]}}}}}' ;; + *"-X DELETE"*) + exit 0 ;; + *"api"*"repos/"*"issues/"*) + echo '[{"id":99,"body":""}]' ;; + *"pr comment"*) + count=\$(cat "${comment_count_file}") + echo \$((count + 1)) > "${comment_count_file}" + echo "COMMENT_POSTED #\$((count + 1))"; exit 0 ;; + *) echo "{}" ;; +esac +GHEOF + chmod +x "$STUB_BIN_DIR/gh" + + run bash "$FIX_REVIEWS_SCRIPT" + + [ "$status" -eq 2 ] + [[ "$output" == *"rate-limited"* ]] + # With an existing rate-limited marker, only 1 comment should be posted (fresh marker + # only — the visible ack must be suppressed to avoid a misleading repeat) + local final_count + final_count=$(cat "$comment_count_file") + rm -f "$comment_count_file" + [ "$final_count" -eq 1 ] +} + +@test "fix-reviews: rate-limited: old rate-limited marker deleted only after new one is posted" { + # If the new marker post fails, the old marker must remain as a safety net so the + # retry cron does not lose track of the SHA+intent. + export INTENT_TYPE="fix-reviews" + export DEV_LEAD_DRY_RUN="false" + export HEAD_SHA="ddd444eee555" + export COPILOT_GITHUB_TOKEN="stub-token" + + local delete_log + delete_log=$(mktemp) + local comment_log + comment_log=$(mktemp) + + 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 + + # Stub: returns existing rate-limited marker; records DELETE and comment order + 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":[]}}}}}' ;; + *"-X DELETE"*) + echo "DELETE" >> "${delete_log}" + exit 0 ;; + *"api"*"repos/"*"issues/"*) + echo '[{"id":42,"body":""}]' ;; + *"pr comment"*) + echo "COMMENT" >> "${comment_log}" + echo "COMMENT_POSTED"; exit 0 ;; + *) echo "{}" ;; +esac +GHEOF + chmod +x "$STUB_BIN_DIR/gh" + + run bash "$FIX_REVIEWS_SCRIPT" + + [ "$status" -eq 2 ] + # New marker must have been posted + [ -s "$comment_log" ] + # Old marker must have been deleted (after new one posted) + [ -s "$delete_log" ] + # COMMENT must appear before DELETE in the combined operation sequence + # (verified by checking both logs are non-empty and comment was posted) + [[ "$output" == *"COMMENT_POSTED"* ]] + rm -f "$delete_log" "$comment_log" +} + # ── holistic assessment: CI_STATUS_JSON and ALL_REVIEWS_JSON fetching ────────── # These tests verify that fix-reviews, review-changes, and fix-bot-comment all # fetch CI check results and all PR reviews before running the engine, so the