[eslint-miner] eslint: add require-return-after-core-setfailed rule#44697
Conversation
core.setFailed() marks the action as failed but does NOT stop execution. Calling it inside an if-branch without a following return/throw lets the action continue in a failed state, which can mask secondary errors or produce confusing output. This rule flags any core.setFailed() call that is immediately followed by a non-control-transfer statement in the same block. It provides a 'return;' autofix suggestion. Findings in actions/setup/js (2 sites), confirming the pattern exists. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #44697 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. ✅ |
There was a problem hiding this comment.
Pull request overview
Adds a new ESLint rule to the eslint-factory plugin that detects core.setFailed(...) calls which are immediately followed by another statement in the same statement list, helping prevent Actions code from continuing to execute after declaring failure.
Changes:
- Implement
require-return-after-core-setfailedrule with a suggestion to insertreturn;. - Add unit tests covering valid/invalid patterns and suggestion output.
- Register and enable the rule in the plugin index and ESLint config.
Show a summary per file
| File | Description |
|---|---|
| eslint-factory/src/rules/require-return-after-core-setfailed.ts | New rule implementation, including detection logic and a return; suggestion. |
| eslint-factory/src/rules/require-return-after-core-setfailed.test.ts | New tests validating rule behavior and the suggestion output. |
| eslint-factory/src/index.ts | Registers the new rule in the exported plugin rules map. |
| eslint-factory/eslint.config.cjs | Enables the new rule at warn level for .cjs files. |
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: 3
- Review effort level: Low
| messageId: "addReturn", | ||
| fix(fixer) { | ||
| return fixer.insertTextAfter(node, "\n return;"); | ||
| }, |
| missingReturnAfterSetFailed: | ||
| "core.setFailed() does not stop execution — add a 'return' (or 'throw') immediately after to prevent the action from continuing in a failed state.", | ||
| addReturn: "Add 'return;' after core.setFailed() to stop execution.", |
| it("invalid: core.setFailed followed by non-control-transfer statement", () => { | ||
| ruleTester.run("require-return-after-core-setfailed", requireReturnAfterCoreSetFailedRule, { | ||
| valid: [], | ||
| invalid: [ | ||
| { | ||
| code: `function f() { core.setFailed("bad"); doMore(); }`, | ||
| errors: [{ messageId: "missingReturnAfterSetFailed", suggestions: [{ messageId: "addReturn", output: `function f() { core.setFailed("bad");\n return; doMore(); }` }] }], | ||
| }, | ||
| { | ||
| code: `function f() { if (x) { core.setFailed("bad"); doMore(); } }`, | ||
| errors: [{ messageId: "missingReturnAfterSetFailed", suggestions: [{ messageId: "addReturn", output: `function f() { if (x) { core.setFailed("bad");\n return; doMore(); } }` }] }], | ||
| }, |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 88/100 — Excellent
📊 Metrics (5 tests)
Verdict✅ Passed. 0% implementation tests (threshold: 30%). All tests verify design contracts and behavioral guarantees. Comprehensive coverage of happy path, error conditions, edge cases (scope boundaries, method/object discrimination), and autofix validation. 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 /tdd and /codebase-design — requesting changes on three actionable issues.
📋 Key Themes & Highlights
Issues Found
- Hardcoded indentation in autofix — the inserted
return;uses fixed 6-space indentation, which will be wrong for most real-world code. SwitchCasepath is untested — the rule registers aSwitchCasevisitor but no test exercises it.Programbody not covered — top-levelcore.setFailed()calls in CommonJS scripts won't be flagged.
Positive Highlights
- ✅ Clean helper-function design:
isCoreSetFailedStatementandisControlTransferare well-scoped and readable - ✅ Good block-statement test coverage, including
break,throw,process.exit, and last-in-block edge cases - ✅ Low false-positive risk by design — only fires on explicit next-sibling statements
- ✅ Validated against two real live findings in
actions/setup/js
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 36 AIC · ⌖ 4.68 AIC · ⊞ 6.6K
Comment /matt to run again
| node, | ||
| messageId: "missingReturnAfterSetFailed", | ||
| suggest: [ | ||
| { |
There was a problem hiding this comment.
[/tdd] The hardcoded "\n return;" indentation in the autofix suggestion will produce incorrectly-indented output for any code that isn't exactly 6 spaces deep — undermining trust in the suggestion.
💡 Use column-aware indentation
Derive indentation from the node's start column:
fix(fixer) {
const src = context.getSourceCode();
const token = src.getFirstToken(node)!;
const indent = " ".repeat(token.loc.start.column);
return fixer.insertTextAfter(node, `\\n${indent}return;`);
}This ensures the inserted return; is aligned with the core.setFailed() call regardless of surrounding indentation.
@copilot please address this.
| ], | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[/tdd] The test suite is missing coverage for core.setFailed() inside a switch case body — the rule explicitly handles SwitchCase nodes, but there's no test exercising that path.
💡 Add a switch-case invalid test
{
code: `switch(x) { case 'a': core.setFailed('bad'); doMore(); break; }`,
errors: [{ messageId: "missingReturnAfterSetFailed" }],
}And a valid counterpart:
`switch(x) { case 'a': core.setFailed('bad'); break; }`Without this, the SwitchCase visitor in the implementation is untested.
@copilot please address this.
| }, | ||
| // Handle single-statement arrow functions (no braces) — rare but safe to skip | ||
| // The main case is BlockStatement above. | ||
| SwitchCase(node: TSESTree.SwitchCase) { |
There was a problem hiding this comment.
[/codebase-design] Top-level ExpressionStatement bodies (module-level core.setFailed() followed by more statements) won't be caught — the rule only walks BlockStatement and SwitchCase, missing the Program body.
💡 Add a Program visitor
Program(node: TSESTree.Program) {
checkStatementList(node.body as TSESTree.Statement[], report);
},In CommonJS scripts, top-level code is common. Without this, a module-level core.setFailed() followed by more code won't be flagged.
@copilot please address this.
|
@copilot please run the
|
…ix and coverage Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed remaining in-scope review feedback in 86a8b1c. Updated the rule/fixer behavior (including Program coverage and safer suggestion behavior), expanded tests for multiline/trailing-comment/switch/program cases, and reran package-level checks ( |
|
@copilot please run the
|
pelikhan
deleted the
eslint-miner/require-return-after-core-setfailed-24ecd14774ea90dc
branch
|
🎉 This pull request is included in a new release. Release: |
Summary
Adds a new ESLint rule
gh-aw-custom/require-return-after-core-setfailedto theeslint-factoryplugin. The rule enforces that every call tocore.setFailed()is immediately followed by a control-transfer statement (return,throw,break,continue, orprocess.exit()), preventing GitHub Actions code from continuing execution in a failed state.Motivation
core.setFailed()only schedules the action exit code — it does not stop execution. Code that continues aftercore.setFailed()runs in a failed state, which is typically a bug.Changes
New rule:
require-return-after-core-setfailedeslint-factory/src/rules/require-return-after-core-setfailed.ts— Rule implementation using@typescript-eslint/utils:core.setFailed(...)inBlockStatement,SwitchCase, andProgramnodes.core.setFailed()call whose immediate sibling statement is not a control-transfer.hasSuggestionsautofix that insertsreturn;before the next statement, preserving indentation for multiline cases.returnis invalid).eslint-factory/src/rules/require-return-after-core-setfailed.test.ts— Vitest + RuleTester test suite:core.setFailed()followed byreturn,throw,break,continue,process.exit();core.setFailed()as last statement in a block; non-core.setFailed()method calls.core.setFailed()followed by any regular statement in block, switch-case, and program scope; autofix output verified for same-line and multiline variants.core.setFailed()as last statement in anif-block where the outer block continues — not flagged (the rule only checks sibling statements in the same block).Registration
eslint-factory/src/index.ts— imports and registers the new rule in the plugin.eslint-factory/eslint.config.cjs— enables the rule at"warn"severity in the default config.Commits
0fcba1de6eslint: add require-return-after-core-setfailed rule86a8b1ccffix(eslint-factory): harden require-return-after-core-setfailed autofix and coverage