diff --git a/docs/reference/review-coverage-threshold.md b/docs/reference/review-coverage-threshold.md index 17478cd8ec..6300e1647e 100644 --- a/docs/reference/review-coverage-threshold.md +++ b/docs/reference/review-coverage-threshold.md @@ -73,6 +73,15 @@ Deux mécanismes, **sans toucher au code** : lève le label. Exclure les bot reviews recréerait le trou sur une surface plus étroite (incident fondateur documenté dans le docstring de `classify`). +- **Passe émise en commentaire** : **compte** depuis #16284. Une persona + contrainte en jetons émet son verdict dans le fil + (`VERDICT: LGTM` suivi de son tag `**[Hermes]**`), et un périmètre + `reviews[]` seul déclarait « aucune review » sur des PR déjà revues + (#16133 à 12:29:47Z, #16145 à 12:31:38Z, label publié 9 et 6 min plus + tard). Le signal est le **tag** de persona, motif de + `check_unaddressed_nits` : un commentaire **nu** d'un login partagé ne + compte pas — `jsboige` pousse pour toutes les lanes (#13316), et la + seule occurrence non taguée du corpus mesuré était une **levée**. ## Pourquoi ADVISORY et pas bloquant diff --git a/scripts/review_coverage.py b/scripts/review_coverage.py index 57bd3d2329..4a1c5ea73b 100644 --- a/scripts/review_coverage.py +++ b/scripts/review_coverage.py @@ -19,11 +19,13 @@ sweep it: - flags OPEN non-draft PRs (base=main) with ``additions > THRESHOLD`` AND - ``reviews[]`` empty (no bot, no human) ; + no review on EITHER surface -- ``reviews[]`` OR a persona-attributed + review pass in the issue comments (cf. ``review_pass_in_comments``) ; - labels ``large-pr-no-review`` (regular) and posts a one-shot comment (marker-guarded, no spam on re-runs) ; - - removes the label when a review arrives (or the diff shrinks below the - threshold), so the label is a current-state signal, not a sticky one. + - removes the label on the next sweep once a review arrives (or the diff + shrinks below the threshold), so the label is a current-state signal, + not a sticky one. Acceptance mirrors the issue body: the signal is the **absence**, not the content. A check that posted ``rien trouve`` on a PR with no review would @@ -58,8 +60,18 @@ import re import subprocess import sys +from pathlib import Path from typing import Iterable +# Le motif qui identifie une voix de persona (`[Hermes]`, `[NanoClaw]`) vit +# dans l'organe canonique `check_unaddressed_nits`, deja importe ainsi par +# `audit/nit_lift_authorship.py`. On ne le redefinit PAS : il a ete durci par +# incidents (un tag en backtick est une CITATION, #13030 ; un en-tete en gras +# `**[NanoClaw]**` doit compter, #14503) et une copie locale divergerait en +# silence. Cf. `review_pass_in_comments`. +sys.path.insert(0, str(Path(__file__).resolve().parent)) +import check_unaddressed_nits as nits # noqa: E402 + # The label is the *signal* -- a current-state flag the reviewer/coordinator # reads. Color is red (the absence is a coverage hole, not a failure). LABEL_DEFAULT = "large-pr-no-review" @@ -101,9 +113,10 @@ "[`scripts/review_coverage.py`](../../scripts/review_coverage.py) " "porte par l'issue #11232. Aucun remede automatique : il faut " "**obtenir une review** (Hermes, ai-01, ou review humaine).\n\n" - "Le label sera **retire des qu'une review arrive** (ou que le diff " - "passe sous le seuil). Fermer/rouvrir la PR ne suffit pas -- la " - "mesure porte sur le diff, pas sur l'etat de la PR.\n\n" + "Le label est **retire au balayage suivant** (quotidien) des qu'une " + "review arrive -- dans ``reviews[]`` ou en commentaire de verdict -- ou " + "que le diff passe sous le seuil. Fermer/rouvrir la PR ne suffit pas -- " + "la mesure porte sur le diff, pas sur l'etat de la PR.\n\n" "Seuil, historique et exceptions : cf. " "[`docs/reference/review-coverage-threshold.md`]" "(../../docs/reference/review-coverage-threshold.md)." @@ -112,11 +125,67 @@ THRESHOLD_DEFAULT = 300 +def review_pass_in_comments(comments: Iterable[dict] | None) -> bool: + """Une passe de review a-t-elle ete emise en COMMENTAIRE d'issue ? + + Le perimetre ``reviews[]`` seul rendait cet organe aveugle a un mode + d'emission **structurel** du cluster : une persona emet son verdict en + commentaire quand elle est contrainte en jetons (verbatim du fil : + « contrainte token : COMMENT only — opener `jsboige`, cap self-review + #3219 »). Mesure du 2026-09-15 sur le jeu labellise : + + | PR | passe emise dans le fil | l'organe publiait | + |---|---|---| + | #16133 | ``VERDICT: CONCERNS`` + tag, 12:29:47Z | « aucune review » 12:38:21Z | + | #16145 | ``VERDICT: LGTM`` + tag, 12:31:38Z | « aucune review » 12:38:10Z | + + **Le signal est le TAG, et lui seul** -- motif durci du canon, qui exclut + la citation en backtick (#13030) et admet l'en-tete en gras (#14503). + + Ce qu'on ne compte PAS, et pourquoi -- chaque exclusion est mesuree : + + - ``nits.classify()`` : cet organe classe les **reserves**, pas les + passes. Sur ces deux memes corps : ``VERDICT: CONCERNS`` -> + ``BOT-CONCERN`` mais ``VERDICT: LGTM`` -> ``None``. Un predicat bati + dessus fermerait #16133 en laissant #16145 faux -- le defaut meme que + ce correctif ferme, deplace sur la surface des approbations ; + - le **login** alias de persona, seul : sur les 121 PR ouvertes du + 2026-09-15, ``clusterManager-Myia`` a commente 10 fois -- 9 passes + portant le tag, et 1 sans tag qui est une **levee** + (``#15795`` : « Levee a la tete exacte ... -- review 5202580554 »). + Ce n'est pas une passe. Honnetement : sur ce corpus les deux predicats + rendent le **meme** verdict (cette levee tombe sur une PR sous le + seuil), donc le login seul ne coute rien **aujourd'hui** -- mais la + classe non taggee existe et n'est pas une revue, et le jour ou une + telle levee tombe sur une PR large non revue, le login la declarerait + **couverte** : le trou de #11232 rendu invisible. Le canon demande le + marqueur, le login n'etant qu'un conjoint (#13316 : sans marqueur, un + login partage ne prouve pas qu'on a relu) ; + - notre **propre** commentaire, reconnu a ses marqueurs. Sans cette + garde, le jour ou le texte de remediation nommerait un tag entre + crochets, l'organe se reconnaitrait comme revu et ne poserait plus + jamais son label -- une auto-desactivation silencieuse. + + ``comments`` : entree de ``gh pr list --json comments`` (``body`` lu seul). + ``None`` (projection anterieure, fixture) = pas de commentaires, jamais + une exception. + """ + for comment in comments or []: + body = comment.get("body") or "" + if COMMENT_MARKER_START in body: + continue # le notre -- ne s'auto-exempte jamais + if nits._PERSONA_MARKERS_RE.search(body): + return True + return False + + def classify(pr: dict, threshold: int = THRESHOLD_DEFAULT) -> str: """Classify a PR (as returned by ``gh pr view --json ...``). Returns one of: - - ``"flag"`` : PR exceeds threshold AND has no review (any author) + - ``"flag"`` : PR exceeds threshold AND has no review (any author, + on either surface -- ``reviews[]`` or a + persona-attributed review pass in the comments) - ``"clear"`` : PR is below threshold OR has at least one review - ``"skip_draft"``: PR is draft (excluded by design) - ``"skip_base"`` : PR base is not ``main`` (excluded by design) @@ -138,6 +207,8 @@ def classify(pr: dict, threshold: int = THRESHOLD_DEFAULT) -> str: return "clear" if len(reviews) > 0: return "clear" + if review_pass_in_comments(pr.get("comments")): + return "clear" return "flag" @@ -146,14 +217,20 @@ def fetch_open_prs(threshold: int) -> list[dict]: Why minimum JSON: ``gh pr list`` on a 800-PR repo with full payloads is slow and noisy. The classifier reads only ``number``, ``title``, - ``isDraft``, ``baseRefName``, ``additions``, ``reviews``. We use - ``--jq`` to project server-side and skip the rest. + ``isDraft``, ``baseRefName``, ``additions``, ``reviews``, ``comments``. + We use ``--jq`` to project server-side and skip the rest. + + ``comments`` joined the projection with the second review surface + (#16284). Its cost is measured, not assumed: on 121 open PRs the sweep + went 4.7 s / 466 Ko -> 11.7 s / 1.67 Mo, once a day, on an advisory cron. + The alternative (a second, per-candidate fetch) trades a bounded constant + for a re-classify dance; the union belongs in ONE place, ``classify``. """ cmd = [ "gh", "pr", "list", "--state", "open", "--base", "main", - "--json", "number,title,isDraft,baseRefName,additions,reviews,author,url", + "--json", "number,title,isDraft,baseRefName,additions,reviews,author,url,comments", "--limit", "300", ] out = subprocess.run(cmd, capture_output=True, text=True, encoding="utf-8", errors="replace", check=True) diff --git a/scripts/tests/test_review_coverage.py b/scripts/tests/test_review_coverage.py index c956115d28..76e65d71d5 100644 --- a/scripts/tests/test_review_coverage.py +++ b/scripts/tests/test_review_coverage.py @@ -278,6 +278,177 @@ def _run(cmd, *a, **k): assert len(warnings) == 1 and "404" in warnings[0], warnings +# --------------------------------------------------------------------------- +# Second review surface : une passe emise en COMMENTAIRE d'issue (#16284). +# +# Le perimetre `reviews[]` seul rendait l'organe aveugle a un mode d'emission +# STRUCTUREL du cluster : les personas emettent leur verdict en commentaire +# quand elles sont contraintes en jetons (« contrainte token : COMMENT only +# — opener `jsboige`, cap self-review #3219 », verbatim des fils). +# +# Les deux fixtures ci-dessous sont les corps REELS des deux cas mesures le +# 2026-09-15 ; l'organe avait publie « aucune review » 9 min et 6 min APRES +# la passe, et les deux PR portent encore le label. +# --------------------------------------------------------------------------- + +# Reel #16133 — clusterManager-Myia, 2026-09-14T12:29:47Z (tronque a la 3e +# ligne, qui suffit : le tag ouvre le paragraphe). +_REAL_PASS_CONCERNS = ( + "VERDICT: CONCERNS\n" + "\n" + "**[Hermes]** — #16133 — reproduction firsthand sur head `250707e2` " + "(fetch raw du notebook au ref branche), 3 artefacts :\n" +) +# Reel #16145 — clusterManager-Myia, 2026-09-14T12:31:38Z. +_REAL_PASS_LGTM = ( + "VERDICT: LGTM\n" + "\n" + "**[Hermes]** — #16145 — reproduction firsthand sur head `f230ac1e` " + "(fetch raw du notebook + patch README au ref branche), 3 artefacts :\n" +) + + +def _comment(login: str, body: str) -> dict: + """Une entree de `gh pr list --json comments` (forme mesuree : author.login).""" + return {"author": {"login": login}, "body": body} + + +def _pr_with_comments(additions: int, comments: list[dict]) -> dict: + return {**_pr(additions, 0), "comments": comments} + + +def test_review_pass_in_comment_lifts_the_flag(): + """#16133 : verdict CONCERNS en commentaire -> clear, plus flag.""" + pr = _pr_with_comments(2531, [_comment("clusterManager-Myia", _REAL_PASS_CONCERNS)]) + assert classify(pr) == "clear" + + +def test_review_pass_approval_in_comment_lifts_the_flag(): + """#16145 : verdict LGTM en commentaire -> clear. + + Meme defaut, surface APPROBATION -- le cas qui condamne un predicat bati + sur `nits.classify()` (cf. la garde ci-dessous). + """ + pr = _pr_with_comments(1705, [_comment("clusterManager-Myia", _REAL_PASS_LGTM)]) + assert classify(pr) == "clear" + + +def test_classify_alone_would_miss_the_approval(): + """Garde du choix d'implementation -- le piege, en executable. + + `nits.classify()` attrape CONCERNS (``BOT-CONCERN``) mais rend **None** + sur LGTM : cet organe classe les RESERVES, pas les passes. Un predicat + bati dessus fermerait #16133 en laissant #16145 faux -- le defaut du + ticket, deplace sur la surface des approbations. + """ + assert rc.nits.classify("clusterManager-Myia", _REAL_PASS_CONCERNS) == "BOT-CONCERN" + assert rc.nits.classify("clusterManager-Myia", _REAL_PASS_LGTM) is None + # ...et pourtant c'est une passe, des deux cotes : + for body in (_REAL_PASS_CONCERNS, _REAL_PASS_LGTM): + assert rc.review_pass_in_comments([_comment("clusterManager-Myia", body)]) + + +def test_bare_login_verdict_is_not_a_review_pass(): + """Un commentaire NU d'un login partage ne prouve pas qu'on a relu. + + `jsboige` est l'identite de poussee de toutes les lanes (#13316), donc un + ``VERDICT: LGTM`` nu sous ce login peut etre une lane qui parle de sa + propre PR. Le canon ne l'admet que porteur d'un marqueur ; on suit le + canon plutot que d'ouvrir un second juge. + """ + pr = _pr_with_comments( + 1000, [_comment("jsboige", "VERDICT: LGTM\n\nOK pour moi.\n")]) + assert classify(pr) == "flag" + + +def test_own_remediation_comment_never_self_exempts(): + """Le commentaire de l'organe n'est JAMAIS une passe de review. + + Garde d'auto-desactivation : le texte de remediation nomme les reviewers + (« Hermes, ai-01, ou review humaine »). Le jour ou il les nommerait entre + crochets, l'organe se reconnaitrait comme revu et ne poserait plus jamais + son label -- en silence. + """ + own = f"{rc.COMMENT_MARKER_START}\n{rc.REMEDIATION}\n{rc.COMMENT_MARKER_END}" + assert rc.review_pass_in_comments([_comment("github-actions[bot]", own)]) is False + # Controle : le MEME texte, sans nos marqueurs mais coiffe d'un tag de + # persona -- la garde tombe, la passe est vue. C'est bien le marqueur de + # l'organe, et non son auteur, qui l'exempte. + tagged = "**[Hermes]**\n" + rc.REMEDIATION + assert rc.review_pass_in_comments([_comment("clusterManager-Myia", tagged)]) is True + + +def test_untagged_persona_comment_is_not_a_pass(): + """Mesure qui a tranche le predicat : le login SEUL compterait une LEVEE. + + Sur les 121 PR ouvertes du 2026-09-15, ``clusterManager-Myia`` a commente + 10 fois : 9 passes portant le tag, et 1 sans tag -- #15795, une levee + (« Levee a la tete exacte ... -- review 5202580554 »). + + Honnetement : sur ce corpus les deux predicats rendent le **meme** + verdict, parce que cette levee tombe sur une PR sous le seuil. Le test + ne pretend donc pas que le login coute quelque chose aujourd'hui -- il + fige la classe : une levee non taggee n'est pas une passe, et le jour ou + elle tombe sur une PR large non revue, le login la declarerait couverte + (le trou de #11232, invisible). Le tag est requis, comme le demande le + canon. + """ + lift = ("Levée à la tête exacte `284d067ee6265d99e42590897d8c38597c47cca4`" + " — review `5202580554`.\n") + pr = _pr_with_comments(1000, [_comment("clusterManager-Myia", lift)]) + assert classify(pr) == "flag" + + +def test_backticked_tag_is_a_citation_not_a_pass(): + """`` `[Hermes]` `` cite est une CITATION, pas une emission (#13030). + + C'est la raison exacte pour laquelle on importe le motif durci du canon au + lieu du `has_marker()` public : ce dernier est un `in` de sous-chaine et + compterait la citation. Le test fige les deux comportements cote a cote -- + si le public devenait utilisable, ce test le dirait. + """ + quoted = "Le ticket dit : `[Hermes]` a rendu un verdict CONCERNS.\n" + assert rc.nits.has_marker(quoted, ("[Hermes]",)) is True # le public matcherait + assert rc.nits._PERSONA_MARKERS_RE.search(quoted) is None # le durci non + pr = _pr_with_comments(1000, [_comment("clusterManager-Myia", quoted)]) + assert classify(pr) == "flag" + + +def test_bold_persona_tag_counts(): + """Un en-tete en GRAS compte (#14503, PR #14548). + + Une variante `(?:^|\\s)` du motif exigeait une espace avant `[` : le `*` + du gras suffisait alors a effacer la passe entiere du radar. + """ + assert rc.review_pass_in_comments( + [_comment("clusterManager-Myia", "**[NanoClaw]** structural review\n")]) is True + + +def test_comment_pass_does_not_override_the_skips(): + """L'ordre des verdicts tient : un draft reste un draft. + + Les skips precedent le seuil ET les surfaces de review ; une passe en + commentaire ne doit pas transformer un hors-perimetre en couvert. + """ + draft = {**_pr(9999, 0, is_draft=True), + "comments": [_comment("clusterManager-Myia", _REAL_PASS_LGTM)]} + assert classify(draft) == "skip_draft" + off_base = {**_pr(9999, 0, base="feature/x"), + "comments": [_comment("clusterManager-Myia", _REAL_PASS_LGTM)]} + assert classify(off_base) == "skip_base" + + +def test_missing_comments_key_is_not_a_crash(): + """Projection anterieure / fixture sans `comments` : pas de passe, pas d'exception. + + Le cron ne doit pas tomber parce qu'une forme de payload a change -- et + les fixtures historiques du haut de ce fichier n'ont pas la cle. + """ + assert rc.review_pass_in_comments(None) is False + assert rc.review_pass_in_comments([]) is False + assert classify(_pr(1000, 0)) == "flag" + + def test_label_constants_within_github_api_limits(): # 422 muet du run 2026-08-29 : LABEL_DESC faisait 189 chars, la limite # REST est 100 -- gh label create echouait en silence et le payload