Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions eslint-factory/eslint.config.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ module.exports = [
"gh-aw-custom/require-fs-sync-try-catch": "warn",
"gh-aw-custom/require-json-parse-try-catch": "warn",
"gh-aw-custom/require-parseInt-radix": "warn",
"gh-aw-custom/require-return-after-core-setfailed": "warn",
},
},
{
Expand Down
2 changes: 2 additions & 0 deletions eslint-factory/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { requireFsSyncTryCatchRule } from "./rules/require-fs-sync-try-catch";
import { requireJsonParseTryCatchRule } from "./rules/require-json-parse-try-catch";
import { requireErrorCauseInRethrowRule } from "./rules/require-error-cause-in-rethrow";
import { requireParseIntRadixRule } from "./rules/require-parseInt-radix";
import { requireReturnAfterCoreSetFailedRule } from "./rules/require-return-after-core-setfailed";

const plugin = {
meta: {
Expand All @@ -31,6 +32,7 @@ const plugin = {
"require-fs-sync-try-catch": requireFsSyncTryCatchRule,
"require-json-parse-try-catch": requireJsonParseTryCatchRule,
"require-parseInt-radix": requireParseIntRadixRule,
"require-return-after-core-setfailed": requireReturnAfterCoreSetFailedRule,
},
};

Expand Down
101 changes: 101 additions & 0 deletions eslint-factory/src/rules/require-return-after-core-setfailed.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
import { RuleTester } from "eslint";
import { describe, expect, it } from "vitest";
import { requireReturnAfterCoreSetFailedRule } from "./require-return-after-core-setfailed";

const ruleTester = new RuleTester({
languageOptions: {
ecmaVersion: 2022,
sourceType: "commonjs",
},
});

describe("require-return-after-core-setfailed", () => {
it("uses the correct docs URL", () => {
expect(requireReturnAfterCoreSetFailedRule.meta.docs.url).toBe("https://github.com/github/gh-aw/tree/main/eslint-factory#require-return-after-core-setfailed");
});

it("valid: core.setFailed followed by return", () => {
ruleTester.run("require-return-after-core-setfailed", requireReturnAfterCoreSetFailedRule, {
valid: [
`function f() { core.setFailed("bad"); return; }`,
`function f() { core.setFailed("bad"); return null; }`,
`function f() { if (x) { core.setFailed("bad"); return; } }`,
`function f() { core.setFailed("bad"); throw new Error("bad"); }`,
`function f() { core.setFailed("bad"); process.exit(1); }`,
`function f() { for (;;) { core.setFailed("bad"); break; } }`,
`switch (x) { case "a": core.setFailed("bad"); break; }`,
// setFailed is the last statement in the block — no next statement to check
`function f() { core.setFailed("bad"); }`,
`function f() { if (x) { core.setFailed("bad"); } }`,
],
invalid: [],
});
});

it("valid: non-core.setFailed calls are ignored", () => {
ruleTester.run("require-return-after-core-setfailed", requireReturnAfterCoreSetFailedRule, {
valid: [`function f() { other.setFailed("bad"); doMore(); }`, `function f() { core.setOutput("x", 1); doMore(); }`, `function f() { setFailed("bad"); doMore(); }`],
invalid: [],
});
});

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"); 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"); return; doMore(); } }` }] }],
},
{
code: `function f() {
if (x) {
core.setFailed("bad"); // keep with setFailed
doMore();
}
}`,
errors: [
{
messageId: "missingReturnAfterSetFailed",
suggestions: [
{
messageId: "addReturn",
output: `function f() {
if (x) {
core.setFailed("bad"); // keep with setFailed
return;
doMore();
}
}`,
},
],
},
],
},
{
code: `switch (x) { case "a": core.setFailed("bad"); doMore(); break; }`,
errors: [{ messageId: "missingReturnAfterSetFailed" }],
},
{
code: `core.setFailed("bad");
doMore();`,
errors: [{ messageId: "missingReturnAfterSetFailed", suggestions: undefined }],
},
Comment on lines +42 to +87
],
});
});

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

it("valid: core.setFailed last in if-block is not flagged (outer block continues)", () => {
ruleTester.run("require-return-after-core-setfailed", requireReturnAfterCoreSetFailedRule, {
valid: [
// setFailed is the last statement in the if-block; no sibling in the same block follows it
`function f() { if (!ok) { core.setFailed("msg"); } doMore(); }`,
],
invalid: [],
});
});
});
141 changes: 141 additions & 0 deletions eslint-factory/src/rules/require-return-after-core-setfailed.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
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 statement is a call to `core.setFailed(...)`.
*/
function isCoreSetFailedStatement(node: TSESTree.Statement): node is TSESTree.ExpressionStatement {
if (node.type !== AST_NODE_TYPES.ExpressionStatement) return false;
const expr = node.expression;
if (expr.type !== AST_NODE_TYPES.CallExpression) return false;
const callee = expr.callee;
if (callee.type !== AST_NODE_TYPES.MemberExpression || callee.computed) return false;
const obj = callee.object;
const prop = callee.property;
return obj.type === AST_NODE_TYPES.Identifier && obj.name === "core" && prop.type === AST_NODE_TYPES.Identifier && prop.name === "setFailed";
}

/**
* Returns true when the statement unconditionally transfers control out of the
* current block: `return`, `throw`, `break`, `continue`, or `process.exit(...)`.
*/
function isControlTransfer(node: TSESTree.Statement): boolean {
const matchesControlTransferType = node.type === AST_NODE_TYPES.ReturnStatement || node.type === AST_NODE_TYPES.ThrowStatement || node.type === AST_NODE_TYPES.BreakStatement || node.type === AST_NODE_TYPES.ContinueStatement;
if (matchesControlTransferType) {
return true;
}
// process.exit(...)
if (node.type === AST_NODE_TYPES.ExpressionStatement && node.expression.type === AST_NODE_TYPES.CallExpression) {
const callee = node.expression.callee;
if (
callee.type === AST_NODE_TYPES.MemberExpression &&
!callee.computed &&
callee.object.type === AST_NODE_TYPES.Identifier &&
callee.object.name === "process" &&
callee.property.type === AST_NODE_TYPES.Identifier &&
callee.property.name === "exit"
) {
return true;
}
}
return false;
}

/**
* Checks a list of sequential statements for `core.setFailed(...)` calls that
* are not immediately followed by a control-transfer statement.
*/
function checkStatementList(stmts: TSESTree.Statement[], report: (node: TSESTree.Statement, next: TSESTree.Statement) => void): void {
for (let i = 0; i < stmts.length; i++) {
const stmt = stmts[i];
if (!isCoreSetFailedStatement(stmt)) continue;
const next = stmts[i + 1];
if (next && !isControlTransfer(next)) {
report(stmt, next);
}
}
}

function isExecutableStatement(node: TSESTree.ProgramStatement): node is TSESTree.Statement {
return (
node.type !== AST_NODE_TYPES.ImportDeclaration &&
node.type !== AST_NODE_TYPES.ExportAllDeclaration &&
node.type !== AST_NODE_TYPES.ExportDefaultDeclaration &&
node.type !== AST_NODE_TYPES.ExportNamedDeclaration &&
node.type !== AST_NODE_TYPES.TSModuleDeclaration
);
}

export const requireReturnAfterCoreSetFailedRule = createRule({
name: "require-return-after-core-setfailed",
meta: {
type: "problem",
hasSuggestions: true,
docs: {
description:
"Require a return, throw, break, continue, or process.exit() statement immediately after core.setFailed() to prevent execution from continuing after a failure is declared. " +
"core.setFailed() only marks the action as failed at the end; it does not stop execution.",
},
schema: [],
messages: {
missingReturnAfterSetFailed:
"core.setFailed() does not stop execution — add a control-transfer statement (for example: return, throw, break, continue, or process.exit(...)) immediately after to prevent the action from continuing in a failed state.",
addReturn: "Add 'return;' after core.setFailed() to stop execution.",
},
},
defaultOptions: [],
create(context) {
const sourceCode = context.sourceCode;

function isInsideFunctionLike(node: TSESTree.Node): boolean {
const ancestors = sourceCode.getAncestors(node);
for (let i = ancestors.length - 1; i >= 0; i--) {
const ancestor = ancestors[i];
const isFunctionLike = ancestor.type === AST_NODE_TYPES.FunctionDeclaration || ancestor.type === AST_NODE_TYPES.FunctionExpression || ancestor.type === AST_NODE_TYPES.ArrowFunctionExpression;
if (isFunctionLike) {
return true;
}
}
return false;
}

function report(node: TSESTree.Statement, next: TSESTree.Statement): void {
context.report({
node,
messageId: "missingReturnAfterSetFailed",
suggest: isInsideFunctionLike(node)
? [
{
messageId: "addReturn",
fix(fixer) {
const isOnSameLine = next.loc.start.line === node.loc.end.line;
if (isOnSameLine) {
return fixer.insertTextBefore(next, "return; ");
}
const line = sourceCode.lines[next.loc.start.line - 1] ?? "";
const indent = /^(\s*)/.exec(line)?.[1] ?? "";
return fixer.insertTextBefore(next, `return;\n${indent}`);
},
},
]
: undefined,
});
}

return {
// Check statement blocks: if body, else body, while body, function body, etc.
BlockStatement(node: TSESTree.BlockStatement) {
checkStatementList(node.body, report);
},
// Handle single-statement arrow functions (no braces) — rare but safe to skip
// The main case is BlockStatement above.
SwitchCase(node: TSESTree.SwitchCase) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

checkStatementList(node.consequent, report);
},
Program(node: TSESTree.Program) {
checkStatementList(node.body.filter(isExecutableStatement), report);
},
};
},
});
Loading