[eslint-miner] eslint: add no-core-error-then-setfailed rule - #48354
Conversation
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>
github-actions
Bot
added
automation
cookie
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ 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. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Adds an ESLint rule intended to detect redundant core.error() calls before core.setFailed().
Changes:
- Implements and registers the rule.
- Enables it as a warning.
- Adds rule tests and suggestions.
Show a summary per file
| File | Description |
|---|---|
eslint-factory/src/rules/no-core-error-then-setfailed.ts |
Implements detection and removal suggestions. |
eslint-factory/src/rules/no-core-error-then-setfailed.test.ts |
Adds rule test cases. |
eslint-factory/src/index.ts |
Registers the rule. |
eslint-factory/eslint.config.cjs |
Enables the rule. |
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: Medium
| const next = stmts[i + 1]; | ||
| if (!isCoreSetFailedStatement(next, sourceCode)) continue; | ||
|
|
||
| const errorCall = current.expression as TSESTree.CallExpression; | ||
| const safeToFix = hasSingleNonSpreadArgument(errorCall); |
| const errorCall = current.expression as TSESTree.CallExpression; | ||
| const safeToFix = hasSingleNonSpreadArgument(errorCall); |
| @@ -0,0 +1,73 @@ | |||
| import { RuleTester } from "@typescript-eslint/rule-tester"; | |||
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: no-core-error-then-setfailed rule
Overall the implementation is clean, well-reasoned, and well-tested. A few minor non-blocking observations:
Non-blocking notes
1. Program node filter is incomplete
The Program handler filters out ImportDeclaration and ExportAllDeclaration but not ExportNamedDeclaration or ExportDefaultDeclaration. In practice this is harmless (those aren't ExpressionStatements), but the filtering intent is inconsistent with itself.
2. Suggestion leaves leading whitespace
fixer.remove(current) removes only the node's text range, not surrounding whitespace — this produces a leading space in the suggestion output (visible in the test: output: core.setFailed(...)``). This is standard ESLint behavior and acceptable for a non-autofix suggestion.
No blocking issues. The rule correctly identifies the duplicate-annotation anti-pattern and the test coverage is solid.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 23.5 AIC · ⌖ 4.56 AIC · ⊞ 5K
There was a problem hiding this comment.
Three issues to fix before merge
The rule logic is sound and the rationale is correct. Three issues need fixing:
hasSingleNonSpreadArgumentsuppresses the autofix incorrectly — the fix removes the entire statement, so the guard is always safe to drop.Programbody filter has an incomplete exclusion list —ExportNamedDeclarationandExportDefaultDeclarationpass through with an incorrect type assertion.- No same-object check between the two calls —
c1.error(); c2.setFailed()with different aliases (both inCORE_ALIASES) triggers a false positive.
Other findings (non-blocking)
isCoreErrorStatement/isCoreSetFailedStatementare near-identical and should be unified into a singleisCoreMethodCallStatement(node, sourceCode, methodName)helper.fixer.remove(current)does not consume the trailing newline, leaving a stray blank line after the autofix is applied. Extend the removal range to cover the trailing
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 39.1 AIC · ⌖ 5.02 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
eslint-factory/src/rules/no-core-error-then-setfailed.ts:207
hasSingleNonSpreadArgument guard incorrectly suppresses the autofix suggestion: the fix is fixer.remove(current), which removes the entire statement — always safe regardless of argument count or spread usage. The guard silently drops the suggestion for common patterns like core.error(msg, {title: "ctx"}).
<details>
<summary>💡 Suggested fix</summary>
Remove the guard — always provide the suggestion:
context.report({
node: current,
messageId: "noCoreErrorThenSetFai…
</details>
<details><summary>eslint-factory/src/rules/no-core-error-then-setfailed.ts:234</summary>
**Program body filter is incomplete — `ExportNamedDeclaration` and `ExportDefaultDeclaration` pass through as `TSESTree.Statement`**: `TSESTree.Program.body` is `(Statement | ModuleDeclaration)[]`. The filter only excludes `ImportDeclaration` and `ExportAllDeclaration`, so `export function foo() { core.error(...); core.setFailed(...); }` and `export default ...` nodes are cast as `Statement` and handed to `checkStatements`. While helpers bail out immediately (they check for `ExpressionStatement…
</details>
<details><summary>eslint-factory/src/rules/no-core-error-then-setfailed.ts:146</summary>
**No cross-object validation — different core aliases trigger a false positive**: when two different identifiers are both in `CORE_ALIASES`, the rule reports `c1.error("x"); c2.setFailed("x")` even though `c1` and `c2` are different objects (e.g., two separate `@actions/core` imports or a mock and a real instance). The adjacency check should verify that both statements reference the same identifier name.
<details>
<summary>💡 Suggested fix</summary>
In `checkStatements`, after confirming both…
</details>
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on a few correctness gaps and a maintainability concern.
📋 Key Themes & Highlights
Key Themes
- DRY violation:
isCoreErrorStatementandisCoreSetFailedStatementare near-identical; a sharedisCoreMethodCallStatement(node, methodName, sc)helper would make future changes safer. - Missing edge-case tests: No test verifies that mixed-receiver pairs (
coreA.error(); coreB.setFailed()) or non-core aliases (const c = notCore) are handled correctly. Programvisitor scope: The filter for top-level statements silently skipsExportNamedDeclaration/ExportDefaultDeclaration, which may cause inconsistent coverage in module-style files.
Positive Highlights
- ✅ Rule rationale is solid and well-evidenced (7 real violations found)
- ✅ Good coverage of computed property access and alias patterns
- ✅ Auto-fix suggestion is correctly scoped to single-argument calls only
- ✅ Test suite structure (valid / invalid) is clear and readable
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 39.8 AIC · ⌖ 6.93 AIC · ⊞ 6.7K
Comment /matt to run again
| if (!isSetFailedNonComputed && !isSetFailedComputed) return false; | ||
| if (obj.type !== AST_NODE_TYPES.Identifier) return false; | ||
|
|
||
| return isCoreLikeIdentifier(obj.name) || isCoreAliasIdentifier(obj, sourceCode); |
There was a problem hiding this comment.
[/codebase-design] isCoreErrorStatement and isCoreSetFailedStatement share ~90% identical logic — only the method name differs. This duplication will require parallel edits if the detection logic ever changes.
💡 Suggested refactor
Extract a single generic helper:
function isCoreMethodCallStatement(
node: TSESTree.Statement,
methodName: string,
sourceCode: SourceCode
): 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) return false;
const obj = callee.object;
const prop = callee.property;
const isTarget =
(!callee.computed && prop.type === AST_NODE_TYPES.Identifier && prop.name === methodName) ||
(callee.computed && prop.type === AST_NODE_TYPES.Literal && prop.value === methodName);
if (!isTarget || obj.type !== AST_NODE_TYPES.Identifier) return false;
return isCoreLikeIdentifier(obj.name) || isCoreAliasIdentifier(obj, sourceCode);
}Then the two exported helpers collapse to one-liners:
const isCoreError = (n: TSESTree.Statement, sc: SourceCode) =>
isCoreMethodCallStatement(n, 'error', sc);
const isCoreSetFailed = (n: TSESTree.Statement, sc: SourceCode) =>
isCoreMethodCallStatement(n, 'setFailed', sc);@copilot please address this.
| // core.error without adjacent setFailed (setFailed is non-adjacent) | ||
| { | ||
| code: `core.error("msg"); doSomething(); core.setFailed("msg");`, | ||
| }, |
There was a problem hiding this comment.
[/tdd] Missing test: core.error and core.setFailed called on different objects (e.g. coreA.error('x'); coreB.setFailed('y')) should be valid — the rule currently doesn't check that both calls share the same receiver.
💡 Suggested test case
// Different receivers — should NOT be flagged
{
code: `coreA.error("msg"); coreB.setFailed("msg");`,
},The implementation checks each identifier independently against CORE_ALIASES, so two different aliased receivers would both pass, resulting in a false positive for mixed-receiver pairs.
@copilot please address this.
| }, | ||
| SwitchCase(node: TSESTree.SwitchCase) { | ||
| checkStatements(node.consequent); | ||
| }, |
There was a problem hiding this comment.
[/tdd] The Program body filter excludes ImportDeclaration and ExportAllDeclaration but not ExportNamedDeclaration or ExportDefaultDeclaration. Top-level patterns inside exported functions won't be reached via Program, though they will be via BlockStatement. Consider simplifying to just rely on BlockStatement + SwitchCase — the Program visitor only catches the rare case of top-level imperative scripts.
@copilot please address this.
| // Computed property access | ||
| { | ||
| code: `core["error"]("msg"); core["setFailed"]("msg");`, | ||
| errors: [{ messageId: "noCoreErrorThenSetFailed" }], |
There was a problem hiding this comment.
[/tdd] The alias test (const c = core; c.error...) relies on isCoreAliasIdentifier resolving c to core, but none of the valid-case tests verify that an aliased call to a non-core object is correctly ignored (e.g. const c = notCore; c.error('x'); c.setFailed('y') should be valid). Adding this would guard against CORE_ALIASES over-matching.
@copilot please address this.
|
@copilot please run the Unresolved review feedback / follow-up (newest first):
Run: https://github.com/github/gh-aw/actions/runs/30264173356
|
- 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>
Addressed all three reviewer concerns in commit
Hand-off: CI must be re-triggered by a maintainer (close/reopen PR or push) before merge. |
|
@copilot run pr-finisher skill |
…-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>
Done. Addressed all remaining
Hand-off: CI must be re-triggered by a maintainer (close/reopen PR or push) before merge. |
Caution
agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.
Details
Potential security threats were detected in the agent output.
Review the workflow run logs for details.
test body