Espions d’écriture aveugles aux fonctions passées en callback (revue PR #39) #40

New issue

Closed

opened 2026-10-01 00:21:45 +00:00 by maximus · 0 comments

maximus commented

2026-10-01 00:21:45 +00:00

Owner

Constat non bloquant de la seconde revue de la PR #39 (commentaire 17331), à ne pas perdre avec la PR fermée. Analyse du 2026-10-01 : prémisse rejouée et confirmée, variante arrêtée.

Contexte

__tests__/writes.test.js n'attribue une écriture à l'API que si une frame d'un fichier runtime se trouve sur la pile synchrone au moment de l'appel (fromRuntimeFile(captureStack())). Une fonction node:fs passée telle quelle à un mécanisme asynchrone s'exécute sans cette frame, et l'écriture passe sur un chemin pourtant exercé (GET /defenseurs nominal) :

	Mutation dans `handleDefenseurs`	Rouges aujourd'hui	Fichier écrit
G0 (témoin)	`process.nextTick(() => writeFileSync(tmp, "x"))`	2	oui
G1	`process.nextTick(writeFileSync, tmp, "x")`	0	oui
G2	`res.on("finish", writeFileSync.bind(null, tmp, "x"))`	0	oui
G3	`new fs.WriteStream(tmp).end("x")`	0	oui
G4	`Promise.resolve(tmp).then(fs.promises.rm)`	0	sans objet

Ces chiffres ont été rejoués le 2026-10-01 sur une copie jetable de main (198e1cf, Node 22.22.0) : G0 à 2 rouges, G1 à G4 à 0.

Ces formes sont rares dans ce dépôt. Mais CLAUDE.md (gotcha « L'API n'est plus read-only » et parenthèse « Limites » de la ligne de writes.test.js) et README.md (section Bind-mounts, vers la ligne 161) affirment que toute écriture par node:fs vise HOSTS_DIR sur les chemins exercés : G1 à G3 le contredisent.

Décision : originaux capturés, sans drapeau `paused`

Les espions comptent tout appel, sans attribution par pile. Les retouches de fixtures du cas dégradé passent par les vraies fonctions node:fs, capturées au chargement du fichier de test, avant la pose des espions (const real = { writeFileSync: fs.writeFileSync, ... }).

Pourquoi pas le drapeau paused du commentaire 17331 : il fonctionne (rejoué, G1 à G4 à 2 rouges, 15/15 passes), mais il rend les espions aveugles pendant la fenêtre des retouches, qui suit immédiatement les 13 requêtes rejetées. Une écriture différée par l'API qui tomberait dans cette fenêtre ne serait pas comptée. Avec les originaux capturés, il n'y a aucune fenêtre.

Piège vérifié : fs.rmSync(dir, { recursive: true }) appelle en interne la fonction publique fs.rmdirSync, donc l'espion, même quand rmSync est l'original capturé. La variante rougit alors sans mutation (rmdirSync(object), rmdirSync(<tmp>/defenseurs/reports)). La suppression récursive de REPORTS_DIR est donc à remplacer par un real.renameSync(REPORTS_DIR, <tmpDir>/reports-moved-away) : un seul appel, le dossier disparaît de son chemin, et afterEach nettoie tmpDir.

Variante ainsi écrite, rejouée sur la copie : 11/11 vert sans mutation, G0 à G4 à 2 rouges chacun, 15/15 passes de writes.test.js, suite complète à 355/355.

Travail à faire

Capturer writeFileSync, rmSync et renameSync réels en tête de writes.test.js, avant tout espion.
Retirer fromRuntimeFile(captureStack()) des espions node:fs et child_process : tout appel est compté.
Faire passer les retouches du cas dégradé par les originaux, et remplacer le rmSync récursif de REPORTS_DIR par un renommage.
Supprimer captureStack() et fromRuntimeFile(), devenus morts. isRuntimeFile() reste, pour la purge du cache. Corriger son commentaire (« Used both to attribute a call... »).
Réécrire le paragraphe d'en-tête sur l'attribution, et noter le piège du rmSync récursif à côté de la capture des originaux.
Mettre à jour CLAUDE.md (ligne de writes.test.js et gotcha « L'API n'est plus read-only ») et vérifier que la phrase de README.md reste exacte.
Rejouer les mutations (voir critères).

Fichiers concernés

__tests__/writes.test.js : espions (l. 127 à 157), helpers de pile (l. 39 à 64), retouches du cas dégradé (l. 391 à 401), en-tête (l. 8 à 32).
CLAUDE.md : ligne writes.test.js de la section Tests, gotcha « L'API n'est plus read-only ».
README.md : section Bind-mounts, relecture seulement.
index.js et metrics.js : aucun changement, cibles des mutations uniquement.

Surface de test

Test existant : __tests__/writes.test.js (11 cas), le seul touché. Le nombre de cas ne change pas (355 au total).
Aucun nouveau cas : la preuve est par mutation, non commitée.
Coût assumé : si une version future de vitest ou de Node écrit par node:fs dans le worker pendant un test, le test rougit au lieu de passer en silence. C'est le bon sens d'échec.

Critères d'acceptation

Les espions node:fs et child_process comptent tout appel, sans condition de pile et sans drapeau. Les seules écritures qui leur échappent sont les retouches de fixtures, faites par des originaux capturés avant la pose.
Rouge prouvé par mutation sur G1, G2, G3 et G4 (0 avant, au moins 1 après). G0 et les mutations des revues de la PR #39 (R1 à R11 du commentaire 17233, V1 à V7, C1 et C3 du commentaire 17331) restent rouges, à l'exception des limites déjà écrites (R8b, D1). Suite verte après restauration.
writes.test.js stable : 15 passes de suite en boucle, CI verte.
En-tête du test et parenthèse « Limites » de CLAUDE.md réécrits : l'attribution par pile disparaît ; restent les limites « chemin qu'aucun cas n'emprunte » et « écriture différée au-delà de 20 ms ».
captureStack() et fromRuntimeFile() absents du fichier.

Complexité estimée

Simple. Un fichier de test, une quinzaine de lignes, plus la documentation. L'essentiel du temps va au rejeu des mutations R, V et C de la PR #39.

Variante écartée : drapeau paused (diff du commentaire 17331)

@@ -114,2 +114,3 @@
 let log = [];
+let paused = false; // set while the test edits its own fixtures
 let commandLog = [];
@@ -137,3 +138,3 @@
       vi.spyOn(owner, key).mockImplementation(function (...args) {
-        if (fromRuntimeFile(captureStack())) {
+        if (!paused) {
           const targets = targetsOf(args);
@@ -152,3 +153,3 @@
     vi.spyOn(child_process, key).mockImplementation(function (...args) {
-      if (fromRuntimeFile(captureStack())) commandLog.push(commandOf(args));
+      if (!paused) commandLog.push(commandOf(args));
       return original.apply(this, args);
@@ -391,3 +392,5 @@
     // A corrupt agents-map.json: where a quarantine rename would go.
+    paused = true;
     fs.writeFileSync(process.env.DEFENSEURS_AGENTS_MAP_PATH, "{corrupt");
+    paused = false;
     expect((await send("/defenseurs/findings?project=proj")).status).toBe(500);
@@ -396,2 +399,3 @@
     // write would go. Each route must degrade, with the content that says so.
+    paused = true;
     fs.writeFileSync(process.env.DEFENSEURS_AGENTS_MAP_PATH, JSON.stringify({ proj: "defenseur-proj" }));
@@ -401,2 +405,3 @@
     fs.writeFileSync(path.join(process.env.HOSTS_DIR, "thinkpad.json"), "{corrupt");
+    paused = false;
     const degraded = [

Constat non bloquant de la seconde revue de la PR #39 (commentaire 17331), à ne pas perdre avec la PR fermée. Analyse du 2026-10-01 : prémisse rejouée et confirmée, variante arrêtée. ## Contexte `__tests__/writes.test.js` n'attribue une écriture à l'API que si une frame d'un fichier runtime se trouve sur la pile synchrone au moment de l'appel (`fromRuntimeFile(captureStack())`). Une fonction `node:fs` passée telle quelle à un mécanisme asynchrone s'exécute sans cette frame, et l'écriture passe sur un chemin pourtant exercé (`GET /defenseurs` nominal) : | | Mutation dans `handleDefenseurs` | Rouges aujourd'hui | Fichier écrit | |---|---|---|---| | G0 (témoin) | `process.nextTick(() => writeFileSync(tmp, "x"))` | 2 | oui | | G1 | `process.nextTick(writeFileSync, tmp, "x")` | 0 | oui | | G2 | `res.on("finish", writeFileSync.bind(null, tmp, "x"))` | 0 | oui | | G3 | `new fs.WriteStream(tmp).end("x")` | 0 | oui | | G4 | `Promise.resolve(tmp).then(fs.promises.rm)` | 0 | sans objet | Ces chiffres ont été rejoués le 2026-10-01 sur une copie jetable de `main` (198e1cf, Node 22.22.0) : G0 à 2 rouges, G1 à G4 à 0. Ces formes sont rares dans ce dépôt. Mais `CLAUDE.md` (gotcha « L'API n'est plus read-only » et parenthèse « Limites » de la ligne de `writes.test.js`) et `README.md` (section Bind-mounts, vers la ligne 161) affirment que toute écriture par `node:fs` vise `HOSTS_DIR` sur les chemins exercés : G1 à G3 le contredisent. ## Décision : originaux capturés, sans drapeau `paused` Les espions comptent **tout** appel, sans attribution par pile. Les retouches de fixtures du cas dégradé passent par les vraies fonctions `node:fs`, capturées au chargement du fichier de test, avant la pose des espions (`const real = { writeFileSync: fs.writeFileSync, ... }`). Pourquoi pas le drapeau `paused` du commentaire 17331 : il fonctionne (rejoué, G1 à G4 à 2 rouges, 15/15 passes), mais il rend les espions aveugles pendant la fenêtre des retouches, qui suit immédiatement les 13 requêtes rejetées. Une écriture différée par l'API qui tomberait dans cette fenêtre ne serait pas comptée. Avec les originaux capturés, il n'y a aucune fenêtre. Piège vérifié : `fs.rmSync(dir, { recursive: true })` appelle en interne la fonction publique `fs.rmdirSync`, donc l'espion, même quand `rmSync` est l'original capturé. La variante rougit alors sans mutation (`rmdirSync(object)`, `rmdirSync(<tmp>/defenseurs/reports)`). La suppression récursive de `REPORTS_DIR` est donc à remplacer par un `real.renameSync(REPORTS_DIR, <tmpDir>/reports-moved-away)` : un seul appel, le dossier disparaît de son chemin, et `afterEach` nettoie `tmpDir`. Variante ainsi écrite, rejouée sur la copie : 11/11 vert sans mutation, G0 à G4 à 2 rouges chacun, 15/15 passes de `writes.test.js`, suite complète à 355/355. ## Travail à faire - [ ] Capturer `writeFileSync`, `rmSync` et `renameSync` réels en tête de `writes.test.js`, avant tout espion. - [ ] Retirer `fromRuntimeFile(captureStack())` des espions `node:fs` et `child_process` : tout appel est compté. - [ ] Faire passer les retouches du cas dégradé par les originaux, et remplacer le `rmSync` récursif de `REPORTS_DIR` par un renommage. - [ ] Supprimer `captureStack()` et `fromRuntimeFile()`, devenus morts. `isRuntimeFile()` reste, pour la purge du cache. Corriger son commentaire (« Used both to attribute a call... »). - [ ] Réécrire le paragraphe d'en-tête sur l'attribution, et noter le piège du `rmSync` récursif à côté de la capture des originaux. - [ ] Mettre à jour `CLAUDE.md` (ligne de `writes.test.js` et gotcha « L'API n'est plus read-only ») et vérifier que la phrase de `README.md` reste exacte. - [ ] Rejouer les mutations (voir critères). ## Fichiers concernés - `__tests__/writes.test.js` : espions (l. 127 à 157), helpers de pile (l. 39 à 64), retouches du cas dégradé (l. 391 à 401), en-tête (l. 8 à 32). - `CLAUDE.md` : ligne `writes.test.js` de la section Tests, gotcha « L'API n'est plus read-only ». - `README.md` : section Bind-mounts, relecture seulement. - `index.js` et `metrics.js` : aucun changement, cibles des mutations uniquement. ## Surface de test - Test existant : `__tests__/writes.test.js` (11 cas), le seul touché. Le nombre de cas ne change pas (355 au total). - Aucun nouveau cas : la preuve est par mutation, non commitée. - Coût assumé : si une version future de vitest ou de Node écrit par `node:fs` dans le worker pendant un test, le test rougit au lieu de passer en silence. C'est le bon sens d'échec. ## Critères d'acceptation - [ ] Les espions `node:fs` et `child_process` comptent tout appel, sans condition de pile et sans drapeau. Les seules écritures qui leur échappent sont les retouches de fixtures, faites par des originaux capturés avant la pose. - [ ] Rouge prouvé par mutation sur G1, G2, G3 et G4 (0 avant, au moins 1 après). G0 et les mutations des revues de la PR #39 (R1 à R11 du commentaire 17233, V1 à V7, C1 et C3 du commentaire 17331) restent rouges, à l'exception des limites déjà écrites (R8b, D1). Suite verte après restauration. - [ ] `writes.test.js` stable : 15 passes de suite en boucle, CI verte. - [ ] En-tête du test et parenthèse « Limites » de `CLAUDE.md` réécrits : l'attribution par pile disparaît ; restent les limites « chemin qu'aucun cas n'emprunte » et « écriture différée au-delà de 20 ms ». - [ ] `captureStack()` et `fromRuntimeFile()` absents du fichier. ## Complexité estimée Simple. Un fichier de test, une quinzaine de lignes, plus la documentation. L'essentiel du temps va au rejeu des mutations R, V et C de la PR #39. <details><summary>Variante écartée : drapeau paused (diff du commentaire 17331)</summary> ```diff @@ -114,2 +114,3 @@ let log = []; +let paused = false; // set while the test edits its own fixtures let commandLog = []; @@ -137,3 +138,3 @@ vi.spyOn(owner, key).mockImplementation(function (...args) { - if (fromRuntimeFile(captureStack())) { + if (!paused) { const targets = targetsOf(args); @@ -152,3 +153,3 @@ vi.spyOn(child_process, key).mockImplementation(function (...args) { - if (fromRuntimeFile(captureStack())) commandLog.push(commandOf(args)); + if (!paused) commandLog.push(commandOf(args)); return original.apply(this, args); @@ -391,3 +392,5 @@ // A corrupt agents-map.json: where a quarantine rename would go. + paused = true; fs.writeFileSync(process.env.DEFENSEURS_AGENTS_MAP_PATH, "{corrupt"); + paused = false; expect((await send("/defenseurs/findings?project=proj")).status).toBe(500); @@ -396,2 +399,3 @@ // write would go. Each route must degrade, with the content that says so. + paused = true; fs.writeFileSync(process.env.DEFENSEURS_AGENTS_MAP_PATH, JSON.stringify({ proj: "defenseur-proj" })); @@ -401,2 +405,3 @@ fs.writeFileSync(path.join(process.env.HOSTS_DIR, "thinkpad.json"), "{corrupt"); + paused = false; const degraded = [ ``` </details>