From 92bfb426ca7fe48f671b95811221c3bc08d09441 Mon Sep 17 00:00:00 2001 From: le king fu Date: Thu, 1 Oct 2026 15:56:32 -0400 Subject: [PATCH] test(ci): close the gaps found in the review of PR #42 (#35) Three findings of the review, each first measured green on the previous tree (18/18), then red on this one. The proxy test dropped the inherited no_proxy, so a script exporting NO_PROXY alone stayed green, although curl reads the lowercase variable first and an inherited no_proxy would win. The test now sets no_proxy to a list that leaves the loopback out. Red when the script exports NO_PROXY alone, and on the script from main. The column-0 require pin missed a space before the parenthesis: an indented `require (` and an `import (` both passed. The regexes now accept whitespace there. Red on each. The continue-on-error detector required a colon after the word, so a YAML anchor reused as a key (`&coe continue-on-error`, then `*coe : true`) passed, while PyYAML reads it as the key. The detector is now the bare word, with a self-check row for the anchor; red when the colon is required again, three rows red with the original anchored regex. An escaped key still passes, since the pin reads text and not YAML: the comment and CLAUDE.md now claim any literal spelling, no longer any spelling, and list the indirections the require pin cannot follow. ci.test.js 18 -> 19 cases, vitest 346 -> 347. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 6 ++--- __tests__/ci.test.js | 55 +++++++++++++++++++++++++++++--------------- 2 files changed, 39 insertions(+), 22 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 8771e3a..8050722 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -62,17 +62,17 @@ curl -H "Authorization: Bearer $(cat ~/.coolify-token)" \ ## Tests - **CI** : `.forgejo/workflows/ci.yml` (issue #24), sur toute PR (pas de filtre `branches:`, pour que les maillons d'une pile soient batis) et sur `push` vers `main`. Un seul job en `container: node:22-alpine`, la base du Dockerfile : `npm ci`, `npm test`, `bash test-curl.sh`, puis `sh scripts/boot-from-copy.sh`. Verdict a lire sur le statut de commit (`GET /repos/maximus/vps-health-api/commits//status`), l'API Actions repondant 404 sur cette instance. -- `npm test` (vitest) — 346 cas +- `npm test` (vitest) — 347 cas - `__tests__/findings.test.js` — `/defenseurs/findings` (14 cas : auth, validation, filtres severity/category, asymetrie INFO, scan clean vs no_data, JSON corrompu) - `__tests__/state.test.js` — `/defenseurs/state` + non-regression `/defenseurs` apres extraction de `serveJsonFile()` (9 cas : corps identique octet pour octet sur un JSON non canonique, no_data, isolation des deux chemins, 401 sans token / mauvais token / token d'ingestion, 404 sur POST) - `__tests__/health.test.js` — module `metrics.js` + `/health` (6 cas : exports, payload champ pour champ, garde de latence) - `__tests__/auth.test.js` — filet de securite auth sur les routes de lecture (67 cas : matrice route x mode d'echec, en-tetes malformes (dont le separateur `Bearer` autre qu'un espace), quasi-rates du token sur les 6 routes de lecture, fail-closed, **2 cas `404 (not 401)` qui figent l'ordre routage -> auth**, **un espion sur `timingSafeEqual`** qui exige deux empreintes de 32 octets a chaque requete, et **des epingles sur le source** : aucune egalite contre une chaine `Bearer`, un seul `tokenMatches()` et un seul `return null` dans `checkAuth()`, egalites limitees a une liste blanche, `tokenMatches()` qui hache les deux cotes sans test de longueur) - `__tests__/hosts.test.js` — matrice exhaustive des endpoints postes (171 cas : regex de route comme controle anti-traversee, 401 avant 403, cloisonnement des deux tokens, 503 fail-closed, reconstruction par liste blanche, `loadAvg` a exactement 3 elements, 413, `neverSeen`, fraicheur serveur sous horloge figee, degradation de lecture, **balayage des `.tmp` orphelins** au demarrage, prouve sur le vrai point d'entree) - `__tests__/agent.test.js` — agent poste (50 cas : contrat de payload verifie contre `sanitizeSnapshot()`, une seule tentative sans rejeu, timeout, codes de sortie, wrapper shell, et **le token absent de toute sortie d'echec, fragments de 5 caracteres compris**) - - `__tests__/ci.test.js` — epingles du workflow (18 cas : image du `container:` identique au `FROM` du Dockerfile, les quatre etapes presentes, aucun filtre `branches:` sous `pull_request`, `push` limite a `main`, ni `defaults:` ni `working-directory` sur une etape `uses:`, **aucun `continue-on-error`, quelle qu'en soit l'ecriture** (une etape rouge passerait au vert en silence ; detecteur non ancre, lui-meme verifie sur la cle en bloc, en element de liste, en style flow et entre guillemets ; seules les lignes de commentaire entieres en sont exemptees), ce que le boot check suppose du Dockerfile : `USER node`, `CMD` en forme exec, le **refus d'un port deja occupe** (voisin HTTP reel, script lance en `spawn` asynchrone), **aucune requete par un proxy de l'environnement** (boot complet derriere un faux proxy qui repond a tout, sa vivacite prouvee d'abord par un `curl` nu), et **des `require` en colonne 0 seulement, sans `import()`**, dans chaque `.js` du `COPY`. Cette derniere epingle est une heuristique a deux trous connus, laisses ouverts (#35) : une fonction ecrite entiere sur une ligne en colonne 0 (`const lazy = () => require("x");`), et un `require(` renvoye a la ligne en colonne 0 dans une fonction, de meme forme que le `} = require("node:fs");` legitime d'`index.js`) + - `__tests__/ci.test.js` — epingles du workflow (19 cas : image du `container:` identique au `FROM` du Dockerfile, les quatre etapes presentes, aucun filtre `branches:` sous `pull_request`, `push` limite a `main`, ni `defaults:` ni `working-directory` sur une etape `uses:`, **aucun `continue-on-error`, sous toute ecriture litterale** (une etape rouge passerait au vert en silence ; detecteur sur le mot nu, sans ancrage ni deux-points exiges, lui-meme verifie sur la cle en bloc, en element de liste, en style flow, entre guillemets et via une ancre YAML ; seules les lignes de commentaire entieres en sont exemptees ; une cle ecrite avec une sequence d'echappement, `"continue-on-error"`, passe encore : l'epingle lit du texte, pas du YAML), ce que le boot check suppose du Dockerfile : `USER node`, `CMD` en forme exec, le **refus d'un port deja occupe** (voisin HTTP reel, script lance en `spawn` asynchrone), **aucune requete par un proxy de l'environnement** (boot complet derriere un faux proxy qui repond a tout, sa vivacite prouvee d'abord par un `curl` nu, avec un `no_proxy` qui laisse la boucle locale hors liste : exporter `NO_PROXY` seul ne suffit pas), et **des `require` en colonne 0 seulement, sans `import()`**, dans chaque `.js` du `COPY`. Cette derniere epingle est une heuristique sur les appels litteraux, espace avant la parenthese comprise (`require (`, `import (`). Trous connus, laisses ouverts (#35) : une fonction ecrite entiere sur une ligne en colonne 0 (`const lazy = () => require("x");`), un `require(` renvoye a la ligne en colonne 0 dans une fonction (de meme forme que le `} = require("node:fs");` legitime d'`index.js`), et toute indirection, comme un alias de `require`) - `__tests__/writes.test.js` — seule surface d'ecriture : `HOSTS_DIR` (11 cas : espions sur toutes les fonctions d'ecriture de `node:fs`, formes callback, Sync et `fs.promises`, et sur `child_process`, poses avant un `require` frais apres purge du cache de **tous** les modules runtime (un `metrics.js` reste en cache garderait les espions restaures d'un test precedent) ; demarrage par le balayage des `.tmp` ; les six routes GET n'ecrivent rien, meme une macrotache apres la reponse, contenu reel exige ; `POST /hosts/` n'ecrit que sous `HOSTS_DIR` ; **chemins degrades** : rejets 401/403/404, entrees 400/413, `agents-map.json` corrompu, fichiers Defenseurs absents, snapshot corrompu, avec le contenu degrade exige (`no_data`, `neverSeen`) ; **commandes shell** figees sur `head -1 /proc/stat` et `df -k /`, `/health` servant de temoin ; chaque cas prouve que ses espions sont vivants ; **epingle sur le bloc `require.main`**, ensemble exact des appels, puisqu'il ne tourne jamais dans le processus de test. Limites : un chemin qu'aucun cas n'emprunte, et une ecriture differee au-dela de 20 ms) - Runtime reste 0-dep ; vitest en devDep uniquement -- `sh scripts/boot-from-copy.sh` — demarre le `CMD` depuis les seules sources du `COPY`, copiees dans un dossier vide, et exige 401 sans token, 200 avec, et des metriques disque/memoire non nulles. Tient lieu de `docker build` en CI : un vrai build demanderait le socket Docker dans le runner d'instance, donc root sur le VPS de prod pour tout workflow de tout depot. Il n'accepte qu'une ligne `COPY` en forme shell sans option, et un `CMD` en forme exec ; toute autre forme echoue bruyamment plutot que de verifier du vide. Port 3097 par defaut (`BOOT_PORT` pour en changer) ; il refuse de demarrer si le port est deja pris (tout code de sortie curl autre que 7, « connexion refusee »), sans quoi ses assertions pourraient interroger un voisin. Il exporte `NO_PROXY='*'` et `no_proxy='*'` en tete : chaque `curl` vise la boucle locale, et un `http_proxy` de l'environnement repondrait a la place du port comme du serveur demarre. Il n'appelle que `/health` : il ne prouve que les modules charges au demarrage, d'ou l'epingle « `require` en colonne 0 » dans `ci.test.js`. +- `sh scripts/boot-from-copy.sh` — demarre le `CMD` depuis les seules sources du `COPY`, copiees dans un dossier vide, et exige 401 sans token, 200 avec, et des metriques disque/memoire non nulles. Tient lieu de `docker build` en CI : un vrai build demanderait le socket Docker dans le runner d'instance, donc root sur le VPS de prod pour tout workflow de tout depot. Il n'accepte qu'une ligne `COPY` en forme shell sans option, et un `CMD` en forme exec ; toute autre forme echoue bruyamment plutot que de verifier du vide. Port 3097 par defaut (`BOOT_PORT` pour en changer) ; il refuse de demarrer si le port est deja pris (tout code de sortie curl autre que 7, « connexion refusee »), sans quoi ses assertions pourraient interroger un voisin. Il exporte `NO_PROXY='*'` et `no_proxy='*'` en tete : chaque `curl` vise la boucle locale, et un `http_proxy` de l'environnement repondrait a la place du port comme du serveur demarre. Les deux graphies, car curl lit d'abord la minuscule : un `no_proxy` herite l'emporterait sur un `NO_PROXY` exporte seul. Il n'appelle que `/health` : il ne prouve que les modules charges au demarrage, d'ou l'epingle « `require` en colonne 0 » dans `ci.test.js`. - `bash test-curl.sh` — passe au niveau socket, complementaire et non redondante (45 cas). Vitest couvre la logique ; ce script couvre ce que seuls un vrai client et un vrai TCP peuvent montrer : le 413 rendu **avant** que la connexion soit coupee, les deux tokens qui se refusent mutuellement leurs routes, le fichier servi octet pour octet par `/defenseurs/state`, et le 503 d'une instance demarree sans `HOSTS_INGEST_TOKEN` (elle tourne sur un second port, le temps du script). Il boote ses propres serveurs sur 3099 et 3098 — aucun service exterieur requis. ## Gotchas diff --git a/__tests__/ci.test.js b/__tests__/ci.test.js index ed68e56..572a924 100644 --- a/__tests__/ci.test.js +++ b/__tests__/ci.test.js @@ -82,24 +82,30 @@ describe("CI workflow", () => { }); // `continue-on-error: true` turns a red step green without a trace in the - // commit status. Any spelling of the key counts: a block key or a list item, - // but also a flow mapping and a quoted key, which a pin anchored at the start - // of the line let through (#35). Unanchored on purpose: the key quoted in a - // `run:` script, a step name or a trailing comment fails the pin too, loudly. - // Only full comment lines are exempt, being stripped from `ciCode`. - const CONTINUE_ON_ERROR = /continue-on-error["']?\s*:/; + // commit status. Any literal spelling of the key counts: a block key or a + // list item, but also a flow mapping and a quoted key, which a pin anchored + // at the start of the line let through (#35), and a YAML anchor reused as a + // key, which only a match on the bare word catches. Unanchored and bare on + // purpose: the word in a `run:` script, a step name or a trailing comment + // fails the pin too, loudly. Only full comment lines are exempt, being + // stripped from `ciCode`. An escaped key (`"continue-on-error": true`) + // still passes: the pin reads text, not YAML. + const CONTINUE_ON_ERROR = /continue-on-error/; test("never lets a failing step pass", () => { expect(ciCode).not.toMatch(CONTINUE_ON_ERROR); }); - // The detector itself, through the same comment stripping as `ciCode`. Anchor - // it at the start of the line again and the last two spellings turn red. + // The detector itself, through the same comment stripping as `ciCode`. + // Anchored at the start of the line again, it would miss the flow mapping, + // the quoted key and the anchor; with a colon required after the word, the + // anchor. test.each([ ["a block key", " - name: x\n run: npm test\n continue-on-error: true"], ["a list item", " - continue-on-error: true\n run: npm test"], ["a flow mapping", " - { name: x, run: false, continue-on-error: true }"], ["a quoted key", ' - name: x\n "continue-on-error": true'], + ["an anchor reused as a key", " - name: x\n env: { STEP_KEY: &coe continue-on-error }\n *coe : true"], ])("the continue-on-error pin catches %s", (_, step) => { expect(stripCommentLines(step)).toMatch(CONTINUE_ON_ERROR); }); @@ -181,8 +187,16 @@ describe("boot-from-copy.sh", () => { try { const proxyUrl = `http://127.0.0.1:${proxy.address().port}`; // ALL_PROXY as well, so that unsetting http_proxy alone would not pass. - // An inherited NO_PROXY would hide the defect, hence dropped. - const env = { http_proxy: proxyUrl, ALL_PROXY: proxyUrl, NO_PROXY: undefined, no_proxy: undefined }; + // no_proxy leaves the loopback out, as an inherited one may: curl reads + // it before NO_PROXY, so a script exporting NO_PROXY alone would still go + // through the proxy. NO_PROXY is dropped, so that nothing the test itself + // inherited decides the outcome. + const env = { + http_proxy: proxyUrl, + ALL_PROXY: proxyUrl, + NO_PROXY: undefined, + no_proxy: "example.invalid", + }; const port = await freePort(); // The proxy is live: a bare curl with the same environment reaches it. @@ -208,13 +222,16 @@ describe("boot-from-copy.sh", () => { // is no dynamic import. Comment lines are skipped, so an explanation quoting // require( cannot break the pin. // -// Known holes, left open (#35). A function written whole on a column-0 line -// passes: `const lazy = () => require("x");`, or the same as a one-line -// `function lazy() { return require("x"); }`. So does a require( wrapped onto a -// column-0 line inside a function. Neither fits the style of this repository, -// and the second has the very shape of the top-level `} = require("node:fs");` -// that closes index.js's multi-line import: telling them apart takes a parser, -// not a line regex. +// The pin reads literal calls only, a space before the parenthesis included +// (`require (`, `import (`). Known holes, left open (#35): +// - a function written whole on a column-0 line, such as +// `const lazy = () => require("x");` or a one-line +// `function lazy() { return require("x"); }`; +// - a require( wrapped onto a column-0 line inside a function, which has the +// very shape of the top-level `} = require("node:fs");` that closes +// index.js's multi-line import: telling them apart takes a parser; +// - any indirection, such as an alias of require. +// None of them fits the style of this repository. describe("runtime modules are all loaded at startup", () => { const copyLine = dockerfile.match(/^\s*COPY\s+(.+)$/m); const sources = copyLine ? copyLine[1].trim().split(/\s+/).slice(0, -1) : []; @@ -229,8 +246,8 @@ describe("runtime modules are all loaded at startup", () => { const offenders = []; lines.forEach((line, i) => { if (/^\s*(\/\/|\/?\*)/.test(line)) return; - if (/\brequire\(/.test(line) && /^\s/.test(line)) offenders.push(`${file}:${i + 1}: ${line.trim()}`); - if (/\bimport\(/.test(line)) offenders.push(`${file}:${i + 1}: ${line.trim()}`); + if (/\brequire\s*\(/.test(line) && /^\s/.test(line)) offenders.push(`${file}:${i + 1}: ${line.trim()}`); + if (/\bimport\s*\(/.test(line)) offenders.push(`${file}:${i + 1}: ${line.trim()}`); }); expect(offenders).toEqual([]);