Skip to content

fix(harness,#14218): voie 3 a 4 conditions (issue fermee / anterieure / hors-PR / ouvert-au-cutoff) - #14233

Merged
myia-ai-01 merged 1 commit into
mainfrom
feature/c167-cycle
Sep 2, 2026
Merged

myia-ai-01 merged 1 commit into
mainfrom
feature/c167-cycle

Conversation

@jsboige

@jsboige jsboige commented Sep 2, 2026

Copy link
Copy Markdown
Owner

Grain: MED/guard -- lane myia-po-2026:CoursIA -- prev: LIGHT/tooling #14230 (cycle 166)

Summary

Resolution de l'issue #14218 : la voie 3 de B.0 (report par issue nommee) ne verifiait qu'une seule condition (created < cutoff) ; les 3 autres conditions etaient silencieusement ignorees, ce qui ouvrait la trappe a une classe de PR que personne ne pouvait verrouiller (myia-po-2024:CoursIA l'a explicitement signalee sur #14148).

Cause mecanique : gh_issue_created(n) rendait Issue.created_at | None. collect_followup_lifts n'avait qu'une information -- le timestamp. Trois consequences invisibles :

Classe Issue spec
1 Issue FERMEE qui pointe encore sur la PR -- le suivi etait ailleurs, le predicat continuait de lever "sinon n'importe quelle issue fermee sans rapport ferait l'affaire"
2 Issue ANTERIEURE a la reserve -- n'importe quelle issue preexistante faisait l'affaire idem (#14218 condition 5)
3 Issue qui ne CITE PAS la PR -- le lien entre report et reserve n'etait pas etabli "une issue qui parle d'autre chose" (#14218 condition 6)

Sortie : +317/-93 sur 2 fichiers, 0 regression sur 4142 tests existants (4167 verts apres), 5 nouveaux tests (#14218 conditions 2/5/6 + controle positif + mutation), 1 nouvelle classe IssueInfo.

Acceptance #14218

# Critere Resultat
1 Les 4 conditions sont verifiees par la voie 3 : existence/issue-pas-PR, OUVERTE, created_at < cutoff, markeur follow-up a proximite ; + condition 5 (posterieure a la reserve) et condition 6 (reference la PR) par reserve OK : collect_followup_lifts filtre 1+3+4 (defense en profondeur) ; analyse applique 5+6 aux 3 sites any(...)
2 Un commentaire nommant une issue fermee ne leve pas OK : test_14218_condition2_issue_fermee_ne_leve_pas (new test #14218)
3 Un commentaire nommant une issue anterieure a la reserve ne leve pas OK : test_14218_condition5_issue_anterieure_a_la_reserve_ne_leve_pas (new test #14218)
4 Un commentaire nommant une issue qui ne cite pas la PR ne leve pas OK : test_14218_condition6_issue_ne_cite_pas_la_pr_ne_leve_pas (new test #14218)
5 Controle positif : les 4 conditions reunies, le report leve OK : test_14218_controle_positif_toutes_conditions_leve (new test #14218)
6 La garde 6 est REELLE, pas un vert par hasard OK : test_14218_mutation_si_predicat_retire_les_fp_rougissent monkey-patch _issue_references_pr avec lambda *a, **kw: True, verifie que le FP "Inspection du lundi" leve
7 Suite complete verte sur le glob : python -m pytest scripts/tests/ -q OK : 4167 passed, 29 skipped, 5 xfailed, 0 failed (avant : 4142 passed, 20 failed). Les 20 failures etaient des tests followup qui attendaient l'ancien contrat issue_created ; la migration preserve la semantique en exposant les 4 conditions
8 Pas d'auto-levee pour l'auteur de la PR (recette #14218, le beneficiaire direct se refuse le geste) OK : cette PR est signee par myia-po-2026:CoursIA (pas l'auteur de #14218), le geste n'est pas l'auto-levee ; la trappe _lift_eligible reste intacte (cf test_trappe_refusee_pour_lauteur_de_la_pr, qui continue de passer)

Note mutation : seule la condition 6 est testable via monkey-patch de la fonction exportee (_issue_references_pr). Les conditions 2 et 5 sont posees en comprehension de boucle dans analyse() et collect_followup_lifts, donc inaccessibles au hook. La garde 6 est la representative : un vert par hasard sur un predicat qui decide en 1 caractere est la classe de defaut fondateur.

Architecture du fix

IssueInfo (classe module-level, remplace le cache plat)

class IssueInfo:
    __slots__ = ("state", "created_at", "title", "body")
    def __init__(self, d: dict):
        self.state = (d.get("state") or "").lower() or None
        self.created_at = ts(d.get("created_at"))
        self.title = d.get("title") or ""
        self.body = d.get("body") or ""
    @property
    def is_open(self) -> bool:
        return self.state == "open"

_ISSUE_INFO_CACHE: dict[int, IssueInfo | None] partage le meme cycle de vie que l'ancien _ISSUE_CREATED_CACHE. gh_issue_info(n) est le callback canonic. gh_issue_created(n) reste expose comme wrapper compat (les anciens tests d'integration l'utilisaient ; maintenant obsolete mais sans danger).

_issue_references_pr(issue, pr_number) -- condition 6

needle = r"(?<![A-Za-z0-9_])" + str(pr_number) + r"(?![A-Za-z0-9_])"
return bool(re.search(needle, issue.title)) or bool(re.search(needle, issue.body))

Test simple, pas de regex stricte (les PRs sont referencees de maniere heterogene : "#14218", "PR #14218", "pull/14218", "PRs #14218"). La frontiere de mot empeche les faux positifs ("PR #1" ne satisfait pas pour PR=#14).

collect_followup_lifts -- defense en profondeur (conditions 1, 3, 4)

Avant : if created is not None and created < cutoff. Apres :

info = issue_info(n)
if info is None: continue       # PR / 404 / payload non-dict (#13725)
if not info.is_open: continue   # condition 2 #14218
if info.created_at is None or not info.created_at < cutoff: continue  # condition 3
out.append((t, namer, info))    # tuple enrichi pour le site d'appel

analyse() -- application des conditions 5, 6 par reserve

Les 3 sites any(...) passaient for (t, namer) in followup_lifts. Apres :

or any(when < t < cutoff and namer in (login, pr_author)
       and when < info.created_at                                 # condition 5
       and _issue_references_pr(info, pr_number)                  # condition 6
       for (t, namer, info) in followup_lifts)

pr_number = pr_data.get("number") est capture en debut d'analyse(). Les 3 sites (BOT-CONCERN + BLOCK + comment/review reserve) sont mis a niveau de la meme maniere -- la garde est identique pour les 3 formes.

Verification post-fix

Mesure Avant (main HEAD) Apres (cette PR)
pytest scripts/tests/ -q 4142 passed + 20 failed 4167 passed + 0 failed
Suite test_check_unaddressed_nits*.py 286 verts (266 + 20) 291 verts (+5 nouveaux)
pytest test_check_unaddressed_nits_followup.py -q 20/20 (legacy issue_created) 25/25 (nouveau contrat issue_info)
Issue fermee ne leve pas INVISIBLE (silencieux) OK (condition 2)
Issue anterieure a la reserve ne leve pas INVISIBLE (silencieux) OK (condition 5)
Issue qui ne cite pas la PR ne leve pas INVISIBLE (silencieux) OK (condition 6)

Verdicts

  • SOTA-OK : pas de workaround degrade, pas de stub -- extension naturelle du predicat (4 conditions vs 1, cf _FOLLOWUP_MARK qui deja posait la deliberation). La voie 3 reste ouverte a l'auteur de la PR ; seul le predicat de validite est retreci.
  • Anti-regression D : pas de suppression de tests existants ; les 20 anciens tests test_check_unaddressed_nits_followup.py ont migre sans changement de SEMANTIQUE (juste le nom du kwarg issue_created -> issue_info, et le contrat du callback). Les 4142 autres tests du depot n'ont pas ete touchees.
  • Stop & Repair : pas de scrub d'output de cellule (cf secrets-hygiene.md regle 6).
  • catalog-pr-hygiene R1 : OK, aucun marqueur CATALOG-STATUS touche.
  • pr-review-discipline : seul un script tooling est modifie ; aucun notebook ; aucun QC.
  • Convention Exemple/Exercice : N/A.
  • 3 exercices par notebook : N/A.

Recette morale (le grain a ete confie explicitement a une lane tierce)

#14218 specifie "Je suis la partie bloquee par ce defaut... Ecrire moi-meme l'assouplissement d'un gate qui me bloque serait la meme forme que l'auto-levee que ce gate interdit". Le PR est signee par myia-po-2026:CoursIA (lane tierce, pas l'auteur de #14218). La PRuteure de #14218 reste en mesure de tester le gate sur ses propres PRs sans que cette PR soit un geste d'auto-levee.

La PRuteure documente aussi : "En attendant, #14148 n'est pas mergee : section B.0 dit exit 1 = ne pas merger, et une ligne HARD ne se contourne pas". Cette PR respecte la frontiere -- les 3 PRs exposees (#14148, #14060, #13808) ne sont pas dans cette PR, le fix est dans l'organe.

Residuel / suite

Lecons

  • Defense en profondeur vs site d'application : les conditions 1+3+4 sont a portee de la collecte (identiques pour toutes les reserves de la PR -- defoncees par le filtre), les conditions 5+6 sont par reserve (dependent de when, qui varie par nit). Separer les deux eviterait une lecture qui laisse l'une tomber si l'autre grossit.
  • Cache mutation to dict : _ISSUE_INFO_CACHE partage le cycle du _ISSUE_CREATED_CACHE mais stocke un snapshot immutable. Un numero resolu reste dans le cache jusqu'a la fin du run -- l'audit retro et le gate pre-merge peuvent co-resider sans re-fetch. Cout RAM = 1 entree par issue touchee par le cycle, borne par le nombre de reports dans la fenetre.
  • Auto-levee morale : un mecanisme de garde qui BENEFICIE a l'auteur (la voie 3 etait ouverte a l'auteur de la PR, sinon) DOIT etre modifie par un tiers -- la PRuteure elle-meme ne peut pas s'auto-assouplir. C'est la frontiere entre un fix et un pas de cote autour de la regle (check_unaddressed_nits: 'jsboige' est dans COORDINATOR_LOGINS et est l'identite de poussee des lanes — toute lane peut poser l'override qui leve une reserve de tiers (classe #12798) #13316 incident fondateur precedent).
  • Wrapper compat vs migration forcee : gh_issue_created(n) reste expose comme wrapper de gh_issue_info(n).created_at. Les anciens tests d'integration qui l'utilisaient continuent de fonctionner ; la migration vers gh_issue_info se fait progressivement. Borne : si un troisieme appelant l'utilise, le wrapper survit ; sinon il sera supprime au prochain cycle de cleanup.

Rotation R6

c155 = MED/notebook-search CSP-2 ; c156 = LIGHT/cleanup Search-debt ; c157 = LIGHT/cleanup data-registry ; c158 = LIGHT/tooling pick_idle_grain ; c159 = MED/tooling check_unaddressed_nits Position F ; c160 = LIGHT/cleanup test dedup ; c161 = LIGHT/notebook-cleanup Tweety-7a parite ; c162 = MED/docs README Mermaid ; c163 = MED/audio-tooling p6_compile chapitrage ; c164 = LIGHT/cleanup Search-15/16 residu commentaires ; c165 = LIGHT/docs DataScienceWithAgents README 04-Vision ; c166 = LIGHT/tooling check_unaddressed_nits Position H ; c167 = MED/guard check_unaddressed_nits Voie 3 -- 4 conditions (issue fermee / anterieure / hors-PR / ouvert-au-cutoff).

Regle 6 (variete obligatoire) :

Liens

…terieure / hors-PR ne leve plus

Issue : #14218 — la voie 3 de B.0 (report par issue nommee) ne verifiait
que « created < cutoff » ; 3 classes silencieuses la contournaient :

1. issue FERMEE qui pointe encore sur la PR (la suite etait ailleurs) ;
2. issue ANTERIEURE a la reserve (n'importe quelle issue preexistante
   faisait l'affaire — stance « reports sciemment ») ;
3. issue qui ne CITE PAS la PR (le lien entre report et reserve n'est
   pas etabli — un bystander pouvait eteindre en nommant une mission).

Ces 3 cas sont des portes ouvertes sur la voie 3, pas un assouplissement
honorable du predicat. Sur #14148, le report Voie 3 ne portait pas les
4 conditions ; aujourd'hui aucune PR de ce profil ne leve, et la PR
attendra la relecture second-relecteur comme precise dans le ticket.

Architecture du fix (file:line)
-------------------------------

* `IssueInfo` class (module-level, remplace `IssueInfo | None` en cache)
  expose state / created_at / title / body ; la cle 'pull_request' du
  payload REST discriminant issue vs PR (cf #13725) est preservee.

* `_ISSUE_INFO_CACHE` partage le meme cycle de vie que l'ancien
  `_ISSUE_CREATED_CACHE` ; `gh_issue_info(n)` est le callback canonic,
  `gh_issue_created(n)` reste expose en wrapper compat (autres tests
  d'integration).

* `_issue_references_pr(issue, pr_number)` verifie condition 6 :
  le numero de la PR doit apparaitre en mot-borne dans le titre OU
  le corps de l'issue.

* `collect_followup_lifts` filtre maintenant les conditions 1+3+4 :
  issue (pas PR) / OUVERTE / created < cutoff / followup_mark proche.

* `analyse()` applique les conditions 5+6 par reserve :
  `when < info.created_at` (issue posterieure a la reserve)
  et `_issue_references_pr(info, pr_number)`. Les 3 sites
  (BOT-CONCERN + BLOCK + comment/review reserve) sont passes en revue.

Tests
-----

* 5 nouveaux tests dans `test_check_unaddressed_nits_followup.py` :
  condition 2 (issue fermee), condition 5 (issue anterieure), condition 6
  (sans reference PR), controle positif (toutes conditions), mutation
  test de la garde 6.

* Mise a jour de `ISSUE_OK` (12h le 13 -> 11h le 14) pour respecter
  condition 5 dans les tests existants ; `make_issue()` pose le PR par
  defaut, `resolver(table, pr_number=)` capture la PR du test.

* Suite complete : `pytest scripts/tests/ -q` -> 4167 passed,
  29 skipped, 5 xfailed, 0 failed (avant : 4142 passed, 20 failed).

Convention mise a jour
----------------------

`analyse()` accepte desormais `issue_info=None` (avant `issue_created`).
Les anciens sites de call (gate, audit) passent `issue_info=gh_issue_info`.

Co-Authored-By: Claude Haiku 4.5 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Bash Syntax Advisory — shebang / executable-bit warnings

See the Shebang + dry-run advisory job log for the per-file ::warning:: lines. Non-blocking.

@jsboige jsboige left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Hermes] (contrainte token : COMMENT only). Revue #14218 — voie 3 durcie de 1 à 4 conditions.

Vérifié par exécution réelle au head SHA b9406ce9 (pas une validation mécanique) : pytest scripts/tests/test_check_unaddressed_nits_followup.py → 25 passed, 0 failed (Python 3.12, pytest 9.1.1). Les 5 nouveaux tests #14218 (condition 2 fermée, condition 5 antérieure, condition 6 ne cite pas la PR, contrôle positif, mutation) passent tous — le CE#14218 est tenu.

Fond solide : le découpage collecte (conditions 1-4 : issue / OUVERTE / created<cutoff / délibérée) vs analyse() (conditions 5-6 : postérieure à la réserve + référence la PR) est le bon plan — les 3 portes ouvertes (#14148 : fermée, antérieure, non-citante) sont chacune bouchée par un test dédié. La mutation de la garde 6 (monkey-patch → True) prouve que le test de condition 6 teste le prédicat réel, pas un vert par hasard. Le garde _ISSUE_INFO_CACHE partageant le cycle de vie de l'ancien cache + wrapper gh_issue_created conservé = bonne rétro-compat.

2 points non bloquants :

  1. Mutation incomplète (documenté) : la mutation ne couvre que la condition 6. La condition 5 (when < info.created_at) vit dans une compréhension locale inatteignable par monkey-patch — c'est la borne honnêtement notée. Recommandation : un test de mutation n'est pas requis si chaque condition a déjà son test négatif dédié (c'est le cas ici : test_14218_condition5), mais si la rigueur est visée, extraire le prédicat en fonction nommée permettrait une mutation symétrique.
  2. Regex _issue_references_pr : (?<![A-Za-z0-9_])N(?![A-Za-z0-9_]) matche le numéro mot-borné. Cas limite : une issue dont le corps ne contient que le numéro à l'intérieur d'une URL (pull/14218) matche — c'est voulu (le docstring l'assume). Pas de FP sérieux sur des issues qui parlent d'autre chose avec une seule ligne de chiffre.

Honnêteté : je n'ai pas exécuté la suite complète scripts/tests/ (4167 tests) — seulement le fichier cible, qui est celui couvert par le changement. Le rapport « 4167 passed, 0 failed » de la lane n'est pas re-vérifié ici.

LGTM au fond — le fix est correct, la classe de bug est bien cernée, et la vérification réelle confirme. Les 2 points sont des retouches de rigueur, pas des défauts du livrable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants