fix(eslint-factory): require-fs-sync-try-catch catches destructured and aliased fs bindings - #44240
Conversation
…nd aliased fs bindings
Resolves the false negative where `const { appendFileSync } = require("fs")`
followed by `appendFileSync(...)` was not flagged. Adds scope analysis to
detect two new binding shapes:
1. Destructured: `const { appendFileSync } = require("fs"/"node:fs")`
and renamed form: `const { appendFileSync: alias } = require("fs")`
2. Member-expression alias: `const alias = fs.appendFileSync`
where `fs` is verified via scope analysis to be require("fs")
Also adds:
- `isRequireFsCall()` helper to check require("fs") / require("node:fs")
- `getFsSyncMethodFromProperty()` helper extracted from the CallExpression handler
- `isIdentifierBoundToFsModule()` helper to prevent false positives in Shape 2
- Valid tests: destructured/aliased bindings inside try blocks
- Invalid tests: bare destructured/aliased calls (including the live FN pattern)
Closes #44218
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR strengthens the require-fs-sync-try-catch ESLint rule so it also flags unguarded synchronous fs I/O calls when the fs methods are invoked via destructured bindings (including aliased destructuring) or via member-expression aliases (e.g., const write = fs.writeFileSync; write(...)), and updates tests to cover these new binding forms.
Changes:
- Extend fs-module detection to handle both
require("fs")andrequire("node:fs"). - Add scope-based resolution for Identifier callees to detect destructured/aliased fs sync method bindings and member-alias bindings.
- Expand the test suite with valid/invalid cases covering the new binding shapes.
Show a summary per file
| File | Description |
|---|---|
| eslint-factory/src/rules/require-fs-sync-try-catch.ts | Adds fs-module specifier handling and scope-based identifier resolution for destructured and aliased fs sync method calls. |
| eslint-factory/src/rules/require-fs-sync-try-catch.test.ts | Updates and expands tests to validate the new detection paths for destructured and aliased bindings. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Low
| // Only matches when the `fs` identifier is itself bound to require("fs") / require("node:fs"). | ||
| if (declarator.id.type === AST_NODE_TYPES.Identifier && declarator.init?.type === AST_NODE_TYPES.MemberExpression) { | ||
| const init = declarator.init; | ||
| if (init.object.type === AST_NODE_TYPES.Identifier && isIdentifierBoundToFsModule(init.object.name, node)) { |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #44240 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (6 tests)
Coverage Highlights
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: require-fs-sync-try-catch — destructured and aliased fs bindings
The scope-walking approach and all four test cases are well-structured. One correctness bug found.
Blocking issue — isIdentifierBoundToFsModule scope anchor
In resolveFsSyncMethodFromIdentifier (Shape 2), isIdentifierBoundToFsModule(init.object.name, node) passes the call site as the scope anchor. If fs is shadowed in a nested scope that encloses only the call (not the alias declaration), the lookup will find the shadow and return false, producing a false negative.
The anchor should be init.object (the fs identifier node in the alias initializer), so scope resolution is pinned to the declaration site.
Example false negative
const fs = require('fs');
const append = fs.appendFileSync; // correctly bound to module
function inner() {
const fs = {}; // shadows outer fs
append(path, data); // call site — scope walk from here finds {} → rule misses it
}See inline comment on line 128 for the one-line fix.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 82.1 AIC · ⌖ 7.75 AIC · ⊞ 4.8K
| // Only matches when the `fs` identifier is itself bound to require("fs") / require("node:fs"). | ||
| if (declarator.id.type === AST_NODE_TYPES.Identifier && declarator.init?.type === AST_NODE_TYPES.MemberExpression) { | ||
| const init = declarator.init; | ||
| if (init.object.type === AST_NODE_TYPES.Identifier && isIdentifierBoundToFsModule(init.object.name, node)) { |
There was a problem hiding this comment.
Bug (false negative): scope anchor should be the declaration site, not the call site.
isIdentifierBoundToFsModule is called with node (the CallExpression at the call site), but it should be anchored at the declaration — either def.node (the VariableDeclarator) or init.object (the fs identifier node in the initializer).
As written, if fs is shadowed between the alias declaration and the call site, the scope walk finds the shadow (not the real require("fs")) and silently skips the flag:
const fs = require("fs");
const append = fs.appendFileSync; // fs === module ✓
function inner() {
const fs = {}; // shadows outer fs
append(path, data); // call site — scope finds {} → isIdentifierBoundToFsModule returns false → false negative
}Suggested fix — use init.object as the scope anchor so lookup is resolved at the point of the declaration:
if (init.object.type === AST_NODE_TYPES.Identifier && isIdentifierBoundToFsModule(init.object.name, init.object)) {@copilot please address this.
|
@copilot run pr-finisher skill |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two correctness gaps and three test-quality improvements.
📋 Key Themes & Highlights
Key Issues
- Incomplete fix (correctness): The
MemberExpressionarm (line 187) still hardcodescallee.object.name !== "fs", leavingconst fileSystem = require("fs"); fileSystem.readFileSync(path)as a live false negative — inconsistent with the newIdentifierarm's scope-aware approach. This is the highest-priority item. - Missing false-positive test: No test covers a same-named method bound to a non-fs library (e.g.
const { appendFileSync } = require("custom-logger")). A future regression here would be silent. - Scope-walk edge case: When a variable exists with no
Variabledefs (e.g. it's a parameter or function name), the outer scope is searched — this is intentional but undocumented and untested.
Positive Highlights
- ✅ Root cause correctly identified and well-described in the PR body
- ✅ New
isRequireFsCall(),resolveFsSyncMethodFromIdentifier(),isIdentifierBoundToFsModule()helpers are well-decomposed and clearly documented - ✅ Live false-negative in
action_setup_otlp.cjs:43is directly addressed with a regression test - ✅ Four binding shapes covered with both valid (in-try) and invalid (bare call) cases
- ✅
getFsSyncMethodFromProperty()extraction is a clean refactor that reduces duplication
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 126.1 AIC · ⌖ 5.89 AIC · ⊞ 6.6K
Comment /matt to run again
| cjsRuleTester.run("require-fs-sync-try-catch", requireFsSyncTryCatchRule, { | ||
| valid: [`const fs = require("fs"); const append = fs.appendFileSync; try { append(p, data); } catch (e) {}`], | ||
| invalid: [], | ||
| }); |
There was a problem hiding this comment.
[/tdd] Missing false-positive edge case: no test verifies that a non-fs identifier with the same name as an fs method is not flagged.
If appendFileSync is destructured from a non-fs library, the scope-walk will return null — but without a test, a future refactor could silently introduce false positives.
💡 Suggested valid test case
it("valid: same-named method bound to non-fs library is not flagged", () => {
cjsRuleTester.run("require-fs-sync-try-catch", requireFsSyncTryCatchRule, {
valid: [
`const { appendFileSync } = require("custom-logger"); appendFileSync(filePath, data);`,
],
invalid: [],
});
});@copilot please address this.
| if (callee.object.type !== AST_NODE_TYPES.Identifier || callee.object.name !== "fs") return; | ||
| if (callee.type === AST_NODE_TYPES.MemberExpression) { | ||
| // Object must be the `fs` identifier (the standard import alias in actions/setup/js). | ||
| if (callee.object.type !== AST_NODE_TYPES.Identifier || callee.object.name !== "fs") return; |
There was a problem hiding this comment.
[/diagnosing-bugs] Inconsistency: the MemberExpression arm still hardcodes callee.object.name !== "fs" instead of using the new isIdentifierBoundToFsModule() helper.
This means const fileSystem = require("fs"); fileSystem.readFileSync(path) is still a false negative — a gap that's inconsistent with the new Identifier arm's scope-aware approach.
💡 Suggested fix
Replace:
if (callee.object.type !== AST_NODE_TYPES.Identifier || callee.object.name !== "fs") return;With:
if (callee.object.type !== AST_NODE_TYPES.Identifier) return;
if (!isIdentifierBoundToFsModule(callee.object.name, node)) return;And add a test:
it("invalid: aliased require('fs') direct method call is flagged", () => {
cjsRuleTester.run("require-fs-sync-try-catch", requireFsSyncTryCatchRule, {
valid: [],
invalid: [{
code: `const fileSystem = require("fs"); fileSystem.readFileSync(path, "utf8");`,
errors: [{ messageId: "requireTryCatch", data: { method: "readFileSync", arg: "path" } }],
}],
});
});@copilot please address this.
| } | ||
| } | ||
| } | ||
| // Variable is locally defined but not as an fs sync method binding — stop searching. |
There was a problem hiding this comment.
[/diagnosing-bugs] Scope-walk stops at the first definition block of a variable regardless of whether it contains any Variable defs. If all defs for an identifier are non-Variable type (e.g. FunctionName, Parameter, ImportBinding), the for loop produces no return and falls through to line 135 — which is correct. But if there are no defs at all (variable.defs.length === 0), the guard on line 104 prevents entry, so we walk up the scope chain, potentially matching an outer scope that has an unrelated binding with the same name.
This is an edge case unlikely in practice, but worth a comment or a test to document the intent.
💡 Low-risk clarifying comment
// If variable is found but has no defs (e.g. global built-in), skip to outer scope.
// This is intentional: we want to walk up to find a require("fs") binding.Alternatively, a test like:
// valid: a globally-named `appendFileSync` that is not from require("fs")
`appendFileSync(filePath, data);` // should NOT be flagged (no binding found at all)@copilot please address this.
| defaultOptions: [], | ||
| create(context) { | ||
| const sourceCode = context.sourceCode; | ||
| type SourceCodeScope = ReturnType<typeof sourceCode.getScope>; |
There was a problem hiding this comment.
[/tdd] The SourceCodeScope type alias is defined inside create() but is not exported or used in the test file. While harmless, it obscures where the type actually comes from and may confuse future readers unfamiliar with the eslint-visitor-keys scope type.
Consider using the more explicit inline type ReturnType<typeof sourceCode.getScope> at both use sites, or extract it to a module-level type alias — keeping it inside create() limits reuse.
@copilot please address this.
| }); | ||
| }); | ||
|
|
||
| it("invalid: destructured fs binding (shorthand) not in try is flagged — live FN in action_setup_otlp.cjs", () => { |
There was a problem hiding this comment.
[/tdd] Test names that encode implementation details ("live FN in action_setup_otlp.cjs") are brittle — if the filename moves the test name becomes stale. The regression motivation is valuable; consider capturing it in a comment inside the test body instead.
💡 Suggested rename
it("invalid: destructured fs binding (shorthand) not in try is flagged", () => {
// Regression: was a live false-negative in action_setup_otlp.cjs:43
cjsRuleTester.run(...);
});This keeps test names stable as specifications while preserving the historical context.
@copilot please address this.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Completed a pr-finisher pass and pushed commit |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
The
require-fs-sync-try-catchrule only matchedfs.method()member-expression calls, silently missing bindings introduced by destructuring or aliasing — including a live unprotectedappendFileSynccall inaction_setup_otlp.cjs:43.Rule changes (
require-fs-sync-try-catch.ts)isRequireFsCall()— recognisesrequire("fs")andrequire("node:fs")resolveFsSyncMethodFromIdentifier()— scope-walks the callee name and resolves two new binding shapes:const { appendFileSync } = require("fs")andconst { appendFileSync: alias } = require("fs")const alias = fs.appendFileSync, gated onisIdentifierBoundToFsModule()to avoid false positives from unrelated objects namedfsgetFsSyncMethodFromProperty()— extracted from the existingCallExpressionhandler and reused for both the directfs.method()and alias pathsThe
CallExpressionhandler now dispatches toresolveFsSyncMethodFromIdentifier()when the callee is anIdentifier.Test changes (
require-fs-sync-try-catch.test.ts)