-
Notifications
You must be signed in to change notification settings - Fork 479
[eslint-miner] eslint-factory: add require-fetch-timeout rule #49310
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| import { RuleTester } from "eslint"; | ||
| import { describe, it } from "vitest"; | ||
| import { requireFetchTimeoutRule } from "./require-fetch-timeout"; | ||
|
|
||
| const cjsRuleTester = new RuleTester({ | ||
| languageOptions: { | ||
| ecmaVersion: 2022, | ||
| sourceType: "commonjs", | ||
| }, | ||
| }); | ||
|
|
||
| describe("require-fetch-timeout", () => { | ||
| it("valid: fetch with AbortSignal.timeout", () => { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] Tests split across multiple 💡 Suggested consolidationCombine into one describe('require-fetch-timeout', () => {
cjsRuleTester.run('require-fetch-timeout', requireFetchTimeoutRule, {
valid: [
`async function f() { const res = await fetch(url, { signal: AbortSignal.timeout(10000) }); }`,
`async function f() { const res = await fetch(url, ...opts); }`,
// ...all other valid cases
],
invalid: [
{ code: `async function f() { const res = await fetch(url); }`, errors: [{ messageId: 'requireSignal' }] },
// ...all invalid cases
],
});
});Check how @copilot please address this. |
||
| cjsRuleTester.run("require-fetch-timeout", requireFetchTimeoutRule, { | ||
| valid: [ | ||
| `async function f() { const res = await fetch(url, { signal: AbortSignal.timeout(10000) }); }`, | ||
| `async function f() { const res = await fetch(url, { method: "POST", signal: ac.signal }); }`, | ||
| `async function f() { const res = await globalThis.fetch(url, { signal: AbortSignal.timeout(10000) }); }`, | ||
| ], | ||
| invalid: [], | ||
| }); | ||
| }); | ||
|
|
||
| it("valid: fetch with spread options or identifier options object (unresolvable statically)", () => { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] Missing edge case: 💡 Suggested test// Should this be valid or invalid?
{ code: `async function f() { const res = await fetch(url, null); }`, errors: [{ messageId: 'requireSignal' }] }If @copilot please address this. |
||
| cjsRuleTester.run("require-fetch-timeout", requireFetchTimeoutRule, { | ||
| valid: [`async function f() { const res = await fetch(url, ...opts); }`, `async function f() { const res = await fetch(url, options); }`, `async function f() { const res = await fetch(url, { ...baseOptions }); }`], | ||
| invalid: [], | ||
| }); | ||
| }); | ||
|
|
||
| it("valid: non-fetch calls are not flagged", () => { | ||
| cjsRuleTester.run("require-fetch-timeout", requireFetchTimeoutRule, { | ||
| valid: [ | ||
| `async function f() { const res = await axios.get(url); }`, | ||
| `function fetch2() { return 1; }`, | ||
| `obj.fetch(url);`, | ||
| `async function f(fetch) { return fetch(url); }`, | ||
| `async function f() { const fetch = (u) => Promise.resolve(u); return fetch(url); }`, | ||
| ], | ||
| invalid: [], | ||
| }); | ||
| }); | ||
|
|
||
| it("invalid: fetch with no options argument", () => { | ||
| cjsRuleTester.run("require-fetch-timeout", requireFetchTimeoutRule, { | ||
| valid: [], | ||
| invalid: [ | ||
| { | ||
| code: `async function f() { const res = await fetch(url); }`, | ||
| errors: [{ messageId: "requireSignal" }], | ||
| }, | ||
| ], | ||
| }); | ||
| }); | ||
|
|
||
| it("invalid: fetch with options object missing signal", () => { | ||
| cjsRuleTester.run("require-fetch-timeout", requireFetchTimeoutRule, { | ||
| valid: [], | ||
| invalid: [ | ||
| { | ||
| code: `async function f() { const res = await fetch(url, { method: "POST", headers: { "Content-Type": "application/json" } }); }`, | ||
| errors: [{ messageId: "requireSignal" }], | ||
| }, | ||
| { | ||
| code: `async function f() { const res = await fetch(url, { method: "GET" }); }`, | ||
| errors: [{ messageId: "requireSignal" }], | ||
| }, | ||
| { | ||
| code: `async function f() { const res = await globalThis.fetch(url, { method: "GET" }); }`, | ||
| errors: [{ messageId: "requireSignal" }], | ||
| }, | ||
| { | ||
| code: `async function f() { const res = await global["fetch"](url, { method: "GET" }); }`, | ||
| errors: [{ messageId: "requireSignal" }], | ||
| }, | ||
| { | ||
| code: `async function f() { const res = await fetch(url, { signal: null }); }`, | ||
| errors: [{ messageId: "requireSignal" }], | ||
| }, | ||
| { | ||
| code: `async function f() { const res = await fetch(url, { signal: undefined }); }`, | ||
| errors: [{ messageId: "requireSignal" }], | ||
| }, | ||
| { | ||
| code: `async function f() { const res = await fetch(url, { signal: void 0 }); }`, | ||
| errors: [{ messageId: "requireSignal" }], | ||
| }, | ||
| { | ||
| code: `async function f() { const res = await fetch(url, undefined); }`, | ||
| errors: [{ messageId: "requireSignal" }], | ||
| }, | ||
| { | ||
| code: `async function f() { const res = await fetch(url, null); }`, | ||
| errors: [{ messageId: "requireSignal" }], | ||
| }, | ||
| ], | ||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| import { AST_NODE_TYPES, ESLintUtils, TSESTree } from "@typescript-eslint/utils"; | ||
|
|
||
| const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/eslint-factory#${name}`); | ||
|
|
||
| /** | ||
| * Returns true when the fetch call's options object argument (if any) carries | ||
| * a `signal` property, which is how callers wire in `AbortSignal.timeout(...)` | ||
| * or an `AbortController`-backed abort deadline. | ||
| */ | ||
| function isStaticallyNullish(node: TSESTree.Node): boolean { | ||
| if (node.type === AST_NODE_TYPES.Literal && node.value == null) return true; | ||
| if (node.type === AST_NODE_TYPES.Identifier && node.name === "undefined") return true; | ||
| if (node.type === AST_NODE_TYPES.UnaryExpression && node.operator === "void") return true; | ||
| return false; | ||
| } | ||
|
|
||
| function hasSignalOption(callExpression: TSESTree.CallExpression): boolean { | ||
| const optionsArg = callExpression.arguments[1]; | ||
| if (!optionsArg) return false; | ||
| if (isStaticallyNullish(optionsArg)) return false; | ||
|
|
||
| // Spread arguments (`fetch(url, ...opts)`) can't be statically inspected; | ||
| // assume the caller may have included a signal to avoid false positives. | ||
| if (optionsArg.type === AST_NODE_TYPES.SpreadElement) return true; | ||
|
|
||
| if (optionsArg.type === AST_NODE_TYPES.ObjectExpression) { | ||
| for (const prop of optionsArg.properties) { | ||
| if (prop.type === AST_NODE_TYPES.SpreadElement) return true; | ||
| if (prop.type === AST_NODE_TYPES.Property) { | ||
| const isSignalProp = (!prop.computed && prop.key.type === AST_NODE_TYPES.Identifier && prop.key.name === "signal") || (!prop.computed && prop.key.type === AST_NODE_TYPES.Literal && prop.key.value === "signal"); | ||
| if (!isSignalProp) continue; | ||
|
|
||
| if (!isStaticallyNullish(prop.value)) return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| // Options passed as an identifier/expression (e.g. a shared config object) | ||
| // can't be statically inspected; assume it may already carry a signal. | ||
| return true; | ||
|
Comment on lines
+39
to
+41
|
||
| } | ||
|
|
||
| export const requireFetchTimeoutRule = createRule({ | ||
| name: "require-fetch-timeout", | ||
|
Comment on lines
+44
to
+45
|
||
| meta: { | ||
| type: "problem", | ||
| docs: { | ||
| description: "Require fetch() calls in actions/setup/js scripts to pass an abort signal so requests cannot hang indefinitely in CI", | ||
| }, | ||
| schema: [], | ||
| messages: { | ||
| requireSignal: "fetch() call has no `signal` option. Pass `signal: AbortSignal.timeout(<ms>)` (or an AbortController-backed signal) so a stalled network request cannot hang the job indefinitely.", | ||
| }, | ||
| }, | ||
| defaultOptions: [], | ||
| create(context) { | ||
| const sourceCode = context.sourceCode; | ||
| type SourceCodeScope = ReturnType<typeof sourceCode.getScope>; | ||
|
|
||
| function hasLocalBinding(node: TSESTree.Node, name: string): boolean { | ||
| let scope: SourceCodeScope | null = sourceCode.getScope(node); | ||
| while (scope) { | ||
| const variable = scope.set.get(name); | ||
| if (variable?.defs.length) return true; | ||
| scope = scope.upper; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| function isGlobalFetchCall(callee: TSESTree.Expression): callee is TSESTree.MemberExpression { | ||
| if (callee.type !== AST_NODE_TYPES.MemberExpression) return false; | ||
|
|
||
| const propertyName = | ||
| !callee.computed && callee.property.type === AST_NODE_TYPES.Identifier | ||
| ? callee.property.name | ||
| : callee.computed && callee.property.type === AST_NODE_TYPES.Literal && typeof callee.property.value === "string" | ||
| ? callee.property.value | ||
| : null; | ||
| if (propertyName !== "fetch") return false; | ||
|
|
||
| return callee.object.type === AST_NODE_TYPES.Identifier && (callee.object.name === "globalThis" || callee.object.name === "global"); | ||
| } | ||
|
|
||
| return { | ||
| CallExpression(node: TSESTree.CallExpression) { | ||
| const callee = node.callee; | ||
| const isBareFetch = callee.type === AST_NODE_TYPES.Identifier && callee.name === "fetch"; | ||
| const isMemberFetch = isGlobalFetchCall(callee); | ||
| if (!isBareFetch && !isMemberFetch) return; | ||
| if (isBareFetch && hasLocalBinding(node, "fetch")) return; | ||
| if (isMemberFetch && callee.object.type === AST_NODE_TYPES.Identifier && hasLocalBinding(node, callee.object.name)) return; | ||
|
|
||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The rule fires on any top-level 💡 Options to considerOption A — Narrow via { files: ['actions/setup/js/**/*.cjs'], rules: { 'gh-aw-custom/require-fetch-timeout': 'warn' } }Option B — Scope inside the rule using if (!context.filename.includes('actions/setup/js')) return;Either way, clarify the intended scope so future contributors know whether this rule is global or file-scoped. @copilot please address this. |
||
| if (hasSignalOption(node)) return; | ||
|
|
||
| context.report({ node, messageId: "requireSignal" }); | ||
| }, | ||
| }; | ||
| }, | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test suite has no coverage for the two biggest gaps in the rule: member-expression callees (
globalThis.fetch) and nullishsignalvalues, both of which the implementation handles incorrectly (see other comments onrequire-fetch-timeout.ts).💡 Why this matters
A rule whose test suite only exercises bare-identifier
fetch(...)calls and truthysignalvalues gives false confidence — the 5/5 passing tests claimed in the PR description don't actually exercise the false-negative paths that matter most for CI-hang prevention. Addinvalidcases forglobalThis.fetch(url)andfetch(url, { signal: undefined })/{ signal: null }once the corresponding fixes land, so regressions in those paths are caught automatically.