diff --git a/.changeset/patient-pumas-frolic.md b/.changeset/patient-pumas-frolic.md new file mode 100644 index 000000000..8117c1da2 --- /dev/null +++ b/.changeset/patient-pumas-frolic.md @@ -0,0 +1,5 @@ +--- +type: Added +pr: 4246 +--- +**Two new ESLint rules catch the #4220 Windows CI hang bug class at author time.** `local/require-full-tmpdir-triad` flags a `TMPDIR` environment override (direct or in a child-process `env:` literal) missing `TEMP`/`TMP` — Node never reads `TMPDIR` on Windows. `local/no-unbounded-dirname-walk` flags a `dirname()` ancestor-walk loop with no fixed-point termination guard, which spins forever at a Windows drive root. (#4244) diff --git a/docs/adr/1703-portability-enforcement-architecture.md b/docs/adr/1703-portability-enforcement-architecture.md index 5c80eca5d..1e51fd80d 100644 --- a/docs/adr/1703-portability-enforcement-architecture.md +++ b/docs/adr/1703-portability-enforcement-architecture.md @@ -78,6 +78,8 @@ Replace all three with a single coherent mechanism: **AST-based ESLint rules in | `require-fs-op-fallback` | `DEFECT.WINDOWS-FS-OPS` | `src/**/*.cts`, build/install | | `no-private-binary-resolution` | `DEFECT.WINDOWS-PRIVATE-BINARY-RESOLUTION` | `src/**/*.cts`, `gsd-core/bin/**`, `scripts/**`, `hooks/**` | | `no-exact-case-env-access` | `DEFECT.WINDOWS-EXACT-CASE-ENV-ACCESS` | `src/**/*.cts`, `gsd-core/bin/**`, `scripts/**`, `hooks/**` | +| `require-full-tmpdir-triad` | `DEFECT.WINDOWS-TEST-PORTABILITY` (#4220) | tests | +| `no-unbounded-dirname-walk` | `DEFECT.WINDOWS-TEST-PORTABILITY` (#4020 / #4220) | tests, scripts | **Amendment (2026-08-18, epic #3411 Phase 3 / #3619).** `no-private-binary-resolution` is the first catalog entry added after the original seven, and it extends this architecture to a @@ -136,6 +138,54 @@ One real pre-existing violation of the tightened rule was found and fixed in the `src/shell-command-projection.cts` specifically so this rule's remediation message ("route through `envGet`") names a real, callable helper. +**Amendment (2026-09-03, #4244).** Two rules add author-time coverage for the bug class behind +two real, hard-evidence Windows CI incidents this week: #4020 (`scripts/run-tests.cjs`'s +`sweepProtectSet` ancestor walk hung every scoped Windows CI lane) and its follow-on #4220 (the +regression test written for #4020's own fix masked a second bug — see below). Per Node's own docs, +`os.tmpdir()` on Windows reads only `TEMP` then `TMP`; `TMPDIR` is never consulted there at all +(on every other platform, `TMPDIR` is checked first). Per empirical verification this session, +`path.dirname()` is a fixed point at the platform root on both OSes, but the fixed-point VALUE +differs: `path.posix.dirname('/') === '/'` (length 1) vs. `path.win32.dirname('C:\\') === 'C:\\'` +(length 3) — so a root check written as a POSIX-shaped length heuristic (`cur.length > 1`) never +fires on Windows. + +- `require-full-tmpdir-triad` flags a `TMPDIR` environment override — `process.env.TMPDIR = …`, + or a `TMPDIR` property in an object literal passed as a spawn-like call's `env:` option — that + is not accompanied by `TEMP` and `TMP` in the same scope. Anti-pattern: `runNode(['-e', probe], + { env: { ...process.env, TMPDIR: outer } })` — on Windows the child inherits the parent's + ambient `TEMP`/`TMP` and its `os.tmpdir()` silently resolves to the wrong place. Fix: set all + three to the same value. This is the exact shape #4220 found already shipped in + `tests/run-tests-temp-root.test.cjs`'s own #4020 regression test, masked because Windows died in + the unrelated dirname-walk hang before ever reaching it. The same #4244 sweep additionally found + and fixed one more live instance in `tests/config-schema.property.test.cjs`'s + `config-set accepts code_quality.fallow keys` test (direct `process.env.TMPDIR = writableTmp` + assignment with no TEMP/TMP counterpart). +- `no-unbounded-dirname-walk` flags a `while`/`do-while` loop that reassigns its condition + variable from `dirname()` (bare, `path.`, `.posix.`/`.win32.`) without a fixed-point termination + guard (`dirname(cur) !== cur`, or `path.parse(cur).root`) in the loop condition. Anti-pattern: + `while (cur && cur !== root && cur.length > 1) cur = dirname(cur);` — on a Windows runner where + `cur` can never equal `root` (e.g. repo on `D:\`, temp root on `C:\`), the walk reaches the + drive root and spins there at 100% CPU forever, since `cur.length` stays 3 (`> 1`) at the fixed + point. Fix: add the `dirname(cur) !== cur` conjunct. The same #4244 sweep found this exact, + still-unfixed shape live in `scripts/run-tests.cjs`'s `sweepProtectSet` block (the original + #4020 site) and fixed it in the same change by extracting a pure `computeSweepProtectSet` + helper with the fixed-point check, mirroring the shape of the (at-authoring-time separately + in-flight, not yet merged) #4220 fix. + +Both rules join the catalog's **zero-escape-hatch** discipline (rule 3 above): neither carries a +bespoke `// allow-*` comment marker, and both are added to `tests/portability-rule-disable-ban.test.cjs`'s +`PROTECTED_RULES` list so an `eslint-disable` naming them is independently banned outside ESLint +too. `no-unbounded-dirname-walk` is registered on **both** `tests/**/*.cjs` and `scripts/**/*.cjs` +(the narrower `scripts/**/*.cjs`-only block, alongside `no-private-binary-resolution`) — the +production surface registration is load-bearing, since the real #4020 bug lived in `scripts/`, not +`tests/`. `require-full-tmpdir-triad` follows the established test-portability convention +(`no-hardcoded-tmp`, `require-userprofile-with-home`) and is registered on `tests/**/*.cjs` only, +matching both real incident sites. + +A repo-wide sweep for other instances of either pattern (beyond the incident sites above) found +none: `require-full-tmpdir-triad` and `no-unbounded-dirname-walk` both ran clean against the rest +of the tree once the three live sites were fixed. + **Taxonomy coverage.** This catalog addresses every `DEFECT.WINDOWS-*` class plus `DEFECT.TEST-SHELL-PIPELINE-NONPORTABLE` in `CONTEXT.md`, to the extent each is *statically* detectable. `DEFECT.TEST-SHELL-PIPELINE-NONPORTABLE` has two parts: (a) the CRLF / literal-`\n` diff --git a/docs/contributing/adding-a-portability-rule.md b/docs/contributing/adding-a-portability-rule.md index 008e52204..239587e3a 100644 --- a/docs/contributing/adding-a-portability-rule.md +++ b/docs/contributing/adding-a-portability-rule.md @@ -136,6 +136,8 @@ Run the full engineering directive (rubber-duck → software laws → architectu | `require-userprofile-with-home` | `WINDOWS-TEST-PORTABILITY` (G6) | tests | 4 | | `normalize-path-in-content` | `WINDOWS-PATH-LEAK-IN-MARKDOWN-CONTENT` | `src/**/*.cts` | 5 | | `require-fs-op-fallback` | `WINDOWS-FS-OPS` | `src/**/*.cts`, `bin/install.js`, `scripts/build-hooks.js` | 6 | +| `require-full-tmpdir-triad` | `WINDOWS-TEST-PORTABILITY` (#4220) | tests | #4244 | +| `no-unbounded-dirname-walk` | `WINDOWS-TEST-PORTABILITY` (#4020/#4220) | tests, `scripts/**/*.cjs` | #4244 | `DEFECT.WINDOWS-ARGV-OVERFLOW` is deliberately **not** in this catalog: argv length is a runtime property (the args-array size is not statically knowable), so no AST rule can soundly detect it. diff --git a/docs/contributing/cross-platform-portability-rules.md b/docs/contributing/cross-platform-portability-rules.md index 2d53bb281..16cd0d406 100644 --- a/docs/contributing/cross-platform-portability-rules.md +++ b/docs/contributing/cross-platform-portability-rules.md @@ -29,6 +29,8 @@ running outside ESLint, fails the build if you try). Legitimately platform-speci | `local/require-userprofile-with-home` | A `process.env.HOME = ` assignment in a test file with no corresponding `process.env.USERPROFILE` **assignment** — Windows uses `USERPROFILE` as the home directory environment variable, not `HOME`. | `tests/**/*.test.cjs` | | `local/normalize-path-in-content` | A path-returning fn result (excluding `path.basename`, which returns a separator-less filename) interpolated **directly** into content without `.replace(/\\/g,'/')` normalization — backslash paths leak into generated content on Windows (`RULESET.CONTENT-PATH-NORMALIZATION`). Two content shapes are detected: (a) the template/string contains an `@`-reference marker (`@~/`, `@$`, `@/`), `$HOME`, or `~/`; (b) the quasi immediately following the interpolation starts with `/…\.md` or `/…\.json`. **Indirect data-flow** (path stored in a variable/field then interpolated) is not detected — normalize at source. Fix: `String(resolvedTarget).replace(/\\/g, '/')`. | `src/**/*.cts` | | `local/require-fs-op-fallback` | An unguarded `fs.rename` / `fs.renameSync` (the atomic-publish primitive) that is NOT inside a `try`/`catch` whose handler references a transient errno (`'EPERM'`/`'EBUSY'`/`'EACCES'`, or a `*RETRY_ERRNOS` set) AND is NOT behind a Windows platform guard — on Windows a concurrent reader / antivirus scanner can transiently hold the target open and throw. A `catch (e) {}` that silently swallows, or a catch that cleans-up-and-rethrows without an errno check, does **not** satisfy the rule. `fs.copyFile` / `fs.unlink` are deliberately **not** flagged (they are the *fallback primitives* named by the defect's own fix-forward, and `unlink` has many intentional best-effort cleanup sites). | `src/**/*.cts`, `bin/install.js`, `scripts/build-hooks.js` | +| `local/require-full-tmpdir-triad` | A `process.env.TMPDIR = …` assignment (direct, or as a property in an object literal passed as the `env:` option to `spawn`/`spawnSync`/`exec`/`execSync`/`execFile`/`execFileSync`/`fork`, or this repo's `runNode(...)` test helper) that is not accompanied by `TEMP` and `TMP` in the same scope — `os.tmpdir()` never reads `TMPDIR` on Windows (only `TEMP`, then `TMP`), so a TMPDIR-only redirect silently no-ops there. | `tests/**/*.test.cjs` | +| `local/no-unbounded-dirname-walk` | A `while`/`do-while` loop that reassigns its condition variable from `dirname(...)` (bare, `path.`, `.posix.`/`.win32.`) with no fixed-point conjunct (`dirname(cur) !== cur`, or `path.parse(cur).root`) in the loop test — `path.dirname()` is a no-op at the platform root, but on win32 that fixed-point value (`'D:\\'`, length 3) fails a POSIX-shaped length or equality check that would have caught a POSIX root (`'/'`, length 1), so the walk spins forever there. | `tests/**/*.test.cjs`, `scripts/**/*.cjs` | (See ADR-1703's catalog and [epic #1702](https://github.com/open-gsd/gsd-core/issues/1702) for the full phase history.) @@ -280,6 +282,57 @@ if (process.platform !== 'win32') { > `RENAME_RETRY_ERRNOS` loop is compliant because the helper's own `renameSync` is recognized; a > bare `fs.renameSync(...)` call is what gets flagged. +## How-to — fix a `require-full-tmpdir-triad` violation + +Per Node's own docs, `os.tmpdir()` on Windows consults `TEMP` then `TMP` — it never reads +`TMPDIR` there. On every other platform `TMPDIR` is checked first. A child-process `env:` override +that redirects only `TMPDIR` therefore does nothing on Windows: the child inherits the parent's +ambient `TEMP`/`TMP` and its `os.tmpdir()` resolves to the wrong directory, silently. + +```js +// ❌ flagged — no-op on Windows +const r = runNode(['-e', probe], { env: { ...process.env, TMPDIR: outer } }); + +// ✅ set all three to the same value +const r = runNode(['-e', probe], { + env: { ...process.env, TMPDIR: outer, TEMP: outer, TMP: outer }, +}); +``` + +The same applies to a direct `process.env.TMPDIR = …` assignment — set `process.env.TEMP` and +`process.env.TMP` alongside it (and restore all three in the teardown), mirroring the +`require-userprofile-with-home` HOME/USERPROFILE convention above. + +## How-to — fix a `no-unbounded-dirname-walk` violation + +`path.dirname()` is a fixed point at the filesystem root on both platforms, but the fixed-point +*value* differs: `path.posix.dirname('/') === '/'` (length 1), while +`path.win32.dirname('C:\\') === 'C:\\'` (length 3). A walk that terminates on a POSIX-shaped +sentinel — a hardcoded length threshold or an equality check against a target that the walk may +never reach — spins forever at 100% CPU on a Windows drive root, since the string simply stops +changing while the sentinel condition never fires. + +```js +// ❌ flagged — never terminates on Windows when cur can't reach root +let cur = file; +while (cur && cur !== root && cur.length > 1) { + protectSet.add(cur); + cur = dirname(cur); +} + +// ✅ add the fixed-point conjunct — terminates on POSIX, win32 drive roots, and UNC roots alike +let cur = file; +while (cur && cur !== root && dirname(cur) !== cur) { + protectSet.add(cur); + cur = dirname(cur); +} +``` + +`path.parse(cur).root` is the other recognized portable sentinel: `while (cur !== path.parse(cur).root)`. +Whichever form you use, prefer breaking out of the loop the moment `dirname(cur) === cur` (as +`scripts/run-tests.cjs`'s `computeSweepProtectSet` does) over relying purely on the condition, so +the loop body never re-adds the fixed point. + ## How-to — add a new path resolver When you add a function that returns a filesystem path (e.g. in `src/runtime-homes.cts`), add its diff --git a/eslint-rules/no-unbounded-dirname-walk.cjs b/eslint-rules/no-unbounded-dirname-walk.cjs new file mode 100644 index 000000000..ac9b4a0c9 --- /dev/null +++ b/eslint-rules/no-unbounded-dirname-walk.cjs @@ -0,0 +1,170 @@ +'use strict'; + +/** + * no-unbounded-dirname-walk + * + * Flag an upward filesystem walk whose loop condition reassigns from + * path.dirname() but has no fixed-point termination guard. + * + * ## Why (DEFECT.WINDOWS-TEST-PORTABILITY — the #4020 / #4220 Windows CI hang) + * + * `path.dirname()` is a NO-OP at the platform root, but the no-op VALUE + * differs by platform: `path.posix.dirname('/') === '/'` (length 1), while + * `path.win32.dirname('C:\\') === 'C:\\'` (length 3, NOT length 1). An + * upward walk like + * + * while (cur && cur !== someRoot && cur.length > 1) cur = dirname(cur); + * + * terminates on POSIX by accident — the walk reaches '/', whose length is 1 + * — but NEVER on Windows when the walk cannot equal `someRoot` (e.g. the + * repo checkout on `D:\a\...` and a temp root on `C:\Users\...`): `cur !== + * someRoot` holds forever, `dirname(cur)` reaches the drive root and stays + * there, and a length check against a POSIX-shaped "root is length 1" + * assumption never fires. The loop spins at 100% CPU with zero output until + * something external kills it. That exact shape hung every scoped Windows + * CI lane for a day — `scripts/run-tests.cjs`'s `sweepProtectSet` walk + * (#4020, re-surfaced against a fresh Windows regression as #4220). + * + * ## What this enforces + * + * Any loop that reassigns its condition variable from a `dirname(...)` call + * must ALSO test that the walk has reached a fixed point — the portable + * root check — i.e. the condition set must include a comparison between the + * variable and its own dirname: + * + * while (cur && cur !== root && dirname(cur) !== cur) { ... } + * + * or compare against `path.parse(cur).root`, the other portable form. A + * length-only or equality-only bound, with no fixed-point conjunct, is + * reported as `unboundedWalk`. + * + * ## Recognized call shapes + * + * - Bare `dirname(...)` from a destructure (`const { dirname } = require('path')`) + * or an aliased one (`const { dirname: dir } = ...`). + * - Chained `path.dirname(...)` / `require('path').dirname(...)`. + * - `path.posix.dirname` / `path.win32.dirname` member chains. + * + * ## Zero escape hatches (ADR-1703) + * + * This rule joins the ADR-1703 `DEFECT.WINDOWS-TEST-PORTABILITY` catalog, + * which deliberately carries no comment-based opt-out: a walk that is + * bounded by some other genuinely portable mechanism (a hard iteration cap, + * a dynamic non-literal boundary) must be structured so the rule recognizes + * it — add the `dirname(cur) !== cur` (or `path.parse(cur).root`) conjunct + * alongside the other bound, which costs nothing at runtime and is what the + * shipped #4020/#4220 fix itself does. A false positive is a rule bug, fixed + * in the rule — see `tests/portability-rule-disable-ban.test.cjs`, which + * independently bans `eslint-disable` of this rule too. + */ + +const DIAGNOSTIC = 'unboundedWalk'; + +const DIRNAME_RE = /^(?:dirname)$/; + +/** Property chain tail of a member-ish callee: path.dirname -> 'dirname'. */ +function propertyName(node) { + return node && node.type === 'MemberExpression' && !node.computed + ? node.property.name + : null; +} + +function isDirnameCall(expr) { + if (!expr || expr.type !== 'CallExpression') return false; + const callee = expr.callee; + if (callee.type === 'Identifier') return DIRNAME_RE.test(callee.name); + // path.dirname / path.posix.dirname / require('path').dirname + return DIRNAME_RE.test(String(propertyName(callee) ?? '')); +} + +module.exports = { + meta: { + type: 'problem', + docs: { + description: 'an upward dirname() walk must carry a fixed-point termination guard', + category: 'Portability', + }, + schema: [], + messages: { + [DIAGNOSTIC]: + 'This dirname() walk has no fixed-point termination guard (DEFECT.WINDOWS-TEST-PORTABILITY): ' + + "path.dirname() is a no-op at the platform root (win32 dirname('D:\\\\') === 'D:\\\\'), so on " + + 'Windows a length- or equality-bounded walk over a path that never equals its target root ' + + 'spins forever at 100% CPU. Add `dirname(cur) !== cur` (or compare against ' + + 'path.parse(cur).root) to the loop condition. (#4020 / #4220 Windows CI hang)', + }, + }, + + create(context) { + function checkWhile(node) { + const test = node.test; + // NOTE: test may be a single BinaryExpression (`while (cur !== root)`), + // not only a compound LogicalExpression (`while (cur !== root && …)`) + // — the minimal #4020/#4220 shape is a SINGLE unguarded condition, so + // this must not require LogicalExpression up front. collect() below + // already handles a non-LogicalExpression test correctly (pushes it as + // the sole conjunct); only this early gate needs to admit that shape. + if (!test) return; + + // The reassignment: cur = dirname(cur) somewhere in the body (or the + // update clause of a for-loop shape routed through the same check). + let reassignsFromDirname = false; + const bodyStatements = node.body && node.body.type === 'BlockStatement' + ? node.body.body + : node.body + ? [node.body] + : []; + for (const stmt of bodyStatements) { + for (const child of [stmt, stmt.expression]) { + if ( + child && child.type === 'AssignmentExpression' && child.operator === '=' && + child.left.type === 'Identifier' && isDirnameCall(child.right) && + child.right.arguments[0] && child.right.arguments[0].type === 'Identifier' && + child.right.arguments[0].name === child.left.name + ) { + reassignsFromDirname = true; + } + } + } + if (!reassignsFromDirname) return; + + // The guard: some conjunct compares the walked variable to its own + // dirname, or to path.parse().root. + let hasFixedPointGuard = false; + const conjuncts = []; + (function collect(e) { + if (e && e.type === 'LogicalExpression') { collect(e.left); collect(e.right); } + else if (e) conjuncts.push(e); + })(test); + for (const c of conjuncts) { + if (c.type !== 'BinaryExpression' || !['!==', '!=', '===', '=='].includes(c.operator)) continue; + // dirname(cur) cur, or cur dirname(cur) + if ((isDirnameCall(c.left) && c.right.type === 'Identifier' && + isDirnameCall(c.right) === false && c.left.arguments[0] && + c.left.arguments[0].name === c.right.name) || + (isDirnameCall(c.right) && c.left.type === 'Identifier' && + c.right.arguments[0] && c.right.arguments[0].name === c.left.name)) { + hasFixedPointGuard = true; + } + // path.parse(cur).root cur — the other portable root sentinel. + const isParseRoot = (n, other) => + n && n.type === 'MemberExpression' && !n.computed && n.property.name === 'root' && + n.object && n.object.type === 'CallExpression' && + /parse/.test(String(n.object.callee.property?.name ?? n.object.callee.name ?? '')) && + n.object.arguments[0] && n.object.arguments[0].type === 'Identifier' && + other && other.type === 'Identifier' && + n.object.arguments[0].name === other.name; + if (isParseRoot(c.left, c.right) || isParseRoot(c.right, c.left)) { + hasFixedPointGuard = true; + } + } + if (hasFixedPointGuard) return; + context.report({ node: test, messageId: DIAGNOSTIC }); + } + + return { + WhileStatement: checkWhile, + DoWhileStatement: checkWhile, + }; + }, +}; diff --git a/eslint-rules/require-full-tmpdir-triad.cjs b/eslint-rules/require-full-tmpdir-triad.cjs new file mode 100644 index 000000000..156ae347e --- /dev/null +++ b/eslint-rules/require-full-tmpdir-triad.cjs @@ -0,0 +1,206 @@ +'use strict'; + +/** + * require-full-tmpdir-triad + * + * Flag a `TMPDIR` environment override — direct `process.env.TMPDIR = …` + * assignment, or a `TMPDIR` property inside a child-process `env:` object + * literal — that is not accompanied by `TEMP` and `TMP` in the same scope. + * + * ## Why (DEFECT.WINDOWS-TEST-PORTABILITY — the #4220 masked child-env bug) + * + * Per Node's own `os.tmpdir()` docs: on Windows, only the `TEMP` and `TMP` + * environment variables are consulted (`TEMP` first) — `TMPDIR` is never + * read there at all. On every other platform, `TMPDIR` is checked first, + * then `TMP`, then `TEMP`. Code that redirects a child process's temp + * directory by setting only `TMPDIR` in that child's `env` therefore does + * nothing on Windows: the child inherits the parent's ambient `TEMP`/`TMP` + * and its own `os.tmpdir()` resolves to the wrong place — silently, with no + * error, so the redirect just doesn't take effect. This exact shape shipped + * in `tests/run-tests-temp-root.test.cjs`'s own regression test for #4020: + * `env: { ...process.env, TMPDIR: outer }` on a `runNode(...)` child-process + * helper call, masked because Windows CI died in the unrelated #4020 + * dirname-walk hang before ever reaching this test (see #4220). + * + * ## What this enforces + * + * Two independent shapes are covered: + * + * 1. Direct assignment: `process.env.TMPDIR = X` (or bracket form) in a + * file, without a `process.env.TEMP = …` AND a `process.env.TMP = …` + * assignment also present anywhere in that file (`Program:exit` + * collection, same pattern as `require-userprofile-with-home.cjs`). + * 2. Object-literal env override: an object literal with a `TMPDIR` + * property, passed as the `env` option to a child-process-spawning call + * (`child_process.spawn`/`spawnSync`/`exec`/`execSync`/`execFile`/ + * `execFileSync`/`fork`, or a bare-named local helper that forwards to + * one, e.g. this repo's `runNode(...)` test helper) — flagged unless + * that SAME object literal also carries `TEMP` and `TMP` properties. + * + * Fix: set all three — `TMPDIR`, `TEMP`, and `TMP` — to the same value. + * + * ## Zero escape hatches (ADR-1703) + * + * This rule joins the ADR-1703 `DEFECT.WINDOWS-TEST-PORTABILITY` catalog, + * which deliberately carries no comment-based opt-out: a legitimately + * POSIX-only TMPDIR override must be structured so the rule never sees it + * (e.g. behind a `process.platform !== 'win32'` guard that also sets + * TEMP/TMP for the Windows branch), not annotated around. A false positive + * is a rule bug, fixed in the rule — see `tests/portability-rule-disable-ban.test.cjs`, + * which independently bans `eslint-disable` of this rule too. + */ + +const ENV_CHILD_PROCESS_METHODS = new Set([ + 'spawn', + 'spawnSync', + 'exec', + 'execSync', + 'execFile', + 'execFileSync', + 'fork', +]); + +// Local helpers in this repo that wrap a child-process call and forward an +// `env` option through unchanged — recognized by bare call name. +const ENV_LOCAL_HELPER_NAMES = new Set(['runNode']); + +const DIAGNOSTIC = 'missingTempTmp'; + +/** @type {import('eslint').Rule.RuleModule} */ +const rule = { + meta: { + type: 'problem', + docs: { + description: + 'Require process.env.TEMP and process.env.TMP to be set alongside any TMPDIR override (Windows portability)', + category: 'Portability', + }, + schema: [], + messages: { + [DIAGNOSTIC]: + 'Setting TMPDIR without TEMP and TMP is not portable (DEFECT.WINDOWS-TEST-PORTABILITY): ' + + "Node's os.tmpdir() never reads TMPDIR on Windows (only TEMP, then TMP) — this override " + + 'silently does nothing there. Set TEMP and TMP to the same value alongside TMPDIR.', + }, + }, + + create(context) { + // ── Shape 1: process.env.TMPDIR = … direct assignment ────────────────── + + /** Collected TMPDIR assignment nodes (process.env.TMPDIR = / process.env['TMPDIR'] =). */ + const tmpdirAssignments = []; + let tempAssigned = false; + let tmpAssigned = false; + + function isProcessEnvAssignment(lhs, key) { + if (!lhs || lhs.type !== 'MemberExpression') return false; + const obj = lhs.object; + if (!obj || obj.type !== 'MemberExpression') return false; + if ( + obj.computed || + obj.object.type !== 'Identifier' || + obj.object.name !== 'process' || + obj.property.type !== 'Identifier' || + obj.property.name !== 'env' + ) { + return false; + } + if (!lhs.computed) { + return lhs.property.type === 'Identifier' && lhs.property.name === key; + } + return lhs.property.type === 'Literal' && lhs.property.value === key; + } + + // ── Shape 2: object literal with a TMPDIR property passed as `env` ───── + + function isSpawnLikeCallee(callee) { + // child_process.spawn(...) / cp.spawnSync(...) / require('child_process').exec(...) + if ( + callee.type === 'MemberExpression' && + !callee.computed && + callee.property.type === 'Identifier' && + ENV_CHILD_PROCESS_METHODS.has(callee.property.name) + ) { + return true; + } + // Bare identifier: either a destructured child_process method + // (`const { spawnSync } = require('child_process'); spawnSync(...)`) + // or a local helper known to wrap one (`runNode(...)`). Matched by + // name only, same lightweight convention as this repo's other + // eslint-rules/*.cjs (e.g. no-hardcoded-tmp.cjs's isFsMethodCall) — + // no import/require data-flow tracing. + if ( + callee.type === 'Identifier' && + (ENV_LOCAL_HELPER_NAMES.has(callee.name) || ENV_CHILD_PROCESS_METHODS.has(callee.name)) + ) { + return true; + } + return false; + } + + function objectHasKey(objExpr, key) { + return objExpr.properties.some((p) => { + if (p.type !== 'Property') return false; + if (!p.computed) { + return ( + (p.key.type === 'Identifier' && p.key.name === key) || + (p.key.type === 'Literal' && p.key.value === key) + ); + } + return p.key.type === 'Literal' && p.key.value === key; + }); + } + + function checkCallExpression(node) { + if (!isSpawnLikeCallee(node.callee)) return; + + for (const arg of node.arguments) { + // opts is either the object literal directly, or nested in a later + // positional options argument — only the literal shape is checked; + // an options identifier passed by reference is out of scope (the + // AST cannot see its shape here). + if (arg.type !== 'ObjectExpression') continue; + + const envProp = arg.properties.find( + (p) => + p.type === 'Property' && + !p.computed && + ((p.key.type === 'Identifier' && p.key.name === 'env') || + (p.key.type === 'Literal' && p.key.value === 'env')), + ); + if (!envProp || envProp.value.type !== 'ObjectExpression') continue; + + const envObj = envProp.value; + if (!objectHasKey(envObj, 'TMPDIR')) continue; + if (objectHasKey(envObj, 'TEMP') && objectHasKey(envObj, 'TMP')) continue; + + context.report({ node: envObj, messageId: DIAGNOSTIC }); + } + } + + return { + AssignmentExpression(node) { + if (isProcessEnvAssignment(node.left, 'TMPDIR')) { + tmpdirAssignments.push(node); + } + if (isProcessEnvAssignment(node.left, 'TEMP')) tempAssigned = true; + if (isProcessEnvAssignment(node.left, 'TMP')) tmpAssigned = true; + }, + + CallExpression(node) { + checkCallExpression(node); + }, + + 'Program:exit'() { + if (tmpdirAssignments.length === 0) return; + if (tempAssigned && tmpAssigned) return; + + for (const node of tmpdirAssignments) { + context.report({ node, messageId: DIAGNOSTIC }); + } + }, + }; + }, +}; + +module.exports = rule; diff --git a/eslint.config.mjs b/eslint.config.mjs index f8221c459..a66279160 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -24,6 +24,8 @@ import noUnboundedQuantifier from './eslint-rules/no-unbounded-quantifier.cjs'; import noHardcodedTmp from './eslint-rules/no-hardcoded-tmp.cjs'; import noBareNpmExec from './eslint-rules/no-bare-npm-exec.cjs'; import requireUserprofileWithHome from './eslint-rules/require-userprofile-with-home.cjs'; +import requireFullTmpdirTriad from './eslint-rules/require-full-tmpdir-triad.cjs'; +import noUnboundedDirnameWalk from './eslint-rules/no-unbounded-dirname-walk.cjs'; import normalizePathInContent from './eslint-rules/normalize-path-in-content.cjs'; import requireFsOpFallback from './eslint-rules/require-fs-op-fallback.cjs'; import noUnboundedSpawn from './eslint-rules/no-unbounded-spawn.cjs'; @@ -52,6 +54,8 @@ const localPlugin = { 'no-hardcoded-tmp': noHardcodedTmp, 'no-bare-npm-exec': noBareNpmExec, 'require-userprofile-with-home': requireUserprofileWithHome, + 'require-full-tmpdir-triad': requireFullTmpdirTriad, + 'no-unbounded-dirname-walk': noUnboundedDirnameWalk, 'normalize-path-in-content': normalizePathInContent, 'require-fs-op-fallback': requireFsOpFallback, 'no-unbounded-spawn': noUnboundedSpawn, @@ -606,6 +610,10 @@ export default tseslint.config( 'local/require-registered-exit': 'error', // #3624: see the src/**/*.cts block above for detail. 'local/no-exact-case-env-access': 'error', + // #4244 (origin #4020 / #4220): scripts/run-tests.cjs is the actual site + // of the shipped Windows CI hang — registered here (not only on + // tests/**/*.cjs below) so the rule covers the real bug's own location. + 'local/no-unbounded-dirname-walk': 'error', }, }, @@ -687,6 +695,12 @@ export default tseslint.config( 'local/no-bare-npm-exec': 'error', // Require USERPROFILE alongside HOME assignments (ADR-1703 Phase 4) 'local/require-userprofile-with-home': 'error', + // Require TEMP+TMP alongside any TMPDIR override — TMPDIR is never read on + // Windows, so a TMPDIR-only redirect silently no-ops there (#4220). + 'local/require-full-tmpdir-triad': 'error', + // Require a fixed-point termination guard on any dirname() ancestor walk — + // a length/equality-only bound spins forever at a Windows drive root (#4020 / #4220). + 'local/no-unbounded-dirname-walk': 'error', // Ban unbounded sync child_process spawns in tests (DEFECT.UNBOUNDED-SUBPROCESS). // No allowlist: the epic (#3064) migrated every site; the rule runs with no // exemption surface. The only sanctioned escapes are an explicit `timeout` on diff --git a/scripts/ci-test-scope.cjs b/scripts/ci-test-scope.cjs index 5602e2d15..7cd32b46f 100644 --- a/scripts/ci-test-scope.cjs +++ b/scripts/ci-test-scope.cjs @@ -97,6 +97,10 @@ const RULES = [ tests: [ 'tests/run-tests-harness.test.cjs', 'tests/workflow-shell-pinning.test.cjs', + // #4220: the run-scoped temp root + computeSweepProtectSet ancestor-walk + // termination coverage — was previously not re-selected by an edit to + // scripts/run-tests.cjs, the exact file that shipped the #4020 hang. + 'tests/run-tests-temp-root.test.cjs', ], }, { @@ -302,6 +306,9 @@ const RULES = [ 'tests/require-userprofile-with-home.rule.test.cjs', 'tests/normalize-path-in-content.rule.test.cjs', 'tests/require-fs-op-fallback.rule.test.cjs', + // #4244 (origin #4020/#4220 Windows CI hang) — see ADR-1703 amendment. + 'tests/require-full-tmpdir-triad.rule.test.cjs', + 'tests/no-unbounded-dirname-walk.rule.test.cjs', ], }, { diff --git a/tests/config-schema.property.test.cjs b/tests/config-schema.property.test.cjs index 4efed9f2e..8d02ccdf1 100644 --- a/tests/config-schema.property.test.cjs +++ b/tests/config-schema.property.test.cjs @@ -1097,10 +1097,16 @@ describe('feat-3210: workflow and config contracts', () => { test('config-set accepts code_quality.fallow keys', () => { const originalTmpDir = process.env.TMPDIR; + const originalTemp = process.env.TEMP; + const originalTmp = process.env.TMP; // L2: fail loudly if no writable tmp dir is found (was silent skip) const writableTmp = getWritableTmp(); // N2: use shared helper assert.ok(writableTmp, 'no writable tmp directory found'); // L2: explicit fail-loud assertion + // #4220 class: os.tmpdir() never reads TMPDIR on Windows (only TEMP, then + // TMP), so redirecting only TMPDIR silently no-ops there — set all three. process.env.TMPDIR = writableTmp; + process.env.TEMP = writableTmp; + process.env.TMP = writableTmp; const tmpDir = createTempProject('gsd-fallow-config-'); try { const cases = [ @@ -1117,6 +1123,10 @@ describe('feat-3210: workflow and config contracts', () => { cleanup(tmpDir); if (originalTmpDir === undefined) delete process.env.TMPDIR; else process.env.TMPDIR = originalTmpDir; + if (originalTemp === undefined) delete process.env.TEMP; + else process.env.TEMP = originalTemp; + if (originalTmp === undefined) delete process.env.TMP; + else process.env.TMP = originalTmp; } }); diff --git a/tests/no-unbounded-dirname-walk.rule.test.cjs b/tests/no-unbounded-dirname-walk.rule.test.cjs new file mode 100644 index 000000000..d390b5a62 --- /dev/null +++ b/tests/no-unbounded-dirname-walk.rule.test.cjs @@ -0,0 +1,207 @@ +'use strict'; + +/** + * no-unbounded-dirname-walk.rule.test.cjs + * + * RuleTester unit tests for the local/no-unbounded-dirname-walk ESLint rule. + * + * Rule: flag a while/do-while loop that reassigns its condition variable + * from path.dirname() without a fixed-point termination guard + * (DEFECT.WINDOWS-TEST-PORTABILITY, the #4020 / #4220 Windows CI hang: + * path.win32.dirname() is a no-op at the drive root, so a length- or + * equality-only bound spins forever there). + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const { RuleTester } = require('eslint'); + +const rule = require('../eslint-rules/no-unbounded-dirname-walk.cjs'); + +const ruleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: 'commonjs', + }, +}); + +describe('no-unbounded-dirname-walk rule module', () => { + test('exports a create function and the unboundedWalk message', () => { + assert.strictEqual(typeof rule.create, 'function'); + assert.strictEqual(rule.meta.type, 'problem'); + assert.ok(rule.meta.messages.unboundedWalk, 'unboundedWalk message must exist'); + }); +}); + +describe('no-unbounded-dirname-walk: invalid — no fixed-point guard', () => { + test('invalid: the real #4020/#4220 shape — length-bounded walk against a POSIX-only sentinel', () => { + ruleTester.run('no-unbounded-dirname-walk', rule, { + valid: [], + invalid: [ + { + // The exact shipped shape: on Windows the repo is on D:\, the target + // root on C:\ — `cur !== runTempRoot` holds forever and win32 + // dirname('D:\\') is a fixed point, so this spins at 100% CPU. + code: ` + const { dirname } = require('path'); + const protectSet = new Set(); + let cur = f; + while (cur && cur !== runTempRoot && cur.length > 1) { + protectSet.add(cur); + cur = dirname(cur); + } + `, + errors: [{ messageId: 'unboundedWalk' }], + }, + ], + }); + }); + + test('invalid: destructured dirname with equality-only bound', () => { + ruleTester.run('no-unbounded-dirname-walk', rule, { + valid: [], + invalid: [ + { + code: ` + const { dirname } = require('path'); + let cur = file; + while (cur !== root) { + cur = dirname(cur); + } + `, + errors: [{ messageId: 'unboundedWalk' }], + }, + ], + }); + }); + + test('invalid: path.dirname member form with equality-only bound', () => { + ruleTester.run('no-unbounded-dirname-walk', rule, { + valid: [], + invalid: [ + { + code: ` + const path = require('path'); + let cur = file; + while (cur !== root) { + cur = path.dirname(cur); + } + `, + errors: [{ messageId: 'unboundedWalk' }], + }, + ], + }); + }); + + test('invalid: do-while form with no fixed-point guard', () => { + ruleTester.run('no-unbounded-dirname-walk', rule, { + valid: [], + invalid: [ + { + code: ` + const path = require('path'); + let cur = file; + do { + cur = path.dirname(cur); + } while (cur !== root && cur.length > 1); + `, + errors: [{ messageId: 'unboundedWalk' }], + }, + ], + }); + }); +}); + +describe('no-unbounded-dirname-walk: valid — fixed-point guard present', () => { + test('valid: the real fixed shape — dirname(cur) !== cur added to the condition', () => { + ruleTester.run('no-unbounded-dirname-walk', rule, { + valid: [ + { + code: ` + const { dirname } = require('path'); + const protectSet = new Set(); + let cur = f; + while (cur && cur !== runTempRoot && dirname(cur) !== cur) { + protectSet.add(cur); + cur = dirname(cur); + } + `, + }, + ], + invalid: [], + }); + }); + + test('valid: walk bounded via path.parse(cur).root', () => { + ruleTester.run('no-unbounded-dirname-walk', rule, { + valid: [ + { + code: ` + const path = require('path'); + let cur = file; + while (cur && cur !== path.parse(cur).root) { + cur = path.dirname(cur); + } + `, + }, + ], + invalid: [], + }); + }); + + test('valid: a length-only bound is still unsafe in principle, but the fixed-point conjunct present here silences it', () => { + ruleTester.run('no-unbounded-dirname-walk', rule, { + valid: [ + { + code: ` + const { dirname } = require('path'); + let cur = file; + while (cur && cur.length > 1 && dirname(cur) !== cur) { + cur = dirname(cur); + } + `, + }, + ], + invalid: [], + }); + }); + + test('valid: not a dirname walk at all — a linked-list traversal must stay silent', () => { + ruleTester.run('no-unbounded-dirname-walk', rule, { + valid: [ + { + code: ` + let cur = list.head; + while (cur && cur.length > 1) { cur = cur.next; } + `, + }, + ], + invalid: [], + }); + }); + + test('invalid: a length comparison against another expression\'s length is still unguarded — no fixed-point conjunct present', () => { + // Confirms the rule does not special-case a dynamic (non-literal) length + // bound as an implicit guard: only an explicit fixed-point conjunct + // silences it. This rule has NO comment-marker escape hatch (ADR-1703 + // zero-escape-hatch) — a loop shaped like this needs an added + // `dirname(cur) !== cur` (or `path.parse(cur).root`) conjunct; there is + // no annotation-based way to silence it instead. + ruleTester.run('no-unbounded-dirname-walk', rule, { + valid: [], + invalid: [ + { + code: ` + let cursor = path.dirname(path.resolve(target)); + const stop = path.resolve(root); + while (cursor.length >= stop.length && cursor.startsWith(stop)) { + if (check(cursor)) return true; + cursor = path.dirname(cursor); + } + `, + errors: [{ messageId: 'unboundedWalk' }], + }, + ], + }); + }); +}); diff --git a/tests/portability-rule-disable-ban.test.cjs b/tests/portability-rule-disable-ban.test.cjs index 82652cce8..9640ac052 100644 --- a/tests/portability-rule-disable-ban.test.cjs +++ b/tests/portability-rule-disable-ban.test.cjs @@ -44,6 +44,9 @@ const PROTECTED_RULES = [ // ADR-1703 Phase 6 rule (issue #1740) — applies to src/**/*.cts AND the build/install // surface (bin/install.js, scripts/build-hooks.js) brought under lint by the glob expansion 'require-fs-op-fallback', + // #4244 hardening (origin #4020 / #4220 Windows CI hang) — applies to tests/**/*.test.cjs + 'require-full-tmpdir-triad', + 'no-unbounded-dirname-walk', ]; // ── Detect disable directives via the comment text ─────────────────────────── diff --git a/tests/require-full-tmpdir-triad.rule.test.cjs b/tests/require-full-tmpdir-triad.rule.test.cjs new file mode 100644 index 000000000..42cb945ec --- /dev/null +++ b/tests/require-full-tmpdir-triad.rule.test.cjs @@ -0,0 +1,214 @@ +'use strict'; + +/** + * require-full-tmpdir-triad.rule.test.cjs + * + * RuleTester unit tests for the local/require-full-tmpdir-triad ESLint rule. + * + * Rule: flag a TMPDIR environment override — direct process.env.TMPDIR + * assignment, or a TMPDIR property inside a child-process env: object + * literal — that is not accompanied by TEMP and TMP in the same scope + * (DEFECT.WINDOWS-TEST-PORTABILITY, the #4220 masked child-env bug: Node's + * os.tmpdir() never reads TMPDIR on Windows). + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const { RuleTester } = require('eslint'); + +const rule = require('../eslint-rules/require-full-tmpdir-triad.cjs'); + +const ruleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: 'commonjs', + }, +}); + +// ─── module shape ───────────────────────────────────────────────────────────── + +describe('require-full-tmpdir-triad rule module', () => { + test('exports meta and create', () => { + assert.strictEqual(typeof rule.meta, 'object'); + assert.strictEqual(typeof rule.create, 'function'); + assert.strictEqual(rule.meta.type, 'problem'); + assert.ok(rule.meta.messages.missingTempTmp, 'missingTempTmp message must exist'); + }); +}); + +// ─── Shape 1: direct process.env.TMPDIR assignment ──────────────────────────── + +describe('require-full-tmpdir-triad: direct assignment shape', () => { + test('invalid: process.env.TMPDIR = X with no TEMP/TMP anywhere', () => { + ruleTester.run('require-full-tmpdir-triad', rule, { + valid: [], + invalid: [ + { + code: `process.env.TMPDIR = outer;`, + filename: 'tests/foo.test.cjs', + errors: [{ messageId: 'missingTempTmp' }], + }, + ], + }); + }); + + test('invalid: process.env["TMPDIR"] = X with only TEMP set (TMP missing)', () => { + ruleTester.run('require-full-tmpdir-triad', rule, { + valid: [], + invalid: [ + { + code: ` + process.env['TMPDIR'] = outer; + process.env.TEMP = outer; + `, + filename: 'tests/foo.test.cjs', + errors: [{ messageId: 'missingTempTmp' }], + }, + ], + }); + }); + + test('valid: process.env.TMPDIR assigned AND TEMP AND TMP also assigned', () => { + ruleTester.run('require-full-tmpdir-triad', rule, { + valid: [ + { + code: ` + process.env.TMPDIR = outer; + process.env.TEMP = outer; + process.env.TMP = outer; + `, + filename: 'tests/foo.test.cjs', + }, + ], + invalid: [], + }); + }); + + test('valid: no TMPDIR assignment at all', () => { + ruleTester.run('require-full-tmpdir-triad', rule, { + valid: [ + { + code: `const t = process.env.TMPDIR;`, + filename: 'tests/foo.test.cjs', + }, + ], + invalid: [], + }); + }); + + test('invalid: multiple TMPDIR assignments — all reported when TEMP/TMP absent', () => { + ruleTester.run('require-full-tmpdir-triad', rule, { + valid: [], + invalid: [ + { + code: ` + process.env.TMPDIR = a; + process.env.TMPDIR = b; + `, + filename: 'tests/foo.test.cjs', + errors: [ + { messageId: 'missingTempTmp' }, + { messageId: 'missingTempTmp' }, + ], + }, + ], + }); + }); +}); + +// ─── Shape 2: object-literal env override passed to a spawn-like call ──────── + +describe('require-full-tmpdir-triad: child-process env object-literal shape', () => { + test('invalid: the real #4220 shape — runNode(..., { env: { ...process.env, TMPDIR: outer } })', () => { + ruleTester.run('require-full-tmpdir-triad', rule, { + valid: [], + invalid: [ + { + code: `const r = runNode(['-e', probe], { timeoutMs: 30000, env: { ...process.env, TMPDIR: outer } });`, + filename: 'tests/foo.test.cjs', + errors: [{ messageId: 'missingTempTmp' }], + }, + ], + }); + }); + + test('invalid: child_process.spawnSync with TMPDIR-only env', () => { + ruleTester.run('require-full-tmpdir-triad', rule, { + valid: [], + invalid: [ + { + code: ` + const { spawnSync } = require('child_process'); + spawnSync('node', ['-e', 'x'], { env: { ...process.env, TMPDIR: outer } }); + `, + filename: 'tests/foo.test.cjs', + errors: [{ messageId: 'missingTempTmp' }], + }, + ], + }); + }); + + test('invalid: execFileSync with TMPDIR-only env', () => { + ruleTester.run('require-full-tmpdir-triad', rule, { + valid: [], + invalid: [ + { + code: ` + const cp = require('child_process'); + cp.execFileSync('node', [], { env: { TMPDIR: outer } }); + `, + filename: 'tests/foo.test.cjs', + errors: [{ messageId: 'missingTempTmp' }], + }, + ], + }); + }); + + test('valid: the real fixed shape — TMPDIR, TEMP, and TMP all in the same env literal', () => { + ruleTester.run('require-full-tmpdir-triad', rule, { + valid: [ + { + code: `const r = runNode(['-e', probe], { timeoutMs: 30000, env: { ...process.env, TMPDIR: outer, TEMP: outer, TMP: outer } });`, + filename: 'tests/foo.test.cjs', + }, + ], + invalid: [], + }); + }); + + test('valid: env object with no TMPDIR key at all', () => { + ruleTester.run('require-full-tmpdir-triad', rule, { + valid: [ + { + code: `runNode(['-e', probe], { env: { ...process.env, FOO: 'bar' } });`, + filename: 'tests/foo.test.cjs', + }, + ], + invalid: [], + }); + }); + + test('valid: a spawn-like call with no env option at all', () => { + ruleTester.run('require-full-tmpdir-triad', rule, { + valid: [ + { + code: `require('child_process').spawnSync('node', ['-v']);`, + filename: 'tests/foo.test.cjs', + }, + ], + invalid: [], + }); + }); + + test('valid: an unrelated call with an object literal containing TMPDIR is not a spawn — stays silent', () => { + ruleTester.run('require-full-tmpdir-triad', rule, { + valid: [ + { + code: `buildConfig({ env: { TMPDIR: outer } });`, + filename: 'tests/foo.test.cjs', + }, + ], + invalid: [], + }); + }); +});