Skip to content

Commit 55b5181

Browse files
github-actions[bot]CopilotCopilotgh-aw-botpelikhan
authored
[eslint-miner] eslint: add no-core-error-then-setfailed rule (#48354)
* eslint: add no-core-error-then-setfailed rule Add a new custom ESLint rule that flags the redundant pattern of calling core.error(msg) immediately before core.setFailed(msg). Since core.setFailed() already logs an error annotation and marks the action as failed, the preceding core.error() call creates a duplicate annotation in the GitHub Actions log. The rule detects this in BlockStatement, SwitchCase, and Program bodies. It provides an auto-fix suggestion to remove the redundant core.error() call. Evidence: 7 violations found across actions/setup/js/*.cjs in the current codebase (check_permissions.cjs, unlock-issue.cjs, add_reaction_and_edit_comment.cjs, add_reaction.cjs, push_repo_memory.cjs, start_mcp_gateway.cjs x3, validate_memory_files.cjs). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: address review feedback on no-core-error-then-setfailed rule - Only flag when core.error() and core.setFailed() use provably equivalent message arguments (same source text); different messages are no longer reported since core.error may carry extra diagnostic context - Exclude core.error() calls with annotation properties (second argument) as they provide context not duplicated by setFailed - Restrict auto-remove suggestion to side-effect-free arguments; function call arguments are no longer silently dropped - Replace @typescript-eslint/rule-tester with RuleTester from eslint plus Vitest describe/it, matching the convention of all sibling rules - Update test cases to reflect the new message-equivalence requirement Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> * fix: address github-actions bot review feedback on no-core-error-then-setfailed - Add same-object identifier check: c1.error("x"); c2.setFailed("x") with different aliases no longer triggers a false positive - Fix Program body filter to exclude all module declarations: adds ExportNamedDeclaration and ExportDefaultDeclaration to the exclusion list - Unify isCoreErrorStatement/isCoreSetFailedStatement into a single isCoreMethodCallStatement helper, eliminating the near-identical duplication - Add getCoreObjectName helper to extract receiver name from a matched call - Add test cases: mixed-receiver alias pair (valid) and non-core alias (valid) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
1 parent 24ba574 commit 55b5181

4 files changed

Lines changed: 295 additions & 0 deletions

File tree

‎eslint-factory/eslint.config.cjs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ module.exports = [
4343
"gh-aw-custom/no-err-stack-then-string-fallback": "warn",
4444
"gh-aw-custom/no-caught-error-interpolation": "warn",
4545
"gh-aw-custom/require-fetch-try-catch": "warn",
46+
"gh-aw-custom/no-core-error-then-setfailed": "warn",
4647
},
4748
},
4849
{

‎eslint-factory/src/index.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import { noSetFailedThenExitZeroRule } from "./rules/no-setfailed-then-exit-zero
2929
import { noErrStackThenStringFallbackRule } from "./rules/no-err-stack-then-string-fallback";
3030
import { noCaughtErrorInterpolationRule } from "./rules/no-caught-error-interpolation";
3131
import { requireFetchTryCatchRule } from "./rules/require-fetch-try-catch";
32+
import { noCoreErrorThenSetFailedRule } from "./rules/no-core-error-then-setfailed";
3233

3334
const plugin = {
3435
meta: {
@@ -67,6 +68,7 @@ const plugin = {
6768
"no-err-stack-then-string-fallback": noErrStackThenStringFallbackRule,
6869
"no-caught-error-interpolation": noCaughtErrorInterpolationRule,
6970
"require-fetch-try-catch": requireFetchTryCatchRule,
71+
"no-core-error-then-setfailed": noCoreErrorThenSetFailedRule,
7072
},
7173
};
7274

‎eslint-factory/src/rules/no-core-error-then-setfailed.test.ts‎

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
// Uses eslint's RuleTester rather than @typescript-eslint/rule-tester, matching the
2+
// convention of all other rule tests in this package. The rule uses @typescript-eslint/utils
3+
// internally but the standard eslint RuleTester is sufficient for all test scenarios here.
4+
import { RuleTester } from "eslint";
5+
import { describe, it } from "vitest";
6+
import { noCoreErrorThenSetFailedRule } from "./no-core-error-then-setfailed";
7+
8+
const ruleTester = new RuleTester({
9+
languageOptions: {
10+
ecmaVersion: "latest",
11+
sourceType: "module",
12+
},
13+
});
14+
15+
describe("no-core-error-then-setfailed", () => {
16+
it("valid and invalid cases", () => {
17+
ruleTester.run("no-core-error-then-setfailed", noCoreErrorThenSetFailedRule, {
18+
valid: [
19+
// Only core.setFailed — no preceding core.error
20+
`core.setFailed("something went wrong");`,
21+
// Only core.error — no following setFailed
22+
`core.error("something went wrong");`,
23+
// core.error followed by something other than setFailed
24+
`function f() { core.error("msg"); return; }`,
25+
// core.warning followed by core.setFailed is allowed (different method)
26+
`core.warning("msg"); core.setFailed("msg");`,
27+
// core.error without adjacent setFailed (setFailed is non-adjacent)
28+
`core.error("msg"); doSomething(); core.setFailed("msg");`,
29+
// Different messages — core.error provides extra context not repeated by setFailed
30+
`core.error("upload failed: " + filename); core.setFailed("action failed");`,
31+
// Different template literals — messages differ in prefix
32+
{
33+
code: `
34+
try {
35+
doSomething();
36+
} catch (err) {
37+
core.error(\`Failed: \${err.message}\`);
38+
core.setFailed(\`ERR: Failed: \${err.message}\`);
39+
}
40+
`,
41+
},
42+
// core.error with annotation properties — carries extra diagnostic context
43+
`core.error("msg", { title: "Upload error" }); core.setFailed("msg");`,
44+
// Different core objects (cross-alias false-positive guard):
45+
// c1 and c2 are different objects even if both are in CORE_ALIASES
46+
`const c1 = core; const c2 = coreObj; c1.error("msg"); c2.setFailed("msg");`,
47+
// Non-core alias is not flagged
48+
`const c = notCore; c.error("msg"); c.setFailed("msg");`,
49+
],
50+
invalid: [
51+
// Adjacent core.error then core.setFailed with same literal — has suggestion
52+
{
53+
code: `core.error("msg"); core.setFailed("msg");`,
54+
errors: [{ messageId: "noCoreErrorThenSetFailed", suggestions: [{ messageId: "removeErrorCall", output: ` core.setFailed("msg");` }] }],
55+
},
56+
// With an alias (const c = core) and matching messages — has suggestion
57+
{
58+
code: `const c = core; c.error("msg"); c.setFailed("msg");`,
59+
errors: [{ messageId: "noCoreErrorThenSetFailed", suggestions: [{ messageId: "removeErrorCall", output: `const c = core; c.setFailed("msg");` }] }],
60+
},
61+
// Computed property access with matching messages — has suggestion
62+
{
63+
code: `core["error"]("msg"); core["setFailed"]("msg");`,
64+
errors: [{ messageId: "noCoreErrorThenSetFailed", suggestions: [{ messageId: "removeErrorCall", output: ` core["setFailed"]("msg");` }] }],
65+
},
66+
// Same template literal in both calls — side-effect-free (identifier inside), has suggestion
67+
{
68+
code: `core.error(\`error: \${msg}\`); core.setFailed(\`error: \${msg}\`);`,
69+
errors: [
70+
{
71+
messageId: "noCoreErrorThenSetFailed",
72+
suggestions: [{ messageId: "removeErrorCall", output: ` core.setFailed(\`error: \${msg}\`);` }],
73+
},
74+
],
75+
},
76+
// Same call-expression argument — NOT side-effect-free: report but no suggestion
77+
{
78+
code: `core.error(nextMessage()); core.setFailed(nextMessage());`,
79+
errors: [{ messageId: "noCoreErrorThenSetFailed", suggestions: [] }],
80+
},
81+
// Inside a block with matching messages — has suggestion
82+
{
83+
code: `function run() { core.error("fatal"); core.setFailed("fatal"); }`,
84+
errors: [
85+
{
86+
messageId: "noCoreErrorThenSetFailed",
87+
suggestions: [{ messageId: "removeErrorCall", output: `function run() { core.setFailed("fatal"); }` }],
88+
},
89+
],
90+
},
91+
],
92+
});
93+
});
94+
});

‎eslint-factory/src/rules/no-core-error-then-setfailed.ts‎

Lines changed: 198 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,198 @@
1+
import { AST_NODE_TYPES, ESLintUtils, TSESLint, TSESTree } from "@typescript-eslint/utils";
2+
import { CORE_ALIASES } from "./core-aliases";
3+
import { isCoreAliasIdentifier } from "./core-method-resolve";
4+
5+
const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/eslint-factory#${name}`);
6+
7+
type SourceCode = Parameters<typeof isCoreAliasIdentifier>[1];
8+
9+
function isCoreLikeIdentifier(name: string): boolean {
10+
return CORE_ALIASES.has(name);
11+
}
12+
13+
/**
14+
* Returns the first non-SpreadElement argument of a call, or null when
15+
* there are no arguments or the first argument is a spread.
16+
*/
17+
function getFirstNonSpreadArg(call: TSESTree.CallExpression): TSESTree.Expression | null {
18+
if (call.arguments.length === 0) return null;
19+
const first = call.arguments[0];
20+
if (first.type === AST_NODE_TYPES.SpreadElement) return null;
21+
return first as TSESTree.Expression;
22+
}
23+
24+
/**
25+
* Returns true when the call has more than one argument (i.e. annotation
26+
* properties are present, e.g. `core.error(msg, { title: "..." })`).
27+
* Such calls carry diagnostic context not duplicated in setFailed and must
28+
* not be flagged as redundant.
29+
*/
30+
function hasAnnotationProperties(call: TSESTree.CallExpression): boolean {
31+
return call.arguments.length > 1;
32+
}
33+
34+
/**
35+
* Returns true when the expression is provably side-effect-free: no call,
36+
* new, or assignment expression at any nesting level. Conservatively returns
37+
* false for any node type not listed here.
38+
*/
39+
function isSideEffectFree(node: TSESTree.Expression): boolean {
40+
switch (node.type) {
41+
case AST_NODE_TYPES.Literal:
42+
case AST_NODE_TYPES.Identifier:
43+
return true;
44+
case AST_NODE_TYPES.TemplateLiteral:
45+
return (node as TSESTree.TemplateLiteral).expressions.every(e => isSideEffectFree(e as TSESTree.Expression));
46+
case AST_NODE_TYPES.MemberExpression: {
47+
const me = node as TSESTree.MemberExpression;
48+
return isSideEffectFree(me.object as TSESTree.Expression) && (!me.computed || isSideEffectFree(me.property as TSESTree.Expression));
49+
}
50+
case AST_NODE_TYPES.BinaryExpression: {
51+
const be = node as TSESTree.BinaryExpression;
52+
return isSideEffectFree(be.left as TSESTree.Expression) && isSideEffectFree(be.right as TSESTree.Expression);
53+
}
54+
case AST_NODE_TYPES.UnaryExpression:
55+
return isSideEffectFree((node as TSESTree.UnaryExpression).argument as TSESTree.Expression);
56+
default:
57+
return false;
58+
}
59+
}
60+
61+
/**
62+
* Returns true when `node` is an expression statement containing a call to
63+
* `<coreObj>.<methodName>(...)` where the receiver is a known core alias
64+
* (direct or assigned alias). Also returns the receiver identifier name via
65+
* the `objectName` out-param so the caller can enforce same-object pairing.
66+
*/
67+
function isCoreMethodCallStatement(node: TSESTree.Statement, sourceCode: SourceCode, methodName: string): node is TSESTree.ExpressionStatement {
68+
if (node.type !== AST_NODE_TYPES.ExpressionStatement) return false;
69+
const expr = node.expression;
70+
if (expr.type !== AST_NODE_TYPES.CallExpression) return false;
71+
const callee = expr.callee;
72+
if (callee.type !== AST_NODE_TYPES.MemberExpression) return false;
73+
74+
const obj = callee.object;
75+
const prop = callee.property;
76+
const isNonComputed = !callee.computed && prop.type === AST_NODE_TYPES.Identifier && (prop as TSESTree.Identifier).name === methodName;
77+
const isComputed = callee.computed && prop.type === AST_NODE_TYPES.Literal && (prop as TSESTree.Literal).value === methodName;
78+
if (!isNonComputed && !isComputed) return false;
79+
if (obj.type !== AST_NODE_TYPES.Identifier) return false;
80+
81+
return isCoreLikeIdentifier((obj as TSESTree.Identifier).name) || isCoreAliasIdentifier(obj as TSESTree.Identifier, sourceCode);
82+
}
83+
84+
function isCoreErrorStatement(node: TSESTree.Statement, sourceCode: SourceCode): node is TSESTree.ExpressionStatement {
85+
return isCoreMethodCallStatement(node, sourceCode, "error");
86+
}
87+
88+
function isCoreSetFailedStatement(node: TSESTree.Statement, sourceCode: SourceCode): node is TSESTree.ExpressionStatement {
89+
return isCoreMethodCallStatement(node, sourceCode, "setFailed");
90+
}
91+
92+
/**
93+
* Returns the receiver identifier name from a matched core-method call statement.
94+
* Precondition: `isCoreErrorStatement` or `isCoreSetFailedStatement` returned true.
95+
*/
96+
function getCoreObjectName(stmt: TSESTree.ExpressionStatement): string {
97+
const call = stmt.expression as TSESTree.CallExpression;
98+
const callee = call.callee as TSESTree.MemberExpression;
99+
return (callee.object as TSESTree.Identifier).name;
100+
}
101+
102+
export const noCoreErrorThenSetFailedRule = createRule({
103+
name: "no-core-error-then-setfailed",
104+
meta: {
105+
type: "suggestion",
106+
hasSuggestions: true,
107+
docs: {
108+
description:
109+
"Disallow the redundant pattern `core.error(msg); core.setFailed(msg)` in GitHub Actions scripts. " +
110+
"`core.setFailed()` already logs the message as an error annotation and marks the action as failed. " +
111+
"Preceding it with `core.error()` using the same message creates a duplicate error annotation " +
112+
"in the GitHub Actions log, adding noise without benefit. Use `core.setFailed(msg)` alone.",
113+
},
114+
schema: [],
115+
messages: {
116+
noCoreErrorThenSetFailed: "`core.error()` immediately before `core.setFailed()` with the same message is redundant: `core.setFailed()` already logs an error annotation and marks the action failed. Remove the `core.error()` call.",
117+
removeErrorCall: "Remove the redundant `core.error()` call — `core.setFailed()` already logs an error annotation.",
118+
},
119+
},
120+
defaultOptions: [],
121+
create(context) {
122+
const sourceCode = context.sourceCode;
123+
124+
function checkStatements(stmts: readonly TSESTree.Statement[]): void {
125+
for (let i = 0; i < stmts.length - 1; i++) {
126+
const current = stmts[i];
127+
if (!isCoreErrorStatement(current, sourceCode)) continue;
128+
129+
const next = stmts[i + 1];
130+
if (!isCoreSetFailedStatement(next, sourceCode)) continue;
131+
132+
// Both calls must reference the same receiver identifier to avoid
133+
// flagging `c1.error("x"); c2.setFailed("x")` where c1 and c2 are
134+
// different objects that happen to both be in CORE_ALIASES.
135+
if (getCoreObjectName(current) !== getCoreObjectName(next)) continue;
136+
137+
const errorCall = (current as TSESTree.ExpressionStatement).expression as TSESTree.CallExpression;
138+
const setFailedCall = (next as TSESTree.ExpressionStatement).expression as TSESTree.CallExpression;
139+
140+
// Do not flag core.error calls that carry annotation properties (e.g.
141+
// core.error(msg, { title: "..." })). The second argument provides
142+
// diagnostic context that is not duplicated by setFailed.
143+
if (hasAnnotationProperties(errorCall)) continue;
144+
145+
// Only report when the message arguments are provably equivalent (same
146+
// source text). Calls with different messages are not redundant — the
147+
// core.error call may log extra context (file names, sizes, stack frames)
148+
// that setFailed does not repeat.
149+
const errorArg = getFirstNonSpreadArg(errorCall);
150+
const setFailedArg = getFirstNonSpreadArg(setFailedCall);
151+
if (errorArg === null || setFailedArg === null) continue;
152+
if (sourceCode.getText(errorArg) !== sourceCode.getText(setFailedArg)) continue;
153+
154+
// The auto-remove suggestion is semantics-preserving only when the shared
155+
// argument is provably side-effect-free. For example,
156+
// `core.error(nextMessage()); core.setFailed(nextMessage())` must not have
157+
// the first call silently removed because that would drop a side-effectful
158+
// function invocation.
159+
const safeToFix = isSideEffectFree(errorArg);
160+
161+
context.report({
162+
node: current,
163+
messageId: "noCoreErrorThenSetFailed",
164+
suggest: safeToFix
165+
? [
166+
{
167+
messageId: "removeErrorCall",
168+
fix(fixer: TSESLint.RuleFixer) {
169+
return fixer.remove(current);
170+
},
171+
},
172+
]
173+
: [],
174+
});
175+
}
176+
}
177+
178+
return {
179+
BlockStatement(node: TSESTree.BlockStatement) {
180+
checkStatements(node.body);
181+
},
182+
SwitchCase(node: TSESTree.SwitchCase) {
183+
checkStatements(node.consequent);
184+
},
185+
Program(node: TSESTree.Program) {
186+
// Filter out all module declarations (ImportDeclaration, ExportAllDeclaration,
187+
// ExportNamedDeclaration, ExportDefaultDeclaration) — they are not Statements
188+
// and their type assertion would be incorrect. BlockStatement visitor handles
189+
// the bodies of any exported function/class declarations separately.
190+
const stmts = node.body.filter(
191+
(s): s is TSESTree.Statement =>
192+
s.type !== AST_NODE_TYPES.ImportDeclaration && s.type !== AST_NODE_TYPES.ExportAllDeclaration && s.type !== AST_NODE_TYPES.ExportNamedDeclaration && s.type !== AST_NODE_TYPES.ExportDefaultDeclaration
193+
);
194+
checkStatements(stmts);
195+
},
196+
};
197+
},
198+
});

0 commit comments

Comments
 (0)