diff --git a/eslint-factory/README.md b/eslint-factory/README.md index 43c617a58be..3abc0ec3830 100644 --- a/eslint-factory/README.md +++ b/eslint-factory/README.md @@ -45,6 +45,7 @@ This project hosts custom ESLint linters for `/actions/setup/js`. | [`require-fetch-response-body-try-catch`](#require-fetch-response-body-try-catch) | Require try/catch around `.json()` or `.text()` on Responses from `fetch(...)` | | [`require-fetch-timeout`](#require-fetch-timeout) | Require `fetch(...)` calls to include a non-nullish abort `signal` option | | [`require-fetch-try-catch`](#require-fetch-try-catch) | Require try/catch around awaited `fetch(...)` calls, including chained promise forms without rejection handlers | +| [`require-fs-chmod-try-catch`](#require-fs-chmod-try-catch) | Require try/catch around `fs.chmodSync` and `fs.fchmodSync` | | [`require-fs-close-sync`](#require-fs-close-sync) | Require `fs.openSync(...)` file descriptors to be closed with `fs.closeSync(fd)` in the same function | | [`require-fs-io-try-catch`](#require-fs-io-try-catch) | Require try/catch around `fs.statSync`, `readdirSync`, `copyFileSync`, `unlinkSync`, and `renameSync` | | [`require-fs-sync-try-catch`](#require-fs-sync-try-catch) | Require try/catch around `fs.readFileSync`, `writeFileSync`, and `appendFileSync` | @@ -396,6 +397,21 @@ try { } ``` +### `require-fs-chmod-try-catch` + +Require `fs.chmodSync` and `fs.fchmodSync` calls to be wrapped in `try/catch`. + +Why: these calls can throw for missing files or descriptors, permission errors, and unsupported filesystems. A call-site catch preserves useful error context. + +**Detected forms:** +- `fs.chmodSync(path, mode)` and `fs["chmodSync"](path, mode)`. +- `fs.fchmodSync(fd, mode)`. +- Bindings imported or required from `fs` / `node:fs`, including destructured bindings. + +**Out of scope:** +- Objects that are not resolved to the Node `fs` / `node:fs` module. +- `try { ... } finally { ... }` without a `catch` clause is still flagged. + ### `require-fs-close-sync` Require file descriptors returned by `fs.openSync(...)` to be closed with `fs.closeSync(fd)` in the same enclosing function. diff --git a/eslint-factory/eslint.config.cjs b/eslint-factory/eslint.config.cjs index 0905e07fa1f..5d2371bbaa4 100644 --- a/eslint-factory/eslint.config.cjs +++ b/eslint-factory/eslint.config.cjs @@ -48,6 +48,7 @@ module.exports = [ "gh-aw-custom/require-execsync-try-catch": "warn", "gh-aw-custom/require-execfilesync-try-catch": "warn", "gh-aw-custom/require-fs-io-try-catch": "warn", + "gh-aw-custom/require-fs-chmod-try-catch": "warn", "gh-aw-custom/no-setfailed-then-exit-zero": "warn", "gh-aw-custom/no-err-stack-then-string-fallback": "warn", "gh-aw-custom/no-caught-error-interpolation": "warn", diff --git a/eslint-factory/src/index.ts b/eslint-factory/src/index.ts index 3fc87c4e13b..776e29bd948 100644 --- a/eslint-factory/src/index.ts +++ b/eslint-factory/src/index.ts @@ -59,6 +59,7 @@ import { noStringFallbackForNonStringMessageRule } from "./rules/no-string-fallb import { requireGetExecOutputExitCodeCheckRule } from "./rules/require-getexecoutput-exitcode-check"; import { preferActionsExecOverChildProcessRule } from "./rules/prefer-actions-exec-over-child-process"; import { noMisplacedErrorCodeDefinitionRule } from "./rules/no-misplaced-error-code-definition"; +import { requireFsChmodTryCatchRule } from "./rules/require-fs-chmod-try-catch"; const plugin = { meta: { @@ -127,6 +128,7 @@ const plugin = { "require-getexecoutput-exitcode-check": requireGetExecOutputExitCodeCheckRule, "prefer-actions-exec-over-child-process": preferActionsExecOverChildProcessRule, "no-misplaced-error-code-definition": noMisplacedErrorCodeDefinitionRule, + "require-fs-chmod-try-catch": requireFsChmodTryCatchRule, }, }; diff --git a/eslint-factory/src/rules/require-fs-chmod-try-catch.test.ts b/eslint-factory/src/rules/require-fs-chmod-try-catch.test.ts new file mode 100644 index 00000000000..f13d09b8c0e --- /dev/null +++ b/eslint-factory/src/rules/require-fs-chmod-try-catch.test.ts @@ -0,0 +1,75 @@ +import { RuleTester } from "eslint"; +import { describe, it } from "vitest"; +import { requireFsChmodTryCatchRule } from "./require-fs-chmod-try-catch"; + +const cjsRuleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: "commonjs", + }, +}); + +function expectedWrapInTryCatchSuggestion(method: string, statement: string, prefix = "") { + return { + messageId: "wrapInTryCatch", + output: `${prefix}try {\n` + ` ${statement}\n` + `} catch (err) {\n` + ` throw new Error(\n` + ` "fs.${method} failed: " + (err instanceof Error ? err.message : String(err)),\n` + ` { cause: err },\n` + ` );\n` + `}`, + }; +} + +describe("require-fs-chmod-try-catch", () => { + it("valid: fs.chmodSync and fs.fchmodSync inside try block pass", () => { + cjsRuleTester.run("require-fs-chmod-try-catch", requireFsChmodTryCatchRule, { + valid: [`try { fs.chmodSync(path, 0o600); } catch (e) {}`, `try { fs.fchmodSync(fd, 0o600); } catch (e) {}`], + invalid: [], + }); + }); + + it("valid: other fs methods and non-fs identifiers are ignored", () => { + cjsRuleTester.run("require-fs-chmod-try-catch", requireFsChmodTryCatchRule, { + valid: [`fs.existsSync(path);`, `fs.readFileSync(path, "utf8");`, `fs.statSync(path);`, `mockFs.chmodSync(path, 0o600);`, `storage.fchmodSync(fd, 0o600);`], + invalid: [], + }); + }); + + it("invalid: fs.chmodSync outside try/catch is flagged", () => { + cjsRuleTester.run("require-fs-chmod-try-catch", requireFsChmodTryCatchRule, { + valid: [], + invalid: [ + { + code: `fs.chmodSync(path, 0o600);`, + errors: [{ messageId: "requireTryCatch", data: { method: "chmodSync", arg: "path" }, suggestions: [expectedWrapInTryCatchSuggestion("chmodSync", "fs.chmodSync(path, 0o600);")] }], + }, + ], + }); + }); + + it("invalid: fs.fchmodSync outside try/catch is flagged", () => { + cjsRuleTester.run("require-fs-chmod-try-catch", requireFsChmodTryCatchRule, { + valid: [], + invalid: [ + { + code: `fs.fchmodSync(fd, 0o600);`, + errors: [{ messageId: "requireTryCatch", data: { method: "fchmodSync", arg: "fd" }, suggestions: [expectedWrapInTryCatchSuggestion("fchmodSync", "fs.fchmodSync(fd, 0o600);")] }], + }, + ], + }); + }); + + it("invalid: fs.chmodSync after an unrelated try block is still flagged", () => { + cjsRuleTester.run("require-fs-chmod-try-catch", requireFsChmodTryCatchRule, { + valid: [], + invalid: [ + { + code: `try { doSomethingElse(); } catch (e) {}\nfs.chmodSync(dir, 0o700);`, + errors: [ + { + messageId: "requireTryCatch", + data: { method: "chmodSync", arg: "dir" }, + suggestions: [expectedWrapInTryCatchSuggestion("chmodSync", "fs.chmodSync(dir, 0o700);", "try { doSomethingElse(); } catch (e) {}\n")], + }, + ], + }, + ], + }); + }); +}); diff --git a/eslint-factory/src/rules/require-fs-chmod-try-catch.ts b/eslint-factory/src/rules/require-fs-chmod-try-catch.ts new file mode 100644 index 00000000000..792d2e29f30 --- /dev/null +++ b/eslint-factory/src/rules/require-fs-chmod-try-catch.ts @@ -0,0 +1,67 @@ +import { ESLintUtils } from "@typescript-eslint/utils"; +import { buildTryCatchSuggestion, createFsSyncMethodResolver, findEnclosingStatement, isInsideTryBlock } from "./try-catch-rule-utils"; + +const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/eslint-factory#${name}`); + +const FS_CHMOD_METHODS = new Set(["chmodSync", "fchmodSync"]); + +export const requireFsChmodTryCatchRule = createRule({ + name: "require-fs-chmod-try-catch", + meta: { + type: "problem", + hasSuggestions: true, + docs: { + description: "Require fs.chmodSync and fs.fchmodSync calls in actions/setup/js scripts to be wrapped in try/catch.", + }, + schema: [], + messages: { + requireTryCatch: "Wrap fs.{{method}}({{arg}}) in try/catch.", + wrapInTryCatch: "Wrap in try { ... } catch { ... } and re-throw with { cause: err } to preserve context.", + }, + }, + defaultOptions: [], + create(context) { + const sourceCode = context.sourceCode; + const resolveFsChmodMethod = createFsSyncMethodResolver(sourceCode, FS_CHMOD_METHODS, { allowUnboundFsIdentifier: true }); + + return { + CallExpression(node) { + const methodName = resolveFsChmodMethod(node); + + if (!methodName) return; + + if (isInsideTryBlock(sourceCode, node)) return; + + const argText = node.arguments.length > 0 ? sourceCode.getText(node.arguments[0]) : ""; + const method = methodName; + const stmt = findEnclosingStatement(sourceCode, node); + + context.report({ + node, + messageId: "requireTryCatch", + data: { method, arg: argText }, + suggest: stmt + ? [ + { + messageId: "wrapInTryCatch", + fix(fixer) { + const stmtText = sourceCode.getText(stmt); + const startLine = stmt.loc?.start.line; + const stmtLine = startLine !== undefined ? (sourceCode.lines[startLine - 1] ?? "") : ""; + const indent = stmtLine.match(/^(\s*)/)?.[1] ?? ""; + return fixer.replaceText( + stmt, + buildTryCatchSuggestion(stmtText, { + indent, + errorPrefix: `fs.${method} failed: `, + }) + ); + }, + }, + ] + : [], + }); + }, + }; + }, +}); diff --git a/eslint-factory/src/rules/try-catch-rule-utils.ts b/eslint-factory/src/rules/try-catch-rule-utils.ts index 2214f82bfc8..e9ff1f91f0a 100644 --- a/eslint-factory/src/rules/try-catch-rule-utils.ts +++ b/eslint-factory/src/rules/try-catch-rule-utils.ts @@ -291,7 +291,7 @@ export function createFsSyncMethodResolver(sourceCode: TSESLint.SourceCode, fsSy type TryCatchSuggestionOptions = { indent: string; - todoComment: string; + todoComment?: string; errorPrefix: string; }; @@ -311,7 +311,7 @@ export function buildTryCatchSuggestion(stmtText: string, options: TryCatchSugge "try {", indentedStatement, `${indent}} catch (err) {`, - `${indent} // ${todoComment}`, + ...(todoComment ? [`${indent} // ${todoComment}`] : []), `${indent} throw new Error(`, `${indent} "${errorPrefix}" + (err instanceof Error ? err.message : String(err)),`, `${indent} { cause: err },`,