From 26b18a36afe75fe2968ff4ee944e5b2aa02d1d50 Mon Sep 17 00:00:00 2001 From: jsboige Date: Mon, 7 Sep 2026 11:33:02 +0200 Subject: [PATCH] fix(ci,#14976): pr_gate conclusion-first + STARVED nomme le couple status/conclusion Un check-run fige a status=in_progress avec une conclusion terminale (mesure #14967 : 90 min apres son propre completed_at) etait pollue jusqu'a STARVED par le test status-first. Conclusion est desormais autoritatif ; status ne decide que si conclusion est nul. Chaque constituant pending rend son couple observe (nom [status/conclusion]), et le message STARVED nomme le geste de reparation (rerun le run ENFANT, jamais le gate). Controle positif : tests nouveau rouges sur le code non-fixe, verts apres (113 tests pr_gate). Co-Authored-By: Claude-Code --- scripts/pr_gate.py | 50 ++++++++++++++++++++++++---- scripts/tests/test_pr_gate.py | 61 ++++++++++++++++++++++++++++++++--- 2 files changed, 101 insertions(+), 10 deletions(-) diff --git a/scripts/pr_gate.py b/scripts/pr_gate.py index d1b4ac60d6..c2ba267cc3 100644 --- a/scripts/pr_gate.py +++ b/scripts/pr_gate.py @@ -140,9 +140,6 @@ {"failure", "timed_out", "cancelled", "action_required", "stale", "startup_failure"} ) -# GitHub check_run.status values that mean "not finished". -STATUS_PENDING = frozenset({"queued", "in_progress", "waiting", "pending", "requested"}) - DEFAULT_SELF_NAME = "PR gate" # Substring that marks a check as advisory (see rule 6). Matched @@ -415,6 +412,18 @@ def dedupe_latest(checks: Sequence[dict]) -> list[dict]: return [entry[1] for entry in best.values()] +def _pending_label(name: str, status: str, conclusion: str) -> str: + """Render a pending constituent with its observed status/conclusion couple. + + #14976 acceptance 3: a STARVED message that names `X` without saying what X + actually reported is indistinguishable from a real wedge. The frozen record + that cost the 90-minute diagnosis on #14967 (`Detect notebook changes` + [`in_progress/success`]) looked identical to a slow-but-alive check until + the couple was printed. + """ + return f"{name} [{status or 'none'}/{conclusion or 'none'}]" + + def classify( checks: Sequence[dict], self_name: str = DEFAULT_SELF_NAME, @@ -450,6 +459,14 @@ def classify( NOT that classify mishandles in-flight checks (it waits on them correctly) but that the *workflow trigger* never asks it to look again -- that trigger fix lives in pr-gate.yml, not here. + + #14976 nuance: `conclusion` is authoritative. A check-run can carry a + terminal conclusion while `status` is still `in_progress` (frozen record, + measured on #14967). Such a check is SETTLED by its conclusion -- never + held in `pending` because of a stale status. `status` only decides when + `conclusion` is null, and then the check is pending regardless (see + ``_pending_label`` for why the observed couple is carried into the wait + log and the starvation message). """ pending: list[str] = [] bad: list[str] = [] @@ -480,8 +497,17 @@ def classify( ok.append(name) continue - if status in STATUS_PENDING or not conclusion: - pending.append(name) + # #14976: conclusion is authoritative. A check-run can freeze at + # `status=in_progress` while already carrying a terminal conclusion -- + # measured on #14967, `Detect notebook changes` sat at + # `in_progress/success` 90 min past its own job's `completed_at`, + # confirmed on two endpoints. The old status-first test (status + # pending OR no conclusion) polled that green job until the budget + # burned and verdicted STARVED on a PR where nothing was red. Status + # only speaks when conclusion is null -- and then the check is pending + # regardless of what `status` claims. + if not conclusion: + pending.append(_pending_label(name, status, conclusion)) elif conclusion in CONCLUSION_OK: ok.append(name) elif conclusion in CONCLUSION_BAD: @@ -516,6 +542,13 @@ def verdict(pending: Sequence[str], bad: Sequence[str], settled: bool) -> tuple[ unknown), but the driver renders it as CANCELLED, not FAILURE, so the stale-sweep re-drives the leg instead of a human reading a red lie. + #14976 acceptance 4: each pending constituent renders as ``name [status/ + conclusion]`` (see ``_pending_label``), so a green-but-wedged record + (`Detect notebook changes [in_progress/success]`) is distinguishable from + a slow-but-alive one. The message also names the repair gesture -- rerun + the CHILD run carrying the frozen check-run, never the aggregator gate -- + because that is what actually clears the wedge (#14967 measured it). + Note (#11751): the wait loop now re-reads the check set one last time at the deadline; an empty `pending` AND empty `bad` at that point is treated as `settled=True` BEFORE this function is reached, so this `not settled` @@ -528,7 +561,12 @@ def verdict(pending: Sequence[str], bad: Sequence[str], settled: bool) -> tuple[ return 1, "FAIL -- failing checks: " + ", ".join(bad) if not settled: if pending: - return 1, "STARVED -- timed out waiting for: " + ", ".join(pending) + return 1, ( + "STARVED -- timed out waiting for: " + ", ".join(pending) + + " -- a constituent showing a terminal conclusion above is " + "wedged, not slow: rerun its CHILD run (gh run rerun ), " + "never the gate (#14976)" + ) return 1, ( "FAIL -- timed out with empty wait set (gate bug, see #11751)" ) diff --git a/scripts/tests/test_pr_gate.py b/scripts/tests/test_pr_gate.py index c444d70b74..2fcaa1dc16 100644 --- a/scripts/tests/test_pr_gate.py +++ b/scripts/tests/test_pr_gate.py @@ -73,7 +73,60 @@ def boom(*_args, **_kwargs): def test_completed_without_conclusion_is_pending_not_pass(): """`status=completed, conclusion=null` is a transient GitHub state.""" pending, bad, ok, _adv = pr_gate.classify([run("Odd", None)]) - assert pending == ["Odd"] and not bad and not ok + assert pending == ["Odd [completed/none]"] and not bad and not ok + + +# --- #14976 -- frozen check-runs: conclusion is authoritative ----------------- +# +# Measured on #14967 (2026-09-06): `Detect notebook changes` (check-run +# 101568709885) sat at `status=in_progress` with `conclusion=success` for +# 90 min past its own job's completed_at, confirmed on two independent +# endpoints. The status-first test (status pending OR no conclusion) polled +# that green job until the 45-min budget burned and the verdict came back +# STARVED on a PR where nothing was red. Conclusion is authoritative; status +# only speaks when conclusion is null. + + +def test_frozen_check_with_terminal_conclusion_settles_14976(): + """Positive control (#14976 acceptance 2): RED on the pre-fix classify, + GREEN after. The frozen shape -- `status=in_progress` carrying + `conclusion=success` -- must land in `ok`, not `pending`.""" + checks = [ + run("Detect notebook changes", "success", status="in_progress", rid=1), + run("Lean CI", "success", rid=2), + ] + pending, bad, ok, _adv = pr_gate.classify(checks, "PR gate") + assert pending == [] and bad == [] + assert ok == ["Detect notebook changes", "Lean CI"] + + +def test_frozen_check_with_failure_conclusion_is_bad_14976(): + """The frozen shape with a red conclusion must fail fast, not STARVE: a + wedged record carrying `failure` is a real red the operator must see.""" + checks = [run("Detect notebook changes", "failure", status="in_progress")] + pending, bad, _ok, _adv = pr_gate.classify(checks, "PR gate") + assert bad == ["Detect notebook changes"] and pending == [] + + +def test_pending_labels_carry_the_observed_couple_14976(): + """Acceptance 3: a pending constituent renders with the status/conclusion + actually observed, else a green-but-wedged check is indistinguishable + from a slow-but-alive one (the 90-minute #14967 diagnosis).""" + checks = [run("Slow CI", None, status="in_progress")] + pending, _bad, _ok, _adv = pr_gate.classify(checks, "PR gate") + assert pending == ["Slow CI [in_progress/none]"] + + +def test_starved_message_documents_the_child_rerun_repair_14976(): + """Acceptance 4: the repair gesture -- rerun the CHILD run carrying the + frozen check-run, never the aggregator -- lives in the starvation message + the operator actually reads.""" + code, msg = pr_gate.verdict( + pending=["Slow CI [in_progress/success]"], bad=[], settled=False + ) + assert code == 1 + assert msg.startswith("STARVED") + assert "child" in msg.lower() # --- 2. de-duplication by check name ---------------------------------------- @@ -175,7 +228,7 @@ def test_the_9858_outage_shape_fails(): A gate that verdicts PASS on this shape is the exact failure mode that let 175 PRs through unpoliced. Both sub-cases must land in `pending` (classify: - `status in STATUS_PENDING or not conclusion`) and the verdict must FAIL -- + no conclusion) and the verdict must FAIL -- the bias-to-fail rule (rule 1) applied to the outage, parallel to the #9762 replay above (which pins the failure-side, this pins the no-verdict-side). """ @@ -530,7 +583,7 @@ def test_pending_legacy_status_blocks_settling(): "started_at": "2026-08-07T00:00:00Z", "id": 9}, ] pending, _bad, _ok, _adv = pr_gate.classify(checks) - assert pending == ["legacy/lint"] + assert pending == ["legacy/lint [in_progress/none]"] # --- fork exemption (issue #10072) ------------------------------------------- @@ -970,7 +1023,7 @@ def test_inflight_required_check_settles_to_pass_on_reroll(): # Rollup 1: guard still in flight -> pending -> verdict timed out (FAIL). pending1, bad1, ok1, _adv = pr_gate.classify([inflight], pr_gate.DEFAULT_SELF_NAME) - assert pending1 == [name] and not bad1 and not ok1 + assert pending1 == [name + " [in_progress/none]"] and not bad1 and not ok1 code1, _msg1 = pr_gate.verdict(pending1, bad1, settled=False) assert code1 == 1