diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 2a9b645b0..e19454d03 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -110,6 +110,7 @@ jobs: tests/test_engine_unavailable_notice.bats \ tests/test_batch_skip_reporting.bats \ tests/test_review_one_pr_ci_pending.bats \ + tests/test_review_one_pr_force_ci_failing.bats \ tests/test_release_registry.bats \ tests/test_readme_refresh_helpers.bats \ tests/test_validate_personas.bats \ diff --git a/scripts/review-one-pr.sh b/scripts/review-one-pr.sh index a9355b47c..329df383b 100755 --- a/scripts/review-one-pr.sh +++ b/scripts/review-one-pr.sh @@ -139,9 +139,24 @@ echo " review decision: $REVIEW_DECISION" # no-op and does not count it against the MAX_PRS review budget. # Reasons that produce a skip: already-reviewed-at-head, ci-failing, ci-pending, changes-requested. if [ "$CI_STATUS" = "failing" ]; then - echo " skip: CI checks are failing — will re-evaluate after fixes are pushed" - echo "{\"pr\":\"$PR_URL\",\"sha\":\"$PR_HEAD_SHA\",\"decision\":\"skip\",\"reason\":\"ci-failing\"}" - exit 100 + if [ "${FORCE_REVIEW:-false}" = "true" ]; then + # Break-glass (#619): a manual mention / dispatch (FORCE_REVIEW=true) overrides + # the ci-failing gate so a fix to the CI gate *itself* can be reviewed and + # approved through ring-0 self-host — the one case the pinned-stable design + # otherwise can't ship without an out-of-band admin merge. This is safe: + # • FORCE_REVIEW is manual-only — it is true only for repository_dispatch + # (human @mention) or an explicit force_review input; the scheduled sweep + # dispatches with force_review=false, so this never fires event-driven. + # • GitHub's ruleset still blocks the merge on any failing REQUIRED check, so + # an override only unblocks the codeowner-approval gate for PRs whose + # failing checks are non-required (e.g. superseded dev-lead orchestration + # jobs) — it cannot merge a PR with a genuinely failing required check. + echo "::warning::force-review: CI is failing but FORCE_REVIEW=true — bypassing the ci-failing gate (break-glass, #619). Required-check failures still block the merge at the ruleset." + else + echo " skip: CI checks are failing — will re-evaluate after fixes are pushed" + echo "{\"pr\":\"$PR_URL\",\"sha\":\"$PR_HEAD_SHA\",\"decision\":\"skip\",\"reason\":\"ci-failing\"}" + exit 100 + fi fi # Pending CI gate: for normal runs, skip immediately. @@ -188,9 +203,13 @@ if [ "$CI_STATUS" = "pending" ]; then unset _poll _FORCE_POLL_MAX _FORCE_POLL_SEC _gh_poll_err _gh_poll_err_content if [ "$CI_STATUS" = "failing" ]; then - echo " skip: CI checks are failing (detected after polling) — will re-evaluate after fixes are pushed" - echo "{\"pr\":\"$PR_URL\",\"sha\":\"$PR_HEAD_SHA\",\"decision\":\"skip\",\"reason\":\"ci-failing\"}" - exit 100 + if [ "${FORCE_REVIEW:-false}" = "true" ]; then + echo "::warning::force-review: CI is failing but FORCE_REVIEW=true — bypassing the ci-failing gate (break-glass, #619). Required-check failures still block the merge at the ruleset." + else + echo " skip: CI checks are failing (detected after polling) — will re-evaluate after fixes are pushed" + echo "{\"pr\":\"$PR_URL\",\"sha\":\"$PR_HEAD_SHA\",\"decision\":\"skip\",\"reason\":\"ci-failing\"}" + exit 100 + fi fi if [ "$CI_STATUS" = "pending" ]; then @@ -275,24 +294,6 @@ fi fi } -# Skip when a human has requested changes, with two guards: -# 1. FORCE_REVIEW bypasses the skip — mention-triggered runs always proceed so -# authors can request a re-review after addressing feedback. -# 2. Only skip when a CHANGES_REQUESTED review targets the current head SHA. -# On repos that don't dismiss stale reviews, reviewDecision can stay -# CHANGES_REQUESTED after the author pushes new commits; in that case the -# review is stale and the cascade should re-engage with the updated code. -if [ "$REVIEW_DECISION" = "CHANGES_REQUESTED" ] && [ "${FORCE_REVIEW:-false}" != "true" ]; then - CHANGES_REQUESTED_AT_HEAD=$(echo "$PR_SNAPSHOT" | jq -r --arg sha "$PR_HEAD_SHA" ' - [.reviews[] | select(.state == "CHANGES_REQUESTED" and .commit.oid == $sha)] - | if length > 0 then "true" else "false" end - ') - if [ "$CHANGES_REQUESTED_AT_HEAD" = "true" ]; then - echo " skip: changes requested at current head — awaiting author response before reviewing" - echo "{\"pr\":\"$PR_URL\",\"sha\":\"$PR_HEAD_SHA\",\"decision\":\"skip\",\"reason\":\"changes-requested\"}" - exit 100 - fi -fi # Skip when a human has requested changes, with two guards: # 1. FORCE_REVIEW bypasses the skip — mention-triggered runs always proceed so # authors can request a re-review after addressing feedback. diff --git a/tests/test_review_one_pr_force_ci_failing.bats b/tests/test_review_one_pr_force_ci_failing.bats new file mode 100644 index 000000000..c0520824e --- /dev/null +++ b/tests/test_review_one_pr_force_ci_failing.bats @@ -0,0 +1,56 @@ +#!/usr/bin/env bats +# Guards the #619 break-glass in scripts/review-one-pr.sh: +# a manual FORCE_REVIEW=true run overrides the ci-failing gate (so a fix to the +# CI gate itself can be reviewed through ring-0 self-host), while a normal run +# still skips on failing CI. FORCE_REVIEW is manual-only (repository_dispatch / +# explicit input); the scheduled sweep dispatches with force_review=false. + +setup() { + REPO_ROOT="$(cd "$(dirname "$BATS_TEST_FILENAME")/.." && pwd)" + export REVIEW_SCRIPT="$REPO_ROOT/scripts/review-one-pr.sh" + export SHA="deadbeefdeadbeefdeadbeefdeadbeefdeadbeef" + export PR_URL="https://github.com/petry-projects/.github-private/pull/999" + export TEST_DIR="$BATS_TEST_TMPDIR" + mkdir -p "$TEST_DIR/bin"; cd "$TEST_DIR" + # A single external check FAILED -> compute_ci_status classifies "failing". + cat > "$TEST_DIR/snapshot.json" < "$TEST_DIR/bin/gh" < "$TEST_DIR/bin/$e"; chmod +x "$TEST_DIR/bin/$e"; done + export PATH="$TEST_DIR/bin:$PATH" + export REVIEW_ENGINE="claude" GH_TOKEN="fake" DRY_RUN="true" +} +teardown() { rm -rf "$TEST_DIR"; } + +@test "normal run (no FORCE_REVIEW): failing CI → skip reason=ci-failing, exit 100" { + unset FORCE_REVIEW + run timeout 25 bash "$REVIEW_SCRIPT" "$PR_URL" + [ "$status" -eq 100 ] + [[ "$output" == *'"reason":"ci-failing"'* ]] + [[ "$output" != *"bypassing the ci-failing gate"* ]] +} + +@test "FORCE_REVIEW=true: failing CI → ci-failing gate bypassed (break-glass #619)" { + export FORCE_REVIEW="true" + run timeout 25 bash "$REVIEW_SCRIPT" "$PR_URL" + # Must NOT emit the ci-failing skip; must announce the break-glass bypass. + [[ "$output" != *'"reason":"ci-failing"'* ]] + [[ "$output" == *"bypassing the ci-failing gate"* ]] + [ "$status" -ne 100 ] +}