diff --git a/bin/fm-pr-merge.sh b/bin/fm-pr-merge.sh index 174c3188dae..0b8aa79aeb2 100755 --- a/bin/fm-pr-merge.sh +++ b/bin/fm-pr-merge.sh @@ -11,7 +11,9 @@ # A GitHub merge is refused unless every pre-merge condition holds, each read # live at merge time rather than taken from recorded metadata: the pull request # is open, not a draft, mergeable, free of conflicts, and every unwaived check -# is green at the exact current head commit. Every failing condition is reported, not +# is green at the exact current head commit, where github_checks_not_green below +# owns what makes a check green and judges each one by its current run. +# Every failing condition is reported, not # just the first. The verified head is then passed to gh as # --match-head-commit, so a push that lands between that read and the merge # fails the merge instead of landing commits nothing verified. Reading that @@ -443,22 +445,80 @@ FIELDS } # Every GitHub check that is not green in the given live pull-request JSON, one -# name per line: a status context whose state is not SUCCESS, or a check run -# that has not completed with SUCCESS, NEUTRAL, or SKIPPED (so a pending -# check is not green either). Exits nonzero when the rollup cannot be read, so -# a malformed answer is a failed read and never an empty red set. +# name per line. An entry is green when it is a status context whose state is +# SUCCESS, or a check run that completed with SUCCESS, NEUTRAL, or SKIPPED (so +# a pending check is not green either). Exits nonzero when the rollup cannot be +# read, so a malformed answer is a failed read and never an empty red set. +# +# The rollup can hold several runs of one check name at the same head, because +# GitHub cancels a pull request's in-flight run when the base branch advances +# and re-triggers it; the cancelled run stays in the rollup beside the passing +# re-run. A check is therefore judged by its current run rather than by any run +# that a later one superseded, which is what makes this agree with GitHub's own +# CLEAN mergeStateStatus instead of refusing a pull request GitHub considers +# mergeable. +# +# Supersession applies only among check runs with the same reported name. A +# name is dropped from the red set only when every non-green run is COMPLETED, +# has a whole-second UTC startedAt, and started strictly before a green run. +# Status contexts are never grouped or superseded, and every non-green one is +# reported independently. A still-running, queued, undated, or tied check run +# stays red. A name whose runs are all green needs no timestamp, while a name +# with no green run stays red. +# +# The reported name is also what --allow-red matches. An unnamed check run is +# grouped alone and can neither supersede nor be superseded, because unrelated +# unnamed checks must not be treated as one. github_checks_not_green() { local json=$1 printf '%s' "$json" | jq -r ' + def settled_at: + if type == "string" and test("^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}Z$") + then . else null end; if (.statusCheckRollup | type) != "array" then error("no check rollup") else . end - | .statusCheckRollup[] - | if .__typename == "CheckRun" then - {name: (.name // ""), ok: (.status == "COMPLETED" and (.conclusion == "SUCCESS" or .conclusion == "NEUTRAL" or .conclusion == "SKIPPED"))} - else - {name: (.context // ""), ok: (.state == "SUCCESS")} - end - | select(.ok | not) - | if .name == "" then "(unnamed check)" else .name end + | [ .statusCheckRollup + | to_entries[] + | .key as $i + | .value + | if .__typename == "CheckRun" then + { + kind: "check_run", + name: (.name // ""), + completed: (.status == "COMPLETED"), + ok: (.status == "COMPLETED" and (.conclusion == "SUCCESS" or .conclusion == "NEUTRAL" or .conclusion == "SKIPPED")), + at: (.startedAt | settled_at) + } + | . + {group: (if .name == "" then ["", $i] else [.name, -1] end)} + else + {kind: "status_context", name: (.context // ""), ok: (.state == "SUCCESS")} + end + ] + | . as $entries + | ( + ($entries[] + | select(.kind == "status_context" and (.ok | not)) + | .name + ), + ($entries + | [.[] | select(.kind == "check_run")] + | group_by(.group)[] + | { + name: .[0].name, + reds: [.[] | select(.ok | not)], + newest_green: ([.[] | select(.ok) | .at | select(. != null)] | max) + } + | select( + (.reds | length) > 0 + and ( + .newest_green == null + or any(.reds[]; (.completed | not) or .at == null) + or ([.reds[] | .at] | max) >= .newest_green + ) + ) + | .name + ) + ) + | if . == "" then "(unnamed check)" else . end ' 2>/dev/null || return 1 } diff --git a/docs/architecture.md b/docs/architecture.md index 7ab4e496f7d..a02e4b9a1e5 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -314,6 +314,7 @@ This repo uses that setting, and its own `.no-mistakes/` directory remains local PR-based task merges go through `bin/fm-pr-merge.sh`, which records `pr=` and any available `pr_head=` through `bin/fm-pr-check.sh` before calling the forge CLI. The helper requires a full canonical URL and rejects malformed URLs or repo override flags before recording merge state. A `https://github.com///pull/` URL requires `gh` and `jq`, is merged only after one live read confirms the pull request is open, not a draft, mergeable, conflict-free, and every unwaived check is green at the current head, then `gh pr merge` binds that verified head with `--match-head-commit`. +A check run is green when its current run is green, because GitHub leaves a cancelled run in the rollup beside the passing re-run it triggered when the base branch advanced; `bin/fm-pr-merge.sh`'s `github_checks_not_green` owns the rule, which uses `startedAt` to clear only an older completed check run that a passing run with the same name provably replaced, while unfinished check runs and non-green status contexts stay red. `--auto`, `--admin`, and branch-deletion flags are refused unless `--attended-override` is passed for an explicit captain instruction; that override never skips the live green check, the away-grant check, or a captain hold. An attended `--allow-red ` may appear once, waives only GitHub checks with that exact name, and is refused while the away-posture record exists. A `https:////-/merge_requests/` URL (see [docs/gitlab-merge-watch.md](gitlab-merge-watch.md)) invokes `glab mr merge -R https:///`, so the instance comes from the URL, and adds no merge-method flag because the project's own merge method applies. diff --git a/tests/fm-pr-merge.test.sh b/tests/fm-pr-merge.test.sh index 7d29879a6d0..5f4122b001e 100755 --- a/tests/fm-pr-merge.test.sh +++ b/tests/fm-pr-merge.test.sh @@ -77,6 +77,41 @@ write_github_red_json() { JSON } +# One CheckRun rollup entry the way GitHub reports it. A conclusion or timestamp +# of "-" is emitted as JSON null. Args: name status conclusion [startedAt] +# [completedAt] +check_run() { + local name=$1 status=$2 conclusion=$3 started=${4:--} completed=${5:-${4:--}} + local conclusion_json='null' started_json='null' completed_json='null' + [ "$conclusion" = - ] || conclusion_json="\"$conclusion\"" + [ "$started" = - ] || started_json="\"$started\"" + [ "$completed" = - ] || completed_json="\"$completed\"" + printf '{"__typename":"CheckRun","name":"%s","status":"%s","conclusion":%s,"startedAt":%s,"completedAt":%s}' \ + "$name" "$status" "$conclusion_json" "$started_json" "$completed_json" +} + +status_context() { + local name=$1 state=$2 + printf '{"__typename":"StatusContext","context":"%s","state":"%s"}' "$name" "$state" +} + +# Live GitHub JSON whose rollup holds the given entries verbatim, so a test can +# put several runs of one check name at the same head the way GitHub does after +# it cancels a pull request's in-flight run and re-triggers it. mergeStateStatus +# stays CLEAN because that is what GitHub reports for exactly this case. +# Args: case_dir head_sha ... +write_github_rollup_json() { + local case_dir=$1 head=$2 entry rollup='' + shift 2 + for entry in "$@"; do + rollup="${rollup:+$rollup,}$entry" + done + printf '%s\n' "$head" > "$case_dir/github-head" + cat > "$case_dir/github-view.json" < "$case_dir/stdout" 2> "$case_dir/stderr" \ + || fail "github-superseded-red: a failed run replaced by a passing re-run must merge"$'\n'"$(cat "$case_dir/stderr")" + assert_logged_gh_merge "$case_dir" 90 example/repo --squash + pass "fm-pr-merge merges when a failed check run was replaced by a passing re-run" +} + +# Legacy status contexts remain independent from check runs, even when their +# reported names match. +test_check_runs_never_supersede_status_contexts() { + local case_dir rc head + head=cdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcd + case_dir=$(make_case github-cross-check-kind) + mkdir -p "$case_dir/wt" + add_gh_mocks "$case_dir" "$head" + write_github_rollup_json "$case_dir" "$head" \ + "$(status_context ci FAILURE)" \ + "$(check_run ci COMPLETED SUCCESS 2026-01-01T00:00:09Z)" + + set +e + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/97 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + expect_code 1 "$rc" "github-cross-check-kind: a failing status context must refuse" + assert_grep "check 'ci' is not green" "$case_dir/stderr" \ + "github-cross-check-kind: the status context was not named" + assert_no_grep 'pr merge' "$case_dir/gh.log" \ + "github-cross-check-kind: a passing check run hid a failing status context" + pass "fm-pr-merge never lets a check run supersede a legacy status context" +} + +# The inverse, and the one that matters most: a check whose current run failed is +# still red however many earlier runs of it passed. +test_current_failed_check_run_still_refuses() { + local case_dir rc head + head=dddddddddddddddddddddddddddddddddddddddd + case_dir=$(make_case github-current-red) + mkdir -p "$case_dir/wt" + add_gh_mocks "$case_dir" "$head" + write_github_rollup_json "$case_dir" "$head" \ + "$(check_run ci COMPLETED SUCCESS 2026-01-01T00:00:01Z)" \ + "$(check_run ci COMPLETED FAILURE 2026-01-01T00:00:09Z)" + + set +e + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/91 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + expect_code 1 "$rc" "github-current-red: a currently failing check must refuse" + assert_grep "check 'ci' is not green" "$case_dir/stderr" \ + "github-current-red: the red check was not named" + assert_no_grep 'pr merge' "$case_dir/gh.log" \ + "github-current-red: gh pr merge ran on a currently failing check" + pass "fm-pr-merge still refuses when a check's current run failed after an earlier pass" +} + +# Run generation follows startedAt rather than the order overlapping runs finish. +test_late_finishing_old_success_does_not_hide_current_failure() { + local case_dir rc head + head=dededededededededededededededededededede + case_dir=$(make_case github-old-success-finishes-last) + mkdir -p "$case_dir/wt" + add_gh_mocks "$case_dir" "$head" + write_github_rollup_json "$case_dir" "$head" \ + "$(check_run ci COMPLETED SUCCESS 2026-01-01T00:00:01Z 2026-01-01T00:00:10Z)" \ + "$(check_run ci COMPLETED FAILURE 2026-01-01T00:00:09Z 2026-01-01T00:00:09Z)" + + set +e + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/98 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + expect_code 1 "$rc" "github-old-success-finishes-last: the later-started failure must refuse" + assert_grep "check 'ci' is not green" "$case_dir/stderr" \ + "github-old-success-finishes-last: the current failure was not named" + assert_no_grep 'pr merge' "$case_dir/gh.log" \ + "github-old-success-finishes-last: completion order hid the current failure" + pass "fm-pr-merge uses start order when the old success finishes last" +} + +# A cancelled old run may settle after the passing re-run that superseded it. +test_late_finishing_old_cancellation_is_superseded() { + local case_dir head + head=dfdfdfdfdfdfdfdfdfdfdfdfdfdfdfdfdfdfdfdf + case_dir=$(make_case github-old-cancellation-finishes-last) + mkdir -p "$case_dir/wt" + add_gh_mocks "$case_dir" "$head" + write_github_rollup_json "$case_dir" "$head" \ + "$(check_run ci COMPLETED CANCELLED 2026-01-01T00:00:01Z 2026-01-01T00:00:10Z)" \ + "$(check_run ci COMPLETED SUCCESS 2026-01-01T00:00:09Z 2026-01-01T00:00:09Z)" + + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/99 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" \ + || fail "github-old-cancellation-finishes-last: the passing re-run must merge"$'\n'"$(cat "$case_dir/stderr")" + assert_logged_gh_merge "$case_dir" 99 example/repo --squash + pass "fm-pr-merge supersedes an old cancellation that finishes last" +} + +# A re-run that has not finished proves nothing, so it can neither be superseded +# nor supersede: the check stays red whether the run it replaces passed or failed. +test_unfinished_rerun_keeps_a_check_red() { + local case_dir rc head prior + head=eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee + for prior in FAILURE SUCCESS; do + case_dir=$(make_case "github-pending-rerun-$prior") + mkdir -p "$case_dir/wt" + add_gh_mocks "$case_dir" "$head" + write_github_rollup_json "$case_dir" "$head" \ + "$(check_run ci COMPLETED "$prior" 2026-01-01T00:00:01Z)" \ + "$(check_run ci IN_PROGRESS - -)" + + set +e + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/92 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + expect_code 1 "$rc" "github-pending-rerun-$prior: an unfinished re-run must refuse" + assert_grep "check 'ci' is not green" "$case_dir/stderr" \ + "github-pending-rerun-$prior: the pending check was not named" + assert_no_grep 'pr merge' "$case_dir/gh.log" \ + "github-pending-rerun-$prior: gh pr merge ran with a re-run still in flight" + done + pass "fm-pr-merge keeps a check red while its re-run is still in flight" +} + +# Supersession is scoped to one check name, which is also the name --allow-red +# matches, so a newer passing check never clears a different check's failure. +test_supersession_never_crosses_check_names() { + local case_dir rc head + head=ffffffffffffffffffffffffffffffffffffffff + case_dir=$(make_case github-cross-name) + mkdir -p "$case_dir/wt" + add_gh_mocks "$case_dir" "$head" + write_github_rollup_json "$case_dir" "$head" \ + "$(check_run lint COMPLETED FAILURE 2026-01-01T00:00:01Z)" \ + "$(check_run ci COMPLETED SUCCESS 2026-01-01T00:00:09Z)" + + set +e + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/93 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + expect_code 1 "$rc" "github-cross-name: another check passing must not clear this failure" + assert_grep "check 'lint' is not green" "$case_dir/stderr" \ + "github-cross-name: the red check was not named" + assert_no_grep 'pr merge' "$case_dir/gh.log" \ + "github-cross-name: gh pr merge ran on a red check of a different name" + pass "fm-pr-merge never lets one check's pass clear another check's failure" +} + +# Supersession has to be proven from the forge's own start timestamps, so a run +# GitHub dated in any other way is treated as undated and clears nothing. +test_undated_runs_never_supersede() { + local case_dir rc spec label older newer + local head=0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a + set -- \ + 'undated-failure|-|2026-01-01T00:00:09Z' \ + 'undated-pass|2026-01-01T00:00:01Z|-' \ + 'fractional-pass|2026-01-01T00:00:01Z|2026-01-01T00:00:09.500Z' \ + 'offset-pass|2026-01-01T00:00:01Z|2026-01-01T00:00:09+00:00' + for spec in "$@"; do + label=${spec%%|*} + older=${spec#*|} + older=${older%%|*} + newer=${spec##*|} + case_dir=$(make_case "github-undated-$label") + mkdir -p "$case_dir/wt" + add_gh_mocks "$case_dir" "$head" + write_github_rollup_json "$case_dir" "$head" \ + "$(check_run ci COMPLETED FAILURE "$older")" \ + "$(check_run ci COMPLETED SUCCESS "$newer")" + + set +e + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/94 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + expect_code 1 "$rc" "github-undated-$label: an unproven supersession must refuse" + assert_grep "check 'ci' is not green" "$case_dir/stderr" \ + "github-undated-$label: the red check was not named" + assert_no_grep 'pr merge' "$case_dir/gh.log" \ + "github-undated-$label: gh pr merge ran on an unproven supersession" + done + pass "fm-pr-merge clears a failure only on a proven later pass of the same check" +} + +# A superseded failure changes nothing about the waiver: --allow-red still covers +# exactly the named check, still needs every other check green, and the merge is +# still bound to the verified head. +test_allow_red_still_waives_only_the_current_failure() { + local case_dir rc head + head=0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b + case_dir=$(make_case github-superseded-allow-red-wrong-name) + mkdir -p "$case_dir/wt" + add_gh_mocks "$case_dir" "$head" + write_github_rollup_json "$case_dir" "$head" \ + "$(check_run ci COMPLETED FAILURE 2026-01-01T00:00:01Z)" \ + "$(check_run ci COMPLETED SUCCESS 2026-01-01T00:00:09Z)" \ + "$(check_run lint COMPLETED FAILURE 2026-01-01T00:00:09Z)" + + set +e + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/95 \ + --allow-red ci > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + expect_code 1 "$rc" "superseded-allow-red-wrong-name: waiving the green check must not merge" + assert_grep "check 'lint' is not green" "$case_dir/stderr" \ + "superseded-allow-red-wrong-name: the unwaived red check was not named" + assert_no_grep 'pr merge' "$case_dir/gh.log" \ + "superseded-allow-red-wrong-name: gh pr merge ran with an unwaived red check" + + case_dir=$(make_case github-superseded-allow-red-named) + mkdir -p "$case_dir/wt" + add_gh_mocks "$case_dir" "$head" + write_github_rollup_json "$case_dir" "$head" \ + "$(check_run ci COMPLETED FAILURE 2026-01-01T00:00:01Z)" \ + "$(check_run ci COMPLETED SUCCESS 2026-01-01T00:00:09Z)" \ + "$(check_run lint COMPLETED FAILURE 2026-01-01T00:00:09Z)" + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/96 \ + --allow-red lint > "$case_dir/stdout" 2> "$case_dir/stderr" \ + || fail "superseded-allow-red-named: the named waiver should merge"$'\n'"$(cat "$case_dir/stderr")" + assert_logged_gh_merge "$case_dir" 96 example/repo --squash + pass "fm-pr-merge keeps --allow-red scoped to its named check beside a superseded failure" +} + test_allow_red_is_refused_while_away() { local case_dir rc head head=abababababababababababababababababababab @@ -2499,6 +2774,15 @@ test_untraversable_user_backend_config_directory_refuses_the_merge test_absent_user_backend_config_directory_and_backlog_still_merge test_backend_override_bypasses_unreadable_user_config test_github_red_checks_refuse_and_allow_red_waives_named +test_superseded_failed_check_run_no_longer_refuses +test_check_runs_never_supersede_status_contexts +test_current_failed_check_run_still_refuses +test_late_finishing_old_success_does_not_hide_current_failure +test_late_finishing_old_cancellation_is_superseded +test_unfinished_rerun_keeps_a_check_red +test_supersession_never_crosses_check_names +test_undated_runs_never_supersede +test_allow_red_still_waives_only_the_current_failure test_allow_red_is_refused_while_away test_allow_red_requires_one_separate_name test_away_grant_and_yolo_and_hold_for_return