Skip to content

Fix(guard,#19475): etendre check-subprocess-encoding aux cellules .ipynb - #19557

Closed
jsboige wants to merge 1 commit into
mainfrom
feature/19475-ipynb-encoding
Closed

jsboige wants to merge 1 commit into
mainfrom
feature/19475-ipynb-encoding

Conversation

@jsboige

@jsboige jsboige commented Oct 6, 2026

Copy link
Copy Markdown
Owner

Grain: MED/guard -- lane myia-po-2024:CoursIA-2 -- prev: DEEP/notebook-python #19552

Sujet

Extension du ratchet pre-commit check-subprocess-encoding (issue #13140,
generalisation de #12813/#12811) aux cellules de carnets Python. Le
guard refusait jusqu'ici les appels subprocess.<fn>(text=True) sans
encoding= dans les fichiers .py, mais la classe d'erreur vise
(UnicodeDecodeError cp1252 sur payload UTF-8) a ete mesuree presque
exclusivement dans des cellules de carnets : 19 cellules de la serie Lean
pendant le tapis #15629 -- toutes corrigees a la main, toutes invisibles
au guard en l'etat. Le commentaire du user sur #19415 a signale le meme
reste sur un carnet non-Conway.

Modifications

scripts/check_subprocess_encoding.py

  • Nouvelle fonction scan_notebook(path) : extrait les cellules code d'un
    .ipynb, filtre par kernelspec.name.startswith("python") (les kernels
    .NET Interactive / Lean sont ignores -- la classe n'a pas de sens),
    et re-applique scan_source sur la source jointe.
  • Sortie : (ligne_absolue, cell_idx, snippet) par violation. Le cell_idx
    permet au reviewer de naviguer directement vers la cellule fautive dans
    le Jupyter UI.
  • Meme predicat que sur .py : subprocess + text=True/universal_newlines=True
    sans encoding= dans le meme appel. Predicat deja valide et teste.
  • Le mode --base inclut maintenant les .ipynb dans changed_python_files
    (renommage interne preserve la compat).
  • Format du rapport de succes adapte : "N .py + M .ipynb changed file(s), 0 violation(s)" (au lieu de l'ancien "N changed .py file(s), ...").

.pre-commit-config.yaml

scripts/tests/test_check_subprocess_encoding.py

8 tests ajoutes (16 -> 25) :

  • test_scan_notebook_violation_in_first_cell : cellule code avec
    subprocess.run(['x'], text=True) => rouge, ligne absolue = 2, cell_idx = 0.
  • test_scan_notebook_clean_when_encoding_present : text=True, encoding='utf-8'
    dans la meme cellule => clean.
  • test_scan_notebook_skips_non_python_kernels : kernelspec .net-csharp
    => scan ignore (meme une cellule qui ressemble a du Python n'est pas
    scannee sous un kernel non-Python).
  • test_scan_notebook_skips_markdown_cells : cell_type: "markdown"
    ignore (meme politique que scan_source sur la prose .py).
  • test_scan_notebook_line_offset_continues_across_cells : cell 0 = 1
    ligne, cell 1 commence a la ligne 3 (separateur +1), violation en cell 1
    ligne 2 interne = ligne 4 absolue.
  • test_scan_notebook_malformed_json_returns_empty : JSON casse => []
    (best-effort, garde dupliquee par cell-source-parses sur la classe
    malformed).
  • test_scan_notebook_list_source_is_joined : nbformat 4.x peut stocker
    source comme liste de strings ; le parser joint avant scan (sinon
    les offsets de ligne sont faux).
  • test_main_files_mode_picks_up_ipynb : integration main(argv) sur un
    .ipynb via le mode files, verifie que le rapport cite cell[0].

Test modifie : test_main_base_mode -- le message de succes a change de
forme ("1 changed .py file(s)" -> "1 .py + 1 .ipynb"), le test attend la
nouvelle forme.

Validation locale

Acceptance issue #19475

  1. Rouge sur nouveau : un notebook staged avec subprocess.run(['x'], text=True) dans une cellule code => hook refuse le commit, rapport
    path:cell[0]:2: text=True without encoding= :: subprocess.run(['x'], text=True).
  2. Vert sur main : la totalite du corpus (cellules .ipynb) reste vert
    -- y compris Lean-15c (test direct) et par inference le reste des 19
    cellules corrigees par le tapis fix(lean): subprocess text=True sans encoding=utf-8 — sorties dégradées (None + traceback reader-thread) hors PYTHONUTF8=1 #15629 (toutes deja fixees avant la
    garde, ne declenchent rien).
  3. Pas de jambe CI : meme arbitrage [infra] Famine CI chiffree : +251 runs/h, 0 verdict sur 49 terminés — 50 % du volume vient de 12 gardes non filtrés #13097 (commit-time, zero slot) que
    le guard original. La couverture reste bornee au pre-commit.
  4. Tests complets : 8 cas de bord (kernel, markdown, multi-cell, JSON
    malforme, source liste, mode files, mode base, scan_source direct) +
    16 tests pre-existants tous verts.

Prong B SOTA (non-degenere)

Le guard reproduit la meme politique de detection que la version
.py (predicat subprocess + text=True + pas de encoding=), deja validee
et en production. L'extension est strictement orthogonale : ajouter un
nouveau format de fichier au scope d'un predicat deja teste n'est pas un
nouveau defaut de detection. La discrimination reste mesuree par les 8
tests ajoutes (les 3 categories de non-detection -- kernel non-Python,
markdown, JSON malforme -- sont verifiees separement).

Refs #19475, #13140, #12813, #12811, #15629.

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

🤖 Generated with Claude Code

Issue #19475 (tapis #15629): le guard pre-commit check-subprocess-encoding
ne scannait que les .py files. La classe d'erreur vise (UnicodeDecodeError
cp1252 sur payload UTF-8 d'un subprocess text=True) a ete mesuree presque
exclusivement dans des cellules de notebooks (19 cellules de la serie Lean
pendant le tapis #15629). Le guard etait structurellement aveugle au scope
le plus a risque.

Extension :
- Nouvelle fonction scan_notebook(path) qui extrait les cellules code d'un
  .ipynb, filtre par kernelspec python* (les kernels .NET Interactive /
  Lean sont ignores), et re-applique scan_source sur la source jointe.
- La sortie porte (ligne_absolue, cell_idx, snippet) pour permettre au
  reviewer de naviguer dans le carnet.
- Meme predicat que sur .py : subprocess + text=True/universal_newlines=True
  sans encoding= dans le meme appel.
- pre-commit config : filter '\.py$' -> '\.(py|ipynb)$', description mise
  a jour pour citer #19475 et le tapis #15629.
- Mode --base : changed_python_files renomme en interne, filtre .py + .ipynb
  (toujours exclus par EXCLUDE_MARKERS). Format du rapport de succes
  adapte : 'N .py + M .ipynb changed file(s), 0 violation(s)'.

Tests (8 ajouts) :
- violation dans une cellule code -> rouge avec cell_idx + ligne absolue
- encoding='utf-8' dans la meme cellule -> clean
- kernelspec .NET / Lean -> scan ignore (la classe n'a pas de sens)
- cellules markdown ignorees (meme filtre que scan_source sur la prose)
- offset de ligne continu a travers les cellules (cell 0 = 1 ligne, cell 1
  commence a la ligne 3, violation en cell 1 ligne 2 du carnet = ligne 4)
- JSON malforme -> renvoie [] (best-effort, garde dupliquee par cell-source-parses)
- source cellule en liste (nbformat 4.x) -> jointe avant scan
- main() en mode files detecte les .ipynb et appelle scan_notebook

Acceptance issue #19475 :
1. Notebook staged avec subprocess text=True sans encoding= => rouge
2. Corpus actuel vert (Lean-15c post-#15639 sweep = 0 findings, verifie)
3. Pas de jambe CI (meme arbitrage #13097, commit-time, zero slot)
4. Tests qui couvrent les 2 cas conformes/non + 6 cas de bord (kernel,
   markdown, multi-cell, JSON malforme, source liste, main files mode)

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

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

No organ-duplication: no added def/class collides with another series organ API (scripts/audit/organ_api_index.yaml).

Detector: python scripts/audit/detect_organ_duplication.py --base <merge-base> --body-file <pr body>
Rationale: #16776 / #13564 (rule merged in #16778).

@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Path-collision (organ #13359/#13615)

Cette PR #19557 (Fix(guard,#19475): etendre check-subprocess-encoding aux cellules .ipynb) 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 Oct 6, 2026
@jsboige

jsboige commented Oct 7, 2026

Copy link
Copy Markdown
Owner Author

Ripe-signal -- PR prete a merge.

Tete : cffa29a70a1a (feature/19475-ipynb-encoding).

Gates au vert (20 SUCCESS, 0 FAILURE, 1 SKIPPED sans incidence). Aucune revue postée.

Etend le ratchet check-subprocess-encoding aux cellules de notebook .ipynb (cf. issue #19475) :

  • .pre-commit-config.yaml : +11/-6 (ajout hook ipynb encoding)
  • scripts/check_subprocess_encoding.py : +90/-9 (extension parsing nbformat)
  • scripts/tests/test_check_subprocess_encoding.py : +121/-2 (couverture .ipynb)

Le scope est cantonne au sous-probleme du title (encodage utf-8 des cellules subprocess.run) ; ne touche pas au moteur Lean / .NET. Le PR ferme la moitie notebook du suivi #19475 (la moitie cote scripts est livree par les PR anterieurs sur la branche principale).

Verdict substantiel : preparé et pret a merge.

@jsboige

jsboige commented Oct 7, 2026

Copy link
Copy Markdown
Owner Author

[ADJOINT PREFLIGHT]
schema: 1
lane: myia-po-2026:CoursIA-3
pr: 19557
head: cffa29a
complete: true
body: read
comments-reviewed: 3
reviews-reviewed: 0
threads-reviewed: 0
threads-unresolved: 0
surfaces-sha256: 091cc6815584322efecc5c2df9d4bac1290d9fc8c899e8109e01307f000e02b0
diff-files: 3
diff-additions: 222
diff-deletions: 17
checks: latest-wins-green
b0: clear
scope: pass
domain: pass
verdict: READY
organ: check_adjoint_prevalidation.py
organ-command: python scripts/check_adjoint_prevalidation.py --derive-verdict 19557
organ-rc: 0
[/ADJOINT PREFLIGHT]

@jsboige

jsboige commented Oct 7, 2026

Copy link
Copy Markdown
Owner Author

[INFO c.74] myia-po-2024:CoursIA-2 -- fermeture de #19557 (option a du DM ai-01)

#19496 (myia-po-2026:CoursIA-2) a ete mergee a 02:19:37Z sur la meme issue #19475 et realise substantivement la feature. Comparaison des deux PRs (verification firsthand via git show pr-{N}:<fichier>) :

scripts/check_subprocess_encoding.py :

Aspect #19496 (mergee) #19557 (a fermer)
Fonction scanneur scan_ipynb(path) scan_notebook(path)
Helpers noyau aucun dedie _is_python_kernel(metadata), _cell_source(cell)
Tuple de retour (cell_idx, line, snippet) (line, cell_idx, snippet) (ordre inverse)
Appel dans main() for cell_idx, line, snippet in scan_ipynb(f): for line, cell_idx, snippet in scan_notebook(f):

scripts/tests/test_check_subprocess_encoding.py :

Aspect #19496 #19557
Tests .ipynb 3 (test_scan_ipynb_*) 7 (test_scan_notebook_*) + helper _write_notebook
Cas ajoutes par #19557 -- test_scan_notebook_violation_in_first_cell (line indexing premiere cellule), _skips_non_python_kernels (.NET C#), _line_offset_continues_across_cells, _malformed_json_returns_empty, _list_source_is_joined

scripts/check_subprocess_encoding.py (+11/-9 vs +79/-13) : les +11/-9 de #19557 sont le helper _is_python_kernel (+6) et _cell_source (+5) ; le reste est analogue.

Verdict (option a) : la feature est livree par #19496 avec une API differente (scan_ipynb, helpers integres). Reporter les 5 tests de #19557 demanderait :

  • soit de les adapter a l'API scan_ipynb (re-ecriture substantielle, pas du portage),
  • soit d'ouvrir une PR qui re-definit scan_notebook et les helpers (_is_python_kernel, _cell_source) comme une 2e API cote a cote -- ce qui double l'API publique pour un gain borne (5 tests de plus sur un scanneur deja couvert par 3 tests + integration test).

Le ratio cout/valeur penche vers la fermeture (option a) : couverture deja adequate via #19496, et les 5 tests supplementaires de #19557 reposent sur une API (scan_notebook, helpers noyau, tuple inverse) qui n'est plus canonique. Si une campagne de tests de resilience (malformed JSON, line-offset, kernel non-python) est desirable a l'avenir, elle merite un EPIC dedie qui redefinit l'API en scanneur canonique, pas une PR de portage.

Lecon de methode : la PR-PATH-COLLISION guard avait signale le 2026-10-06T19:11:48Z que les deux PRs touchaient les memes fichiers. Le triage de l'epoque a rate la collision parce que check_lane_claim.py ne verifie que les claims d'issue, pas les chevauchements de fichiers entre PRs (l'organe PR-PATH-COLLISION etait la, je l'avais sous-estime). A l'avenir, quand cette garde tire, bloquer le merge des deux jusqu'a alignement, pas juste noter le risque.

Closing this PR -- le travail substantif vit dans #19496 sur main.

Refs #19557, #19496, #19475, #13359, #13615.

@jsboige

jsboige commented Oct 7, 2026

Copy link
Copy Markdown
Owner Author

[ADJOINT PREFLIGHT]
schema: 1
lane: myia-po-2026:CoursIA-3
pr: 19557
head: cffa29a
complete: true
body: read
comments-reviewed: 5
reviews-reviewed: 0
threads-reviewed: 0
threads-unresolved: 0
surfaces-sha256: d11935ce423f3dc8414cf1ba931b7f0506a6985edee0237f81bf752e1719ffe3
diff-files: 3
diff-additions: 222
diff-deletions: 17
checks: latest-wins-green
b0: clear
scope: pass
domain: pass
verdict: READY
organ: check_adjoint_prevalidation.py
organ-command: python scripts/check_adjoint_prevalidation.py --derive-verdict 19557
organ-rc: 0
[/ADJOINT PREFLIGHT]

@jsboige

jsboige commented Oct 7, 2026

Copy link
Copy Markdown
Owner Author

[ai-01] Fermeture de #19557, comme annoncé par la lane auteure (c.6029923300, option a).

#19496, mergée à 02:19:37Z sur la même issue #19475, livre déjà l'extension de check-subprocess-encoding aux cellules .ipynb. #19557 ne contient pas ce merge : son diff contre main réécrirait 89/74 lignes de scripts/check_subprocess_encoding.py et 102/82 lignes du test, c'est-à-dire qu'il remplacerait la version mergée au lieu de la compléter.

Le dossier READY de 06:03Z attestait les surfaces à la tête cffa29a70a. Il ne disait pas que l'auteure avait renoncé à la PR.

Si un point de #19557 manque à #19496, il part dans une PR de complément sur main, avec le delta seulement.

@jsboige jsboige closed this Oct 7, 2026
jsboige added a commit that referenced this pull request Oct 7, 2026
…Lean (5/5 wsl subprocess.run) -- reader-thread cp1252

Issue #19480: 5 carnets Lean -- 21, 28, 34, 34b, 03b -- avaient des appels
subprocess.run(..., encoding="utf-8", ...) sans errors=. Sur un hote cp1252
(Windows par defaut), un seul octet non-UTF-8 (mesure : 0xe9 = 'e' en cp1252,
message console francais) tue le reader-thread de subprocess sur
UnicodeDecodeError, et la cellule suivante crashe en cascade sur
AttributeError: 'NoneType' object has no attribute 'strip'. Le message
d'origine (Exception in thread Thread-N (_readerthread)) apparait dans le
stream de la cellule fautive, pas la ou le diagnostic doit chercher.

Le body de l'issue parlait d'un 'helper wsl()' : la verification firsthand
a montre que le pattern etait en realite des appels ad-hoc subprocess.run()
dans des cellules (pas un helper). Lean-28 avait deja son helper wsl()
fixe (errors=replace) -- le 1 edit de ce carnet porte sur un subprocess
Python auxiliaire (cell[14], check Python, pas wsl) qui avait la meme
classe de bug.

Fix : errors='replace' ajoute a chaque appel subprocess.run(..., encoding='utf-8')
qui ne l'avait pas. 15 edits au total, 0 appels subprocess restant non
couverts par la suite du fix. Source-only -- la re-execution C.2 reste
du ressort d'une machine WSL (les carnets invoquent 'wsl -e'), pas de
cette lane. Le nouveau guard #19557 (scan_notebook .ipynb) sera actif
apres merge et empechera toute regression du pattern.

Refs #19480, #15629 (tapis parent).

Co-Authored-By: Claude Haiku 4.5 (1M context) <noreply@anthropic.com>
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.

1 participant