diff --git a/eslint-factory/src/rules/no-core-error-then-process-exitcode.test.ts b/eslint-factory/src/rules/no-core-error-then-process-exitcode.test.ts index ca3eebd3238..9dbd9353183 100644 --- a/eslint-factory/src/rules/no-core-error-then-process-exitcode.test.ts +++ b/eslint-factory/src/rules/no-core-error-then-process-exitcode.test.ts @@ -27,25 +27,40 @@ describe("no-core-error-then-process-exitcode", () => { `core.warning("msg"); process.exitCode = 1;`, // Variable assignment — runtime value unknown `core.error("msg"); process.exitCode = code;`, - // Exports between statements break adjacency at module scope + // Exports between statements break the scan at module scope `const helper = 1; core.error("msg"); export { helper }; process.exitCode = 1;`, // process.exit (covered by the sibling rule) `core.error("msg"); process.exit(1);`, // Not a simple assignment: += is not flagged `core.error("msg"); process.exitCode += 1;`, + // core.setFailed between error and exitCode stops scanning + `core.error("x"); core.setFailed("y"); process.exitCode = 1;`, + // return between error and exitCode stops scanning (inside a function) + `function run() { core.error("x"); return; process.exitCode = 1; }`, + // throw between error and exitCode stops scanning + `function run() { core.error("x"); throw new Error("x"); process.exitCode = 1; }`, + // break between error and exitCode stops scanning (inside a loop) + `while (true) { core.error("x"); break; process.exitCode = 1; }`, + // continue between error and exitCode stops scanning (inside a loop) + `for (let i = 0; i < 10; i++) { core.error("x"); continue; process.exitCode = 1; }`, + // process.exit() between error and exitCode stops scanning (dot access) + `core.error("x"); process.exit(1); process.exitCode = 1;`, + // process["exit"]() between error and exitCode stops scanning (computed access) + `core.error("x"); process["exit"](1); process.exitCode = 1;`, ], invalid: [ { + // module top-level: autofix is safe — no caller continues after replacement code: `core.error("fatal"); process.exitCode = 1;`, - errors: [{ messageId: "noCoreErrorThenProcessExitCode", suggestions: [] }], + errors: [{ messageId: "noCoreErrorThenProcessExitCode", suggestions: [{ messageId: "replaceWithSetFailed", output: 'core.setFailed("fatal");\n ' }] }], }, { code: `core.error("something went wrong"); process.exitCode = 1;`, - errors: [{ messageId: "noCoreErrorThenProcessExitCode", suggestions: [] }], + errors: [{ messageId: "noCoreErrorThenProcessExitCode", suggestions: [{ messageId: "replaceWithSetFailed", output: 'core.setFailed("something went wrong");\n ' }] }], }, { code: "core.error(`ERROR: ${msg}`); process.exitCode = 1;", - errors: [{ messageId: "noCoreErrorThenProcessExitCode", suggestions: [] }], + errors: [{ messageId: "noCoreErrorThenProcessExitCode", suggestions: [{ messageId: "replaceWithSetFailed", output: "core.setFailed(`ERROR: ${msg}`);\n " }] }], }, { // Inside a named function — no autofix suggestion because return; only exits the helper @@ -78,13 +93,30 @@ describe("no-core-error-then-process-exitcode", () => { errors: [{ messageId: "noCoreErrorThenProcessExitCode", suggestions: [] }], }, { - // SwitchCase path reports the pattern without an autofix outside main() + // SwitchCase at module top level: autofix is safe (enclosingFn === null) code: `switch (x) { case 1: core.error("fatal"); process.exitCode = 1; break; }`, - errors: [{ messageId: "noCoreErrorThenProcessExitCode", suggestions: [] }], + errors: [{ messageId: "noCoreErrorThenProcessExitCode", suggestions: [{ messageId: "replaceWithSetFailed", output: 'switch (x) { case 1: core.setFailed("fatal");\n break; }' }] }], }, { // exitCode = 2 is also flagged code: `core.error("critical"); process.exitCode = 2;`, + errors: [{ messageId: "noCoreErrorThenProcessExitCode", suggestions: [{ messageId: "replaceWithSetFailed", output: 'core.setFailed("critical");\n ' }] }], + }, + { + // Non-adjacent pair: intervening statement does not defeat detection; no autofix suggestion + code: `core.error("x"); core.info("y"); process.exitCode = 1;`, + errors: [{ messageId: "noCoreErrorThenProcessExitCode", suggestions: [] }], + }, + { + // Two intervening statements + code: `core.error("fatal"); core.info("a"); core.info("b"); process.exitCode = 1;`, + errors: [{ messageId: "noCoreErrorThenProcessExitCode", suggestions: [] }], + }, + { + // Two consecutive core.error calls before the same process.exitCode: only the first + // core.error reports (non-adjacent, no autofix) — deduplication prevents a second + // diagnostic and any conflicting autofix from the adjacent core.error("b"). + code: `core.error("a"); core.error("b"); process.exitCode = 1;`, errors: [{ messageId: "noCoreErrorThenProcessExitCode", suggestions: [] }], }, ], diff --git a/eslint-factory/src/rules/no-core-error-then-process-exitcode.ts b/eslint-factory/src/rules/no-core-error-then-process-exitcode.ts index b03a7ade4a6..a9d1d27773d 100644 --- a/eslint-factory/src/rules/no-core-error-then-process-exitcode.ts +++ b/eslint-factory/src/rules/no-core-error-then-process-exitcode.ts @@ -83,6 +83,56 @@ function isProcessExitCodeNonZero(node: TSESTree.Statement): node is TSESTree.Ex return true; } +/** + * Returns true when `node` is an expression statement containing a call to + * `core.setFailed(...)` (direct, computed, or aliased). + * Accepts `sourceCode` for alias resolution via `isCoreAliasIdentifier`; contrast + * with `isControlTransferStatement` which is a pure syntax check and needs no source-code context. + */ +function isCoreSetFailedStatement(node: TSESTree.Statement, sourceCode: SourceCode): boolean { + 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 isSetFailedNonComputed = !callee.computed && prop.type === AST_NODE_TYPES.Identifier && prop.name === "setFailed"; + const isSetFailedComputed = callee.computed && prop.type === AST_NODE_TYPES.Literal && prop.value === "setFailed"; + if (!isSetFailedNonComputed && !isSetFailedComputed) return false; + if (obj.type !== AST_NODE_TYPES.Identifier) return false; + + return isCoreLikeIdentifier(obj.name) || isCoreAliasIdentifier(obj, sourceCode); +} + +/** + * Returns true when `node` is a control-transfer statement that definitively + * exits the current block: return, throw, break, continue, or process.exit(...). + */ +function isControlTransferStatement(node: TSESTree.Statement): boolean { + // prettier-ignore + if ( + node.type === AST_NODE_TYPES.ReturnStatement || + node.type === AST_NODE_TYPES.ThrowStatement || + node.type === AST_NODE_TYPES.BreakStatement || + node.type === AST_NODE_TYPES.ContinueStatement + ) { + return true; + } + // process.exit(...) — any call, regardless of exit code; handles both dot and computed access + 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.object.type === AST_NODE_TYPES.Identifier && callee.object.name === "process") { + const prop = callee.property; + const isExitDot = !callee.computed && prop.type === AST_NODE_TYPES.Identifier && prop.name === "exit"; + const isExitComputed = callee.computed && prop.type === AST_NODE_TYPES.Literal && prop.value === "exit"; + if (isExitDot || isExitComputed) return true; + } + } + return false; +} + function hasSingleNonSpreadArgument(call: TSESTree.CallExpression): boolean { return call.arguments.length === 1 && call.arguments[0].type !== AST_NODE_TYPES.SpreadElement; } @@ -98,11 +148,13 @@ export const noCoreErrorThenProcessExitCodeRule = createRule({ hasSuggestions: true, docs: { description: - "Disallow the pattern `core.error(msg); process.exitCode = nonzero` in GitHub Actions scripts. " + + "Disallow the pattern `core.error(msg); ... ; process.exitCode = nonzero` in GitHub Actions scripts. " + "`core.error()` annotates the log but does not mark the action as failed. " + "Prefer `core.setFailed(msg)` which correctly marks the action as failed and allows post-action " + "cleanup hooks to run. Unlike `process.exit(1)`, `process.exitCode = 1` does not immediately halt " + - "execution, so subsequent code still runs in the failed state.", + "execution, so subsequent code still runs in the failed state. " + + "The rule scans forward from `core.error(...)` for a later `process.exitCode = nonzero`, " + + "stopping at `core.setFailed(...)` or a control-transfer statement.", }, schema: [], messages: { @@ -110,7 +162,7 @@ export const noCoreErrorThenProcessExitCodeRule = createRule({ "Avoid `core.error()` followed by `process.exitCode = nonzero`. Prefer `core.setFailed(msg)` to signal " + "action failure; it marks the action failed and allows post-action cleanup hooks to run. " + "Unlike `process.exit(1)`, `process.exitCode = 1` does not halt execution immediately.", - replaceWithSetFailed: "Replace `core.error(msg); process.exitCode = nonzero` with `core.setFailed(msg); return;`.", + replaceWithSetFailed: "Replace `core.error(msg); process.exitCode = nonzero` with `core.setFailed(msg)` (at module top level) or `core.setFailed(msg); return;` (inside main()).", }, }, defaultOptions: [], @@ -118,31 +170,62 @@ export const noCoreErrorThenProcessExitCodeRule = createRule({ const sourceCode = context.sourceCode; function checkStatements(stmts: readonly TSESTree.Statement[]): void { + // Track which process.exitCode nodes have already been reported so that two consecutive + // core.error() calls before the same exitCode do not each fire their own diagnostic + // (which could produce conflicting autofixers on the same node). + const reported = new WeakSet(); for (let i = 0; i < stmts.length - 1; i++) { const current = stmts[i]; - const next = stmts[i + 1]; - if (isCoreErrorStatement(current, sourceCode) && isProcessExitCodeNonZero(next)) { - const enclosingFn = getImmediateEnclosingFunction(current, sourceCode); - const errorCall = current.expression as TSESTree.CallExpression; - const safeToFix = enclosingFn !== null && isFunctionNamedMain(enclosingFn) && hasSingleNonSpreadArgument(errorCall); - - context.report({ - node: current, - messageId: "noCoreErrorThenProcessExitCode", - suggest: safeToFix - ? [ - { - messageId: "replaceWithSetFailed", - fix(fixer: TSESLint.RuleFixer) { - const args = errorCall.arguments.map(a => sourceCode.getText(a)).join(", "); - const callee = errorCall.callee as TSESTree.MemberExpression; - const objectName = sourceCode.getText(callee.object); - return [fixer.replaceText(current, `${objectName}.setFailed(${args}); return;\n`), fixer.remove(next)]; - }, - }, - ] - : [], - }); + if (!isCoreErrorStatement(current, sourceCode)) continue; + + // Scan forward for process.exitCode = nonzero, stopping at setFailed or control-transfer. + // Adjacent (j === i+1) keeps autofix; non-adjacent reports without suggestion. + for (let j = i + 1; j < stmts.length; j++) { + const candidate = stmts[j]; + + if (isProcessExitCodeNonZero(candidate)) { + if (!reported.has(candidate)) { + reported.add(candidate); + const isAdjacent = j === i + 1; + // The autofix suggestion is only safe when the pair is at module top level or directly + // inside a `main()` entrypoint. Inside helper functions, `return;` only exits the helper + // and lets the caller continue. For non-adjacent pairs we omit the suggestion to avoid + // a fixer that leaves intervening statements between a deleted exitCode and the new setFailed. + const enclosingFn = getImmediateEnclosingFunction(current, sourceCode); + const errorCall = current.expression as TSESTree.CallExpression; + const safeToFix = isAdjacent && (enclosingFn === null || isFunctionNamedMain(enclosingFn)) && hasSingleNonSpreadArgument(errorCall); + + context.report({ + node: current, + messageId: "noCoreErrorThenProcessExitCode", + suggest: safeToFix + ? [ + { + messageId: "replaceWithSetFailed", + fix(fixer: TSESLint.RuleFixer) { + const args = errorCall.arguments.map(a => sourceCode.getText(a)).join(", "); + const callee = errorCall.callee as TSESTree.MemberExpression; + const objectName = sourceCode.getText(callee.object); + + // At module top-level (enclosingFn === null) there is nothing to `return` from, + // so we just replace with setFailed. Inside main() we append `return;` to exit + // the entrypoint cleanly. + const replacement = enclosingFn !== null ? `${objectName}.setFailed(${args}); return;` : `${objectName}.setFailed(${args});`; + + return [fixer.replaceText(current, replacement + "\n"), fixer.remove(candidate)]; + }, + }, + ] + : [], + }); + } + break; + } + + // Stop scanning if setFailed already handles the failure or a control-transfer exits the block. + if (isCoreSetFailedStatement(candidate, sourceCode) || isControlTransferStatement(candidate)) { + break; + } } } } @@ -155,13 +238,19 @@ export const noCoreErrorThenProcessExitCodeRule = createRule({ checkStatements(node.consequent); }, Program(node: TSESTree.Program) { - for (let i = 0; i < node.body.length - 1; i++) { - const current = node.body[i]; - const next = node.body[i + 1]; - if (isProgramStatement(current) && isProgramStatement(next)) { - checkStatements([current, next]); + // At module top level, export declarations act as segment boundaries (they separate + // "regions" of the module). We split the program body at export/import declarations + // so that an export between core.error and process.exitCode breaks the scan. + let segment: TSESTree.Statement[] = []; + for (const stmt of node.body) { + if (isProgramStatement(stmt)) { + segment.push(stmt); + } else { + checkStatements(segment); + segment = []; } } + checkStatements(segment); }, }; },