[eslint-miner] eslint-factory: add require-fetch-timeout rule - #49310
Conversation
Flags fetch() calls in actions/setup/js that lack an abort signal, which can hang indefinitely in CI if the network stalls. Several call sites (artifact_client.cjs, apply_samples.cjs, check_version_updates.cjs, send_otlp_span.cjs) call fetch() with no timeout protection at all, while other files (validate_secrets.cjs, awf_reflect.cjs) already use AbortSignal.timeout()/AbortController correctly and are unaffected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. Great job! 🎉 This PR looks solid. The new Why this is excellent:
The rule correctly identifies missing abort signals in fetch() calls and provides actionable guidance. The false-positive avoidance (spread operators, dynamic expressions) shows careful implementation. Ready for review!
|
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #49310 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ PR Code Quality Reviewer completed the code quality review. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
There was a problem hiding this comment.
Pull request overview
Adds an ESLint rule intended to prevent unbounded fetch() requests in setup scripts.
Changes:
- Implements signal-option detection.
- Adds RuleTester coverage.
- Registers and enables the rule as a warning.
Show a summary per file
| File | Description |
|---|---|
eslint-factory/src/rules/require-fetch-timeout.ts |
Implements the new rule. |
eslint-factory/src/rules/require-fetch-timeout.test.ts |
Tests valid and invalid fetch calls. |
eslint-factory/src/index.ts |
Registers the rule in the plugin. |
eslint-factory/eslint.config.cjs |
Enables the rule for setup scripts. |
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: 4
- Review effort level: Balanced
| create(context) { | ||
| return { | ||
| CallExpression(node: TSESTree.CallExpression) { | ||
| const callee = node.callee; | ||
| if (callee.type !== AST_NODE_TYPES.Identifier || callee.name !== "fetch") return; | ||
|
|
||
| if (hasSignalOption(node)) return; |
| if (prop.type === AST_NODE_TYPES.Property) { | ||
| if (!prop.computed && prop.key.type === AST_NODE_TYPES.Identifier && prop.key.name === "signal") return true; | ||
| if (!prop.computed && prop.key.type === AST_NODE_TYPES.Literal && prop.key.value === "signal") return true; | ||
| } |
| export const requireFetchTimeoutRule = createRule({ | ||
| name: "require-fetch-timeout", |
| // 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; |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
The new require-fetch-timeout ESLint rule is well-structured and correct. Correctly skips spread args and non-literal options to avoid false positives. Good test coverage. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 19.8 AIC · ⊞ 5.3K
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Verdict: Request Changes
The new require-fetch-timeout rule has real false-negative gaps (member-expression fetch callees, nullish signal values) that undermine its stated purpose of preventing hung CI requests.
Themes
- Callee matching too narrow: only bare
fetch(...)identifier calls are checked;globalThis.fetch/window.fetchbypass the rule entirely. - Signal presence check is purely structural:
{ signal: undefined }or{ signal: null }is treated as compliant, missing the most likely accidental-regression pattern. - Test coverage doesn't exercise these gaps, so the claimed 5/5 pass rate doesn't validate the rule's robustness against the cases most likely to occur in real refactors.
These are all fixable with small, targeted additions to hasSignalOption/the CallExpression visitor plus corresponding tests; not a fundamental design problem.
🔎 Code quality review by PR Code Quality Reviewer · auto · 30.9 AIC · ⊞ 7.8K
Comment /review to run again
| return { | ||
| CallExpression(node: TSESTree.CallExpression) { | ||
| const callee = node.callee; | ||
| if (callee.type !== AST_NODE_TYPES.Identifier || callee.name !== "fetch") return; |
There was a problem hiding this comment.
Only bare fetch(...) identifier calls are matched — window.fetch(...)/globalThis.fetch(...) (both valid in Node 18+/Actions runners) silently bypass the rule.
💡 Why this matters and how to fix
The callee check is callee.type !== AST_NODE_TYPES.Identifier || callee.name !== "fetch". Any MemberExpression callee whose property is fetch (e.g. globalThis.fetch(url), window.fetch(url)) is skipped entirely, defeating the rule's core purpose of preventing hung requests if any script in actions/setup/js uses that calling style.
CallExpression(node: TSESTree.CallExpression) {
const callee = node.callee;
const isFetchIdentifier = callee.type === AST_NODE_TYPES.Identifier && callee.name === "fetch";
const isFetchMember =
callee.type === AST_NODE_TYPES.MemberExpression &&
!callee.computed &&
callee.property.type === AST_NODE_TYPES.Identifier &&
callee.property.name === "fetch";
if (!isFetchIdentifier && !isFetchMember) return;
...
}Add a corresponding invalid test case for globalThis.fetch(url).
| for (const prop of optionsArg.properties) { | ||
| if (prop.type === AST_NODE_TYPES.SpreadElement) return true; | ||
| if (prop.type === AST_NODE_TYPES.Property) { | ||
| if (!prop.computed && prop.key.type === AST_NODE_TYPES.Identifier && prop.key.name === "signal") return true; |
There was a problem hiding this comment.
hasSignalOption only checks that a signal key exists, so { signal: undefined } or { signal: null } is (wrongly) treated as compliant even though no abort deadline is actually wired up.
💡 Why this matters and how to fix
This is exactly the kind of pattern the rule is meant to catch: a lint-satisfying but functionally useless signal property left over from refactors, copy-paste, or a variable that resolves to undefined. Since the check is purely structural (key presence), it produces a false negative for the most likely accidental-regression case.
if (!prop.computed && prop.key.type === AST_NODE_TYPES.Identifier && prop.key.name === "signal") {
const isNullish =
prop.value.type === AST_NODE_TYPES.Identifier && prop.value.name === "undefined" ||
(prop.value.type === AST_NODE_TYPES.Literal && prop.value.value === null);
if (!isNullish) return true;
}Add invalid test cases for { signal: undefined } and { signal: null }.
| }, | ||
| }); | ||
|
|
||
| describe("require-fetch-timeout", () => { |
There was a problem hiding this comment.
Test suite has no coverage for the two biggest gaps in the rule: member-expression callees (globalThis.fetch) and nullish signal values, both of which the implementation handles incorrectly (see other comments on require-fetch-timeout.ts).
💡 Why this matters
A rule whose test suite only exercises bare-identifier fetch(...) calls and truthy signal values 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. Add invalid cases for globalThis.fetch(url) and fetch(url, { signal: undefined })/{ signal: null } once the corresponding fixes land, so regressions in those paths are caught automatically.
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (5 tests)
Quality Highlights✅ 100% design-focused tests — Every test verifies a user-visible behavioral contract:
✅ Excellent edge/error coverage — Tests use real ✅ Healthy test:production ratio — 61 test lines : 59 production lines (≈1.03:1), well under the 2:1 inflation threshold ✅ No violations — All tests use vitest + RuleTester (no forbidden Go mock libraries, no missing build tags) Verdict
|
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — requesting changes on two actionable issues (scope clarity + test structure).
📋 Key Themes & Highlights
Issues
- Scope mismatch: The rule description says "actions/setup/js scripts" but the implementation fires on every
fetchcall in any linted file. Theeslint.config.cjsenablement is global, not path-scoped. This is the most important correctness concern. - Test structure: Five separate
it + RuleTester.run()combos instead of one consolidated call — deviates from sibling test files and makes the case inventory harder to review. - Unspecified edge case:
fetch(url, null)silently passes as "safe" (falls into the non-object branch) — intentional or not, it should be documented and tested.
Positive Highlights
- ✅ Well-motivated: real production evidence cited with specific file+line references
- ✅ Sensible false-positive avoidance for spread/identifier options
- ✅ Clear, actionable error message with example fix
- ✅ PR description is thorough and validates against production files
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 32.5 AIC · ⊞ 7K
Comment /matt to run again
| }); | ||
|
|
||
| describe("require-fetch-timeout", () => { | ||
| it("valid: fetch with AbortSignal.timeout", () => { |
There was a problem hiding this comment.
[/tdd] Tests split across multiple it + cjsRuleTester.run() calls instead of a single RuleTester.run() with all valid/invalid cases. This deviates from the pattern in sibling test files and makes case inventory harder to scan.
💡 Suggested consolidation
Combine into one cjsRuleTester.run() inside the describe:
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 require-fetch-try-catch.test.ts is structured — it's a good reference.
@copilot please address this.
| CallExpression(node: TSESTree.CallExpression) { | ||
| const callee = node.callee; | ||
| if (callee.type !== AST_NODE_TYPES.Identifier || callee.name !== "fetch") return; | ||
|
|
There was a problem hiding this comment.
[/tdd] The rule fires on any top-level fetch call, not just those inside actions/setup/js. The docs.description says "in actions/setup/js scripts" but the implementation doesn't scope by filename. This means the rule will flag fetch in any JS file linted by the plugin — an undocumented expansion of scope.
💡 Options to consider
Option A — Narrow via include in eslint.config.cjs (no rule changes needed):
{ files: ['actions/setup/js/**/*.cjs'], rules: { 'gh-aw-custom/require-fetch-timeout': 'warn' } }Option B — Scope inside the rule using context.filename:
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.
| }); | ||
| }); | ||
|
|
||
| it("valid: fetch with spread options or identifier options object (unresolvable statically)", () => { |
There was a problem hiding this comment.
[/tdd] Missing edge case: fetch(url, null) or fetch(url, undefined) as the second argument. These are valid JS and would bypass the ObjectExpression branch, falling into the final return true (non-object = assume safe). If that's intentional, a comment and test would make it explicit.
💡 Suggested test
// Should this be valid or invalid?
{ code: `async function f() { const res = await fetch(url, null); }`, errors: [{ messageId: 'requireSignal' }] }If null/undefined should be treated the same as no options (i.e. flagged), the rule's hasSignalOption needs an extra check: if (optionsArg.type === AST_NODE_TYPES.Literal && optionsArg.value === null) return false;.
@copilot please address this.
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in commit 2bec43e. Changes made:
Validation run:
|
|
🎉 This pull request is included in a new release. Release: |
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Overview
Adds a new custom ESLint rule,
require-fetch-timeout, toeslint-factory. The rule requiresfetch(...)calls to include a non-nullishsignaloption so requests can be aborted instead of hanging indefinitely, which could otherwise block a CI job until the workflow/job timeout kills it.What changed
eslint-factory/src/rules/require-fetch-timeout.tseslint-factory/src/rules/require-fetch-timeout.test.tsRuleTestertest suiteeslint-factory/src/index.tsrequireFetchTimeoutRulein the plugin's rule mapeslint-factory/eslint.config.cjsgh-aw-custom/require-fetch-timeoutat"warn"eslint-factory/README.mdFollow-up commit
2bec43etightened edge-case handling after the initial rule was added.Key implementation details
isStaticallyNullish(node)— detects literalnull, theundefinedidentifier, or avoidunary expression.hasSignalOption(callExpression)— inspects the second argument (options) for a non-nullishsignalproperty. Spread arguments/properties and non-object (identifier/expression) options are conservatively treated as "signal may be present" to avoid false positives, since they can't be statically resolved.isGlobalFetchCall(callee)— matchesglobalThis.fetch/global.fetch, including computed access (global["fetch"]).hasLocalBinding(node, name)— walks the scope chain to detect shadowedfetchbindings (e.g. a function parameter or local variable namedfetch), so user-definedfetch-like functions aren't flagged.messageId: "requireSignal"when a globalfetchcall has no usablesignaloption.Edge cases handled
Flagged:
fetch(url);fetch(url, null);/fetch(url, undefined);fetch(url, { method: "GET" });fetch(url, { signal: null });globalThis.fetch(url, { method: "GET" });global["fetch"](url, { method: "GET" });Not flagged:
fetch(url, { signal: AbortSignal.timeout(10_000) });fetch(url, { signal: controller.signal });fetch(url, options);(identifier options object, not statically resolvable)fetch(url, { ...options });(spread may already includesignal)obj.fetch(url);(only globalfetchis in scope)fetchbindings (e.g.async function f(fetch) { await fetch(url); })Testing
New
RuleTester-based suite (require-fetch-timeout.test.ts) covers:AbortSignal.timeout,AbortController-backed signals, spread options, identifier options objects, non-fetch calls (e.g.axios.get), shadowed localfetchbindings.signal,globalThis/global["fetch"]member calls missingsignal, and explicitly nullishsignalvalues (null,undefined,void 0).