* fix(#4244): repoint TEMP/TMP alongside TMPDIR and fix the sweepProtectSet fixed-point walk Repo-wide sweep (ahead of adding lint rules for these exact bug classes) found both incident patterns still live and unfixed on `next`: - scripts/run-tests.cjs's sweepProtectSet walk stopped on `cur !== runTempRoot && cur.length > 1` — a POSIX-only sentinel. win32 dirname('D:\') is a fixed point (length 3, never satisfies `> 1`... wait, it does satisfy length>1), so a selected file living outside runTempRoot (the common case) spins the walk forever on Windows. Extracted a pure, exported computeSweepProtectSet helper that terminates on dirname(cur) === cur instead, with in-process RuleTester-style coverage for both win32 and posix paths. - tests/run-tests-temp-root.test.cjs's own #4020 regression test set only TMPDIR on its runNode(...) child env. Node's os.tmpdir() never reads TMPDIR on Windows (only TEMP, then TMP), so the redirect silently no-oped there — masked because Windows CI died in the dirname-walk hang above before ever reaching this test. - tests/config-schema.property.test.cjs's fallow config-set test had the same TMPDIR-only pattern, direct process.env assignment this time, restored in its own finally block. Origin: #4220 and its shared root cause #4020. * feat(#4244): require-full-tmpdir-triad and no-unbounded-dirname-walk ESLint rules Two custom local ESLint rules catch the #4220 / #4020 Windows CI hang bug class at author time, joining the ADR-1703 DEFECT.WINDOWS-TEST-PORTABILITY catalog. Neither eslint-plugin-unicorn nor eslint-plugin-n has a rule for either shape. - local/require-full-tmpdir-triad: flags a TMPDIR environment override (direct process.env.TMPDIR assignment, or a TMPDIR property in a spawn-like call's env: object literal) not accompanied by TEMP and TMP in the same scope. Node's os.tmpdir() never reads TMPDIR on Windows. Registered on tests/**/*.cjs, matching the require-userprofile-with-home precedent. - local/no-unbounded-dirname-walk: flags a while/do-while loop reassigning from dirname() with no fixed-point termination guard (dirname(cur) !== cur, or path.parse(cur).root). path.dirname() is a no-op at the platform root, but the value differs by platform (win32 'D:\' is length 3, posix '/' is length 1), so a POSIX-shaped length/equality bound never fires on Windows. Registered on BOTH tests/**/*.cjs and scripts/**/*.cjs — the real #4020 bug lived in scripts/run-tests.cjs, not tests/. Both rules join the zero-escape-hatch discipline already established for this catalog (no bespoke comment marker; PROTECTED_RULES in tests/portability-rule-disable-ban.test.cjs independently bans eslint-disable of either). ADR-1703 and its two companion contributing docs get an amendment documenting the mechanism, code examples, and the repo-wide sweep (three live instances found and fixed in the prior commit; no others found). CI test-scope selection updated so an edit to either rule or to scripts/run-tests.cjs re-runs the right suites. * fix(#4244): no-unbounded-dirname-walk must analyze a single-condition loop test too checkWhile bailed out early unless node.test was a LogicalExpression, so a single-condition loop -- while (cur !== root) { cur = dirname(cur); } -- was silently skipped and never reported. That is the EXACT minimal shape of the original #4020/#4220 bug, and it is literally the shape used by this rule's own shipped RuleTester fixtures (the "equality-only bound" invalid cases), which were failing (0 errors reported, 1 expected) until this fix -- confirmed by running RuleTester directly against both fixtures, not just via a passing test-runner exit code. The conjunct-collection helper already handled a non-LogicalExpression test correctly (it pushes a single node as the sole conjunct); only the early-return gate needed to stop requiring a compound && / || test. Verified: RuleTester run directly against both previously-broken fixtures plus two new sanity cases (a guarded single-condition loop stays valid; an unrelated single-condition loop stays silent), and a fresh `npx eslint .` across the whole repo remains clean (no other single-condition dirname-walk shape exists in the tree). * fix(#4244): require-full-tmpdir-triad must recognize a destructured child_process call isSpawnLikeCallee only recognized a MemberExpression callee (child_process.spawnSync(...)) or a bare identifier in ENV_LOCAL_HELPER_NAMES (runNode). A destructured import called bare -- const { spawnSync } = require('child_process'); spawnSync(...) -- has an Identifier callee named "spawnSync", which matched neither branch, so the whole env-literal check was skipped. gsd-test caught this: both "invalid: child_process.spawnSync with TMPDIR-only env" cases in tests/require-full-tmpdir-triad.rule.test.cjs were failing (0 errors reported, 1 expected). Widened the bare-identifier branch to also match any of the known ENV_CHILD_PROCESS_METHODS names, matched by name only -- the same lightweight convention this repo's other eslint-rules/*.cjs use (e.g. no-hardcoded-tmp.cjs's isFsMethodCall), not full import data-flow tracing. Verified: RuleTester run directly against all 11 cases in tests/require-full-tmpdir-triad.rule.test.cjs (not just the two that were failing), all pass; a fresh npx eslint . and npm run lint:ci across the whole repo remain clean. * fix(#4244): correct a stale escape-hatch reference in a test comment The comment on the "length comparison against another expression's length" case referenced a "// allow-dirname-walk marker" that doesn't exist -- the rule has zero comment-based escape hatches by design (ADR-1703), and an earlier draft's marker mechanism was removed before this branch's first commit. Spec-axis review caught the stale reference. No behavior change; comment-only. * chore(#4244): backfill changeset PR number (pr:0 -> pr:4246) --------- Co-authored-by: sim <sim@local>
9.8 KiB
Adding a cross-platform portability lint rule
GSD must run correctly on Windows as well as macOS/Linux. The DEFECT.WINDOWS-*
taxonomy in CONTEXT.md names the recurring failure shapes; a family of
AST-based ESLint rules (the local/* plugin) enforces them at write-time (in your editor)
and in CI, so a Windows-only defect is caught before it ships — not after it reaches the
windows-latest CI lane.
This page is the forward recipe: how to add a new rule when you identify a portability
defect class that isn't yet mechanically enforced. The architecture and rationale live in
ADR-1703; the per-rule reference and
fix how-tos live in cross-platform-portability-rules.md.
Diátaxis note: this is an Explanation — it describes the architecture and the reasoning behind the seams, so the recipe at the end makes sense. For "how do I fix a violation I got", see the per-rule how-tos in the reference page.
Why AST rules, not regex
The original enforcement was a regex scanner with a hand-rolled balanced-paren parser, a frozen
KNOWN_OFFENDERS ratchet, and a bespoke // windows-portability-ok: comment opt-out. Adversarial
review of an attempt to extend the regex found it silently could not match deepStrictEqual,
had loose normalizer recognition, and hand-rolled paren-splitting fragility (Kernighan's Law /
Greenspun's Tenth — parsing a language with regex). The rip-and-replace decision: one mechanism,
AST-based ESLint rules using the parsers already in the stack, hard-fail with zero escape
hatches, no ratchet/grandfathering. Full rationale: ADR-1703 "Alternatives considered".
The five seams (and where each lives)
Every portability rule composes the same five seams. Adding a rule means touching each one.
1. The rule — eslint-rules/<rule-name>.cjs
One file per rule, exporting { meta, create }. Matches real syntax nodes (CallExpression,
MemberExpression, Literal, TemplateLiteral, BinaryExpression, TryStatement, …), not
text. Each rule runs in-editor and in CI via the existing eslint . (invoked by lint:ci).
The two shapes that recur:
- Test-side rules (surface
tests/**/*.test.cjs) — flag a non-portable assertion or test fixture shape (path-literal-in-assert, posix-mode-bit-assert, unguarded exec, CRLF split, hardcoded/tmp, bare npm, HOME-without-USERPROFILE). - Production rules (surface
src/**/*.cts,bin/install.js,scripts/build-hooks.js) — flag a non-portable production shape (path-leak-in-content, unguarded fs-rename).
2. The shared vocabulary — eslint-rules/lib/portability-vocab.cjs
The single source of truth for path-related portability: PATH_RETURNING_FNS (Node builtins +
project resolvers), the POSIX-normalizer recognizers (.replace(/\\/g,'/'), toPosixPath, …),
and string-unwrap helpers. A new path resolver added to src/runtime-homes.cts MUST be registered
here — the drift-guard test (tests/portability-vocab-drift.test.cjs) parses that source and
fails CI if a path-returning export is missing from PATH_RETURNING_FNS.
3. The platform guard — eslint-rules/lib/platform-guard.cjs
The precision backbone. isWindowsExcludedNode(node, sourceCode) answers "is this node
control-dependent on a Windows platform condition?" via a dominator check, not a textual mention:
if (process.platform !== 'win32') { … }, early-return guards (if (process.platform === 'win32') return;),
os.platform(), and hoisted binding-aware booleans (const isWindows = … consumed by
if (!isWindows), with reassignment detection). This is what makes zero escape hatches viable:
legitimately POSIX-only code is structured behind a recognized guard, never annotated around
(Postel's Law mitigation). If a legitimate shape isn't recognized, teach the helper — never add
an opt-out.
4. The disable ban — tests/portability-rule-disable-ban.test.cjs
Because there is no opt-out, an eslint-disable of a portability rule would silently bypass it.
This test runs outside ESLint (so it cannot itself be eslint-disabled) and fails the build on
any eslint-disable[-next-line|-line] that names a protected portability rule, or any blanket
disable. Every new rule MUST be appended to PROTECTED_RULES here, and if the rule covers a
new surface (e.g. bin/install.js), that surface MUST be added to collectTestFiles().
5. CI test selection — scripts/ci-test-scope.cjs
The portability lint rules (ADR-1703) rule selects the rule suites + disable-ban when
eslint-rules/, eslint.config.mjs, or a covered production surface changes. Add new test files
to its tests: list.
The zero-escape-hatch contract
Two things must hold, and they are the epic's primary risk:
- Rules must be precise. Every rule recognizes legitimate platform-gating via
platform-guard.cjsand the canonical compliant shapes, so correctly-written platform-specific code is never flagged. A false positive is a rule bug, fixed in the rule — never by adding an opt-out. - Recognition must mean the cure, not just the symptom. When a rule's compliance shape is "the
catch handles the transient errno", the handler must actually retry/fallback (a loop
continuebackedge or areturn <call>delegation), not merely reference the errno and rethrow. Therequire-fs-op-fallbackrule encodes this (codex-review-tightened): the defect's cure is retry, not just recognition.
An unrecognized legitimate shape is fixed by teaching the helper/rule, never by annotation. This is the discipline that keeps the rules honest as the codebase grows.
Recipe — add a new local/* portability rule
Run the full engineering directive (rubber-duck → software laws → architecture → qa-test-architect
→ strict TDD via RuleTester → adversarial review → Diátaxis → rebase+PR). Concretely:
- Classify the defect. Confirm it's a real
DEFECT.WINDOWS-*shape (or a new class worth a predicate inCONTEXT.md). Decide the sound statically-detectable scope — narrow or document rather than ship FP-prone (Phase 5/6 each narrowed scope and documented the boundary). - Write the rule —
eslint-rules/<rule-name>.cjs({ meta, create },type: 'problem', message cites theDEFECT.*predicate). Reuseportability-vocab.cjs/platform-guard.cjs. - TDD via
RuleTester—tests/<rule-name>.rule.test.cjs. Cover: the violation shape(s), every recognized compliant shape (platform guard, normalizer, retry signal, …), and the anti-patterns that must NOT satisfy compliance (silent-swallow catch, rethrow-only, unrelated errno). Use both espree (.cjs) and@typescript-eslint/parser(.cts) where the rule spans both. Tests are written FIRST and must fail before the rule exists, then pass. - Register + scope — in
eslint.config.mjs: add the rule to thelocalplugin'srulesmap and enable at'error'in the matching file-glob block. If the rule covers a new surface (e.g.bin/install.js), add a config block for it — apply ONLY the portability rules to generated code, not the full recommended set. - Disable ban — append the rule name to
PROTECTED_RULESintests/portability-rule-disable-ban.test.cjs; add any new surface tocollectTestFiles(). - CI selection — add the new test file to the
portability lint rules (ADR-1703)rule inscripts/ci-test-scope.cjs. - Fix every violation — no ratchet, no grandfathering. Every existing + grandfathered offender is fixed in the same phase (route through a shared helper, add a guard, or normalize).
- Docs — add the rule to the reference table + a fix how-to in
cross-platform-portability-rules.md; rewrite theDEFECT.*predicate'sdetect=/fix-forward=inCONTEXT.mdto point at the rule; record known boundaries honestly. - Verify —
npm run lint:cigreen; the touched modules' tests green; thewindows-latestCI lane is the only true Windows signal.
Catalog (shipped)
| Rule | DEFECT | Surface | Phase |
|---|---|---|---|
no-path-literal-in-assert |
WINDOWS-PATH-LITERAL-IN-ASSERT |
tests | 1 |
no-posix-mode-bit-assert |
WINDOWS-POSIX-MODE-BIT-ASSERT |
tests | 2 |
no-unguarded-nonportable-exec |
WINDOWS-TEST-PORTABILITY (chmod+sh -c) |
tests | 3 |
no-crlf-fragile-split |
WINDOWS-TEST-PORTABILITY (G1–G3) |
tests | 4 |
no-hardcoded-tmp |
WINDOWS-TEST-PORTABILITY (G4) |
tests | 4 |
no-bare-npm-exec |
WINDOWS-TEST-PORTABILITY (G5) |
tests | 4 |
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.
It is addressed at the source (run-tests.cjs chunking under RUN_TESTS_MAX_CMDLINE_CHARS).
Teardown (complete)
The legacy machinery this architecture replaced is fully retired: the regex scanner
scripts/lint-windows-test-portability.cjs (Phase 3), the tests/windows-test-parity-guard.test.cjs
ratchet (Phase 4), allowlist-ratchet.cjs usage for portability classes (the module remains for
unrelated size-budget lints), and the // windows-portability-ok: comment convention (swept — zero
remain). Every DEFECT.WINDOWS-* predicate in CONTEXT.md now points at its enforcing rule.