[eslint-miner] eslint: add require-fs-sync-try-catch rule#43994
Conversation
Adds a new custom ESLint rule that requires fs.readFileSync,
fs.writeFileSync, and fs.appendFileSync calls in actions/setup/js
scripts to be wrapped in a try/catch block.
## Motivation
Synchronous fs methods throw on I/O errors (missing file, permission
denied, disk full). An unhandled throw crashes the action without
surfacing a useful, actionable error message in the GitHub Actions log.
Scan of actions/setup/js found 69 real unguarded callsites across 20+
production files — including daily_aic_workflow_helpers.cjs,
detect_agent_errors.cjs, evaluate_outcomes.cjs, and
create_code_scanning_alert.cjs — none of which had surrounding
try/catch protection.
## Rule design
- Name: require-fs-sync-try-catch
- Scope: fs.readFileSync, fs.writeFileSync, fs.appendFileSync only
(other sync methods like mkdirSync, unlinkSync kept out of scope to
minimise FP risk on first iteration)
- Deferred-callback awareness: a surrounding try is only treated as
protective when no deferred-sink boundary (EventEmitter .on,
setTimeout, Promise executor, etc.) has been crossed — matching the
existing require-json-parse-try-catch pattern exactly
- Suggestion: wraps the enclosing statement in a try/catch that
re-throws with { cause: err } to preserve the original stack trace
- False-positive risk: very low — only the 'fs' identifier is matched;
aliased references are intentionally out of scope
## Validation
- npm run build: passes (tsc)
- npx vitest run: 170/170 tests pass across 13 test files
- npm run lint:setup-js: rule fires on 69 real violations in production
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43994 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
Adds a new custom ESLint rule to the eslint-factory plugin to require fs.readFileSync, fs.writeFileSync, and fs.appendFileSync callsites in actions/setup/js scripts to be protected by try/catch, including awareness of deferred callback boundaries.
Changes:
- Implement
require-fs-sync-try-catchrule with suggestions to wrap the enclosing statement intry/catch. - Add Vitest/RuleTester coverage for valid/invalid patterns including deferred-callback cases.
- Register and enable the rule in the plugin entrypoint and the factory’s ESLint config.
Show a summary per file
| File | Description |
|---|---|
| eslint-factory/src/rules/require-fs-sync-try-catch.ts | New rule implementation for detecting unguarded sync fs calls and offering a wrapping suggestion. |
| eslint-factory/src/rules/require-fs-sync-try-catch.test.ts | Test coverage for detection and suggested output across CJS/ESM and deferred-callback boundaries. |
| eslint-factory/src/index.ts | Registers the new rule in the exported plugin rule map. |
| eslint-factory/eslint.config.cjs | Enables the new rule under gh-aw-custom/* as a warning. |
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: 2
- Review effort level: Low
| AST_NODE_TYPES.ExpressionStatement, | ||
| AST_NODE_TYPES.VariableDeclaration, | ||
| AST_NODE_TYPES.ReturnStatement, | ||
| AST_NODE_TYPES.ThrowStatement, |
| function buildTryCatchSuggestion(stmtText: string, indent: string, method: string): string { | ||
| return [ | ||
| "try {", | ||
| `${indent} ${stmtText}`, | ||
| `${indent}} catch (err) {`, |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 92/100 — Excellent
📊 Metrics (11 tests)
Verdict
References: 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.
|
|
🛠️ Agentic Maintenance updated this pull request branch. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on code duplication and a few missing test cases.
📋 Key Themes & Highlights
Key Themes
- Duplication risk (comments 1–2):
DEFERRED_SINK_NAMES,isDeferredCallback,isFunctionExpressionLike, andbuildTryCatchSuggestionare copied verbatim fromrequire-json-parse-try-catch.ts. A shared utility module (src/utils/deferred-callback.ts) would eliminate the drift risk and make the pattern reusable for future rules. - Missing edge-case tests (comments 3–5): The deferred-callback boundary only has
.on/setTimeout/new Promisecoverage;async functionbodies, destructured bindings, and thefindEnclosingStatementnull path all lack test pins. - Implicit range assumption (comment 6): the
node.rangecontainment check has no guard for therange: undefinedcase, which would silently skip the try-block detection.
Positive Highlights
- ✅ Strong motivation: 69 real unguarded callsites identified by a production scan — this rule targets a real, high-frequency risk.
- ✅ Excellent test structure: 12 distinct named cases across CJS and ESM, with suggestion-output assertions.
- ✅ Deliberate FP scope:
existsSync,mkdirSync, aliased refs intentionally excluded — the PR description explains why. - ✅ Deferred-callback boundary detection is architecturally sound and correctly mirrors the sibling rule.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 114.3 AIC · ⌖ 5.87 AIC · ⊞ 6.6K
Comment /matt to run again
|
|
||
| // Callback sinks whose callbacks run outside the dynamic extent of the surrounding try/catch. | ||
| // Mirrors the same constant in require-json-parse-try-catch for consistency. | ||
| const DEFERRED_SINK_NAMES = new Set([ |
|
|
||
| // Callback sinks whose callbacks run outside the dynamic extent of the surrounding try/catch. | ||
| // Mirrors the same constant in require-json-parse-try-catch for consistency. | ||
| const DEFERRED_SINK_NAMES = new Set([ |
There was a problem hiding this comment.
[/codebase-design] DEFERRED_SINK_NAMES, isFunctionExpressionLike, and isDeferredCallback are duplicated verbatim from require-json-parse-try-catch.ts. If a new deferred sink is discovered (e.g., process.nextTick variant), it must be updated in two places — a maintenance hazard.
💡 Suggested fix
Extract to a shared utility, e.g. eslint-factory/src/utils/deferred-callback.ts:
export const DEFERRED_SINK_NAMES = new Set(["then", "catch", "finally", "on", "once", ...]);
export function isDeferredCallback(node: TSESTree.Node): boolean { ... }
export function buildTryCatchSuggestion(stmtText: string, indent: string, comment: string, errorMsg: string): string { ... }WRAPPABLE_STATEMENT_TYPES, findEnclosingStatement, and isFunctionExpressionLike are also candidates. Both rules import from it and the logic is maintained once.
@copilot please address this.
| } | ||
|
|
||
| function buildTryCatchSuggestion(stmtText: string, indent: string, method: string): string { | ||
| return [ |
There was a problem hiding this comment.
[/codebase-design] buildTryCatchSuggestion hardcodes "fs.${method} failed: " — the error prefix is baked in rather than parameterised. The sibling rule (require-json-parse-try-catch) has its own buildTryCatchSuggestion with a different prefix. This prevents sharing via a common helper without changing call sites later.
💡 Suggested fix
Accept errorPrefix and todoComment as parameters so the helper is reusable:
function buildTryCatchSuggestion(stmtText: string, indent: string, errorPrefix: string, todoComment: string): string {
return [
"try {",
`${indent} ${stmtText}`,
`${indent}} catch (err) {`,
`${indent} // ${todoComment}`,
`${indent} throw new Error(`,
`${indent} "${errorPrefix}" + (err instanceof Error ? err.message : String(err)),`,
`${indent} { cause: err },`,
`${indent} );`,
`${indent}}`,
].join("\n");
}@copilot please address this.
| }); | ||
| }); | ||
|
|
||
| it("valid: synchronous callbacks inside try block are protected", () => { |
There was a problem hiding this comment.
[/tdd] No test for fs.readFileSync used as a bare expression inside an async function body without any await (i.e., async function f() { fs.readFileSync(p); }). The deferred-callback logic is only tested for .on, setTimeout, and new Promise — a regular async function body is not a deferred sink and should pass, but there is no explicit test confirming this.
💡 Suggested test to add
it("valid: fs.readFileSync in async function body without try is still flagged (not a deferred sink)", () => {
cjsRuleTester.run("require-fs-sync-try-catch", requireFsSyncTryCatchRule, {
valid: [],
invalid: [
{
code: `async function load() { const x = fs.readFileSync(p, "utf8"); }`,
errors: [{ messageId: "requireTryCatch" }],
},
],
});
});This pins the boundary: async function bodies are not deferred sinks — the call is still synchronous relative to the function's frame.
@copilot please address this.
| // Object must be the `fs` identifier (the standard import alias in actions/setup/js). | ||
| // Aliased references (const r = fs.readFileSync; r(path)) are intentionally out of scope. | ||
| if (callee.object.type !== AST_NODE_TYPES.Identifier || callee.object.name !== "fs") return; | ||
|
|
There was a problem hiding this comment.
[/tdd] The findEnclosingStatement function returns null when the call is not inside a wrappable statement (e.g. it appears in a class field initialiser or a default export expression). In that case fix returns null and the suggestion silently produces no output. There is no test covering this silent-no-op path.
💡 Suggested guard + test
Either suppress the suggestion entirely when stmt is null (cleaner UX) or report a secondary message:
// Only offer the suggestion when a wrappable enclosing statement exists.
...
suggestions: stmt ? [{ messageId: "wrapInTryCatch", fix(...) { ... } }] : [],Add a test:
// export default with fs.readFileSync — no wrappable statement ancestor
{
code: `export default fs.readFileSync(config, "utf8");`,
errors: [{ messageId: "requireTryCatch", suggestions: [] }],
}@copilot please address this.
| }); | ||
| }); | ||
|
|
||
| it("invalid: bare fs.readFileSync is flagged with correct message and suggestion", () => { |
There was a problem hiding this comment.
[/tdd] The valid-case test for mockFs.readFileSync and storage.writeFileSync (non-fs objects) correctly uses the same method names, but there is no test for a destructured import pattern: const { readFileSync } = require('fs'); readFileSync(p);. The current implementation intentionally does not flag this, but the omission isn't documented by a test — a future contributor could unknowingly add destructured-reference support and break the scope guarantee.
💡 Suggested test to add (one-liner)
// Destructured binding — intentionally out of scope for this rule
`const { readFileSync } = require('fs'); readFileSync(path, 'utf8');`,Adding this to the valid cases explicitly documents the intentional gap.
@copilot please address this.
| } | ||
|
|
||
| function findEnclosingStatement(node: TSESTree.Node): TSESTree.Statement | null { | ||
| const ancestors = sourceCode.getAncestors(node); |
There was a problem hiding this comment.
[/codebase-design] isInsideTryBlock uses node.range for the containment check but node.range is only guaranteed to be defined when the parser is configured with range: true. ESLint's default config enables ranges, but this assumption is implicit and could silently mis-detect containment if range is ever absent (returns undefined, causing undefined >= number to be false).
💡 Suggested guard
if (
ancestor.type === "TryStatement" &&
!crossedDeferredBoundary &&
node.range != null &&
block.range != null
) {
if (node.range[0] >= block.range[0] && node.range[1] <= block.range[1]) {
return true;
}
}This matches the defensive pattern used in the sibling rule and makes the assumption explicit.
@copilot please address this.
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · 167.5 AIC · ⌖ 6.67 AIC · ⊞ 5.4K
Comment /review to run again
| function buildTryCatchSuggestion(stmtText: string, indent: string, method: string): string { | ||
| return [ | ||
| "try {", | ||
| `${indent} ${stmtText}`, |
There was a problem hiding this comment.
Broken indentation for multi-line statements in the autofix suggestion: ${indent} ${stmtText} prefixes only the first line, so any multi-line statement body is misaligned inside the generated try block.
💡 Suggested fix
For input like:
const content = fs.readFileSync(
filePath,
"utf8"
);The current code produces:
try {
const content = fs.readFileSync(
filePath, // ← not re-indented
"utf8"
);
} catch (err) { ... }Fix by splitting on \n and prefixing every line:
const indented = stmtText
.split("\n")
.map(line => `${indent} ${line}`)
.join("\n");
// then use `indented` instead of `${indent} ${stmtText}`All current tests use single-line statements, so this is entirely uncovered. Add a multi-line test case.
|
|
||
| // Callback sinks whose callbacks run outside the dynamic extent of the surrounding try/catch. | ||
| // Mirrors the same constant in require-json-parse-try-catch for consistency. | ||
| const DEFERRED_SINK_NAMES = new Set([ |
There was a problem hiding this comment.
Shared deferred-sink logic is copy-pasted, not shared: DEFERRED_SINK_NAMES, isDeferredCallback, and isFunctionExpressionLike are duplicated verbatim from require-json-parse-try-catch.ts — the comment even admits it. These two files will silently diverge on the next update.
💡 Why this matters / suggested fix
Adding a new deferred sink (e.g. Worker message handlers, AbortSignal.addEventListener, a custom scheduler) must now be done in two files. Miss one and you get false negatives in whichever rule you forgot to update. There is no compiler or test safety net for this divergence.
Extract to a shared utility module, e.g. eslint-factory/src/rules/rule-utils.ts:
// rule-utils.ts
export const DEFERRED_SINK_NAMES = new Set([ "then", "catch", ... ]);
export function isFunctionExpressionLike(...) { ... }
export function isDeferredCallback(...) { ... }Both rule files import from it. One place to update, one place to test.
| return DEFERRED_SINK_NAMES.has(callee.property.name); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
nextTick in DEFERRED_SINK_NAMES causes false negatives for user-defined synchronous functions with that name: the bare-identifier branch (line 64) treats any call nextTick(cb) as a deferred sink, even a locally-defined synchronous helper. The actual Node API is process.nextTick, which is correctly handled by the MemberExpression branch — the bare-identifier entry is wrong.
💡 Impact and fix
function nextTick(fn) { fn(); } // synchronous!
try {
nextTick(() => {
fs.readFileSync(path); // ← rule sees this as deferred, does NOT flag it
});
} catch (e) {}The try block genuinely protects this call, but the rule marks it as a deferred boundary and reports a false negative — an unguarded fs call is missed.
Fix: remove "nextTick" from DEFERRED_SINK_NAMES. Only process.nextTick (MemberExpression) is a real deferred sink; the bare identifier is not a reliable signal.
| `${indent} );`, | ||
| `${indent}}`, | ||
| ].join("\n"); | ||
| } |
There was a problem hiding this comment.
fix returns null when findEnclosingStatement fails, silently breaking the suggestion UI: ESLint renders the suggestion as a clickable action in editors, but applying it does nothing.
💡 Impact and fix
findEnclosingStatement returns null for fs calls used as for-loop initializers, switch discriminants, class field initializers, or other non-wrappable statement positions. In those cases fix() returns null, which ESLint treats as a no-op fixer — but the suggestion is still listed with messageId: "wrapInTryCatch". Users see a "Wrap in try/catch" action that silently does nothing when applied.
Options:
- Suppress the suggestion when no enclosing statement is found — only add the
suggestarray entry whenstmt !== null(compute it beforecontext.report). - Add a fallback message — emit a second
suggestentry explaining the limitation.
Option 1 is cleanest:
const stmt = findEnclosingStatement(node);
context.report({
node,
messageId: "requireTryCatch",
data: { method, arg: argText },
suggest: stmt
? [{ messageId: "wrapInTryCatch", fix(fixer) { ... } }]
: [],
});Add test coverage for at least one unsupported position (e.g. for (fs.readFileSync(p);;) {}).
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the unresolved review feedback in 8b70c9d. Local validation passed ( |
|
@copilot please run the
|
pelikhan
deleted the
eslint-miner/require-fs-sync-try-catch-ba95c8c094d0beaf
branch
Summary
Adds a new custom ESLint rule
require-fs-sync-try-catchtoeslint-factory.Motivation
Synchronous
fsmethods (readFileSync,writeFileSync,appendFileSync) throw on I/O errors — missing file, permission denied, disk full. An unhandled throw crashes the GitHub Actions step without surfacing an actionable error message in the log.A full scan of
actions/setup/jsfound 69 real unguarded callsites across 20+ production files:apply_safe_outputs_replay.cjsapply_samples.cjsbuild_checkout_manifest.cjscheck_workflow_recompile_needed.cjsconvert_gateway_config_shared.cjscopilot_harness.cjscreate_code_scanning_alert.cjsdaily_aic_workflow_helpers.cjsdetect_agent_errors.cjsevaluate_outcomes.cjsRule design
require-fs-sync-try-catchfs.readFileSync,fs.writeFileSync,fs.appendFileSynconly. Other sync methods (mkdirSync,unlinkSync, ...) left out to keep FP risk low on the first iteration.require-json-parse-try-catchexactly — a surroundingtryis only treated as protective when no deferred-sink boundary (.on,setTimeout,Promiseexecutor, etc.) has been crossed.try/catchand re-throws with{ cause: err }to preserve the original stack trace.fsidentifier is matched; aliased references are intentionally out of scope.Evidence from scan
Validation
cd eslint-factory && npm install→ cleannpm run build→ passes (tsc clean, no type errors)npx vitest run→ 170/170 tests pass across 13 test files (12 new tests for this rule)npm run lint:setup-js→ rule fires on 69 real violations in production files