eslint-factory: consolidate fs-sync try/catch rules and unify ESM/CJS fs binding detection#45964
Conversation
…rules Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Thanks for the ESLint rule consolidation work! The PR has one unchecked item remaining (final validations). Please complete the checklist — run agent-report-progress, secret scan, and parallel validation — before marking ready for review.
|
There was a problem hiding this comment.
Pull request overview
Consolidates filesystem sync-call detection and try/catch analysis across two ESLint rules.
Changes:
- Adds shared filesystem binding resolution and try/catch utilities.
- Refactors both rules to use shared logic.
- Adds ESM import coverage for filesystem sync methods.
Show a summary per file
| File | Description |
|---|---|
try-catch-rule-utils.ts |
Adds shared resolution and boundary utilities. |
require-mkdirsync-try-catch.ts |
Adopts shared rule machinery. |
require-fs-sync-try-catch.ts |
Adopts shared resolver with existing global behavior. |
require-fs-sync-try-catch.test.ts |
Tests ESM named and aliased imports. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Medium
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions detected in src/, lib/, pkg/, internal/, app/, core/, domain/, services/, api/). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Review: Clean Refactor ✅
The consolidation of isInsideTryBlock, findEnclosingStatement, and the fs binding resolution logic into try-catch-rule-utils.ts is well-structured and eliminates the prior drift between the two rules.
Key observations:
- The shared
createFsSyncMethodResolverfactory correctly threadsallowUnboundFsIdentifierfor the originalfs.*global pattern inrequire-fs-sync-try-catch. isIdentifierBoundToFsModulein the new shared implementation gains ESM namespace import detection (viaisFsImportBinding) that the oldrequire-fs-sync-try-catchversion lacked — this is a correctness improvement, not a regression.isRequireFsCallevaluatesnode.type === AST_NODE_TYPES.CallExpressiontwice (once to extractfirstArg, once in the return expression) — minor redundancy, no impact on correctness.- New ESM test cases for
readFileSync,writeFileSync, andappendFileSyncprovide good parity coverage.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 43.8 AIC · ⌖ 5.44 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report
📊 Metrics (20 tests)
📋 Test Classification
✅ Strengths
Verdict✅ Passed. 25% implementation tests (threshold: 30%). No coding guideline violations. Why this score:
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.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — leaving comments, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Small redundancy in
isRequireFsCall: intermediatefirstArg/firstArgValuevariables are computed but the conjunction also accessesnode.arguments[0]andnode.arguments.lengthdirectly — worth a micro-cleanup. - Boolean option flag:
allowUnboundFsIdentifieris the one rough edge in the factory interface; a comment or rename would make the intent clearer at call sites. - Coverage gap:
import * as fs(namespace import) is tested formkdirSyncbut not yet forreadFileSync/writeFileSync/appendFileSyncnow that the resolver is shared.
Positive Highlights
- ✅ Excellent reduction: ~320 lines deleted, duplicated logic eliminated cleanly.
- ✅
createFsSyncMethodResolverfactory is a genuinely deep module — simple interface, rich binding-resolution behaviour. - ✅ Good parity work: ESM named-import and aliased-import tests for all three fs-sync methods are new and correct.
- ✅
require-mkdirsync-try-catchis now a thin wrapper — easy to audit and maintain.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 69.9 AIC · ⌖ 4.74 AIC · ⊞ 6.7K
Comment /matt to run again
| node.arguments.length >= 1 && | ||
| node.arguments[0].type === AST_NODE_TYPES.Literal && | ||
| typeof firstArgValue === "string" && | ||
| FS_MODULE_SPECIFIERS.has(firstArgValue) |
There was a problem hiding this comment.
[/codebase-design] isRequireFsCall pre-extracts firstArg/firstArgValue but then re-accesses node.arguments[0] directly in the conjunction — the intermediate variables are redundant and create a subtle inconsistency.
💡 Suggestion
Simplify to a single extraction:
function isRequireFsCall(node: TSESTree.Node | null | undefined): boolean {
if (node?.type !== AST_NODE_TYPES.CallExpression) return false;
const arg = node.arguments[0];
return (
node.callee.type === AST_NODE_TYPES.Identifier &&
node.callee.name === 'require' &&
arg?.type === AST_NODE_TYPES.Literal &&
typeof arg.value === 'string' &&
FS_MODULE_SPECIFIERS.has(arg.value)
);
}@copilot please address this.
|
|
||
| if (callee.type === AST_NODE_TYPES.MemberExpression) { | ||
| if (callee.object.type !== AST_NODE_TYPES.Identifier) return null; | ||
| const canUseUnboundFsIdentifier = options.allowUnboundFsIdentifier === true && callee.object.name === "fs"; |
There was a problem hiding this comment.
[/codebase-design] allowUnboundFsIdentifier is a boolean flag that selectively changes the resolution strategy — this is a classic boolean trap that makes createFsSyncMethodResolver harder to understand at call sites ({ allowUnboundFsIdentifier: true } is a leaky detail).
💡 Suggestion
Consider expressing the two resolution modes as distinct factory signatures or named presets:
// Explicit: callers declare the strategy by name
export function createFsSyncMethodResolver(sourceCode, fsSyncMethods, options?: { allowUnboundFsGlobal?: boolean })Or split into createBoundFsSyncMethodResolver and createUnboundFsSyncMethodResolver. The current name and flag don't communicate why the rule for fs.* global calls is different (legacy CJS global pattern), so a comment at minimum would help.
@copilot please address this.
| valid: [ | ||
| `try { const x = fs.readFileSync(path, "utf8"); } catch (e) {}`, | ||
| `import { readFileSync } from "node:fs"; try { readFileSync(path, "utf8"); } catch (e) {}`, | ||
| `import { writeFileSync as write } from "fs"; try { write(path, data); } catch (e) {}`, |
There was a problem hiding this comment.
[/tdd] The new ESM valid/invalid tests cover named imports (import { readFileSync }) and renamed aliases (import { writeFileSync as write }), but import * as fs from "fs"; fs.readFileSync(...) (namespace import) is not tested here — it is tested for mkdirSync (line 187 in the mkdirSync test file) but not for the fs-sync methods, creating a coverage gap now that both rules share the same resolver.
💡 Suggested test to add
// valid
`import * as fs from "node:fs"; try { fs.readFileSync(path, "utf8"); } catch (e) {}`
// invalid
{ code: `import * as fs from "fs"; fs.readFileSync(p, "utf8");`, errors: [...] }This would confirm namespace-import detection is covered by the shared resolver for all fs-sync methods, not just mkdirSync.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — 2 correctness issues + 3 maintainability concerns
The consolidation is a solid approach and the shared-utility extraction is clean overall. However there are two correctness issues that can cause rules to silently miss violations, plus gaps in test coverage that leave the refactored CJS path unvalidated.
Blocking:
- Raw string
"TryStatement"— the centralizedisInsideTryBlockuses a string literal instead ofAST_NODE_TYPES.TryStatement. Now that both rules funnel through this one path, a typo here silently disables both rules. - Optional-chaining bypass —
fs?.readFileSync(path)wraps its callee inChainExpression, which falls throughresolveFsSyncMethodunreported. This is a trivially exploitable escape hatch from both rules.
Non-blocking but should fix:
3. allowUnboundFsIdentifier asymmetry — undocumented behavioral divergence between the two rules.
4. Missing CJS invalid test cases — refactor added only ESM invalid tests; CJS path regression is undetectable.
5. Dead precomputation in isRequireFsCall — confusing indirection with no benefit.
🔎 Code quality review by PR Code Quality Reviewer · 81 AIC · ⌖ 4.93 AIC · ⊞ 5.6K
Comment /review to run again
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
Raw string literal "TryStatement" instead of AST_NODE_TYPES.TryStatement: if mistyped or the upstream value changes, both rules silently stop recognizing try-blocks, defeating the lint entirely. Every other node-type check in this file uses the enum. Replace with AST_NODE_TYPES.TryStatement.
| if (!canUseUnboundFsIdentifier && !isIdentifierBoundToFsModule(callee.object.name, callee.object)) return null; | ||
| return getFsSyncMethodFromProperty(callee); | ||
| } | ||
|
|
There was a problem hiding this comment.
Optional-chaining calls bypass the rule: fs?.readFileSync(path) produces a ChainExpression wrapping a MemberExpression, so callee.type is never MemberExpression, and the call falls through to return null unreported.
💡 Suggested fix
Unwrap ChainExpression before the type-switch:
return function resolveFsSyncMethod(node: TSESTree.CallExpression): string | null {
let callee: TSESTree.Node = node.callee;
// Unwrap optional chaining: fs?.readFileSync() → callee is ChainExpression
if (callee.type === AST_NODE_TYPES.ChainExpression) {
callee = callee.expression;
}
if (callee.type === AST_NODE_TYPES.MemberExpression) { ... }
if (callee.type === AST_NODE_TYPES.Identifier) { ... }
return null;
};Without this, fs?.readFileSync(path) is a trivial escape hatch from both rules.
| } | ||
|
|
||
| function isRequireFsCall(node: TSESTree.Node | null | undefined): boolean { | ||
| if (!node) return false; |
There was a problem hiding this comment.
Dead precomputation in isRequireFsCall: firstArg and firstArgValue are computed on lines 106–107 but the return expression (lines 109–116) independently re-checks every condition from scratch. The variables add no short-circuit benefit and make the function harder to reason about.
💡 Suggested fix
Remove the early extraction and simplify to a single expression (matching the original pattern):
function isRequireFsCall(node: TSESTree.Node | null | undefined): boolean {
if (!node) return false;
return (
node.type === AST_NODE_TYPES.CallExpression &&
node.callee.type === AST_NODE_TYPES.Identifier &&
node.callee.name === "require" &&
node.arguments.length >= 1 &&
node.arguments[0].type === AST_NODE_TYPES.Literal &&
typeof node.arguments[0].value === "string" &&
FS_MODULE_SPECIFIERS.has(node.arguments[0].value)
);
}|
|
||
| return false; | ||
| } | ||
| const resolveFsSyncMethod = createFsSyncMethodResolver(sourceCode, FS_SYNC_METHODS); |
There was a problem hiding this comment.
Silent behavioral asymmetry with require-fs-sync-try-catch: require-fs-sync-try-catch passes allowUnboundFsIdentifier: true so bare fs.readFileSync() (no import/require) is still flagged. Here, require-mkdirsync-try-catch omits the option entirely, so fs.mkdirSync() with no binding is silently skipped. This divergence is not documented and will surprise users.
💡 Suggested fix
Either add { allowUnboundFsIdentifier: true } to match the sibling rule's behavior, or add an explicit comment explaining why the asymmetry is intentional:
const resolveFsSyncMethod = createFsSyncMethodResolver(sourceCode, FS_SYNC_METHODS, {
allowUnboundFsIdentifier: true, // flag bare fs.mkdirSync() the same as the fs-sync rule
});| @@ -351,6 +355,51 @@ describe("require-fs-sync-try-catch", () => { | |||
| }, | |||
There was a problem hiding this comment.
No new invalid CJS test cases after refactor: the three new invalid entries only exercise ESM import patterns. CJS destructuring (const { readFileSync } = require('fs')) and aliased CJS (const { appendFileSync: append } = require('fs')) are not covered in the new tests. If createFsSyncMethodResolver's CJS path regressed in this refactor, the test suite would not catch it.
💡 Suggested fix
Add at minimum one new invalid CJS case per newly-added method to provide parity coverage:
{
code: `const { readFileSync } = require('fs'); readFileSync(p, 'utf8');`,
errors: [{ messageId: 'requireTryCatch', data: { method: 'readFileSync', arg: 'p' } }],
},
{
code: `const { writeFileSync: write } = require('node:fs'); write(outputPath, data);`,
errors: [{ messageId: 'requireTryCatch', data: { method: 'writeFileSync', arg: 'outputPath' } }],
},|
🎉 This pull request is included in a new release. Release: |
require-fs-sync-try-catchandrequire-mkdirsync-try-catchhad near-duplicate implementations and had drifted:mkdirSynchandled ESM import bindings while other fs-sync methods did not. This change centralizes the shared logic so both rules enforce the same binding-resolution behavior acrossrequire(...)andimport ... from "fs"/"node:fs".Shared fs-sync rule machinery
try-catch-rule-utils.ts.fs.readFileSync(...), computed property access),Rule refactor with behavior preserved
require-fs-sync-try-catchnow delegates call matching to the shared resolver (with existingfs.*global behavior preserved).require-mkdirsync-try-catchnow uses the same resolver path, eliminating duplicated binding logic and keeping its rule-specific diagnostics/suggestions unchanged.Coverage parity for ESM fs-sync imports
require-fs-sync-try-catchtests to cover previously-missed ESM cases for:readFileSyncwriteFileSyncappendFileSyncmkdirSyncESM behavior.Example of newly-covered detection parity: