Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions eslint-factory/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ This project hosts custom ESLint linters for `/actions/setup/js`.
| [`require-json-parse-try-catch`](#require-json-parse-try-catch) | Require try/catch around `JSON.parse(...)` calls |
| [`require-mkdirsync-try-catch`](#require-mkdirsync-try-catch) | Require try/catch around `fs.mkdirSync` calls |
| [`require-mkdtempsync-try-catch`](#require-mkdtempsync-try-catch) | Require try/catch around `fs.mkdtempSync` calls |
| [`require-realpathsync-try-catch`](#require-realpathsync-try-catch) | Require try/catch around `fs.realpathSync` calls |
| [`require-new-url-try-catch`](#require-new-url-try-catch) | Require try/catch around `new URL(variable)` calls |
| [`require-parseInt-radix`](#require-parseInt-radix) | Require an explicit radix argument to `parseInt()` |
| [`require-nan-check-after-env-numeric-parse`](#require-nan-check-after-env-numeric-parse) | Require NaN validation after parsing numeric values from `process.env` |
Expand Down Expand Up @@ -515,6 +516,35 @@ try {
}
```

### `require-realpathsync-try-catch`

Require `fs.realpathSync` calls to be wrapped in `try/catch`.

Why: `realpathSync` throws synchronously when the target path is missing, permissions are denied, or a symlink cycle is encountered. Wrapping the call preserves call-site-specific error context and ensures path containment checks are not skipped on failure.

**Detected forms:**
- `fs.realpathSync(path)` — direct call on a known `require("fs")` result.
- `fs["realpathSync"](path)` — computed string-literal property access.
- `const { realpathSync } = require("fs"); realpathSync(path)` — destructured binding from `require("fs")` or `require("node:fs")`.
- ESM namespace imports: `import * as fs from "fs"; fs.realpathSync(path)`.
- ESM named imports: `import { realpathSync } from "fs"; realpathSync(path)`.

**Out of scope:**
- Objects whose `require` source is not the Node `fs` / `node:fs` module.
- Calls already inside a `try` block with a `catch` clause.
- `try { ... } finally { ... }` without a `catch` clause is still flagged.

**Known limitation — no autofix for `VariableDeclaration`:** when the flagged call appears as a variable initializer, the rule reports the error but emits no autofix suggestion. Only `ExpressionStatement` and `ReturnStatement` positions receive an autofix suggestion.

**Safe alternative:**
```js
try {
const resolved = fs.realpathSync(path);
} catch (err) {
throw new Error("fs.realpathSync failed: " + (err instanceof Error ? err.message : String(err)), { cause: err });
}
```

### `require-new-url-try-catch`

Require `new URL(variable)` calls to be wrapped in `try/catch`.
Expand Down
1 change: 1 addition & 0 deletions eslint-factory/eslint.config.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ module.exports = [
"gh-aw-custom/require-json-parse-try-catch": "warn",
"gh-aw-custom/require-mkdirsync-try-catch": "warn",
"gh-aw-custom/require-mkdtempsync-try-catch": "warn",
"gh-aw-custom/require-realpathsync-try-catch": "warn",
"gh-aw-custom/require-rmsync-try-catch": "warn",
"gh-aw-custom/require-parseInt-radix": "warn",
"gh-aw-custom/require-return-after-core-setfailed": "warn",
Expand Down
2 changes: 2 additions & 0 deletions eslint-factory/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { requireErrorCauseInRethrowRule } from "./rules/require-error-cause-in-r
import { requireParseIntRadixRule } from "./rules/require-parseInt-radix";
import { requireMkdirSyncTryCatchRule } from "./rules/require-mkdirsync-try-catch";
import { requireMkdtempSyncTryCatchRule } from "./rules/require-mkdtempsync-try-catch";
import { requireRealpathSyncTryCatchRule } from "./rules/require-realpathsync-try-catch";
import { requireRmSyncTryCatchRule } from "./rules/require-rmsync-try-catch";
import { requireReturnAfterCoreSetFailedRule } from "./rules/require-return-after-core-setfailed";
import { requireSpawnSyncErrorCheckRule } from "./rules/require-spawnsync-error-check";
Expand Down Expand Up @@ -84,6 +85,7 @@ const plugin = {
"require-json-parse-try-catch": requireJsonParseTryCatchRule,
"require-mkdirsync-try-catch": requireMkdirSyncTryCatchRule,
"require-mkdtempsync-try-catch": requireMkdtempSyncTryCatchRule,
"require-realpathsync-try-catch": requireRealpathSyncTryCatchRule,
"require-rmsync-try-catch": requireRmSyncTryCatchRule,
"require-parseInt-radix": requireParseIntRadixRule,
"require-return-after-core-setfailed": requireReturnAfterCoreSetFailedRule,
Expand Down
83 changes: 83 additions & 0 deletions eslint-factory/src/rules/require-realpathsync-try-catch.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
import { RuleTester } from "eslint";
import { describe, it } from "vitest";
import { requireRealpathSyncTryCatchRule } from "./require-realpathsync-try-catch";

const cjsRuleTester = new RuleTester({ languageOptions: { ecmaVersion: 2022, sourceType: "commonjs" } });
const esmRuleTester = new RuleTester({ languageOptions: { ecmaVersion: 2022, sourceType: "module" } });

describe("require-realpathsync-try-catch", () => {
it("allows calls inside try/catch and ignores non-fs receivers", () => {
cjsRuleTester.run("require-realpathsync-try-catch", requireRealpathSyncTryCatchRule, {
valid: [
`const fs = require("fs"); try { fs.realpathSync(path); } catch (e) {}`,
`const { realpathSync } = require("node:fs"); try { realpathSync(path); } catch (e) {}`,
`mockFs.realpathSync(path);`,
`const fs = require("mock-fs"); fs.realpathSync(path);`,
],
invalid: [],
});
});

it("flags CommonJS calls and offers an autofix", () => {
cjsRuleTester.run("require-realpathsync-try-catch", requireRealpathSyncTryCatchRule, {
valid: [],
invalid: [
{
code: `fs.realpathSync(unresolved);`,
errors: [
{
messageId: "requireTryCatch",
data: { arg: "unresolved" },
suggestions: [
{
messageId: "wrapInTryCatch",
output: `try {\n fs.realpathSync(unresolved);\n} catch (err) {\n // TODO: handle filesystem failure for this fs.realpathSync call.\n throw new Error(\n "fs.realpathSync failed: " + (err instanceof Error ? err.message : String(err)),\n { cause: err },\n );\n}`,
},
],
},
],
},
{
code: `const fs = require("fs"); fs["realpathSync"](root);`,
errors: [{ messageId: "requireTryCatch", data: { arg: "root" }, suggestions: 1 }],
},
{
code: `const { realpathSync } = require("fs"); realpathSync(directoryPath);`,
errors: [{ messageId: "requireTryCatch", data: { arg: "directoryPath" }, suggestions: 1 }],
},
],
});
});

it("handles ESM namespace and named imports", () => {
esmRuleTester.run("require-realpathsync-try-catch", requireRealpathSyncTryCatchRule, {
valid: [`import * as fs from "fs"; try { fs.realpathSync(path); } catch (e) {}`],
invalid: [
{
code: `import * as fs from "node:fs"; fs.realpathSync(path);`,
errors: [{ messageId: "requireTryCatch", data: { arg: "path" }, suggestions: 1 }],
},
{
code: `import { realpathSync } from "fs"; realpathSync(path);`,
errors: [{ messageId: "requireTryCatch", data: { arg: "path" }, suggestions: 1 }],
},
],
});
});

it("flags calls in async functions and try/finally without catch", () => {
cjsRuleTester.run("require-realpathsync-try-catch", requireRealpathSyncTryCatchRule, {
valid: [],
invalid: [
{
code: `async function run() { fs.realpathSync(path); }`,
errors: [{ messageId: "requireTryCatch", suggestions: 1 }],
},
{
code: `try { fs.realpathSync(path); } finally { cleanup(); }`,
errors: [{ messageId: "requireTryCatch", suggestions: 1 }],
},
],
});
});
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The README explicitly documents "no autofix for VariableDeclaration" as a known limitation, but there is no test case covering that path — the rule could silently regress for const resolved = fs.realpathSync(path); without any test catching it.

💡 Suggested test case

Add an it block that asserts the call is still flagged but produces zero suggestions:

it("flags VariableDeclaration calls with no autofix suggestion", () => {
  cjsRuleTester.run("require-realpathsync-try-catch", requireRealpathSyncTryCatchRule, {
    valid: [],
    invalid: [
      {
        code: `const resolved = fs.realpathSync(path);`,
        errors: [
          {
            messageId: "requireTryCatch",
            data: { arg: "path" },
            suggestions: 0, // documented limitation: no autofix for VariableDeclaration
          },
        ],
      },
    ],
  });
});

This pins the documented behaviour and prevents a silent regression.

@copilot please address this.

69 changes: 69 additions & 0 deletions eslint-factory/src/rules/require-realpathsync-try-catch.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
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_SYNC_METHODS = new Set(["realpathSync"]);

export const requireRealpathSyncTryCatchRule = createRule({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eslint-factory/src/rules/require-realpathsync-try-catch.ts:8: yagni: one-off realpathSync rule added as a bespoke 69-line wrapper. Replace it with a generic fs-sync rule generator so mkdirSync/mkdtempSync/rmSync/realpathSync all share one implementation.

name: "require-realpathsync-try-catch",
meta: {
type: "problem",
hasSuggestions: true,
docs: {
description:
"Require fs.realpathSync calls in actions/setup/js scripts to be wrapped in try/catch. " +
"realpathSync throws synchronously when the target path is missing, permissions are denied, or a symlink cycle is encountered; " +
"without a call-site try/catch, the containment check is skipped and the original error loses useful call-site context.",
},
schema: [],
messages: {
requireTryCatch:
"Wrap fs.realpathSync({{arg}}) in try/catch — realpathSync throws on missing paths, permission denied, or symlink cycles; without a call-site try/catch, you lose the original error context and get a generic engine-level stack instead of a specific message with `{ cause }`.",
wrapInTryCatch: "Wrap in try { ... } catch { ... } and re-throw with { cause: err } to preserve context.",
},
},
defaultOptions: [],
create(context) {
const sourceCode = context.sourceCode;
const resolveFsSyncMethod = createFsSyncMethodResolver(sourceCode, FS_SYNC_METHODS, { allowUnboundFsIdentifier: true });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rule inherits allowUnboundFsIdentifier: true, so it will flag any bare fs.realpathSync(...) call even when the file does not import or require Node's fs module, which creates false positives against projects that happen to have a different global or ambient fs object.

💡 Why this is a real correctness bug

createFsSyncMethodResolver(..., { allowUnboundFsIdentifier: true }) explicitly treats an unbound identifier named fs as the Node fs module. That means code like this will now be warned on incorrectly:

const fs = getSandboxFacadeSomehow();
// or ambient/global `fs` supplied by another runtime
fs.realpathSync(path);

This is not just a theoretical lint nit: once the rule ships as part of the shared config, consumers get noise on code the rule cannot prove is Node fs, which erodes trust in the whole ruleset.

Please either disable allowUnboundFsIdentifier for this rule or add coverage proving that unbound fs.realpathSync(...) is intentionally safe to treat as Node fs in this codebase.


return {
CallExpression(node) {
const methodName = resolveFsSyncMethod(node);
if (methodName !== "realpathSync") return;
if (isInsideTryBlock(sourceCode, node)) return;

const argText = node.arguments.length > 0 ? sourceCode.getText(node.arguments[0]) : "";
const stmt = findEnclosingStatement(sourceCode, node);

context.report({
node,
messageId: "requireTryCatch",
data: { 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,
todoComment: "TODO: handle filesystem failure for this fs.realpathSync call.",
errorPrefix: "fs.realpathSync failed: ",
})
);
},
},
]
: [],
});
},
};
},
});
Loading