From 871621c3c8d89a1609be763599b7c4dfaa2df02a Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 25 Jun 2026 23:55:58 -0400 Subject: [PATCH] feat(#1740): require-fs-op-fallback production AST rule + Windows transient-lock retry (Phase 6) (#1742) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(#1740): require-fs-op-fallback production AST rule + Windows transient-lock retry (Phase 6) ADR-1703 Phase 6 of the cross-platform portability epic (#1702). Adds the second production-code portability AST rule + the ADR-mandated glob expansion to bin/install.js and scripts/build-hooks.js. - eslint-rules/require-fs-op-fallback.cjs: flags an unguarded fs.rename / fs.renameSync (the atomic-publish primitive named first in DEFECT.WINDOWS-FS-OPS.symptom) that is NOT inside a try/catch whose handler references a transient errno ('EPERM'/'EBUSY'/'EACCES' or a *RETRY_ERRNOS set) AND NOT behind a Windows platform guard. A catch that silently swallows or cleans-up-and-rethrows without an errno check does NOT satisfy the defect's 'never silently swallow' clause. copyFile/unlink are deliberately not flagged (they are the fallback primitives per the defect's own fix-forward). Scope narrowed to rename per Phase 5's precision discipline; documented on #1740. - src/shell-command-projection.cts: export retryRenameSync(from, to) — the drop-in bounded-retry helper over the existing atomicRenameWithRetry. - 27 bare fs.renameSync sites across 11 modules routed through retryRenameSync (capability-lifecycle/lock/source, installer-migrations, milestone, phase, planning-workspace, roadmap-upgrade, runtime-hooks-surface, state, workstream). Idempotent on POSIX; resilient to AV/indexer transient locks on Windows. - eslint.config.mjs: register rule at error on src/**/*.cts; new focused portability-rules block covering bin/install.js + scripts/build-hooks.js (ADR-1703 L124-126 glob expansion — both files are compliant: zero rename violations). - tests: 15-case RuleTester suite; portability-rule-disable-ban extended (PROTECTED_RULES + scans bin/install.js/build-hooks.js with shebang handling); ci-test-scope portability-lint selection rule. - CONTEXT.md DEFECT.WINDOWS-FS-OPS predicate rewritten to point at the rule; docs/contributing/cross-platform-portability-rules.md reference + how-to. Closes #1740 * chore(#1740): backfill changeset pr:1742 * fix(#1740): tighten require-fs-op-fallback precision (codex review HIGH-1/HIGH-2) Addresses two false-negative findings from the codex (gpt-5.5/high) adversarial review of PR #1742: HIGH-1 — a catch that REFERENCES a transient errno but only rethrows (no retry/fallback) was marked compliant. The DEFECT.WINDOWS-FS-OPS fix-forward requires retry, not just recognition. Fix: catchHandlerHasRetrySignal now requires a loop `continue` backedge OR a `return ` delegation; a bare rethrow is flagged. The misleading `/* retry logic */` valid test is replaced with a real retry loop, and the rethrow-only shape is added as invalid. HIGH-2 — the nested-try ancestor walk treated an OUTER errno-catch as protecting the rename even when an INNER catch intercepted/swallowed the error (the outer catch is unreachable). Fix: isInsideTransientErrnoTryCatch now stops at the NEAREST enclosing TryStatement WITH A CATCH HANDLER whose block contains the rename (try-finally is skipped — it doesn't catch); outer catches are no longer consulted. The unsound nested-try valid test is converted to invalid, and a try-finally-skipped valid case is added. Verified: 17 RuleTester cases pass; zero new production violations (the 27 fixed sites use retryRenameSync; the real retry loops — atomicRenameWithRetry, capability-ledger/consent, build-hooks — remain compliant via continue/errno); lint:ci green; disable-ban + vocab-drift green. --------- Co-authored-by: review-bot --- .changeset/humble-sloths-jump.md | 5 + CONTEXT.md | 4 +- .../cross-platform-portability-rules.md | 54 +++ eslint-rules/require-fs-op-fallback.cjs | 336 ++++++++++++++++ eslint.config.mjs | 32 ++ scripts/ci-test-scope.cjs | 34 +- src/capability-lifecycle.cts | 15 +- src/capability-lock.cts | 5 +- src/capability-source.cts | 9 +- src/installer-migrations.cts | 4 +- src/milestone.cts | 6 +- src/phase.cts | 10 +- src/planning-workspace.cts | 4 +- src/roadmap-upgrade.cts | 5 +- src/runtime-hooks-surface.cts | 2 +- src/shell-command-projection.cts | 15 + src/state.cts | 4 +- src/workstream.cts | 10 +- tests/portability-rule-disable-ban.test.cjs | 20 +- tests/require-fs-op-fallback.rule.test.cjs | 368 ++++++++++++++++++ 20 files changed, 896 insertions(+), 46 deletions(-) create mode 100644 .changeset/humble-sloths-jump.md create mode 100644 eslint-rules/require-fs-op-fallback.cjs create mode 100644 tests/require-fs-op-fallback.rule.test.cjs diff --git a/.changeset/humble-sloths-jump.md b/.changeset/humble-sloths-jump.md new file mode 100644 index 000000000..88e081286 --- /dev/null +++ b/.changeset/humble-sloths-jump.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 1742 +--- +**Windows install/upgrade/state-write operations no longer fail on transient antivirus/indexer file locks** — the fs.renameSync atomic-publish sites (install state, hooks config, capability ledger/lifecycle, phase/workstream/milestone dirs, roadmap, planning/state locks) now retry EPERM/EBUSY/EACCES via retryRenameSync instead of propagating the transient lock; enforced by the new local/require-fs-op-fallback lint rule (ADR-1703 Phase 6). (#1740) diff --git a/CONTEXT.md b/CONTEXT.md index 11db8e83e..0eb8a720e 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -711,8 +711,8 @@ The prompt-level data/instruction isolation seam for untrusted web/document ingr `DEFECT.WINDOWS-FS-OPS.symptom=fs.renameSync / fs.copyFileSync hits EPERM/EBUSY on Windows when antivirus or another process holds a transient handle on the target` `DEFECT.WINDOWS-FS-OPS.examples=c47c2c5d build-hooks rename → copy fallback, d2412271 install Windows persistent SDK shim` -`DEFECT.WINDOWS-FS-OPS.detect=any rename/copy in build/install path without try/catch fallback` -`DEFECT.WINDOWS-FS-OPS.fix-forward=catch EPERM/EBUSY/EACCES, fall back to copy + unlink with retry, surface degraded-mode message; never silently swallow` +`DEFECT.WINDOWS-FS-OPS.detect=ADR-1703 Phase 6: enforced by local/require-fs-op-fallback (AST ESLint rule, error) over src/**/*.cts + bin/install.js + scripts/build-hooks.js — flags an unguarded fs.rename/fs.renameSync (the atomic-publish primitive named in .symptom) that lacks a transient-errno retry or a Windows platform guard; a catch that silently swallows or cleans-up-and-rethrows without an errno check does NOT satisfy the .fix-forward clause. copyFile/unlink are the fallback primitives (out of scope); delegated retry helpers (retryRenameSync from shell-command-projection) are the recognized compliant shape` +`DEFECT.WINDOWS-FS-OPS.fix-forward=catch EPERM/EBUSY/EACCES, fall back to copy + unlink with retry, surface degraded-mode message; never silently swallow; the canonical production cure is retryRenameSync (shell-command-projection.cjs) or a bounded RENAME_RETRY_ERRNOS = new Set(['EPERM','EBUSY','EACCES']) loop` `DEFECT.UNBOUNDED-SUBPROCESS.symptom=git/npm subprocess shelled out without timeout; CLI hangs indefinitely on stuck remote, large repo, or missing network` `DEFECT.UNBOUNDED-SUBPROCESS.examples=a33cbe72 worktree fix bound git subprocesses with timeout` diff --git a/docs/contributing/cross-platform-portability-rules.md b/docs/contributing/cross-platform-portability-rules.md index 13e2c7bda..1435ec194 100644 --- a/docs/contributing/cross-platform-portability-rules.md +++ b/docs/contributing/cross-platform-portability-rules.md @@ -24,6 +24,7 @@ running outside ESLint, fails the build if you try). Legitimately platform-speci | `local/no-bare-npm-exec` | An `execFileSync`/`spawnSync`/`spawn` call with `"npm"` as the command and no `{ shell: true }` option (or a platform-guarded equivalent) — `npm` is a `.cmd` batch wrapper on Windows and is not found without a shell. (`execSync`/`exec` already run via a shell, so they are not flagged.) | `tests/**/*.test.cjs` | | `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` | (See ADR-1703's catalog and [epic #1702](https://github.com/open-gsd/gsd-core/issues/1702) for the full phase history.) @@ -222,6 +223,59 @@ if (origHome === undefined) delete process.env.HOME; else process.env.HOME = ori if (origUserProfile === undefined) delete process.env.USERPROFILE; else process.env.USERPROFILE = origUserProfile; ``` +## How-to — fix a `require-fs-op-fallback` violation + +Why it fails on Windows: `fs.renameSync(tmp, target)` (the atomic-publish primitive) uses Windows +`MoveFileEx` with `MOVEFILE_REPLACE_EXISTING`, which throws `EPERM`/`EBUSY`/`EACCES` when an +antivirus scanner, indexer, or concurrent reader transiently holds the target open. On macOS/Linux +`rename(2)` atomically replaces regardless of open handles, so the bare call passes everywhere +except the `windows-latest` CI lane (DEFECT.WINDOWS-FS-OPS). + +**Fix option A (preferred for production): route through `retryRenameSync`** — the shared drop-in +from `shell-command-projection.cjs` that retries the transient errnos a bounded number of times +before rethrowing. It is idempotent on POSIX (the transient errnos do not occur there): + +```js +import { retryRenameSync } from './shell-command-projection.cjs'; + +// ❌ flagged — EPERM/EBUSY propagates unhandled on Windows +fs.renameSync(tmpPath, target); + +// ✅ drop-in — retries transient locks, throws on persistent failure +retryRenameSync(tmpPath, target); +``` + +**Fix option B: inline the `RENAME_RETRY_ERRNOS` loop** (the convention already used by +`capability-ledger`, `capability-consent`, and `shell-command-projection`'s own `atomicRenameWithRetry`): + +```js +const RENAME_RETRY_ERRNOS = new Set(['EPERM', 'EBUSY', 'EACCES']); +for (let attempt = 1; attempt <= 3; attempt++) { + try { + fs.renameSync(tmpPath, target); + break; + } catch (err) { + if (attempt < 3 && RENAME_RETRY_ERRNOS.has(err.code)) { backoff(); continue; } + throw err; + } +} +``` + +**Fix option C: gate behind a platform check** when the rename is genuinely POSIX-only: + +```js +// ✅ platform-guarded — not flagged +if (process.platform !== 'win32') { + fs.renameSync(tmpPath, target); +} +``` + +> **`copyFile` / `unlink` are not flagged.** Per the defect's own fix-forward, they are the +> *fallback primitives* ("catch EPERM/EBUSY/EACCES, fall back to copy + unlink with retry"), not +> separate defect sites. A retry delegated to a helper that itself wraps `renameSync` in the +> `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 — 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/require-fs-op-fallback.cjs b/eslint-rules/require-fs-op-fallback.cjs new file mode 100644 index 000000000..2dc68e989 --- /dev/null +++ b/eslint-rules/require-fs-op-fallback.cjs @@ -0,0 +1,336 @@ +'use strict'; + +/** + * require-fs-op-fallback + * + * Flag: a bare fs.rename / fs.renameSync call (the atomic-publish primitive + * named first in DEFECT.WINDOWS-FS-OPS.symptom) that is NOT either: + * + * (a) inside a try/catch whose catch handler BOTH references a transient + * errno ('EPERM' / 'EBUSY' / 'EACCES', literally OR via a *RETRY_ERRNOS- + * style set identifier) AND carries a retry signal (a loop `continue` + * backedge or a `return ` delegation — NOT a bare rethrow: the + * defect's cure is retry/fallback, not just errno recognition), OR + * (b) control-dependent on a Windows platform guard + * (process.platform !== 'win32' / early-return — isWindowsExcludedNode). + * + * The canonical defect: on Windows, when an antivirus scanner, indexer, or + * concurrent reader transiently holds the target open, fs.renameSync throws + * EPERM/EBUSY/EACCES. A bare renameSync (or one wrapped in a try/catch that + * only cleans up + rethrows without distinguishing the transient errno) fails + * on the windows-latest CI lane where macOS/Linux CI passed — the established + * cure is the RENAME_RETRY_ERRNOS = new Set(['EPERM','EBUSY','EACCES']) retry + * loop already present in five production modules. + * + * "never silently swallow": a catch (e) {} or catch (_) {} with no transient- + * errno reference does NOT satisfy the defect's fix-forward and is still + * flagged. The fix is to add the bounded retry (the RENAME_RETRY_ERRNOS + * pattern) or gate behind a Windows platform check. + * + * copyFile / unlink are deliberately NOT flagged: per the defect's own + * .fix-forward ("catch EPERM/EBUSY/EACCES, fall back to copy + unlink with + * retry") they are the FALLBACK PRIMITIVES, not separate defect sites, and + * unlink has many intentional best-effort try/catch-swallow cleanup sites. + * + * References: + * DEFECT.WINDOWS-FS-OPS (CONTEXT.md) + * ADR-1703 (docs/adr/1703-portability-enforcement-architecture.md) + * issue #1740 (scope note: rename-only v1) + * + * Message: + * Cite DEFECT.WINDOWS-FS-OPS: fs.renameSync can throw EPERM/EBUSY/EACCES on + * Windows when a reader/AV transiently holds the target. Wrap in a bounded + * retry on the transient errno (the RENAME_RETRY_ERRNOS pattern) or gate + * behind a Windows platform check. + * + * ── Known boundaries ───────────────────────────────────────────────────────── + * + * (a) Name-based matching only. The rule recognizes `fs.rename` / `fs.renameSync` + * by spelling (MemberExpression: object=Identifier{fs}). A bare + * `renameSync(...)` call (when `fs` is destructured or the function is + * imported bare) is NOT matched — the production survey showed 100% + * `fs.renameSync` dotted usage, so dotted-only is the v1 shape. + * + * (b) Retry delegated to a helper function is NOT statically traceable. A + * bare `fs.renameSync` inside `atomicRenameWithRetry` IS detected as + * compliant because that helper wraps it in its own try/catch with the + * RENAME_RETRY_ERRNOS reference — but a call site that delegates via + * `atomicRenameWithRetry(tmp, target)` (calling the helper, no bare + * renameSync at the call site) has nothing to flag in the first place. + * + * (c) The catch-handler errno check is a subtree scan for transient-errno + * string literals OR *RETRY_ERRNOS identifiers. A catch that builds the + * errno set from a non-literal source (e.g. reading from config) is not + * recognized — the established convention is a module-level Set literal. + */ + +const { isWindowsExcludedNode } = require('./lib/platform-guard.cjs'); + +// fs mutation methods that are the atomic-publish transient-lock primitives. +const RENAME_METHODS = new Set(['rename', 'renameSync']); + +// Transient Windows lock errnos (the DEFECT.WINDOWS-FS-OPS.fix-forward set). +const TRANSIENT_ERRNOS = new Set(['EPERM', 'EBUSY', 'EACCES']); + +// Recognize retry-errno set identifiers by naming convention, e.g. +// RENAME_RETRY_ERRNOS, WRITE_RETRY_ERRNOS. Matches the established pattern +// across capability-ledger / capability-consent / shell-command-projection. +const RETRY_ERRNO_SET_NAME_RE = /RETRY_ERRNOS$/; + +/** + * True if `node` is an `fs.rename` / `fs.renameSync` CallExpression. + */ +function isFsRenameCall(node) { + if (!node || node.type !== 'CallExpression') return false; + const callee = node.callee; + if ( + callee.type === 'MemberExpression' && + !callee.computed && + callee.object.type === 'Identifier' && + callee.object.name === 'fs' && + callee.property.type === 'Identifier' && + RENAME_METHODS.has(callee.property.name) + ) { + return true; + } + return false; +} + +/** + * Walk a catch-clause subtree looking for evidence the handler distinguishes + * a transient errno. Recognized evidence: + * - a string Literal whose value is in TRANSIENT_ERRNOS ('EPERM'/'EBUSY'/'EACCES') + * - an Identifier (or MemberExpression object) whose name matches RETRY_ERRNO_SET_NAME_RE + * + * Skips `parent`/`tokens`/`comments` keys to avoid cycles. + */ +function catchHandlerReferencesTransientErrno(handlerNode) { + if (!handlerNode || typeof handlerNode !== 'object') return false; + // The CatchClause node has { type, param, body, parent }. Inspect body + // (and param name — not needed, but walk body subtree). + const seen = new WeakSet(); + function walk(n) { + if (!n || typeof n !== 'object') return false; + if (seen.has(n)) return false; + seen.add(n); + + // String literal errno: 'EPERM' / 'EBUSY' / 'EACCES' + if (n.type === 'Literal' && typeof n.value === 'string' && TRANSIENT_ERRNOS.has(n.value)) { + return true; + } + // *RETRY_ERRNOS identifier (bare or as a MemberExpression object) + if (n.type === 'Identifier' && RETRY_ERRNO_SET_NAME_RE.test(n.name)) { + return true; + } + + for (const key of Object.keys(n)) { + if (key === 'parent' || key === 'tokens' || key === 'comments') continue; + const child = n[key]; + if (Array.isArray(child)) { + for (const item of child) { + if (item && typeof item === 'object' && item.type) { + if (walk(item)) return true; + } + } + } else if (child && typeof child === 'object' && child.type) { + if (walk(child)) return true; + } + } + return false; + } + return walk(handlerNode); +} + +/** + * True when `handlerNode` (a CatchClause) contains a RETRY SIGNAL — evidence the + * catch actually re-attempts the rename rather than merely observing the errno. + * + * Recognized retry signals: + * - ContinueStatement — a loop backedge (`for { try{rename}catch{continue} }`) + * - ReturnStatement whose argument is a CallExpression — delegation + * (`return retry()`, `return atomicRenameWithRetry(...)`) + * + * This closes the "errno-check-then-rethrow" false-negative: a catch like + * `catch (e) { if (e.code === 'EPERM') throw e; throw e; }` references the + * errno but never retries, so it still fails on Windows transient locks. The + * DEFECT.WINDOWS-FS-OPS fix-forward requires retry/fallback, not just recognition. + * + * Skips `parent`/`tokens`/`comments` keys to avoid cycles. + */ +function catchHandlerHasRetrySignal(handlerNode) { + if (!handlerNode || typeof handlerNode !== 'object') return false; + const seen = new WeakSet(); + function walk(n) { + if (!n || typeof n !== 'object') return false; + if (seen.has(n)) return false; + seen.add(n); + // Loop backedge: `continue` re-enters the enclosing retry loop. + if (n.type === 'ContinueStatement') return true; + // Delegation: `return retry()` / `return atomicRenameWithRetry(...)` hands + // the rename off to a helper that performs its own bounded retry. + if ( + n.type === 'ReturnStatement' && + n.argument != null && + n.argument.type === 'CallExpression' + ) { + return true; + } + for (const key of Object.keys(n)) { + if (key === 'parent' || key === 'tokens' || key === 'comments') continue; + const child = n[key]; + if (Array.isArray(child)) { + for (const item of child) { + if (item && typeof item === 'object' && item.type) { + if (walk(item)) return true; + } + } + } else if (child && typeof child === 'object' && child.type) { + if (walk(child)) return true; + } + } + return false; + } + return walk(handlerNode); +} + +/** + * True when `renameNode` is protected by a transient-errno retry: i.e. the + * NEAREST enclosing TryStatement WITH A CATCH HANDLER whose `block` contains + * the rename has a handler that BOTH references a transient errno AND carries a + * retry signal (loop backedge or delegation return). + * + * Walks bottom-up and STOPS at the first TryStatement that (a) contains the + * rename in its `block` and (b) has a `handler`. A try with only a `finally` + * (no handler) does not intercept the rename error — it is skipped and the + * climb continues. The nearest catching try is where the rename's error lands; + * an outer catch is UNREACHABLE once the nearest catch intercepts (it may + * swallow, transform, or rethrow-as-other), so walking past it would be + * unsound (a false negative — see the nested-try case). An errno reference + * alone is insufficient; the handler must also retry (see catchHandlerHasRetrySignal). + */ +function isInsideTransientErrnoTryCatch(renameNode, sourceCode) { + const ancestors = _getAncestors(renameNode, sourceCode); + for (let i = ancestors.length - 1; i >= 0; i--) { + const anc = ancestors[i]; + if (anc.type !== 'TryStatement') continue; + if (!_containsNode(anc.block, renameNode)) continue; + if (!anc.handler) continue; // try-finally: error propagates, keep climbing + // Nearest catching try found — its handler is authoritative. An outer + // catch cannot protect the rename if this one intercepts first. + return ( + catchHandlerReferencesTransientErrno(anc.handler) && + catchHandlerHasRetrySignal(anc.handler) + ); + } + return false; +} + +// ── AST traversal helpers (mirror platform-guard.cjs internals) ────────────── + +function _getAncestors(node, sourceCode) { + if (sourceCode && typeof sourceCode.getAncestors === 'function') { + try { + return sourceCode.getAncestors(node); + } catch (_) { + // fall through to manual walk + } + } + return _findAncestors(sourceCode.ast, node); +} + +function _findAncestors(root, target) { + const chain = []; + function walk(node, ancestors) { + if (!node || typeof node !== 'object') return false; + if (node === target) { + chain.push(...ancestors); + return true; + } + for (const key of Object.keys(node)) { + if (key === 'parent' || key === 'tokens' || key === 'comments') continue; + const child = node[key]; + if (Array.isArray(child)) { + for (const item of child) { + if (item && typeof item === 'object' && item.type) { + if (walk(item, [...ancestors, node])) return true; + } + } + } else if (child && typeof child === 'object' && child.type) { + if (walk(child, [...ancestors, node])) return true; + } + } + return false; + } + walk(root, []); + return chain; +} + +function _containsNode(container, target) { + if (!container || typeof container !== 'object') return false; + if (container === target) return true; + const seen = new WeakSet(); + function walk(n) { + if (!n || typeof n !== 'object') return false; + if (seen.has(n)) return false; + seen.add(n); + if (n === target) return true; + for (const key of Object.keys(n)) { + if (key === 'parent' || key === 'tokens' || key === 'comments') continue; + const child = n[key]; + if (Array.isArray(child)) { + for (const item of child) { + if (item && typeof item === 'object' && item.type) { + if (walk(item)) return true; + } + } + } else if (child && typeof child === 'object' && child.type) { + if (walk(child)) return true; + } + } + return false; + } + return walk(container); +} + +/** @type {import('eslint').Rule.RuleModule} */ +const rule = { + meta: { + type: 'problem', + docs: { + description: + 'Require fs.rename/fs.renameSync to carry a transient-errno fallback (EPERM/EBUSY/EACCES) ' + + 'or a Windows platform guard (DEFECT.WINDOWS-FS-OPS)', + category: 'Portability', + }, + schema: [], + messages: { + requireFsOpFallback: + 'Unguarded fs.rename/fs.renameSync: on Windows a concurrent reader or antivirus scanner ' + + 'can transiently hold the target open, throwing EPERM/EBUSY/EACCES ' + + '(DEFECT.WINDOWS-FS-OPS). Wrap in a bounded retry on the transient errno ' + + "(the RENAME_RETRY_ERRNOS = new Set(['EPERM','EBUSY','EACCES']) pattern) " + + "or gate behind if (process.platform !== 'win32').", + }, + }, + + create(context) { + const sourceCode = context.sourceCode ?? context.getSourceCode(); + + return { + CallExpression(node) { + if (!isFsRenameCall(node)) return; + + // (a) inside a try/catch whose catch handles a transient errno + if (isInsideTransientErrnoTryCatch(node, sourceCode)) return; + + // (b) control-dependent on a Windows platform guard + if (isWindowsExcludedNode(node, sourceCode)) return; + + // Otherwise: unguarded atomic-publish rename — report. + context.report({ node, messageId: 'requireFsOpFallback' }); + }, + }; + }, +}; + +module.exports = rule; diff --git a/eslint.config.mjs b/eslint.config.mjs index 34d1aaf08..0ee756042 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -23,6 +23,7 @@ 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 normalizePathInContent from './eslint-rules/normalize-path-in-content.cjs'; +import requireFsOpFallback from './eslint-rules/require-fs-op-fallback.cjs'; const localPlugin = { rules: { @@ -40,6 +41,7 @@ const localPlugin = { 'no-bare-npm-exec': noBareNpmExec, 'require-userprofile-with-home': requireUserprofileWithHome, 'normalize-path-in-content': normalizePathInContent, + 'require-fs-op-fallback': requireFsOpFallback, }, }; @@ -224,6 +226,36 @@ export default tseslint.config( // excluded; content heuristic tightened to genuine reference/config-dir // markers). See RULESET.CONTENT-PATH-NORMALIZATION in CONTEXT.md. 'local/normalize-path-in-content': 'error', + // ADR-1703 Phase 6: flag an unguarded fs.rename/fs.renameSync (the + // atomic-publish primitive) that lacks a transient-errno fallback + // (EPERM/EBUSY/EACCES retry or a Windows platform guard). See + // DEFECT.WINDOWS-FS-OPS in CONTEXT.md. + 'local/require-fs-op-fallback': 'error', + }, + }, + + // ── bin/install.js + scripts/build-hooks.js — ADR-1703 Phase 6 glob expansion ─ + // The top-level `bin/install.js` (generated installer) and `scripts/build-hooks.js` + // (the build-side atomic-replace helper) are the two production surfaces named by + // DEFECT.WINDOWS-FS-OPS that were NOT covered by the src/**/*.cts / gsd-core/bin/**/*.cjs + // globs (ADR-1703 L124-126). This block brings them under the two production + // portability rules. It deliberately does NOT apply the full js.recommended set — + // bin/install.js is ~12k lines of generated code; the ADR's mandate is the + // portability defect surface, not a broader generated-code style sweep. + { + files: ['bin/install.js', 'scripts/build-hooks.js'], + plugins: { + local: localPlugin, + }, + languageOptions: { + sourceType: 'commonjs', + globals: { + ...globals.node, + }, + }, + rules: { + 'local/normalize-path-in-content': 'error', + 'local/require-fs-op-fallback': 'error', }, }, diff --git a/scripts/ci-test-scope.cjs b/scripts/ci-test-scope.cjs index 53a2f0cde..9c47399b1 100644 --- a/scripts/ci-test-scope.cjs +++ b/scripts/ci-test-scope.cjs @@ -214,17 +214,35 @@ const RULES = [ ], }, { - name: 'configuration', - match: path => ['config', 'configuration', 'model-catalog', 'model-profile'].some(k => path.includes(k)), + name: 'configuration', + match: path => ['config', 'configuration', 'model-catalog', 'model-profile'].some(k => path.includes(k)), + tests: [ + 'tests/config.test.cjs', + 'tests/config-get-default.test.cjs', + 'tests/configuration-migrate-config.test.cjs', + 'tests/model-catalog-runtime-defaults.test.cjs', + 'tests/model-profiles.test.cjs', + ], + }, + { + // ADR-1703 portability lint surface. Editing a rule, the shared vocab/guard + // helpers, or the eslint config that wires them must re-run the rule suites + // + the disable-ban. The disable-ban also scans bin/install.js and + // scripts/build-hooks.js (the Phase 6 glob-expansion surface), so changes + // to those files re-run it too. + name: 'portability lint rules (ADR-1703)', + match: path => path.startsWith('eslint-rules/') || + path === 'eslint.config.mjs' || + path === 'bin/install.js' || + path === 'scripts/build-hooks.js', tests: [ - 'tests/config.test.cjs', - 'tests/config-get-default.test.cjs', - 'tests/configuration-migrate-config.test.cjs', - 'tests/model-catalog-runtime-defaults.test.cjs', - 'tests/model-profiles.test.cjs', + 'tests/portability-rule-disable-ban.test.cjs', + 'tests/portability-vocab-drift.test.cjs', + 'tests/require-fs-op-fallback.rule.test.cjs', + 'tests/normalize-path-in-content.rule.test.cjs', ], }, -]; + ]; function usage() { return [ diff --git a/src/capability-lifecycle.cts b/src/capability-lifecycle.cts index c6a258334..fab97ccbe 100644 --- a/src/capability-lifecycle.cts +++ b/src/capability-lifecycle.cts @@ -83,8 +83,9 @@ const lockMod = require('./capability-lock.cjs') as { _setLockProbes: (probes: Partial<{ isPidAlive: (pid: number) => boolean; getProcessStartTime: (pid: number) => string | null }>) => void; _resetLockProbes: () => void; }; -const { platformWriteSync } = require('./shell-command-projection.cjs') as { +const { platformWriteSync, retryRenameSync } = require('./shell-command-projection.cjs') as { platformWriteSync: (filePath: string, content: string) => void; + retryRenameSync: (fromPath: string, toPath: string) => void; }; // #1463: numeric major.minor.patch comparison for the outdated check (the SAME compare the resolver // and capability list use). -1 (ab). @@ -506,14 +507,14 @@ function promoteStagingToFinal( ? path.join(parent, backupName) // CONC-3: a random nonce in the unnamed-branch backup name prevents same-ms cross-process collision. : path.join(parent, newBackupName(path.basename(finalDir))); - fs.renameSync(finalDir, backupDir); + retryRenameSync(finalDir, backupDir); // DUR-3: fsync the parent dir so the old→backup rename is durable BEFORE the second rename — // a crash here must not lose the backup (the only recovery path for reconcile). fsyncDir(parent); try { - fs.renameSync(stagingDir, finalDir); + retryRenameSync(stagingDir, finalDir); } catch (err) { - try { fs.renameSync(backupDir, finalDir); } catch { /* best-effort restore */ } + try { retryRenameSync(backupDir, finalDir); } catch { /* best-effort restore */ } throw err; } // DUR-3: fsync the parent dir again so the staging→final rename is durable too. @@ -521,7 +522,7 @@ function promoteStagingToFinal( return { backupDir }; } fs.mkdirSync(parent, { recursive: true }); - fs.renameSync(stagingDir, finalDir); + retryRenameSync(stagingDir, finalDir); fsyncDir(parent); // DUR-3: durable fresh-install promotion. return { backupDir: null }; } @@ -1535,8 +1536,8 @@ function reconcileCapabilities(opts: { runtimeDir: string; scope?: 'global' | 'p // - crash after step (a): backup still present + `_pending` still references it → retry. // - crash after step (b): old bundle live at finalDir; only the aside copy leaks → swept. const discard = `${finalDir}.discard-${process.pid}-${Date.now()}-${crypto.randomBytes(4).toString('hex')}`; - if (fs.existsSync(finalDir)) fs.renameSync(finalDir, discard); // (a) set the new dir aside - fs.renameSync(backupDir, finalDir); // (b) restore the old bundle + if (fs.existsSync(finalDir)) retryRenameSync(finalDir, discard); // (a) set the new dir aside + retryRenameSync(backupDir, finalDir); // (b) restore the old bundle fsyncDir(root); // make the restore durable try { fs.rmSync(discard, { recursive: true, force: true }); } catch { /* swept later */ } restored = true; diff --git a/src/capability-lock.cts b/src/capability-lock.cts index c611dd370..bf3cb2ab8 100644 --- a/src/capability-lock.cts +++ b/src/capability-lock.cts @@ -42,12 +42,13 @@ import crypto from 'node:crypto'; const ledgerMod = require('./capability-ledger.cjs') as { readSmallRegularFile: (filePath: string, maxBytes: number) => string | null; }; -const { execTool } = require('./shell-command-projection.cjs') as { +const { execTool, retryRenameSync } = require('./shell-command-projection.cjs') as { execTool: ( program: string, args: string[], opts?: { cwd?: string; env?: Record; timeout?: number }, ) => { exitCode: number; stdout: string; stderr: string; signal: NodeJS.Signals | null; error: Error | null }; + retryRenameSync: (fromPath: string, toPath: string) => void; }; /* eslint-enable @typescript-eslint/no-require-imports */ @@ -494,7 +495,7 @@ function acquireLock(lockPath: string, opts?: { maxAttempts?: number; waitForFre // Steal atomically (only one racer can rename the inode). const stolen = `${lockPath}.stale-${process.pid}-${Date.now()}-${crypto.randomBytes(4).toString('hex')}`; - try { fs.renameSync(lockPath, stolen); } catch { return null; } // another process won the steal + try { retryRenameSync(lockPath, stolen); } catch { return null; } // another process won the steal try { fs.rmSync(stolen, { force: true }); } catch { /* best-effort */ } if (attempt + 1 < maxAttempts) lockBackoff(); } diff --git a/src/capability-source.cts b/src/capability-source.cts index 32c267585..eee1c3023 100644 --- a/src/capability-source.cts +++ b/src/capability-source.cts @@ -34,6 +34,7 @@ const shellSeam = require('./shell-command-projection.cjs') as { execGit: (args: string[], opts?: { cwd?: string; timeout?: number }) => SpawnResult; execNpm: (args: string[], opts?: { cwd?: string; timeout?: number }) => SpawnResult; execTool: (program: string, args: string[], opts?: { cwd?: string; timeout?: number }) => SpawnResult; + retryRenameSync: (fromPath: string, toPath: string) => void; }; // eslint-disable-next-line @typescript-eslint/no-require-imports @@ -752,16 +753,16 @@ function stageValidated(opts: { // lives in capability-lifecycle.cjs and uses promote:false above.) if (fs.existsSync(finalDir)) { const backupDir = `${finalDir}.old-${process.pid}-${Date.now()}`; - fs.renameSync(finalDir, backupDir); + shellSeam.retryRenameSync(finalDir, backupDir); try { - fs.renameSync(stagingDir, finalDir); + shellSeam.retryRenameSync(stagingDir, finalDir); } catch (err) { - try { fs.renameSync(backupDir, finalDir); } catch { /* best-effort restore */ } + try { shellSeam.retryRenameSync(backupDir, finalDir); } catch { /* best-effort restore */ } throw err; } try { fs.rmSync(backupDir, { recursive: true, force: true }); } catch { /* best-effort */ } } else { - fs.renameSync(stagingDir, finalDir); + shellSeam.retryRenameSync(stagingDir, finalDir); } const version = typeof cap['version'] === 'string' ? cap['version'] : ''; diff --git a/src/installer-migrations.cts b/src/installer-migrations.cts index 95105cd5b..bbc82f355 100644 --- a/src/installer-migrations.cts +++ b/src/installer-migrations.cts @@ -16,7 +16,7 @@ import { type MigrationRecord, type MigrationAction, } from './installer-migration-authoring.cjs'; -import { platformWriteSync } from './shell-command-projection.cjs'; +import { platformWriteSync, retryRenameSync } from './shell-command-projection.cjs'; import { realClock, type Clock } from './clock.cjs'; const MANIFEST_NAME = 'gsd-file-manifest.json'; @@ -105,7 +105,7 @@ function atomicWriteInstallState(configDir: string, content: string): void { const tmpPath = `${filePath}.tmp-${process.pid}-${Date.now()}`; try { fs.writeFileSync(tmpPath, content, 'utf8'); - fs.renameSync(tmpPath, filePath); + retryRenameSync(tmpPath, filePath); } catch (error) { try { fs.rmSync(tmpPath, { force: true }); } catch { /* best-effort */ } throw error; diff --git a/src/milestone.cts b/src/milestone.cts index 9bf81cc05..2b8e83f9e 100644 --- a/src/milestone.cts +++ b/src/milestone.cts @@ -14,7 +14,7 @@ import planningWorkspace = require('./planning-workspace.cjs'); import frontmatterMod = require('./frontmatter.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports -- state.cjs is an export= CommonJS module import stateMod = require('./state.cjs'); -import { platformWriteSync, platformEnsureDir, execGit } from './shell-command-projection.cjs'; +import { platformWriteSync, platformEnsureDir, execGit, retryRenameSync } from './shell-command-projection.cjs'; import { formatGsdSlash, resolveRuntime } from './runtime-slash.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import ioMod = require('./io.cjs'); @@ -283,7 +283,7 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo // Archive audit file if exists const auditFile = path.join(cwd, '.planning', `${version}-MILESTONE-AUDIT.md`); if (fs.existsSync(auditFile)) { - fs.renameSync(auditFile, path.join(archiveDir, `${version}-MILESTONE-AUDIT.md`)); + retryRenameSync(auditFile, path.join(archiveDir, `${version}-MILESTONE-AUDIT.md`)); } // Create/append MILESTONES.md entry @@ -364,7 +364,7 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo let archivedCount = 0; for (const dir of phaseDirNames) { if (!isDirInMilestone(dir)) continue; - fs.renameSync(path.join(phasesDir, dir), path.join(phaseArchiveDir, dir)); + retryRenameSync(path.join(phasesDir, dir), path.join(phaseArchiveDir, dir)); archivedCount++; } phasesArchived = archivedCount > 0; diff --git a/src/phase.cts b/src/phase.cts index 4e0a3ae4a..3e95e08f6 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -49,7 +49,7 @@ import planningWorkspace = require('./planning-workspace.cjs'); import frontmatterMod = require('./frontmatter.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports -- state.cjs is an export= CommonJS module import stateMod = require('./state.cjs'); -import { platformWriteSync, platformReadSync, platformEnsureDir } from './shell-command-projection.cjs'; +import { platformWriteSync, platformReadSync, platformEnsureDir, retryRenameSync } from './shell-command-projection.cjs'; import { formatGsdSlash, resolveRuntime } from './runtime-slash.cjs'; import { deriveProgressFromRoadmap, clampPercent } from './phase-lifecycle.cjs'; import { realClock } from './clock.cjs'; @@ -1068,12 +1068,12 @@ function renameDecimalPhases( const oldPhaseId = `${baseInt}.${item.oldDecimal}`; const newPhaseId = `${baseInt}.${newDecimal}`; const newDirName = `${item.prefix}.${newDecimal}-${item.slug}`; - fs.renameSync(path.join(phasesDir, item.dir), path.join(phasesDir, newDirName)); + retryRenameSync(path.join(phasesDir, item.dir), path.join(phasesDir, newDirName)); renamedDirs.push({ from: item.dir, to: newDirName }); for (const f of fs.readdirSync(path.join(phasesDir, newDirName))) { if (f.includes(oldPhaseId)) { const newFileName = f.replace(oldPhaseId, newPhaseId); - fs.renameSync( + retryRenameSync( path.join(phasesDir, newDirName, f), path.join(phasesDir, newDirName, newFileName), ); @@ -1120,12 +1120,12 @@ function renameIntegerPhases( const oldPrefix = `${oldPadded}${letterSuffix}${decimalSuffix}`; const newPrefix = `${newPadded}${letterSuffix}${decimalSuffix}`; const newDirName = `${newPrefix}-${item.slug}`; - fs.renameSync(path.join(phasesDir, item.dir), path.join(phasesDir, newDirName)); + retryRenameSync(path.join(phasesDir, item.dir), path.join(phasesDir, newDirName)); renamedDirs.push({ from: item.dir, to: newDirName }); for (const f of fs.readdirSync(path.join(phasesDir, newDirName))) { if (f.startsWith(oldPrefix)) { const newFileName = newPrefix + f.slice(oldPrefix.length); - fs.renameSync( + retryRenameSync( path.join(phasesDir, newDirName, f), path.join(phasesDir, newDirName, newFileName), ); diff --git a/src/planning-workspace.cts b/src/planning-workspace.cts index b86147eb4..d3017d9b3 100644 --- a/src/planning-workspace.cts +++ b/src/planning-workspace.cts @@ -15,7 +15,7 @@ import fs from 'node:fs'; import path from 'node:path'; -import { platformEnsureDir } from './shell-command-projection.cjs'; +import { platformEnsureDir, retryRenameSync } from './shell-command-projection.cjs'; import { realClock } from './clock.cjs'; import type { Clock } from './clock.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports @@ -276,7 +276,7 @@ function withPlanningLock(cwd: string, fn: () => T, clock?: Clock): T { // we must NOT fall through to a delete — back off and retry the create. const stolen = lockPath + '.stale-' + process.pid + '-' + clock.now() + '-' + (_planningStealSeq++); let renamed = false; - try { fs.renameSync(lockPath, stolen); renamed = true; } catch { /* another racer won */ } + try { retryRenameSync(lockPath, stolen); renamed = true; } catch { /* another racer won */ } if (renamed) { try { fs.rmSync(stolen, { force: true }); } catch { /* best-effort */ } continue; // dead/garbage/expired holder freed — retry immediately to grab it. diff --git a/src/roadmap-upgrade.cts b/src/roadmap-upgrade.cts index 76632d293..3487a0334 100644 --- a/src/roadmap-upgrade.cts +++ b/src/roadmap-upgrade.cts @@ -10,6 +10,7 @@ import fs from 'node:fs'; import path from 'node:path'; import { execSync } from 'node:child_process'; +import { retryRenameSync } from './shell-command-projection.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import planningWorkspace = require('./planning-workspace.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports @@ -523,7 +524,7 @@ function applyMigration(cwd: string, plan: MigrationPlan, options: { dryRun?: bo const oldPath = path.join(phasesDir, phaseEntry.oldDir); const newPath = path.join(phasesDir, phaseEntry.newDir); if (fs.existsSync(oldPath)) { - fs.renameSync(oldPath, newPath); + retryRenameSync(oldPath, newPath); performedRenames.push({ oldPath, newPath }); renamedDirs.push(`${phaseEntry.oldDir} → ${phaseEntry.newDir}`); } @@ -597,7 +598,7 @@ function applyMigration(cwd: string, plan: MigrationPlan, options: { dryRun?: bo for (let i = performedRenames.length - 1; i >= 0; i--) { const { oldPath, newPath } = performedRenames[i]; try { - if (fs.existsSync(newPath)) fs.renameSync(newPath, oldPath); + if (fs.existsSync(newPath)) retryRenameSync(newPath, oldPath); } catch { /* best-effort */ } } for (const [filePath, backup] of fileBackups) { diff --git a/src/runtime-hooks-surface.cts b/src/runtime-hooks-surface.cts index 3497f18ed..b98a5eacf 100644 --- a/src/runtime-hooks-surface.cts +++ b/src/runtime-hooks-surface.cts @@ -110,7 +110,7 @@ function atomicWriteFileSync(target: string, data: string, options: fs.WriteFile __atomicWrittenTmps.add(tmp); try { fs.writeFileSync(tmp, data, options); - fs.renameSync(tmp, target); + shellCmdProjection.retryRenameSync(tmp, target); // Successful rename: the tmp path no longer exists, but leave it in the // Set so _cleanTmpFiles can recognise it as installer-owned if it somehow // lingers (e.g. a rename succeeded but left a stale entry on some FS). diff --git a/src/shell-command-projection.cts b/src/shell-command-projection.cts index ca762b3ed..3397bb149 100644 --- a/src/shell-command-projection.cts +++ b/src/shell-command-projection.cts @@ -596,6 +596,21 @@ function atomicRenameWithRetry(tmpPath: string, filePath: string): NodeJS.ErrnoE return renameErr; } +/** + * Drop-in replacement for `fs.renameSync(from, to)` that retries the transient + * Windows lock errnos (EPERM/EBUSY/EACCES — see DEFECT.WINDOWS-FS-OPS) a bounded + * number of times with a short backoff before rethrowing the final error. + * + * Idempotent on POSIX (the transient errnos do not occur), so callers retain + * identical semantics on macOS/Linux while gaining resilience on Windows where + * an antivirus scanner, indexer, or concurrent reader may briefly hold the + * target open. Enforced by local/require-fs-op-fallback (ADR-1703 Phase 6). + */ +export function retryRenameSync(fromPath: string, toPath: string): void { + const err = atomicRenameWithRetry(fromPath, toPath); + if (err !== null) throw err; +} + export function platformWriteSync(filePath: string, content: string, opts: { encoding?: BufferEncoding } = {}): void { const { content: normalized, encoding } = normalizeContent(filePath, content, opts); fs.mkdirSync(path.dirname(filePath), { recursive: true }); diff --git a/src/state.cts b/src/state.cts index f35b56879..ed072f0a1 100644 --- a/src/state.cts +++ b/src/state.cts @@ -20,7 +20,7 @@ const { escapeRegex, normalizePhaseName, extractPhaseToken } = phaseIdMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import roadmapParserMod = require('./roadmap-parser.cjs'); const { getMilestoneInfo, getMilestonePhaseFilter, extractCurrentMilestone } = roadmapParserMod; -import { platformWriteSync, platformReadSync, platformEnsureDir } from './shell-command-projection.cjs'; +import { platformWriteSync, platformReadSync, platformEnsureDir, retryRenameSync } from './shell-command-projection.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import planningWorkspace = require('./planning-workspace.cjs'); const { planningDir, planningPaths } = planningWorkspace; @@ -1903,7 +1903,7 @@ function acquireStateLock(statePath: string, clock?: StateLockClock): string { // we must NOT fall through to a delete — back off and retry the create. const stolen = lockPath + '.stale-' + process.pid + '-' + clock.now() + '-' + (_stateStealSeq++); let renamed = false; - try { fs.renameSync(lockPath, stolen); renamed = true; } catch { /* another racer won */ } + try { retryRenameSync(lockPath, stolen); renamed = true; } catch { /* another racer won */ } if (renamed) { try { fs.rmSync(stolen, { force: true }); } catch { /* best-effort */ } // Successful steal — retry immediately to grab the just-freed lock. diff --git a/src/workstream.cts b/src/workstream.cts index 8e9bd5be4..2ec04089c 100644 --- a/src/workstream.cts +++ b/src/workstream.cts @@ -23,7 +23,7 @@ const { toPosixPath, generateSlugInternal } = coreUtils; // eslint-disable-next-line @typescript-eslint/no-require-imports import roadmapParser = require('./roadmap-parser.cjs'); const { getMilestoneInfo } = roadmapParser; -import { platformWriteSync, platformEnsureDir } from './shell-command-projection.cjs'; +import { platformWriteSync, platformEnsureDir, retryRenameSync } from './shell-command-projection.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import planningWorkspace = require('./planning-workspace.cjs'); const { planningRoot, setActiveWorkstream, getActiveWorkstream } = planningWorkspace; @@ -92,13 +92,13 @@ function migrateToWorkstreams(cwd: string, workstreamName: string): MigrateResul const src = path.join(baseDir, item.name); if (fs.existsSync(src)) { const dest = path.join(wsDir, item.name); - fs.renameSync(src, dest); + retryRenameSync(src, dest); filesMoved.push(item.name); } } } catch (err) { for (const name of filesMoved) { - try { fs.renameSync(path.join(wsDir, name), path.join(baseDir, name)); } catch { /* ignore */ } + try { retryRenameSync(path.join(wsDir, name), path.join(baseDir, name)); } catch { /* ignore */ } } try { fs.rmSync(wsDir, { recursive: true }); } catch { /* ignore */ } try { fs.rmdirSync(path.join(baseDir, 'workstreams')); } catch { /* ignore */ } @@ -310,12 +310,12 @@ function cmdWorkstreamComplete(cwd: string, name: string | null | undefined, opt try { const entries = fs.readdirSync(wsDir, { withFileTypes: true }); for (const entry of entries) { - fs.renameSync(path.join(wsDir, entry.name), path.join(archivePath, entry.name)); + retryRenameSync(path.join(wsDir, entry.name), path.join(archivePath, entry.name)); filesMoved.push(entry.name); } } catch (err) { for (const fname of filesMoved) { - try { fs.renameSync(path.join(archivePath, fname), path.join(wsDir, fname)); } catch { /* ignore */ } + try { retryRenameSync(path.join(archivePath, fname), path.join(wsDir, fname)); } catch { /* ignore */ } } try { fs.rmSync(archivePath, { recursive: true }); } catch { /* ignore */ } if (active === name) setActiveWorkstream(cwd, name!); diff --git a/tests/portability-rule-disable-ban.test.cjs b/tests/portability-rule-disable-ban.test.cjs index 29c6c68e1..82652cce8 100644 --- a/tests/portability-rule-disable-ban.test.cjs +++ b/tests/portability-rule-disable-ban.test.cjs @@ -41,6 +41,9 @@ const PROTECTED_RULES = [ 'require-userprofile-with-home', // ADR-1703 Phase 5 rule (issue #1733) — applies to src/**/*.cts (production sources) 'normalize-path-in-content', + // 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', ]; // ── Detect disable directives via the comment text ─────────────────────────── @@ -92,6 +95,10 @@ function classifyComment(commentValue) { // protect normalize-path-in-content, which applies to // src/**/*.cts; an eslint-disable there would bypass the // production rule entirely) +// - bin/install.js + scripts/build-hooks.js — the ADR-1703 Phase 6 glob expansion +// surface (DEFECT.WINDOWS-FS-OPS); an eslint-disable in the +// generated installer or build-side atomic-replace helper +// would bypass require-fs-op-fallback / normalize-path-in-content const SELF_ABS = __filename; @@ -102,7 +109,12 @@ function collectTestFiles() { .filter(absPath => absPath !== SELF_ABS); const srcFiles = globSync('src/**/*.cts', { cwd: root }) .map(rel => path.join(root, rel)); - return [...testFiles, ...srcFiles]; + // ADR-1703 Phase 6: the two production portability-rule surfaces outside src/ + tests/. + const prodExtra = [ + path.join(root, 'bin', 'install.js'), + path.join(root, 'scripts', 'build-hooks.js'), + ].filter(absPath => fs.existsSync(absPath)); + return [...testFiles, ...srcFiles, ...prodExtra]; } // ── Scan ────────────────────────────────────────────────────────────────────── @@ -115,6 +127,12 @@ function scanFile(absPath) { throw new Error(`Could not read ${absPath}: ${err.message}`); } + // bin/install.js (generated installer) starts with a `#!/usr/bin/env node` shebang + // that espree cannot parse. Rewrite the leading `#!` to `//` so it becomes a valid + // line comment — this preserves byte length and line numbers so any reported + // directive stays at the correct source line. + if (src.startsWith('#!')) src = '//' + src.slice(2); + // .cts files use TypeScript syntax — use @typescript-eslint/typescript-estree. // .cjs files use plain JS — use espree (the original parser). const isCts = absPath.endsWith('.cts'); diff --git a/tests/require-fs-op-fallback.rule.test.cjs b/tests/require-fs-op-fallback.rule.test.cjs new file mode 100644 index 000000000..35d2952ed --- /dev/null +++ b/tests/require-fs-op-fallback.rule.test.cjs @@ -0,0 +1,368 @@ +'use strict'; + +/** + * require-fs-op-fallback.rule.test.cjs + * + * RuleTester unit tests for the local/require-fs-op-fallback ESLint rule. + * + * Rule: flag a bare fs.rename / fs.renameSync call (the atomic-publish + * primitive named first in DEFECT.WINDOWS-FS-OPS.symptom) that is NOT either: + * (a) inside a try/catch (the NEAREST catching try) whose catch handler BOTH + * references a transient errno ('EPERM' / 'EBUSY' / 'EACCES', literally + * or via a *RETRY_ERRNOS set) AND carries a retry signal (a loop + * `continue` backedge or a `return ` delegation — NOT a bare + * rethrow: the cure is retry, not just errno recognition), OR + * (b) control-dependent on a Windows platform guard + * (process.platform !== 'win32' / early-return — isWindowsExcludedNode). + * + * copyFile / unlink are deliberately NOT flagged: per the defect's own + * .fix-forward ("catch EPERM/EBUSY/EACCES, fall back to copy + unlink with + * retry") they are the FALLBACK PRIMITIVES, not separate defect sites, and + * unlink has ~30 intentional best-effort try/catch-swallow cleanup sites that + * would be a FP minefield. See the issue #1740 scope note. + * + * DEFECT category: DEFECT.WINDOWS-FS-OPS + * + * INVALID (violation expected): + * - bare fs.renameSync(tmp, target) — no try/catch, no guard + * - fs.renameSync inside try/catch (e) {} — silent swallow, no errno ref + * - fs.renameSync inside try/catch that cleans up + rethrows, no errno ref + * (the atomicWriteFileSync / atomicWriteInstallState shape — the real bug) + * - bare fs.rename(...) async + * - fs.renameSync inside try/catch whose catch checks errno but only RETHROWS + * (HIGH-1 from codex review: errno reference alone is insufficient — no retry) + * - fs.renameSync inside an INNER try whose catch swallows, even with an OUTER + * try whose catch handles EPERM (HIGH-2: outer catch is unreachable) + * + * VALID (no violation): + * - fs.renameSync inside a retry loop whose catch checks errno + `continue` + * - fs.renameSync inside try/catch whose catch references RENAME_RETRY_ERRNOS set + * - fs.renameSync inside try/catch with switch(err.code) + return retry() (delegation) + * - fs.renameSync inside if (process.platform !== 'win32') { ... } + * - fs.renameSync after early-return guard / hoisted isWindows boolean + * - fs.renameSync inside a try-finally, protected by the NEXT enclosing catching try + * - fs.copyFileSync / fs.unlinkSync — NOT flagged (out of scope — fallback primitives) + * - fs.readFileSync / fs.writeFileSync — NOT flagged (not rename) + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const { RuleTester } = require('eslint'); + +const requireFsOpFallback = require('../eslint-rules/require-fs-op-fallback.cjs'); + +const ruleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: 'commonjs', + }, +}); + +// ─── module shape ───────────────────────────────────────────────────────────── + +describe('require-fs-op-fallback rule module', () => { + test('exports meta and create', () => { + assert.strictEqual(typeof requireFsOpFallback.meta, 'object'); + assert.strictEqual(typeof requireFsOpFallback.create, 'function'); + assert.strictEqual(requireFsOpFallback.meta.type, 'problem'); + assert.ok(requireFsOpFallback.meta.messages.requireFsOpFallback); + }); +}); + +// ─── INVALID cases (violation expected) ─────────────────────────────────────── + +describe('require-fs-op-fallback invalid cases', () => { + test('invalid: bare fs.renameSync with no try/catch and no guard', () => { + // The canonical atomic-publish defect: a reader holding the target open + // makes renameSync throw EPERM/EBUSY on Windows, which propagates unhandled. + ruleTester.run('require-fs-op-fallback', requireFsOpFallback, { + valid: [], + invalid: [ + { + code: `function publish(tmp, target) { + fs.renameSync(tmp, target); +}`, + errors: [{ messageId: 'requireFsOpFallback' }], + }, + ], + }); + }); + + test('invalid: fs.renameSync inside try/catch (e) {} — silent swallow, no errno ref', () => { + // "never silently swallow" — the defect fix-forward explicitly forbids this. + ruleTester.run('require-fs-op-fallback', requireFsOpFallback, { + valid: [], + invalid: [ + { + code: `function publish(tmp, target) { + try { fs.renameSync(tmp, target); } catch (e) {} +}`, + errors: [{ messageId: 'requireFsOpFallback' }], + }, + ], + }); + }); + + test('invalid: fs.renameSync inside try/catch that cleans up + rethrows, no errno ref (atomicWriteFileSync shape)', () => { + // This is the real production bug: the catch handles a write-failure cleanup + // path but does NOT retry the transient Windows lock — EPERM/EBUSY throws + // immediately without the established RENAME_RETRY_ERRNOS backoff. + ruleTester.run('require-fs-op-fallback', requireFsOpFallback, { + valid: [], + invalid: [ + { + code: `function atomicWriteFileSync(target, data) { + const tmp = target + '.tmp'; + try { + fs.writeFileSync(tmp, data); + fs.renameSync(tmp, target); + } catch (e) { + try { fs.rmSync(tmp, { force: true }); } catch { /* ignore */ } + throw e; + } +}`, + errors: [{ messageId: 'requireFsOpFallback' }], + }, + ], + }); + }); + + test('invalid: bare fs.rename(...) async', () => { + ruleTester.run('require-fs-op-fallback', requireFsOpFallback, { + valid: [], + invalid: [ + { + code: `function publishAsync(tmp, target, cb) { + fs.rename(tmp, target, cb); +}`, + errors: [{ messageId: 'requireFsOpFallback' }], + }, + ], + }); + }); + + test('invalid: fs.renameSync inside try/catch whose catch checks errno but only RETHROWS (no retry)', () => { + // HIGH-1 (codex review): referencing the errno is not enough — the defect's + // cure is retry/fallback, not just recognition. A catch that checks the + // errno and rethrows (no continue / no delegation) still fails on Windows + // transient locks, so it is a violation. + ruleTester.run('require-fs-op-fallback', requireFsOpFallback, { + valid: [], + invalid: [ + { + code: `function publish(tmp, target) { + try { + fs.renameSync(tmp, target); + } catch (e) { + if (e.code === 'EPERM') throw e; + throw e; + } +}`, + errors: [{ messageId: 'requireFsOpFallback' }], + }, + ], + }); + }); + + test('invalid: fs.renameSync inside try/catch whose catch references an UNRELATED errno (ENOENT) only', () => { + // A catch handling ENOENT does NOT protect against the EPERM/EBUSY/EACCES + // transient-lock family — still a violation. + ruleTester.run('require-fs-op-fallback', requireFsOpFallback, { + valid: [], + invalid: [ + { + code: `function publish(tmp, target) { + try { + fs.renameSync(tmp, target); + } catch (e) { + if (e.code === 'ENOENT') return; + throw e; + } +}`, + errors: [{ messageId: 'requireFsOpFallback' }], + }, + ], + }); + }); +}); + +// ─── VALID cases (no violation) ─────────────────────────────────────────────── + +describe('require-fs-op-fallback valid cases', () => { + test('valid: fs.renameSync inside a retry loop whose catch checks err.code === "EPERM" and continues', () => { + // The minimal compliant shape: errno check + loop backedge (continue). + ruleTester.run('require-fs-op-fallback', requireFsOpFallback, { + valid: [ + `function publish(tmp, target) { + for (let attempt = 1; attempt <= 3; attempt++) { + try { + fs.renameSync(tmp, target); + return; + } catch (e) { + if (e.code === 'EPERM') { backoff(); continue; } + throw e; + } + } +}`, + ], + invalid: [], + }); + }); + + test('valid: fs.renameSync inside try/catch whose catch references RENAME_RETRY_ERRNOS set (canonical pattern)', () => { + ruleTester.run('require-fs-op-fallback', requireFsOpFallback, { + valid: [ + `const RENAME_RETRY_ERRNOS = new Set(['EPERM', 'EBUSY', 'EACCES']); +function atomicRenameWithRetry(tmpPath, filePath) { + for (let attempt = 1; attempt <= 3; attempt++) { + try { + fs.renameSync(tmpPath, filePath); + return null; + } catch (err) { + if (attempt < 3 && RENAME_RETRY_ERRNOS.has(err.code)) { + backoff(); + continue; + } + break; + } + } +}`, + ], + invalid: [], + }); + }); + + test('valid: fs.renameSync inside try/catch with switch(err.code) casing EBUSY and EACCES, delegating via return retry()', () => { + // The `return retry()` is a ReturnStatement-with-CallExpression — a retry + // signal (delegation to a helper that performs its own bounded retry). + ruleTester.run('require-fs-op-fallback', requireFsOpFallback, { + valid: [ + `function publish(tmp, target) { + try { + fs.renameSync(tmp, target); + } catch (e) { + switch (e.code) { + case 'EBUSY': + case 'EACCES': + return retry(); + } + throw e; + } +}`, + ], + invalid: [], + }); + }); + + test('valid: fs.renameSync inside if (process.platform !== "win32") block (platform guard)', () => { + ruleTester.run('require-fs-op-fallback', requireFsOpFallback, { + valid: [ + `function publish(tmp, target) { + if (process.platform !== 'win32') { + fs.renameSync(tmp, target); + } +}`, + ], + invalid: [], + }); + }); + + test('valid: fs.renameSync after early-return Windows guard', () => { + ruleTester.run('require-fs-op-fallback', requireFsOpFallback, { + valid: [ + `function publish(tmp, target) { + if (process.platform === 'win32') return; + fs.renameSync(tmp, target); +}`, + ], + invalid: [], + }); + }); + + test('valid: fs.copyFileSync and fs.unlinkSync are NOT flagged (out of scope — fallback primitives)', () => { + ruleTester.run('require-fs-op-fallback', requireFsOpFallback, { + valid: [ + `function stage(src, dest) { fs.copyFileSync(src, dest); }`, + `function cleanup(p) { fs.unlinkSync(p); }`, + `function cleanupSwallow(p) { try { fs.unlinkSync(p); } catch (_) {} }`, + ], + invalid: [], + }); + }); + + test('valid: fs.readFileSync / fs.writeFileSync are NOT flagged (not rename)', () => { + ruleTester.run('require-fs-op-fallback', requireFsOpFallback, { + valid: [ + `function read(p) { return fs.readFileSync(p, 'utf8'); }`, + `function write(p, d) { fs.writeFileSync(p, d); }`, + ], + invalid: [], + }); + }); + + test('invalid: fs.renameSync inside an INNER try whose catch swallows, with an OUTER try whose catch handles EPERM (HIGH-2)', () => { + // HIGH-2 (codex review): the inner catch intercepts the rename error + // (swallows it), so the outer errno-handling catch is UNREACHABLE for that + // failure. Walking the full ancestor chain and treating the outer catch as + // protective was a false negative. The nearest catching try's handler is + // authoritative; since it swallows without retry, this is a violation. + ruleTester.run('require-fs-op-fallback', requireFsOpFallback, { + valid: [], + invalid: [ + { + code: `function publish(tmp, target) { + try { + try { + fs.renameSync(tmp, target); + } catch (inner) { + // inner cleanup, swallows the rename error — no retry + } + } catch (e) { + if (e.code === 'EPERM') { return retry(); } + } +}`, + errors: [{ messageId: 'requireFsOpFallback' }], + }, + ], + }); + }); + + test('valid: try-finally (no catch) is skipped — rename protected by the NEXT enclosing catching try', () => { + // A try with only a finally does not intercept the rename error, so the + // climb continues to the next enclosing TryStatement with a handler. + ruleTester.run('require-fs-op-fallback', requireFsOpFallback, { + valid: [ + `function publish(tmp, target) { + for (let attempt = 1; attempt <= 3; attempt++) { + try { + try { + fs.renameSync(tmp, target); + } finally { + meter.tick(); + } + return; + } catch (e) { + if (e.code === 'EPERM') { continue; } + throw e; + } + } +}`, + ], + invalid: [], + }); + }); + + test('valid: hoisted isWindows boolean guard consumed by if (!isWindows)', () => { + ruleTester.run('require-fs-op-fallback', requireFsOpFallback, { + valid: [ + `function publish(tmp, target) { + const isWindows = process.platform === 'win32'; + if (!isWindows) { + fs.renameSync(tmp, target); + } +}`, + ], + invalid: [], + }); + }); +});