From 83685ea7a371b00bdf1b8585a7222a875a9ba714 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 26 Jun 2026 00:19:44 -0400 Subject: [PATCH] docs(#1744): portability architecture guide + ADR-1703 acceptance (Phase 7 closeout) (#1745) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR-1703 Phase 7 / epic #1702 closeout. The mechanical teardown is already complete across phases 1-6 (regex scanner retired P3, ratchet deleted P4, allowlist portability-usage gone, windows-portability-ok comments swept, every DEFECT.WINDOWS-* predicate rewritten per-phase). This phase delivers the two remaining ADR-mandated closeout items: - docs/contributing/adding-a-portability-rule.md: the forward architecture recipe (Diátaxis Explanation) — the five seams (rule / portability-vocab / platform-guard / disable-ban / ci-test-scope), the zero-escape-hatch contract, the shipped-rule catalog, and the step-by-step checklist for adding a new local/* portability rule. - docs/adr/1703: Status Proposed -> Accepted (all seven phases shipped); a Phase 7 as-built note recording the two deviations from the Phase 0 catalog — Phase 6's rename-only scope decision (#1740) and the codex-review precision tightening on require-fs-op-fallback. - docs/contributing/cross-platform-portability-rules.md: cross-link to the new guide from the rule reference. No code, no test, no runtime change. Epic #1702 Phase 6 box checked; Phase 7 box + epic closure to follow at PR merge. Closes #1744 Co-authored-by: review-bot --- ...03-portability-enforcement-architecture.md | 30 +++- .../contributing/adding-a-portability-rule.md | 150 ++++++++++++++++++ .../cross-platform-portability-rules.md | 4 + 3 files changed, 182 insertions(+), 2 deletions(-) create mode 100644 docs/contributing/adding-a-portability-rule.md diff --git a/docs/adr/1703-portability-enforcement-architecture.md b/docs/adr/1703-portability-enforcement-architecture.md index 1adfd4800..4ae15608e 100644 --- a/docs/adr/1703-portability-enforcement-architecture.md +++ b/docs/adr/1703-portability-enforcement-architecture.md @@ -1,7 +1,7 @@ # ADR-1703: Cross-platform portability enforcement as AST ESLint rules -- **Status:** Proposed -- **Date:** 2026-06-25 +- **Status:** Accepted +- **Date:** 2026-06-25 (Phase 0); **Accepted 2026-06-26** (Phase 7 closeout — all phases shipped) - **Issue:** [#1703](https://github.com/open-gsd/gsd-core/issues/1703) — Phase 0 of epic [#1702](https://github.com/open-gsd/gsd-core/issues/1702) - **Supersedes:** the regex-based `scripts/lint-windows-test-portability.cjs`, the `tests/windows-test-parity-guard.test.cjs` named-set ratchet (G1–G6), the @@ -186,6 +186,32 @@ Each implementation phase runs the full engineering directive (rubber-duck → l → qa-test-architect → strict TDD via `RuleTester` → codex adversarial → Diátaxis → rebase+PR) and is its own approved child issue + PR under epic #1702. +## Phase 7 — as-built / acceptance (2026-06-26) + +All seven phases shipped; the architecture is exercised in production and accepted. Two +as-built deviations from the Phase 0 catalog, both within this ADR's precision discipline: + +- **Phase 6 scope — `require-fs-op-fallback` narrowed to rename.** The catalog row named + `DEFECT.WINDOWS-FS-OPS` for "`src/**/*.cts`, build/install". The defect's own `.fix-forward` + defines the cure as *"catch EPERM/EBUSY/EACCES, fall back to copy + unlink with retry"* — so + `copyFile`/`unlink` are the **fallback primitives**, not separate defect sites, and flagging + them would flag the cure (`unlink` also has ~30 intentional best-effort cleanup sites that would + be a FP minefield). v1 recognition is therefore `fs.rename`/`fs.renameSync` only, with the + `RENAME_RETRY_ERRNOS` retry loop as the recognized compliant shape; `copyFile`/`unlink` + transient-lock sub-classes are documented for a possible follow-up. The ADR-mandated glob + expansion to `bin/install.js` + `scripts/build-hooks.js` (L124-126) landed as specified. + Documented on [#1740](https://github.com/open-gsd/gsd-core/issues/1740). + +- **Phase 6 precision tightening (codex review).** The rule's compliance shape was tightened after + an adversarial gpt-5.5 review: a catch must BOTH reference a transient errno AND carry a retry + signal (a loop `continue` backedge or a `return ` delegation — NOT a bare rethrow), and + only the **nearest catching** try/catch counts (an outer errno-catch is unreachable once an inner + catch intercepts). This enforces the defect's *"never silently swallow"* + cure-is-retry clauses + honestly. See [`eslint-rules/require-fs-op-fallback.cjs`](../../eslint-rules/require-fs-op-fallback.cjs). + +The forward "how to add a portability rule" recipe delivered by this phase lives at +[`docs/contributing/adding-a-portability-rule.md`](../contributing/adding-a-portability-rule.md). + ## Alternatives considered 1. **Keep extending the regex lint.** Rejected — the adversarial review proved it is diff --git a/docs/contributing/adding-a-portability-rule.md b/docs/contributing/adding-a-portability-rule.md new file mode 100644 index 000000000..008e52204 --- /dev/null +++ b/docs/contributing/adding-a-portability-rule.md @@ -0,0 +1,150 @@ +# 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`](../../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](../adr/1703-portability-enforcement-architecture.md); the per-rule reference and +fix how-tos live in [`cross-platform-portability-rules.md`](./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/.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: + +1. **Rules must be precise.** Every rule recognizes legitimate platform-gating via + `platform-guard.cjs` and 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. +2. **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 `continue` + backedge or a `return ` delegation), not merely *reference* the errno and rethrow. The + `require-fs-op-fallback` rule 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: + +1. **Classify the defect.** Confirm it's a real `DEFECT.WINDOWS-*` shape (or a new class worth a + predicate in `CONTEXT.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). +2. **Write the rule** — `eslint-rules/.cjs` (`{ meta, create }`, `type: 'problem'`, + message cites the `DEFECT.*` predicate). Reuse `portability-vocab.cjs` / `platform-guard.cjs`. +3. **TDD via `RuleTester`** — `tests/.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. +4. **Register + scope** — in `eslint.config.mjs`: add the rule to the `local` plugin's `rules` map + 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. +5. **Disable ban** — append the rule name to `PROTECTED_RULES` in + `tests/portability-rule-disable-ban.test.cjs`; add any new surface to `collectTestFiles()`. +6. **CI selection** — add the new test file to the `portability lint rules (ADR-1703)` rule in + `scripts/ci-test-scope.cjs`. +7. **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). +8. **Docs** — add the rule to the reference table + a fix how-to in + `cross-platform-portability-rules.md`; rewrite the `DEFECT.*` predicate's `detect=`/`fix-forward=` + in `CONTEXT.md` to point at the rule; record known boundaries honestly. +9. **Verify** — `npm run lint:ci` green; the touched modules' tests green; the `windows-latest` CI + 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 | + +`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. diff --git a/docs/contributing/cross-platform-portability-rules.md b/docs/contributing/cross-platform-portability-rules.md index 1435ec194..2d53bb281 100644 --- a/docs/contributing/cross-platform-portability-rules.md +++ b/docs/contributing/cross-platform-portability-rules.md @@ -7,6 +7,10 @@ Windows-only defect is caught before it ships — not after it reaches the `wind lane. The architecture and rationale are in [ADR-1703](../adr/1703-portability-enforcement-architecture.md); this page is the practical reference + how-to. +> **Adding a new rule?** See [`adding-a-portability-rule.md`](./adding-a-portability-rule.md) — +> the five seams (rule / vocab / platform-guard / disable-ban / ci-scope), the zero-escape-hatch +> contract, and the step-by-step recipe. + These rules are **hard-fail with zero escape hatches**: there is no `// windows-portability-ok:` comment and no `eslint-disable` for them (a `tests/portability-rule-disable-ban.test.cjs` check, running outside ESLint, fails the build if you try). Legitimately platform-specific code must be