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 ed889a799..d97ea5328 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -261,6 +261,226 @@ 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 + 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); 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. + 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. + 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 "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 "::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 + + # 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. + # 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)[] | . 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 + fi + 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. +# +# 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 + 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 + + # 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. + 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}}}} + } + } + } + }' + 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}" + 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_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" or + .conclusion == "pending" + ))] | 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) +# - 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" or + .conclusion == "pending" + ))] | length' 2>/dev/null || echo "0") + + changes_requested=$(printf '%s' "${ALL_REVIEWS_JSON:-[]}" | \ + jq '[.[] | select(.state == "CHANGES_REQUESTED")] | length' \ + 2>/dev/null || echo "0") + + # 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 + 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}}}} + } + } + } + }' + 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 "::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 + 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 ] +} + # 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. @@ -393,6 +613,54 @@ commit_and_push() { return 0 } +# 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 terminal markers for intent=${intent} sha=${sha}" + return 0 + fi + 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_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_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 +} + # 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() { @@ -414,10 +682,32 @@ 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 + # 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" + + # 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 @@ -460,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() { @@ -533,6 +835,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 +845,18 @@ 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_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 + 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 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 +865,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 +875,18 @@ 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_hard_blockers; then + 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" + 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 +928,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 +938,18 @@ 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_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 + 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 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..1c886c0ce 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)" { @@ -918,3 +922,1577 @@ 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, +# 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, 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 + # 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"} + ]' + + 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 + [[ "$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"} + ]' + + 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 + local count + count=$(echo "$output" | jq 'length') + [ "$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"'* ]] +} + +@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.'* ]] +} + +@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 +# 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: 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)" + _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 ] + # 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)" { + 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. +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 + # 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"} + ]' + + 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 + 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: bot-thread-only blocker suppresses no-changes terminal (review-changes)" { + 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 ] + # 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: suppresses no-changes terminal when only bot threads block (review-changes)" { + local tmpdir + tmpdir="$(mktemp -d)" + + # Stub: graphql returns unresolved bot threads but no failing CI + 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 ] + # 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 and suppresses no-changes (review-changes)" { + 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 → must NOT post no-changes + [[ "$output" != *"status=no-changes"* ]] + [[ "$output" == *"Unresolved bot review threads remain"* ]] + [[ "$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" { + 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, + # 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"* ]] + # 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" { + 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"* ]] + # 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" { + 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"* ]] + # 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 suppresses no-changes terminal (fix-reviews intent)" { + local tmpdir + tmpdir="$(mktemp -d)" + + # Stub: success CI, no CHANGES_REQUESTED, unresolved bot thread 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":"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 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="* ]] +} + +@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"* ]] +} + +# ── 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 terminal markers so the + # retry cron is not blocked by a prior terminal marker for the same SHA+intent. + [[ "$output" == *"would expire stale terminal markers"* ]] + [[ "$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" +} + +# ── 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" + # 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" +}