* 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>
369 lines
13 KiB
JavaScript
369 lines
13 KiB
JavaScript
'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: [],
|
|
});
|
|
});
|
|
});
|