feat(#1740): require-fs-op-fallback production AST rule + Windows transient-lock retry (Phase 6) (#1742)
* 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 <call>` 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 <review-bot@gsd>
This commit is contained in:
Tom Boucher
@@ -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');
|
||||
|
||||
368
tests/require-fs-op-fallback.rule.test.cjs
Normal file
368
tests/require-fs-op-fallback.rule.test.cjs
Normal file
@@ -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 <call>` 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: [],
|
||||
});
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user