fix(eslint-factory): require-async-entrypoint-catch — track async arrow/fn-expr entrypoints and flag .then() without .catch()#43356
Conversation
…then() without .catch() Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
1 similar comment
|
Hey This PR is still a draft with no committed changes yet, so a few things will be needed before it's ready for review:
If you'd like a hand completing the implementation, here's a ready-to-use prompt:
|
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43356 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR improves the require-async-entrypoint-catch ESLint rule in eslint-factory by expanding what it considers an async “entrypoint” and by detecting promise chains (.then(...)) that omit a rejection handler, reducing false negatives when linting GitHub Actions scripts.
Changes:
- Track module-scope async entrypoints declared via
const/let/var X = async () => {}andconst/let/var X = async function() {}. - Detect and flag
main().then(...)-style chains without a.catch(...)handler, while allowing chains that are already caught. - Add new tests covering async arrow/function-expression entrypoints and
.then()-without-.catch()cases.
Show a summary per file
| File | Description |
|---|---|
| eslint-factory/src/rules/require-async-entrypoint-catch.ts | Adds module-scope async variable entrypoint tracking and extends call detection to handle chained promise calls. |
| eslint-factory/src/rules/require-async-entrypoint-catch.test.ts | Adds new valid/invalid cases to verify the new entrypoint tracking and .then() chain behavior. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Low
| VariableDeclaration(node) { | ||
| if (node.parent.type !== AST_NODE_TYPES.Program) return; | ||
| for (const declarator of node.declarations) { |
| /** Returns true if the outermost call in the chain ends with `.catch(...)`. */ | ||
| function chainEndsWithCatch(node: TSESTree.CallExpression): boolean { | ||
| const callee = node.callee; | ||
| if (callee.type === AST_NODE_TYPES.MemberExpression) { | ||
| const prop = callee.property; | ||
| return prop.type === AST_NODE_TYPES.Identifier && prop.name === "catch"; | ||
| } | ||
| return false; | ||
| } |
| // Chained call: main().then(...) etc. | ||
| // If the chain ends with .catch(...), it's handled — skip. | ||
| if (chainEndsWithCatch(node)) return; |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (6 tests)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — commenting with test coverage gaps and one logic-clarity note. No blocking issues; the core fix is sound.
📋 Key Themes & Highlights
Key Themes
- Test coverage gaps (minor): The new
VariableDeclarationhandler's scope guard (parent.type !== Program) and the recursivegetRootCallNamepath have partial test coverage. A few additional edge-case tests would lock in the invariants (see inline comments). - Logic clarity: The relationship between
chainEndsWithCatch(checks the outermost node) andgetRootCallName(walks inward) is non-obvious — a short comment would help future readers.
Positive Highlights
- ✅ Both false-negative gaps are correctly identified in the PR description with clear before/after examples
- ✅
chainEndsWithCatch+getRootCallNameare clean, pure helper functions — easy to unit-test independently if needed - ✅ The
VariableDeclarationhandler correctly guards onnode.parent.type === Programanddeclarator.init.async, mirroring the existingFunctionDeclarationguard precisely - ✅ 140 lines of tests added covering all acceptance criteria paths — the signal-to-noise ratio on the test descriptions is excellent
- ✅ The autofix suggestion correctly appends
.catch(...)after the full expression node, which works for both bare and chained calls without special-casing
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 109 AIC · ⌖ 6.35 AIC · ⊞ 6.6K
Comment /matt to run again
Comments that could not be inline-anchored
eslint-factory/src/rules/require-async-entrypoint-catch.test.ts:80
[/tdd] Missing valid test: non-module-scope async arrow should not be flagged — this directly validates the node.parent.type !== Program guard added to the new VariableDeclaration handler.
<details>
<summary>💡 Suggested test case to add to the existing "valid: async function called as part of an expression" group</summary>
// Nested async arrow is not a module-scope entrypoint — should not be tracked
`function setup() {
const inner = async () => { return 42; }
inner();
}`…
</details>
<details><summary>eslint-factory/src/rules/require-async-entrypoint-catch.ts:12</summary>
**[/diagnosing-bugs]** `chainEndsWithCatch` only checks the immediate callee — it misses the intermediate `.then()` in `main().then(cb).catch(err)`. However, the current logic treats that chain as **valid** (it returns early), which is correct behaviour. The concern is whether deeply nested chains like `main().then(cb1).then(cb2)` without `.catch()` are properly flagged.
<details>
<summary>💡 Trace through the logic for `main().then(cb1).then(cb2)`</summary>
For `main().then(cb1).then(cb2)` a…
</details>
<details><summary>eslint-factory/src/rules/require-async-entrypoint-catch.test.ts:296</summary>
**[/tdd]** Missing invalid test: a multi-step chain ending in `.then()` (e.g. `main().then(cb1).then(cb2)`) is not covered — this exercises `getRootCallName`'s recursive path more thoroughly.
<details>
<summary>💡 Suggested test case to add to the "invalid: .then() chain without .catch()" group</summary>
Add a third case with a double-chained `.then().then()` to the existing invalid test group:
```ts
{
code: `async function main() {}\nmain().then(() => {}).then(() => process.exit(0));`,
…
</details>
<details><summary>eslint-factory/src/rules/require-async-entrypoint-catch.ts:101</summary>
**[/diagnosing-bugs]** The `ExpressionStatement > CallExpression` handler only handles `Identifier` and `MemberExpression` callees, but silently ignores computed member expressions like `obj["catch"](err)`. This is fine in practice (the rule targets simple entrypoint patterns), but worth a brief comment explaining the intentional narrowness so future maintainers don't add a false-positive case.
<details>
<summary>💡 Suggested clarifying comment</summary>
```ts
// Only handle Identifier (bare …
</details>
<details><summary>eslint-factory/src/rules/require-async-entrypoint-catch.test.ts:61</summary>
**[/tdd]** Missing valid test: `const main = async () => {}; main();` inside an async IIFE — i.e. `(async () => { await main(); })()` already exists (line 73), but a variant where the async arrow is called *without* await inside an async context is absent. That is the case exercised by the existing `FunctionDeclaration` test at line 62: `const run = async () => { main(); }`. Adding the async-arrow analogue would fully close the gap.
<details>
<summary>💡 Suggested test case to add to the "vali…
</details>
There was a problem hiding this comment.
Review: require-async-entrypoint-catch — async arrow/fn-expr tracking + .then() without .catch()
The approach is sound and the two gaps (missing arrow/fn-expression entrypoints, and .then() chains without .catch()) are correctly addressed. The implementation is clean and the tests cover the main acceptance criteria.
Issues found:
| # | Severity | Location | Summary |
|---|---|---|---|
| 1 | Low | getRootCallName, line 26 |
Unbounded recursion on deep chains — add a depth guard |
| 2 | Low | Call handler, line 130 | Add a test for main().then(() => main()) to confirm no double-fire |
| 3 | Info | Line 112 | .catch(err => { throw err; }) is accepted — document intentional scope |
| 4 | Info | Line 84 | Reassigned-async-variable gap — document as known limitation |
| 5 | Info | Line 109 | Non-Identifier/MemberExpression callees silently skipped — add comment |
Correctness assessment
- The
VariableDeclarationhandler correctly gates onnode.parent.type === AST_NODE_TYPES.Program— only module-scope declarations are tracked. ✅ - The
"ExpressionStatement > CallExpression"selector uses the CSS child combinator, so only direct-childCallExpressionnodes match. Nested callbacks do not trigger the rule. ✅ chainEndsWithCatchlooks only at the outermost method name; it does not traverse into the callback body. This is intentional and correct for a syntactic rule. ✅- The suggestion fixer calls
fixer.insertTextAfter(node, ".catch(...)")wherenodeis the outermostCallExpression. Because the semicolon belongs to the parentExpressionStatement, the insertion lands before the semicolon — producing valid output. ✅ (verified by test expectations) getRootCallNamehandlesmain().then(cb)→"main", andmain().then(cb).catch(err)→ caught beforegetRootCallNameis called. ✅
No blocking correctness bugs were found. Inline comments above are non-blocking improvements.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 133.6 AIC · ⌖ 6.6 AIC · ⊞ 4.9K
Comments that could not be inline-anchored
eslint-factory/src/rules/require-async-entrypoint-catch.ts:130
Potential double-fire: ExpressionStatement > CallExpression matches every descendant CallExpression.
The selector "ExpressionStatement > CallExpression" (with the CSS child combinator >) only matches CallExpression nodes that are direct children of an ExpressionStatement, so nested calls like the inner main() inside main().then(() => main()) will NOT match — they are children of an ArrowFunctionExpression, not an ExpressionStatement. That is correct behaviour.
Howeve…
eslint-factory/src/rules/require-async-entrypoint-catch.ts:26
Unbounded recursion risk in getRootCallName.
For deeply nested method chains like main().a().b().c().d(), each level recurses into the next inner CallExpression. In practice chains are shallow, but there is no depth guard. For a linter called on arbitrary user code, an adversarially long chain (e.g. from generated code) could produce a stack overflow. Consider adding a max-depth counter (e.g. 20 levels) and returning null when exceeded.
function getRootCallName(node: TSESTre…
</details>
<details><summary>eslint-factory/src/rules/require-async-entrypoint-catch.ts:112</summary>
**False negative: `main().catch(err => { throw err; })` re-throws but is accepted as valid.**
`chainEndsWithCatch` returns `true` for any call whose outermost method is `.catch()`, regardless of what the handler does (e.g. `throw err`, which re-throws). Re-throwing converts a rejected promise into a thrown exception rather than calling `process.exit`, which may silently fail in GitHub Actions scripts. This is intentional (the rule only enforces the presence of `.catch`, not its body), but it s…
</details>
<details><summary>eslint-factory/src/rules/require-async-entrypoint-catch.ts:84</summary>
**Missing: reassigned async variable is not tracked.**
The `VariableDeclaration` handler only picks up declarations of the form `const/let/var X = async () => {}`. If code later reassigns an existing variable (`let X = () => {}; X = async () => {}`), the reassignment (an `AssignmentExpression`) is never seen. This is a pre-existing gap (same limitation existed for `FunctionDeclaration`), but since this PR is specifically expanding entrypoint tracking, it's worth noting as a known limitation wi…
</details>
<details><summary>eslint-factory/src/rules/require-async-entrypoint-catch.ts:109</summary>
**False negative: computed member access `main[method]()` is silently ignored.**
When `callee` is a `MemberExpression` with a computed property (`main['then']()`), `chainEndsWithCatch` checks `prop.type === AST_NODE_TYPES.Identifier` which returns `false` for a `Literal`, so it falls through to `return false`. Then `getRootCallName` is called and correctly walks back to find `'main'`. The final check `asyncFunctionNames.has(name)` returns `true`, so the node IS flagged — but this is correct be…
</details>
|
@copilot please run the Unresolved review feedback to address:
|
…lly chains Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the outstanding Copilot feedback in commit Changes made:
Validation run locally:
I couldn’t refresh PR/check metadata from |
|
@copilot please run the Unresolved review feedback to address:
|
Addressed in I re-ran local checks on the current branch:
No additional code changes were needed. |
🤖 PR Triage — Run §28706384347
Score breakdown: impact 35 + urgency 20 + quality 14 Closes two false-negative gaps in
|
|
@copilot please run the Unresolved review feedback to address:
Run: https://github.com/github/gh-aw/actions/runs/28707474242
|
Addressed by Re-ran:
Also reviewed the referenced run ( |
|
@copilot please run the
|
Addressed in
All passed locally, and the latest PR check runs for this head are successful/in progress. |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot please
|
Reviewed the outstanding feedback and revalidated this branch; no additional code changes were needed beyond In this run I verified the rule/test updates are present, checked PR review thread status, inspected the latest CI signal, and reran:
All local checks passed. |
require-async-entrypoint-catchhad two false-negative gaps: it only trackedFunctionDeclarationentrypoints (missingconst main = async () => {}andconst run = async function() {}), and its call handler bailed early on any non-Identifiercallee, silently acceptingmain().then(cb)chains with no rejection handler.Rule changes (
require-async-entrypoint-catch.ts)Gap 1 — async variable entrypoints: added a
VariableDeclarationhandler that registers module-scopeconst/let/var X = async () => {}andconst/let/var X = async function() {}intoasyncFunctionNames, alongside existingFunctionDeclarationtracking.Gap 2 —
.then()without.catch(): replaced the earlyIdentifier-only guard with aMemberExpressionbranch in theExpressionStatement > CallExpressionhandler. Adds two helpers:chainEndsWithCatch(node)— returns true if the outermost call's property iscatchgetRootCallName(node)— recursively walks a chainedCallExpressionto resolve the root identifierChains terminating in
.catch(...)remain valid; chains ending in.then(...)(or any non-catch method) are flagged.Tests (
require-async-entrypoint-catch.test.ts)Added four new test groups covering the acceptance criteria:
.catch(),.then().catch()chain, async arrow awaited inside async context, sync arrow.then()without.catch()on bothFunctionDeclarationand arrow entrypoints — each with expected suggestion output