From 16901956afd7168a3d5d05555fd403168e86c66c Mon Sep 17 00:00:00 2001 From: jsboige Date: Sat, 12 Sep 2026 10:20:05 +0200 Subject: [PATCH] =?UTF-8?q?fix(ci,#15621):=20pr=5Fgate=5Fmissing=20?= =?UTF-8?q?=E2=80=94=20une=20seule=20forme=20producteur/consommateur?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deux defauts mesures + un troisieme effet (Hermes) : 1. `list_open_prs()` emet les cles REST plates mais `main()` relisait les noms GraphQL (`baseRefName`/`isDraft`/`author`) : trois des cinq verdicts (`excluded_base`, `draft`, `bot_missing`) etaient structurellement inatteignables, le champ auteur rendait vide, et 7 PRs saines publiees comme defauts. `labels` n'etait pas emis du tout, donc `has_label()` etait toujours faux et la migration generic->conflict n'a jamais tire. Correctif : la forme est declaree UNE fois (`PR_ROW_KEYS`), `normalize_row` est l'unique point de traduction, et le test de contrat epingle la forme du PRODUCTEUR (pas un dict construit a la main — l'angle mort). 2. Les 3 labels n'ont jamais existe : descriptions de 108/121/145 caracteres contre le plafond GitHub de 100, `gh label create` en 404, echec avale par `check=False`. Descriptions <= 100 epinglees par test ; un echec de creation est desormais nomme au log (WARN + stderr gh). 3. La retombee de label n'existait que sur `has_gate` : une PR mal classee puis correctement reclassee gardait son label et son commentaire a vie. `draft`/`excluded_base` retiennent desormais idempotemment. Bonus mesure en live : REST epele le bot `github-actions[bot]` la ou GraphQL epele `app/github-actions` — `bot_missing` restait inatteignable pour cette seconde raison. `is_bot_author()` couvre les deux orthographes. Dry-run live avant/apres (recensement) : 7 missing/0 draft/0 excluded_base -> 1 missing (retarget reel #15600) + 1 bot_missing (#15678) + 2 draft + 9 excluded_base. Tests : 28 passed dont 9 neufs ; controle positif contre les sources d'origine : 9 echecs / 19. Co-Authored-By: Claude Sonnet 5 --- scripts/pr_gate_missing.py | 160 ++++++++++++++++---- scripts/tests/test_pr_gate_missing.py | 205 +++++++++++++++++++++++++- 2 files changed, 336 insertions(+), 29 deletions(-) diff --git a/scripts/pr_gate_missing.py b/scripts/pr_gate_missing.py index 37b8cce6c8..efa75f0a99 100644 --- a/scripts/pr_gate_missing.py +++ b/scripts/pr_gate_missing.py @@ -56,10 +56,17 @@ LABEL_BOT_DEFAULT = "pr-gate-missing-bot" LABEL_COLOR = "b60205" # red -- "invisible required-context blocker, needs a push" LABEL_BOT_COLOR = "d93f0b" # orange -- "structural bot case (GITHUB_TOKEN anti-recursion)" -LABEL_DESC = ("PR gate absent du rollup: contexte requis jamais rapporte -- " - "PR verrouillee malgre des checks verts (#10928)") -LABEL_BOT_DESC = ("PR du bot sans PR gate: structural (push GITHUB_TOKEN ne cree " - "pas de workflow run) -- merge admin ou push humain (#10928)") +# GitHub refuses a label description longer than 100 characters, and `gh label +# create` fails on it. Measured 2026-09-12 (#15621): the three descriptions +# were 108 / 121 / 145 characters, so all three creations failed with HTTP 404 +# and the failure was invisible -- `ensure_label` swallowed the exit code. The +# "label" half of this organ's payload had never existed. `MAX_LABEL_DESC` +# makes the ceiling explicit and `test_pr_gate_missing.py` pins it, because +# these strings drift by nature (one appended issue number is enough). +MAX_LABEL_DESC = 100 + +LABEL_DESC = "PR gate absent du rollup alors que des checks sont verts (#10928)" +LABEL_BOT_DESC = "PR du bot sans PR gate: push GITHUB_TOKEN, merge admin (#10928)" # Issue #14477 (cause 5, mesuree 2026-09-03 sur #14220) : une PR en conflit # avec main ne recoit AUCUN run `pull_request` -- GitHub ne calcule pas de @@ -68,9 +75,7 @@ # aucun run). Label distinct : le remede n'est pas un push, c'est un conflit. LABEL_CONFLICT_DEFAULT = "pr-gate-conflict" LABEL_CONFLICT_COLOR = "fdd0a2" # saumon -- "PR dirty, remede = resoudre le conflit" -LABEL_CONFLICT_DESC = ("PR gate absent car la PR est en conflit avec main " - "(mergeable_state=dirty) -- aucun run pull_request tant " - "que le conflit n'est pas resolu (#14477)") +LABEL_CONFLICT_DESC = "PR gate absent car la PR est en conflit avec main (#14477)" # The exact check-run name posted by pr_gate.py --self-name "PR gate" and # required by main's branch protection. Renaming here silently detaches the @@ -78,6 +83,21 @@ GATE_NAME = "PR gate" BOT_LOGIN = "app/github-actions" + +def is_bot_author(login: str) -> bool: + """The same GitHub App bot, spelled by two APIs (#15621). + + REST (what ``list_open_prs`` reads since #14488) renders the Actions app + as ``github-actions[bot]``; GraphQL rendered it ``app/github-actions`` -- + the spelling ``BOT_LOGIN`` still carries. A bot must not flip to a human + verdict because the collector changed API: measured live 2026-09-12, the + catalog's long-lived PR #15678 (author ``app/github-actions`` per GraphQL, + ``github-actions[bot]`` per REST) classified ``missing`` instead of + ``bot_missing``. Any ``*[bot]`` suffix is a bot login by GitHub's own + naming rule, which also covers a future app bot. + """ + return login == BOT_LOGIN or login.endswith("[bot]") + # Marker framing the advisory comment, so re-runs can find and update it. COMMENT_MARKER_START = "" COMMENT_MARKER_END = "" @@ -193,7 +213,7 @@ def classify(pr: dict) -> tuple[str, str]: return ("draft", f"#{number} draft PR, non mergeable") if GATE_NAME in rollup_names(pr): return ("has_gate", f"#{number} PR gate present (conclusion: {len(rollup_names(pr))} checks)") - if pr.get("author_login") == BOT_LOGIN: + if is_bot_author(pr.get("author_login") or ""): return ("bot_missing", f"#{number} bot PR, no PR gate (structural)") return ("missing", f"#{number} PR gate absent du rollup") @@ -237,8 +257,8 @@ def prescribe(pr: dict) -> tuple[str, str]: f"base_ref_changed={changed}, dernier run PR gate={last or 'aucun'}") if "[skip ci]" in head_subject(pr): return ("skip_ci", f"sujet de tete porte le token [skip ci] : {head_subject(pr)[:72]!r}") - if pr.get("author_login") == BOT_LOGIN: - return ("bot", "auteur app/github-actions -- push GITHUB_TOKEN sans run") + if is_bot_author(pr.get("author_login") or ""): + return ("bot", f"auteur {pr.get('author_login')} -- push GITHUB_TOKEN sans run") return ("unknown", f"mergeable_state={ms}, pas de base_ref_changed, sujet sans [skip ci], " f"auteur {pr.get('author_login')}") @@ -327,6 +347,50 @@ def _gh_lines(args: list[str]) -> list[str]: return [line for line in proc.stdout.splitlines() if line.strip()] +#: The keys :func:`classify` reads off one row produced by +#: :func:`list_open_prs`. Declared ONCE, here, because a re-map between the +#: producer and the consumer is SILENT: a missing key raises nothing, it +#: renders the ``.get()`` default, and the verdict it guards becomes +#: structurally unreachable rather than wrong. +#: +#: Measured 2026-09-12 (#15621): the producer had been migrated to REST +#: (flat ``base_ref_name`` / ``is_draft`` / ``author_login``) while ``main()`` +#: still rebuilt each row with the GraphQL names (``baseRefName`` / +#: ``isDraft`` / ``author``). Three of the five verdicts -- ``excluded_base``, +#: ``draft``, ``bot_missing`` -- could therefore never be returned, the pool's +#: author field printed empty, and 7 healthy PRs were published as defects +#: with a comment demanding a manual investigation. `test_pr_gate_missing.py` +#: pins this set against the PRODUCER's real output, not against a row built +#: by hand in the shape the consumer happens to want -- that is the blind spot +#: that let the collapse live. +PR_ROW_KEYS = frozenset({ + "number", + "base_ref_name", + "is_draft", + "author_login", + "statusCheckRollup", + "labels", +}) + + +def normalize_row(pr: dict) -> dict: + """One row of :func:`list_open_prs` -> the dict :func:`classify` reads. + + The single translation point between the producer and the consumer. It + reads exactly :data:`PR_ROW_KEYS` and no GraphQL alias: if the producer + ever renames a field, the contract test fails here rather than a verdict + vanishing behind a ``.get()`` default. + """ + return { + "number": pr.get("number"), + "base_ref_name": pr.get("base_ref_name"), + "is_draft": bool(pr.get("is_draft")), + "author_login": pr.get("author_login") or "", + "statusCheckRollup": pr.get("statusCheckRollup") or [], + "labels": pr.get("labels") or [], + } + + def list_open_prs(repo: str) -> list[dict]: """Open PRs with the fields classify() needs -- REST, not GraphQL. @@ -346,7 +410,8 @@ def list_open_prs(repo: str) -> list[dict]: pulls = _gh_rows([ "api", f"repos/{repo}/pulls?state=open&per_page=100", "--paginate", "--jq", '.[] | {number, draft: .draft, base: .base.ref, ' - 'author: .user.login, sha: .head.sha}', + 'author: .user.login, sha: .head.sha, ' + 'labels: [.labels[].name]}', ]) out: list[dict] = [] for p in pulls: @@ -363,6 +428,11 @@ def list_open_prs(repo: str) -> list[dict]: "is_draft": bool(p.get("draft")), "author_login": p.get("author") or "", "statusCheckRollup": rollup, + # #15621: `labels` was never emitted, so `has_label()` was always + # false and the generic -> conflict label migration never fired -- + # the generic label was re-applied on every pass instead of being + # replaced. The row promises PR_ROW_KEYS; this is the key it owed. + "labels": p.get("labels") or [], }) return out @@ -413,18 +483,39 @@ def enrich_candidate(repo: str, number: int) -> dict: } -def ensure_label(repo: str, name: str, color: str, desc: str, dry_run: bool) -> None: +def ensure_label(repo: str, name: str, color: str, desc: str, dry_run: bool) -> bool: + """Create-or-update a label. Returns False when the write failed (#15621). + + The exit code used to be dropped on the floor (``check=False`` plus a + captured stderr nobody read), so three failed creations -- HTTP 404, one + per description over GitHub's 100-character ceiling -- produced a silently + label-less organ: the run printed 7 flagged PRs, `gh pr list --label + pr-gate-missing` returned ``[]``, and nothing in the sweep log said why. + A failed write is now named with its `gh` stderr. + """ if dry_run: - return - subprocess.run( + return True + completed = subprocess.run( ["gh", "label", "create", name, "--repo", repo, "--color", color, "--description", desc, "--force"], capture_output=True, text=True, check=False, encoding="utf-8", ) + if completed.returncode != 0: + print( + f"[pr-gate-missing] WARN -- label {name!r} non cree " + f"(exit {completed.returncode}): " + f"{(completed.stderr or completed.stdout).strip()[:300]}", + flush=True, + ) + return False + return True def has_label(pr: dict, name: str) -> bool: - return any((lab.get("name") == name) for lab in (pr.get("labels") or [])) + # #15621: `labels` is a list of NAME STRINGS -- the shape the producer + # emits (`labels: [.labels[].name]`). It used to be read as the GraphQL + # list of objects (`.name` on each), matching nothing the producer sent. + return name in (pr.get("labels") or []) def apply_label(repo: str, number: int, name: str, dry_run: bool) -> None: @@ -548,14 +639,7 @@ def main(argv: list[str] | None = None) -> int: for pr in prs: number = pr["number"] - enriched = { - "number": number, - "base_ref_name": pr.get("baseRefName"), - "is_draft": pr.get("isDraft", False), - "author_login": (pr.get("author") or {}).get("login", ""), - "statusCheckRollup": pr.get("statusCheckRollup") or [], - "labels": (pr.get("labels") or []), - } + enriched = normalize_row(pr) verdict, why = classify(enriched) counts[verdict] = counts.get(verdict, 0) + 1 @@ -575,10 +659,32 @@ def main(argv: list[str] | None = None) -> int: if number in labeled_conflict: remove_label(repo, number, args.label_conflict, args.dry_run) print(f" #{number:<6} has_gate {why} (conflict label retracted)") - elif verdict == "draft": - pass # quiet -- the common non-defect case - else: # excluded_base - pass # quiet -- PRs targeting a feature branch never see PR gate + elif verdict in ("draft", "excluded_base"): + # Not defects: a draft is not mergeable yet, and a PR targeting a + # feature branch never sees `pr-gate.yml` at all (it fires on + # `pull_request: branches: [main]`). + # + # #15621: these two used to fall through silently -- the label + # fall-through existed ONLY on `has_gate`. So the shape collapse + # above was not merely mis-counting: the 5 PRs with a base != main + # and the 2 drafts each received a label AND a comment asserting an + # unmeasured cause ("auteur : (pas une PR bot)", "investigation + # manuelle"), and correcting the shape alone would leave them + # labelled FOR LIFE -- reclassified correctly, but with no path + # back out of the flag. The retraction is what makes the + # correction retroactive. + retracted = False + for name, holders, tag in ( + (args.label, labeled, "label"), + (args.label_bot, labeled_bot, "bot label"), + (args.label_conflict, labeled_conflict, "conflict label"), + ): + if number in holders: + remove_label(repo, number, name, args.dry_run) + print(f" #{number:<6} {verdict:<10} {why} ({tag} retracted)") + retracted = True + if not retracted: + pass # quiet -- the common non-defect case print(f"[pr-gate-missing] done: {counts} causes={causes}") return 0 diff --git a/scripts/tests/test_pr_gate_missing.py b/scripts/tests/test_pr_gate_missing.py index 9b9c5b2dbb..6d881ca990 100644 --- a/scripts/tests/test_pr_gate_missing.py +++ b/scripts/tests/test_pr_gate_missing.py @@ -1,8 +1,12 @@ #!/usr/bin/env python3 """Unit tests for the pure classification core of pr_gate_missing.py (#10928). -The ``classify`` and ``rollup_names`` functions are network-free; ``main`` (the -gh wiring) is exercised end-to-end in CI dry-runs, not here. These fixtures +The ``classify`` and ``rollup_names`` functions are network-free. ``main``'s +gh wiring is exercised end-to-end in CI dry-runs -- but since #15621 the +PRODUCER's row shape is also pinned here (see the contract tests at the +bottom): the CI dry-run compares the organ's verdicts to no expectation, and +the fixtures below used to build the CONSUMER's shape by hand, which is +exactly how a producer/consumer re-map lived silently for days. These fixtures encode the verdicts measured firsthand on the #10928 sample (2026-08-14): - #10902 : rollup = 5 CodeQL checks only, no ``PR gate`` -> missing @@ -217,3 +221,200 @@ def test_unknown_names_the_measurements(): assert "pas determinee" in remedy assert "git merge" not in remedy assert "commit-tree" not in remedy + + +# --------------------------------------------------------------------------- +# #15621 -- the producer's shape, pinned (not the consumer's) +# --------------------------------------------------------------------------- +# +# Every fixture above builds the row the CONSUMER wants (see `_pr`). That is +# the blind spot that let the collapse live: `list_open_prs()` was migrated to +# REST (flat `base_ref_name` / `is_draft` / `author_login`) while `main()` kept +# rebuilding each row with the GraphQL names (`baseRefName` / `isDraft` / +# `author`). A `.get()` on a missing key raises nothing -- it renders the +# default, and `classify()`'s truthy guards made `excluded_base`, `draft` and +# `bot_missing` structurally UNREACHABLE. Measured 2026-09-12: 7 healthy PRs +# (5 with a base != main, 2 drafts) published as defects, empty author field, +# a comment demanding a manual investigation of a by-design non-defect, and +# three labels that had never existed (description over GitHub's 100-char +# ceiling, creation failing silently). The tests below drive the PRODUCER. + +import pr_gate_missing as pm # noqa: E402 (post-fixtures: the module under test) + + +def _rest_rows(): + """Raw rows as the producer's jq projection emits them.""" + return [ + # #15620-shaped: base != main -- never sees pr-gate.yml by design. + {"number": 15620, "draft": False, + "base": "fix/15489-kernel-suffix-canon-guard", + "author": "jsboige", "sha": "aa1", "labels": ["pr-gate-missing"]}, + # #15610-shaped: draft. + {"number": 15610, "draft": True, "base": "main", "author": "jsboige", + "sha": "bb2", "labels": []}, + # #10902-shaped: healthy base/main, no PR gate in the rollup. + {"number": 10902, "draft": False, "base": "main", "author": "jsboige", + "sha": "cc3", "labels": []}, + # #10558-shaped: bot author, GraphQL spelling. + {"number": 10558, "draft": False, "base": "main", + "author": "app/github-actions", "sha": "dd4", "labels": []}, + # #15678-shaped: the SAME app bot, REST spelling -- what /pulls + # actually returns (`github-actions[bot]`, not `app/github-actions`). + # Measured live 2026-09-12: this spelling classified `missing`. + {"number": 15678, "draft": False, "base": "main", + "author": "github-actions[bot]", "sha": "ee5", "labels": []}, + ] + + +def _patch_producer(monkeypatch): + calls = {"check_runs": []} + monkeypatch.setattr(pm, "_gh_rows", lambda _args: _rest_rows()) + + def fake_check_runs(args): + calls["check_runs"].append(args[1]) + return ["Analyze (python)", "CodeQL"] + + monkeypatch.setattr(pm, "_gh_json", fake_check_runs) + return calls + + +def test_producer_rows_satisfy_the_declared_contract(monkeypatch): + """Acceptance 1: one declared shape, pinned against the PRODUCER's real + output. A future re-map that drops or renames a key fails HERE -- before + any verdict silently becomes unreachable.""" + _patch_producer(monkeypatch) + rows = pm.list_open_prs("o/r") + assert rows, "le producteur doit emettre des lignes" + for row in rows: + missing = pm.PR_ROW_KEYS - set(row) + assert not missing, f"cle(s) absente(s) de la ligne produite: {missing}" + + +def test_producer_does_not_pay_the_rollup_call_for_excluded_rows(monkeypatch): + """The documented optimization stays true through the fix: excluded rows + (base != main, draft) never trigger the per-PR check-runs call.""" + calls = _patch_producer(monkeypatch) + pm.list_open_prs("o/r") + fetched = [c for c in calls["check_runs"] if "/check-runs?" in c] + assert len(fetched) == 3, "seules les PRs base=main non-draft sont sondees" + + +def test_the_three_unreachable_verdicts_are_reachable(monkeypatch): + """The defect itself, end to end from the producer's rows: excluded_base, + draft and bot_missing must all be returned. Before #15621 all four of + these rows classified as `missing`.""" + _patch_producer(monkeypatch) + verdicts = { + row["number"]: pm.classify(pm.normalize_row(row)) + for row in pm.list_open_prs("o/r") + } + assert verdicts[15620][0] == "excluded_base" + assert verdicts[15610][0] == "draft" + assert verdicts[10558][0] == "bot_missing" + assert verdicts[15678][0] == "bot_missing", ( + "l'orthographe REST du bot doit compter comme le bot" + ) + assert verdicts[10902][0] == "missing" + + +def test_producer_emits_the_labels_the_migration_reads(monkeypatch): + """`labels` was never emitted, so `has_label()` was always false and the + generic -> conflict migration never fired. The producer now owes it.""" + _patch_producer(monkeypatch) + rows = {row["number"]: row for row in pm.list_open_prs("o/r")} + assert rows[15620]["labels"] == ["pr-gate-missing"] + + +def test_graphql_aliases_are_not_read(): + """The collapse in one assertion: a row carrying ONLY the GraphQL names + normalizes to the falsy defaults that killed three verdicts. Pinned so a + future re-map cannot quietly reintroduce the alias.""" + graphql_row = { + "number": 1, "baseRefName": "feature/x", "isDraft": True, + "author": {"login": "app/github-actions"}, + "labels": [{"name": "pr-gate-missing"}], + } + normalized = pm.normalize_row(graphql_row) + assert normalized["base_ref_name"] is None + assert normalized["is_draft"] is False + assert normalized["author_login"] == "" + # The false defect the collapse manufactured: a healthy feature-branch PR + # published as `missing`. + assert pm.classify(normalized)[0] == "missing" + + +def test_has_label_reads_the_producers_string_names(): + """#15621: the label list is a list of NAME STRINGS (the producer's jq + `.labels[].name`), not the GraphQL list of objects. `has_label` must + answer on that shape -- this is the generic -> conflict migration's + `if`, and it was always false.""" + pr = {"labels": ["pr-gate-missing"]} + assert pm.has_label(pr, "pr-gate-missing") is True + assert pm.has_label(pr, "pr-gate-conflict") is False + # An object-shaped entry (the old assumption) must not crash and must not + # match: a mismatched shape reads as "label absent", never as an error. + assert pm.has_label({"labels": [{"name": "x"}]}, "x") is False + + +def test_label_descriptions_fit_the_github_ceiling(): + """Acceptance 2: `gh label create` fails on a description over 100 chars + (measured: 108/121/145 -> HTTP 404, labels never existed).""" + for name in ("LABEL_DESC", "LABEL_BOT_DESC", "LABEL_CONFLICT_DESC"): + desc = getattr(pm, name) + assert len(desc) <= pm.MAX_LABEL_DESC, f"{name} = {len(desc)} chars" + + +def test_ensure_label_names_a_failed_creation(capsys, monkeypatch): + """Acceptance 3: a failed creation is VISIBLE. Before #15621 the exit + code was dropped (check=False, stderr captured but never read).""" + class _Refused: + returncode = 1 + stdout = "" + stderr = "HTTP 404: Not Found" + + monkeypatch.setattr(pm.subprocess, "run", lambda *_a, **_k: _Refused()) + ok = pm.ensure_label("o/r", "pr-gate-missing", "b60205", pm.LABEL_DESC, False) + assert ok is False + out = capsys.readouterr().out + assert "WARN -- label" in out + assert "404" in out + + +def test_ensure_label_silent_when_the_write_succeeds(capsys, monkeypatch): + """Positive control of the previous test: a green creation prints + nothing -- the warning is diagnostic, not ambient noise.""" + class _Done: + returncode = 0 + stdout = "" + stderr = "" + + monkeypatch.setattr(pm.subprocess, "run", lambda *_a, **_k: _Done()) + assert pm.ensure_label("o/r", "pr-gate-missing", "b60205", pm.LABEL_DESC, False) is True + assert capsys.readouterr().out == "" + + +def test_draft_and_excluded_base_retract_their_stale_label(monkeypatch, capsys): + """3rd effet (#15621): the label fall-through existed only on `has_gate`. + A PR misclassified by the shape collapse, then correctly reclassified, + would keep its label AND its comment for life. The retraction is what + makes the correction retroactive.""" + monkeypatch.setattr(pm, "ensure_label", lambda *_a, **_k: True) + monkeypatch.setattr(pm, "list_open_prs", lambda _repo: [ + {"number": 15620, "base_ref_name": "fix/15489-kernel-suffix", + "is_draft": False, "author_login": "jsboige", + "statusCheckRollup": [], "labels": ["pr-gate-missing"]}, + {"number": 15610, "base_ref_name": "main", "is_draft": True, + "author_login": "jsboige", "statusCheckRollup": [], "labels": []}, + ]) + monkeypatch.setattr(pm, "labeled_prs", + lambda _repo, _label: {15620: True, 15610: True}) + removed = [] + monkeypatch.setattr(pm, "remove_label", + lambda _repo, number, name, _dry: removed.append((number, name))) + code = pm.main(["--repo", "o/r"]) + assert code == 0 + assert (15620, pm.LABEL_DEFAULT) in removed, "excluded_base doit retirer le label" + assert (15610, pm.LABEL_DEFAULT) in removed, "draft doit retirer le label" + out = capsys.readouterr().out + assert "done: {'missing': 0" in out, "plus aucun faux defaut publie" + assert "'excluded_base': 1" in out and "'draft': 1" in out