From ab8610a28f458f0381c78c270be680a799ab56d1 Mon Sep 17 00:00:00 2001 From: jsboige Date: Tue, 29 Sep 2026 00:32:03 +0200 Subject: [PATCH] fix(ci,#18324): la garde de chemins locaux ne rend plus de verdict sur une surface qu'elle n'a pas lue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Un `gh` refuse par un quota GraphQL epuise faisait remonter un RuntimeError nu en traceback : le job sortait en 1, ce 1 remontait dans `PR gate` (requis) et la PR victime payait pour l'incident d'infrastructure -- alors que le cablage CI est `--report-only`, dont le contrat ecrit dans le script EST « exit 0 ». Mesure sur #18284, tete f406b34e15, job 109104951640 : RuntimeError: gh pr view 18284 failed (exit 1): GraphQL: API rate limit already exceeded for site ID installation. - `_gh_json` leve une exception typee `InstrumentUnavailable` au lieu d'un RuntimeError nu ; - `check()` la rattrape : verdict UNKNOWN, `::warning` + exit 0 en `--report-only` (le contrat du mode, honore au lieu d'etre viole), exit 2 en usage manuel -- distinct de 0 (propre) comme de 1 (findings), pour que « je n'ai pas pu lire » ne soit confondable avec aucun des deux ; - `main()` rattrape le chemin `--scan-plage` de la meme facon : « a measurement is not a verdict » vaut pour des findings, pas pour une mesure qui n'a pas eu lieu. Deux precedents portaient deja la regle dans ce depot : check_exec_ratchet (#16164, exit 2 -- « "n'a pas pu mesurer" n'est pas "a mesure 0" ») et check_gh_comment_traps (#14849, UNKNOWN -- « infrastructure never forges a red »). Controle negatif : les quatre tests qui portent le defaut ECHOUENT sur le code d'avant, verifie en rejouant la suite contre une copie du script d'origin/main. Le cinquieme ne le prouve pas par construction -- il garde contre la sur-correction (un correctif qui rendrait 0 partout passerait les quatre autres). Tests : 19 passed (scripts/tests/test_check_local_path_waivers.py). Garde corrigee verifiee sur une PR reelle : OK: 0 finding, rc=0 dans les deux modes. Closes #18324 Co-Authored-By: Claude Sonnet 5 --- scripts/check_local_path_waivers.py | 58 +++++++++++-- .../tests/test_check_local_path_waivers.py | 86 +++++++++++++++++++ 2 files changed, 137 insertions(+), 7 deletions(-) diff --git a/scripts/check_local_path_waivers.py b/scripts/check_local_path_waivers.py index d71ffa97d0..1720f826d1 100644 --- a/scripts/check_local_path_waivers.py +++ b/scripts/check_local_path_waivers.py @@ -35,6 +35,12 @@ - ``PR_NUMBER`` (check mode): fetch the PR's issue comments via ``gh`` and exit 1 with one line per finding (hand-run before a merge). With ``--report-only``: ``::warning`` annotations, exit 0 -- the CI wiring. +- Either mode, when ``gh`` did not ANSWER (quota exhausted, network down, + binary absent): verdict ``UNKNOWN``, and no verdict at all on the surface + -- a guard never judges a surface it did not read (#16164, #14849). + ``--report-only`` then honours its own exit-0 contract with a + ``::warning``; the hand-run exits 2, distinct from 0 (clean) and 1 + (findings), so "could not read" is confusable with neither. - ``--scan-plage START END``: measurement mode (acceptance 3). Enumerates repo comments newest-first via the issue-comments listing endpoint, keeps those attached to items numbered START..END, reports findings by @@ -132,6 +138,19 @@ def comment_findings(comments: list[dict]) -> list[Finding]: # --- gh plumbing -------------------------------------------------------------- +class InstrumentUnavailable(RuntimeError): + """``gh`` n'a pas repondu : la garde n'a rien lu (#18324). + + Distinct d'un ``gh`` qui repond : un quota GraphQL epuise, une coupure + reseau ou un binaire absent ne disent rien de la surface visee. Deux + precedents du depot portent la meme regle -- ``check_exec_ratchet`` + sort en 2, « "n'a pas pu mesurer" n'est pas "a mesure 0" » (#16164), et + ``check_gh_comment_traps`` rend ``UNKNOWN``, « infrastructure never + forges a red » (#14849). Un garde ne rend jamais un verdict sur une + surface qu'il n'a pas lue. + """ + + def _gh_json(args: list[str]) -> str: proc = subprocess.run( ["gh", *args], @@ -142,7 +161,7 @@ def _gh_json(args: list[str]) -> str: errors="replace", ) if proc.returncode != 0: - raise RuntimeError( + raise InstrumentUnavailable( f"gh {' '.join(args[:3])} failed (exit {proc.returncode}): {proc.stderr.strip()}" ) return proc.stdout @@ -154,7 +173,20 @@ def pr_comments(pr_number: int) -> list[dict]: def check(pr_number: int, report_only: bool = False) -> int: - findings = comment_findings(pr_comments(pr_number)) + try: + findings = comment_findings(pr_comments(pr_number)) + except InstrumentUnavailable as exc: + # #18324. Le wiring CI est ``--report-only``, dont le contrat EST + # exit 0 ; l'exception le violait en sortant en 1 sur un quota + # epuise, et ce 1 remontait dans ``PR gate`` (requis) -- la PR + # victime payait pour l'incident d'infrastructure. Ici : UNKNOWN, + # et aucun verdict sur la surface. + print( + "::warning title=Local-path guard uninstrumented::gh n'a pas " + f"repondu -- verdict UNKNOWN, la garde n'a pas lu la PR #{pr_number} ; " + f"ce n'est pas un « 0 finding ». ({exc})" + ) + return 0 if report_only else 2 for f in findings: if report_only: # Arbitrage ai-01 #16780 : la FUITE est rendue visible mais ne @@ -253,11 +285,23 @@ def main(argv: list[str]) -> int: ap.add_argument("--scan-plage", nargs=2, type=int, metavar=("START", "END"), help="measurement mode: scan comments of items START..END") args = ap.parse_args(argv) - if args.scan_plage: - return scan_plage(args.scan_plage[0], args.scan_plage[1]) - if args.pr_number is None: - ap.error("give a PR number, or --scan-plage START END") - return check(args.pr_number, report_only=args.report_only) + try: + if args.scan_plage: + return scan_plage(args.scan_plage[0], args.scan_plage[1]) + if args.pr_number is None: + ap.error("give a PR number, or --scan-plage START END") + return check(args.pr_number, report_only=args.report_only) + except InstrumentUnavailable as exc: + # « Exit 0 always -- a measurement is not a verdict » (mode + # --scan-plage) vaut pour des FINDINGS, pas pour une mesure qui n'a + # pas eu lieu : exit 2, comme check_exec_ratchet (#16164). + print(f"instrument indisponible : {exc}", file=sys.stderr) + print( + "la garde n'a rien mesure -- relancer, ne pas lire ceci comme " + "« 0 finding ».", + file=sys.stderr, + ) + return 2 if __name__ == "__main__": diff --git a/scripts/tests/test_check_local_path_waivers.py b/scripts/tests/test_check_local_path_waivers.py index 2af66a19ee..0c7fbd1257 100644 --- a/scripts/tests/test_check_local_path_waivers.py +++ b/scripts/tests/test_check_local_path_waivers.py @@ -6,11 +6,15 @@ classify_body is pure by design. """ +import json import sys from pathlib import Path +import pytest + sys.path.insert(0, str(Path(__file__).resolve().parents[1])) +import check_local_path_waivers as lpw from check_local_path_waivers import LONE_PATH_RE, PROFILE_PATH_RE, classify_body # Verbatim from issue #16780 (the comment itself is gone from GitHub). @@ -115,3 +119,85 @@ def test_workflow_group_isolates_comment_runs_from_pr_runs(): concurrency = wf["concurrency"] if concurrency.get("cancel-in-progress"): assert _group_isolates_events(concurrency["group"]), concurrency["group"] + + +# --- #18324 : l'instrument n'a pas repondu ----------------------------------- +# gh a rendu un ECHEC (quota GraphQL de l'installation epuise) au lieu d'un +# resultat. La garde n'a donc pas lu la surface : elle ne rend alors aucun +# verdict sur elle. Le defaut mesure : un RuntimeError nu remontait en +# traceback, le job sortait en 1, et ce 1 remontait dans `PR gate` (requis) -- +# la PR victime payait pour un incident d'infrastructure. + +INCIDENT_STDERR_18324 = "GraphQL: API rate limit already exceeded for site ID installation." + + +def _instrument_down(monkeypatch): + """Rejoue l'incident : gh ne repond pas.""" + + class _P: + returncode = 1 + stdout = "" + stderr = INCIDENT_STDERR_18324 + + monkeypatch.setattr(lpw.subprocess, "run", lambda *a, **k: _P()) + + +def test_gh_failure_raises_the_typed_exception(monkeypatch): + """Le type est la moitie du correctif : check() rattrape CELUI-CI.""" + _instrument_down(monkeypatch) + with pytest.raises(lpw.InstrumentUnavailable): + lpw._gh_json(["pr", "view", "18284", "--json", "comments"]) + + +def test_negative_control_a_bare_runtimeerror_is_a_different_type(): + """Controle negatif : sans type dedie, le rattrapage n'a rien a viser. + + L'assertion qui compte est l'inegalite -- un test qui accepterait + ``RuntimeError`` passerait aussi sur le code defectueux, puisque + l'ancien ``raise`` etait exactement cela. + """ + assert issubclass(lpw.InstrumentUnavailable, RuntimeError) + assert lpw.InstrumentUnavailable is not RuntimeError + + +def test_report_only_honours_its_own_exit_zero_contract(monkeypatch, capsys): + """Le wiring CI est --report-only : son contrat EST exit 0 (#18324).""" + _instrument_down(monkeypatch) + assert lpw.check(18284, report_only=True) == 0 + out = capsys.readouterr().out + assert "UNKNOWN" in out + assert "::warning" in out + + +def test_hand_run_distinguishes_could_not_read_from_clean_and_findings(monkeypatch, capsys): + """Exit 2 : ni 0 (« propre ») ni 1 (« findings ») -- #16164.""" + _instrument_down(monkeypatch) + rc = lpw.check(18284, report_only=False) + assert rc == 2 + assert rc not in (0, 1) + assert "UNKNOWN" in capsys.readouterr().out + + +def test_positive_control_findings_still_render_a_verdict(monkeypatch, capsys): + """La garde n'est pas devenue inoffensive : un vrai finding rend toujours 1. + + Sans ce controle, un correctif qui rendrait 0 partout passerait les + trois tests ci-dessus. + """ + payload = json.dumps( + { + "comments": [ + { + "body": INCIDENT_BODY_16670, + "author": {"login": "jsboige"}, + "createdAt": "2026-09-19T00:00:00Z", + "url": "https://github.com/jsboige/CoursIA/pull/16670#issuecomment-1", + } + ] + } + ) + monkeypatch.setattr(lpw, "_gh_json", lambda args: payload) + assert lpw.check(16670, report_only=False) == 1 + capsys.readouterr() + assert lpw.check(16670, report_only=True) == 0 + assert "LOCAL_PATH_WAIVER" in capsys.readouterr().out