[eslint-miner] fix(eslint): extend require-fetch-try-catch to detect member-chained fetch calls

[github-actions]

Summary

Fixes a false negative in the require-fetch-try-catch ESLint rule where await fetch(url).then(...), await fetch(url).json(), and similar member-chained patterns silently escaped detection.

Problem

The original isAwaitFetchCall function only matched an AwaitExpression whose argument was a CallExpression with a bare fetch Identifier callee:

return callee.type === AST_NODE_TYPES.Identifier && callee.name === "fetch";

Any method chain moved the callee to a MemberExpression, bypassing the rule entirely. For example:

• await fetch(url).then(r => r.json()) — not detected (should be flagged)
• await fetch(url).json() — not detected (should be flagged)
• await fetch(url).catch(handler) — not detected (already safe, should NOT be flagged)

This was documented in issues #47950 and #47932.

Solution

Two new helpers:

1. getRootFetchCall() — walks down a call/member chain and returns the root fetch(...) CallExpression if fetch is at the root, otherwise returns null.
2. chainHasRejectionHandler() — checks whether the chain includes a .catch(handler) or a two-argument .then(onFulfilled, onRejected), which satisfy the error-handling requirement.

The AwaitExpression visitor now:

• Detects await fetch(url).method(...) chains as fetch calls
• Skips chains that already have a rejection handler
• Still respects enclosing try/catch

Evidence from codebase

validate_secrets.cjs:65,85 use await fetch(...).catch(rethrowAbortError) — this shape was silently ignored before. With this fix it remains not flagged (has a rejection handler).

Tests

Added 3 new test groups:

• valid: .catch() and two-arg .then() chains are not flagged
• invalid: one-arg .then() and response method chains (.json()) are flagged
• valid: chained fetch inside try block is not flagged

All 11 require-fetch-try-catch tests pass. Pre-existing require-fs-io-try-catch failures are unrelated.

Validation

cd eslint-factory && npm run build   # ✓ 0 errors
cd eslint-factory && npm run lint:setup-js  # ✓ 0 errors, 32 pre-existing warnings
vitest run require-fetch-try-catch.test.ts  # ✓ 11/11 passed

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• proxy.golang.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "proxy.golang.org"

See Network Configuration for more information.

Generated by ESLint Miner · sonnet46 · 76.5 AIC · ⌖ 9.64 AIC · ⊞ 4.5K · ◷

• expires on Aug 1, 2026, 1:25 AM UTC-08:00

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions in default business logic directories).

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[Copilot]

Pull request overview

Extends the ESLint rule to detect unprotected chained fetch() calls.

Changes:

• Traverses member-call chains rooted at fetch().
• Recognizes .catch() and two-argument .then() handlers.
• Adds chained-fetch tests.

Show a summary per file

File	Description
require-fetch-try-catch.ts	Adds chained-fetch detection and rejection-handler checks.
require-fetch-try-catch.test.ts	Tests protected and unprotected fetch chains.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (2)

eslint-factory/src/rules/require-fetch-try-catch.ts:49

• Arity does not prove that a rejection handler exists. Promise methods ignore non-callable handlers, so await fetch(url).catch(undefined) and await fetch(url).then(ok, null) still propagate the fetch rejection but are exempted here. Inspect the relevant argument rather than only its presence (at minimum rejecting undefined, null, and other statically non-callable literals), and add invalid tests for these cases.

    if (name === "catch" && call.arguments.length >= 1) return true;
    if (name === "then" && call.arguments.length >= 2) return true;

eslint-factory/src/rules/require-fetch-try-catch.ts:64

• Optional member chains are wrapped in a ChainExpression, so await fetch(url)?.then(ok) returns here without detection even though the fetch rejection remains unhandled. Unwrap ChainExpression before checking for a call, and use that same unwrapped call as outerCall in the visitor.

  const argument = node.argument;
  if (argument.type !== AST_NODE_TYPES.CallExpression) return false;
  return getRootFetchCall(argument) !== null;

• Files reviewed: 2/2 changed files
• Comments generated: 3
• Review effort level: Medium

[github-actions]

🧪 Test Quality Sentinel Report

✅ Test Quality Score: 90/100 — Excellent

Analyzed 2 test(s): 2 design, 0 implementation, 0 violation(s).

📊 Metrics (2 tests)

Metric	Value
Analyzed	2 (Go: 0, JS/TS: 2)
✅ Design	2 (100%)
⚠️ Implementation	0 (0%)
Edge/error coverage	2 (100%)
Duplicate clusters	0
Inflation	No (0.48:1 ratio)
🚨 Violations	0

Test Breakdown

New Tests Added:

1. ✅ "valid: await fetch chain with .catch() rejection handler" (require-fetch-try-catch.test.ts:128)

   • Classification: behavioral_contract / design_test / high_value
   • Verifies the fix: rejection handlers (.catch() and .then(ok, err)) are now recognized and don't trigger the rule
   • Covers 3 valid code patterns including .catch(), .catch() with rethrow, and two-arg .then()
   • ✅ Direct test of new chainHasRejectionHandler() logic

2. ✅ "invalid: await fetch chain without rejection handler is flagged" (require-fetch-try-catch.test.ts:139)

   • Classification: behavioral_contract / design_test / high_value
   • Verifies chains WITHOUT rejection handlers are still flagged (maintains rule fidelity)
   • Covers edge cases: .then(x => x.json()) (one-arg .then) and .json() (member chain without .then)
   • ✅ Direct test of new getRootFetchCall() chain-walking logic

Verdict

✅ Passed. 0% implementation tests (threshold: 30%). Both tests verify user-facing rule behavior and the specific fix: detecting fetch member chains and properly handling rejection handlers. Test-to-code ratio 0.48:1 (well under 2:1). No violations detected.

🧪 Test quality analysis by Test Quality Sentinel · haiku45 · 13.4 AIC · ⌖ 10.6 AIC · ⊞ 8.1K · ◷
_{Comment /review to run again}

[github-actions]

✅ Test Quality Sentinel: 90/100. 0% implementation tests (threshold: 30%). Both new tests verify user-facing rule behavior with solid design coverage and boundary testing. No violations detected.

[github-actions]

The implementation correctly extends require-fetch-try-catch to detect member-chained fetch patterns. The two helper functions are clean, the logic is sound, and test coverage addresses the key cases. No blocking issues found.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 19.6 AIC · ⌖ 5.13 AIC · ⊞ 5K

[github-actions]

Skills-Based Review 🧠

Applied /diagnosing-bugs and /tdd — solid fix overall, a few small improvements suggested.

📋 Key Themes & Highlights

Key Themes

• Loop sentinel fragility (chainHasRejectionHandler): the current !== fetchCall identity check is correct today but relies on an implicit invariant (same node reference) that is not documented and could silently break if the AST helpers change.
• Test coverage gap: multi-hop chain (fetch().then(...).catch(handler)) is not exercised, leaving the loop body untested past its first iteration.
• Test naming: "valid: ..." / "invalid: ..." prefixes describe outcome rather than behaviour — spec-style names would make failures self-documenting.

Positive Highlights

• ✅ Root-cause fix: getRootFetchCall correctly unifies bare and chained fetch detection.
• ✅ chainHasRejectionHandler correctly handles both .catch() and two-argument .then().
• ✅ Existing validate_secrets.cjs patterns are explicitly validated in the PR description.
• ✅ 11/11 tests pass; no regression to pre-existing behavior.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 37.1 AIC · ⌖ 4.88 AIC · ⊞ 6.7K
_{Comment /matt to run again}

[github-actions]

[/tdd] Multi-hop chain fetch().then(...).catch(handler) is not tested — add a valid case to prevent regressions.

💡 Suggested test case

chainHasRejectionHandler walks each link; a 3-hop chain exercises the loop past its first iteration. A future refactor breaking multi-hop traversal would silently pass all current tests.

// add to valid: block in "await fetch chain with .catch() rejection handler"
`async function f() { const r = await fetch(url).then(x => x).catch(handler); }`,

@copilot please address this.

[github-actions]

[/diagnosing-bugs] chainHasRejectionHandler uses object identity (current !== fetchCall) as the loop sentinel, which is fragile — if getRootFetchCall ever returns a different node object for the same AST node, the loop will over-traverse and silently skip the handler check.

💡 Safer alternative

Consider stopping the loop by position/type rather than identity. A simple guard is to break when current is the same CallExpression as fetchCall by checking current === fetchCall before the member-walk, or by tracking depth from getRootFetchCall. Adding a comment explaining the identity invariant would also help future maintainers.

// Add a safety comment at the sentinel check
while (current !== fetchCall) { // relies on the same node reference returned by getRootFetchCall

@copilot please address this.

[github-actions]

[/tdd] Test description says "valid" but does not encode why — consider renaming to describe the expected behaviour as a specification, not just a status label.

💡 Example rename

// before
it("valid: await fetch chain with .catch() rejection handler", ...)

// after — reads as a spec
it("does not flag await fetch chains that have a .catch() rejection handler", ...)
it("does not flag await fetch chains that pass an error handler to .then()", ...)

Spec-style names make test output self-documenting and reduce the need to read the test body to understand intent.

@copilot please address this.

[github-actions]

REQUEST_CHANGES — two correctness bugs must be fixed before merge

The chain-detection improvement is solid and the approach is correct, but two false-negative bugs in chainHasRejectionHandler and one false positive need to be addressed.

Blocking issues (3)

1. .catch(null) is accepted as a valid handler (line 88) — arguments.length >= 1 is satisfied by null, which is a no-op per spec. Fix: check that the argument is not a null literal.
2. .then(fn, null) is accepted as a valid rejection handler (line 89) — same root cause; the second argument being null leaves the rejection unhandled.
3. fetch(url)['catch'](handler) produces a false positive (line 87) — bracket-notation method calls use a Literal property node, not an Identifier, so name is null and the chain is never recognized as safe. Fix: also extract the name from string-literal computed properties.

Additionally: the double invocation of getRootFetchCall (once inside isAwaitFetchChain, once via the ! assertion on line 117) is a latent maintenance hazard — consolidating to one call would be cleaner.

Test gaps: no coverage for .catch(null), .then(fn, null), or multi-hop chains like .then(r=>r.json()).catch(handler).

🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 45.5 AIC · ⌖ 5.12 AIC · ⊞ 5.7K
_{Comment /review to run again}

Comments that could not be inline-anchored

eslint-factory/src/rules/require-fetch-try-catch.ts:88

False negative: .catch(null) counts as a valid rejection handler — call.arguments.length >= 1 is true for .catch(null), but null is a no-op per the Promises spec and the rejection will still propagate unhandled.

<details>
<summary>💡 Suggested fix</summary>

Add a null/undefined literal guard before accepting the argument as a real handler:

const isNullish = (n: TSESTree.Node) =>
  n.type === AST_NODE_TYPES.Literal && (n as TSESTree.Literal).value === null;

if (name === "ca…

</details>

<details><summary>eslint-factory/src/rules/require-fetch-try-catch.ts:117</summary>

**Double-call redundancy creates a maintenance trap** — `getRootFetchCall(outerCall)!` is called after `isAwaitFetchChain` already invoked it internally. If one function is modified without the other, the `!` assertion will hide a null-dereference at runtime.

&lt;details&gt;
&lt;summary&gt;💡 Suggested fix&lt;/summary&gt;

Consolidate to a single call: return the `CallExpression | null` directly from the check instead of throwing it away and recalling:

```ts
AwaitExpression(node) {
  if (node.type !== AST_NODE…

</details>

<details><summary>eslint-factory/src/rules/require-fetch-try-catch.ts:87</summary>

**False positive: `fetch(url)[&#39;catch&#39;](handler)` is flagged as missing a rejection handler** — bracket-notation method calls (`computed: true` MemberExpression) are not recognized because the property is a `Literal`, not an `Identifier`, so `name` is `null`.

&lt;details&gt;
&lt;summary&gt;💡 Suggested fix&lt;/summary&gt;

Extract the method name from both identifier and string-literal computed properties:

```ts
const prop = member.property;
const name =
  prop.type === AST_NODE_TYPES.Identifier
    ? (prop as …

</details>

<details><summary>eslint-factory/src/rules/require-fetch-try-catch.test.ts:17</summary>

**Missing test cases for `null` handler arguments** — the test suite has no coverage for `.catch(null)` or `.then(fn, null)`, which are currently false negatives (the rule incorrectly suppresses them).

&lt;details&gt;
&lt;summary&gt;💡 Suggested additions to the invalid group&lt;/summary&gt;

```ts
{
  code: `async function f() { await fetch(url).catch(null); }`,
  errors: [{ messageId: &quot;requireTryCatch&quot; }],
},
{
  code: `async function f() { await fetch(url).then(r =&gt; r.json(), null); }`,
  errors: [{ messageI…

</details>

[pelikhan]

@copilot run pr-finisher skill

[Copilot]

@copilot run pr-finisher skill

All three review items addressed in commit fix(eslint): address review feedback on require-fetch-try-catch rule:

1. Computed member fix: Added !member.computed guard in chainHasRejectionHandler so fetch(url)[then](ok, err) is not misidentified as a two-argument .then() rejection handler. Added test cases to cover this.
2. README documentation: Added require-fetch-try-catch to the rules table and added a full section documenting detected forms, exemptions (.catch(), two-arg .then(), try-block), and safe alternatives.
3. Chained-fetch-in-try test: Added valid test case valid: chained fetch inside try block is not flagged covering await fetch(url).then(x => x.json()) and await fetch(url).json() inside a try/catch.

All 13 tests pass locally.

[pelikhan]

@copilot resolve the merge conflicts in this pull request

[gh-aw-bot]

@copilot this branch has merge conflicts. Please run make merge-main to merge the latest main branch and resolve any conflicts, then push the result.

There is also a blocking code-quality review requesting follow-up on nullish rejection handlers and bracket-notation .catch() detection.

Run: https://github.com/github/gh-aw/actions/runs/30155206984

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 9.98 AIC · ⌖ 8.65 AIC · ⊞ 7.1K · ◷
_{Comment /souschef to run again}

[pelikhan]

@copilot merge main and recompile

[Copilot]

@copilot resolve the merge conflicts in this pull request

Merge conflicts resolved. Main had landed two related commits (#47963, #47969) that extended the same rule with a more comprehensive implementation (getAwaitedFetchInfo, isStaticallyNonCallable, optional chain support). I accepted main's version for all three conflicted files — it supersedes the helpers added in this PR while preserving the core intent. All 12 tests pass locally.

[Copilot]

@copilot merge main and recompile

Done. Main was already merged in the previous commit. Ran make recompile — all 262 workflow lock files were already in sync, no changes needed. Applied prettier formatting via make fmt (commit style(eslint): apply prettier formatting to require-fetch-try-catch.ts). All 12 require-fetch-try-catch tests pass.

[github-actions]

🎉 This pull request is included in a new release.

Release: v0.83.3