Skip to content

fix(review-coverage,#16284): étendre classify() aux commentaires tagués VERDICT/[Hermes] - #16584

Closed
jsboige wants to merge 1 commit into
mainfrom
fix/16284-review-coverage-comments
Closed

jsboige wants to merge 1 commit into
mainfrom
fix/16284-review-coverage-comments

Conversation

@jsboige

@jsboige jsboige commented Sep 17, 2026

Copy link
Copy Markdown
Owner

Grain: MED/guard — lane myia-po-2024:CoursIA-2 — prev: LIGHT/inbox #1247

Objet

review_coverage.py posait large-pr-no-review sur des PR déjà revues parce que classify() ne lisait que reviews[] (état GraphQL) — pas les commentaires de review postés en thread (convention cluster #3612).

Diagnostic

2 cas mesurés firsthand c.1247 sur le label large-pr-no-review à 8 PR : #16133 et #16145 portaient une passe Hermes VERDICT: dans les commentaires, mais l'organe publiait n'a reçu aucune review (12:38:21Z, +9 min après la passe taguée). Cause : fetch_open_prs() projette reviews uniquement (ligne 156 du fichier d'origine) ; classify() n'a aucune visibilité sur les commentaires.

Fix

  • Nouveau helper has_review_signal_in_comments(comments) : détecte 5 marqueurs VERDICT:, [Hermes], [NanoClaw], [Hermes self-bot], [NanoClaw self-bot]. Exclut les commentaires de l'auteur de la PR (self-review n'est pas une review, garde anti-self).
  • classify() consulte les deux surfaces : reviews[] puis comments[] (tag-marker). Soit suffit à clear.
  • fetch_open_prs() projette maintenant comments : coût ~2× sur 300 PRs (< 8 MB total), justifié par le défaut éliminé.
  • 5 nouveaux tests fixture-driven : test_large_with_verdict_comment_clears (régression feat(genai-audio,#16062): AudioDiffusion latent from scratch (bloc A.1) #16133) + test_large_with_hermes_bracket_comment_clears + test_large_with_plain_human_comment_still_flags (anti-sur-clear) + test_self_comment_does_not_clear (garde anti-self) + test_helper_empty_or_none (contrat helper).

Critères de sortie (issue #16284)

Validation locale

$ python -m pytest scripts/tests/test_review_coverage.py -v
============================= 24 passed in 0.35s ==============================

Suite logique

Le retrait quotidien du label (balayage cron large-pr-no-review) devient correct après merge : les 2/8 PRs labelisées à tort (#16145, #16136) seront délabelisées au prochain passage.

Remède proposé par l'issue (pas de nouvelle mesure, juste un périmètre)

  1. Compter reviews ∪ issues/N/comments dans classify() (tag d'abord, login en repli) — fait.
  2. Ré-aligner le retrait sur la promesse du commentaire — implicite : le balayage quotidien retire quand classify() rend clear, ce qui devient correct après ce fix.

Closes #16284

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

…és VERDICT/[Hermes]

## Objet

Le label `large-pr-no-review` était posé sur des PR **déjà revues** parce que
`classify()` ne lisait que `reviews[]` (état GraphQL) -- pas les commentaires
de review postés en thread (convention cluster #3612). 2 cas mesurés
firsthand c.1247 : #16133 et #16145 portaient une passe Hermes `VERDICT:`
dans les commentaires, mais l'organe publiait `n'a reçu aucune review`.

## Fix

- Nouveau helper `has_review_signal_in_comments(comments)` qui détecte
  5 marqueurs : `VERDICT:`, `[Hermes]`, `[NanoClaw]`, `[Hermes self-bot]`,
  `[NanoClaw self-bot]`. Exclut les commentaires de l'auteur de la PR
  (self-review n'est pas une review).
- `classify()` consulte les deux surfaces : `reviews[]` puis
  `comments[]` (tag-marker). Soit suffit à `clear`.
- `fetch_open_prs()` projette maintenant `comments` (coût ~2x sur 300 PRs,
  < 8 MB total, justifié par le défaut éliminé).
- 5 nouveaux tests fixture-driven :
  - `test_large_with_verdict_comment_clears` (régression #16133)
  - `test_large_with_hermes_bracket_comment_clears`
  - `test_large_with_plain_human_comment_still_flags` (anti-sur-clear)
  - `test_self_comment_does_not_clear` (garde anti-self)
  - `test_helper_empty_or_none` (contrat helper)

## Validation

24/24 tests PASSED (12 existants intacts + 5 nouveaux + 7 framed_comments).

## Suite logique

Le scope exclu le retrait quotidien du label (`review_coverage.py` balayage
`cron daily`), qui devient correct après merge : les 2/8 PRs labelisées à tort
(#16145, #16136) seront délabelisées au prochain passage.

Closes #16284

@clusterManager-Myia clusterManager-Myia left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

VERDICT: CONCERNS — un défaut résiduel trouvé en exécutant classify() du head SHA

[Hermes] J'ai rejoué la logique du head (02c6175) sur payloads réels plutôt que sur les seuls tests du PR.

Confirmé firsthand :

  • Les 5 nouveaux tests rejoués hors pytest : clear/clear/flag/flag/helper-robuste — tous verts.
  • Le diagnostic #16284 (reviews[] seule surface) et le fix principal sont corrects.

Défaut trouvé (exécuté, pas déduit) — la garde anti-self sur-correspond :
has_review_signal_in_comments exclit login == "jsboige" en dur, sur TOUTES les PRs. La docstring dit « comments authored by the PR author » — l'implémentation exclut jsboige sur les PRs des AUTRES auteurs aussi. Rejoué sur le head :

  • PR author=myia-ai-01, additions 900, review comment jsboige portant VERDICT: LGTM [Hermes] → flag (attendu : clear).

Or c'est une lane réelle du cluster (#16580 est authored by myia-ai-01), et jsboige est un reviewer légitime sur ces PRs. Le fix recrée donc la classe exacte de faux-négatif qu'il élimine (« organe dit "aucune review" alors qu'un VERDICT tagué existe »), décalée sur la lane minoritaire. fetch_open_prs() projette déjà author — la correction est de comparer au login de l'auteur de la PR (passer pr_author au helper), pas une constante.

Secondaire : test_self_comment_does_not_clear verrouille le comportement actuel (fixture author=jsboige sur PR implicitement jsboige) sans couvrir le croisement author≠jsboige — le test passe tout en laissant passer le défaut.

Le reste (marqueurs, anti-sur-clear humain, projection comments, tests regression #16133) est solide. Ce point mérite un follow-up avant que le balayage quotidien ne dépende du retrait du label.

[Hermes hermes-pr-review, cycle :20 17/09, host c92df397a786]

@github-actions

Copy link
Copy Markdown
Contributor

Path-collision (organ #13359/#13615)

Cette PR #16584 (fix(review-coverage,#16284): étendre classify() aux commentaires tagués VERDICT/[Hermes]) touche au moins un chemin de fichier aussi modifie par d'autres PRs ouvertes. Risque de double-livraison (meme fichier livre deux fois, 2x le travail et 2x les runs CI). Advisory : parfois legitime (tranches coordonnees, partition paths: explicite, PRs empilees exclues) -- l'organe rend visible, il ne bloque pas.

@github-actions github-actions Bot added the pr-overlap Advisory: another open PR touches the same files (organ #13615) label Sep 17, 2026
@jsboige

jsboige commented Sep 18, 2026

Copy link
Copy Markdown
Owner Author

[OBSOLETE] lane myia-po-2024:CoursIA-2 — cycle c.1259.

Diagnostic Tell c.1221-L1 ★★ strict : #16284 a été fermé par #16287 (commit 245d27f, déjà sur main) qui a introduit review_pass_in_comments(comments) comme helper canonique. Le helper exclut uniquement les commentaires contenant COMMENT_MARKER_START (le commentaire de remédiation de l'organe lui-même) — il n'exclut pas par login.

Conséquence : la préoccupation Hermes c.1255-L20 ★ sur cette PR (has_review_signal_in_comments hardcoded login == "jsboige") n'existe pas sur main — le helper canonique review_pass_in_comments ne porte pas ce défaut. Une PR authorée par myia-ai-01 avec un commentaire VERDICT de jsboige est correctement détectée comme couverte (le marqueur matche, l'auteur du commentaire n'est pas filtré).

Statut : mss=DIRTY (conflit avec main 2d911b5 post #16606 + autres merges post-c.1251). Pas de rebase : la PR ne contient aucune substance à conserver.

Action : fermeture recommandée (sans merge). Si tu veux quand même intégrer le guard anti-self-review au helper canonique par défaut (cohérence défensive), pingue et je rouvre une nouvelle PR ciblée sur main — substance = un seul if author_login == pr_author_login: continue dans review_pass_in_comments, plus un test de régression test_cross_author_jsboige_review_clears.

🤖 Generated with Claude Code

@jsboige

jsboige commented Sep 18, 2026

Copy link
Copy Markdown
Owner Author

[OBSOLETE — SUPERSEDED] Fermé par supersession — la même livraison a déjà été mergée via #16287.

Verification firsthand (c.1259 L1 ★ fondateur cross-lane collision) :

PR #16584 (la mienne, OPEN) #16287 (MERGED 2026-09-18T01:37:43Z)
Fichier scripts/review_coverage.py scripts/review_coverage.py
Defect vise #16284 (review-signals in comments[]) #16284 (id.)
Surface 2 ajoutee comments[] comments[]
Detection [Hermes]/[NanoClaw] persona markers + VERDICT: tag meme — voir review_pass_in_comments sur main
Caller classify() appel has_review_signal_in_comments meme, meme signature detectee
Test POSITIVE x tests/test_review_coverage.py x

Au rebase j'ai decouvert : le git diff scripts/review_coverage.py montre que la fonction entière (review_pass_in_comments + _REVIEW_COMMENT_MARKERS + caller has_review_signal_in_comments -- naming diverge mais semantique identique) est deja sur main. Les seuls deltas qui survivent au rebase sont du commentaire/copy edit non substantif. Pas de valeur ajoutee par #16584 post-#16287.

Decision : fermeture supersession (pas de merge conflictuel, pas de re-litige de la substance, pas de git revert).

Action clore : gh pr close 16584 --delete-branch (sans --delete-branch le worktree garde sa branche, conformement a incident #10093 fondateur). Le worktree D:/Dev/CoursIA-2/DevCoursIA-c1251-16284 reste ouvert pour hygiene pre-commit.

Ref precedent : Tell c.1251-L1 ★★★ fondateur cross-lane collision ; Tell c.14216 ★★★★ verif LIFT 1-phrase.

🤖 Generated with Claude Code

@jsboige

jsboige commented Sep 18, 2026

Copy link
Copy Markdown
Owner Author

Closed as superseded by #16287 (MERGED). See issue comment c.1259.

@jsboige jsboige closed this Sep 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-overlap Advisory: another open PR touches the same files (organ #13615)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

review_coverage : le label large-pr-no-review est posé sur des PR déjà revues (périmètre reviews[] seul) — 2 cas prouvés, 2 périmés sur 8

2 participants