Repository navigation
Conversation
Detects the pattern core.error(msg) followed immediately by process.exitCode = nonzero. This is a sibling to the existing no-core-error-then-process-exit rule and closes the gap where mcp_cli_bridge.cjs (and potentially new code) uses process.exitCode assignment instead of process.exit(). Unlike process.exit(), process.exitCode = 1 does NOT halt execution immediately — subsequent code continues to run in the failed state. Prefer core.setFailed() which both marks the action as failed and allows post-action cleanup hooks to run. Observed in: mcp_cli_bridge.cjs (4 instances at lines 963, 1204, 1230, 1243, 1361, 1372). 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 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions detected). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Adds an ESLint rule detecting core.error() immediately followed by non-zero process.exitCode assignment.
Changes:
- Implements and tests the new rule.
- Registers and enables it as a warning.
Show a summary per file
| File | Description |
|---|---|
eslint-factory/src/rules/no-core-error-then-process-exitcode.ts |
Implements detection and suggestions. |
eslint-factory/src/rules/no-core-error-then-process-exitcode.test.ts |
Adds valid and invalid 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.
Comments suppressed due to low confidence (1)
eslint-factory/src/rules/no-core-error-then-process-exitcode.ts:139
- The matcher accepts any
core.error(...)argument list, butcore.error(message, properties)supports annotation metadata whilecore.setFailed(message)accepts only the message. Passing both arguments tosetFailedsilently drops file/line/title annotation properties. Only offer this suggestion when there is exactly one non-spread message argument; multi-argument calls can still be reported without an automatic suggestion, with a regression test for that case.
const errorCall = (current as TSESTree.ExpressionStatement).expression as TSESTree.CallExpression;
const args = errorCall.arguments.map(a => sourceCode.getText(a)).join(", ");
const callee = errorCall.callee as TSESTree.MemberExpression;
const objectName = sourceCode.getText(callee.object);
const replacement = enclosingFn !== null ? `${objectName}.setFailed(${args}); return;` : `${objectName}.setFailed(${args});`;
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Medium
| const next = stmts[i + 1]; | ||
| if (isCoreErrorStatement(current, sourceCode) && isProcessExitCodeNonZero(next)) { | ||
| const enclosingFn = getImmediateEnclosingFunction(current, sourceCode); | ||
| const safeToFix = enclosingFn === null || isFunctionNamedMain(enclosingFn); |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The no-core-error-then-process-exitcode rule is well-structured and mirrors the sibling no-core-error-then-process-exit rule closely. The logic for identifying core.error + process.exitCode = nonzero pairs, the safe-to-fix heuristic (main() or top-level only), the fix generation, and the test coverage all look correct. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 19.8 AIC · ⌖ 4.35 AIC · ⊞ 4.9K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (1 test)
Test Details
Quality signals:
Verdict✅ Passed. Test Quality Score: 100/100. 0% implementation tests (threshold: 30%). This is exemplary ESLint rule test coverage — comprehensive, focused, and properly weighted.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — requesting changes on three correctness gaps.
📋 Key Themes & Highlights
Issues Found
-
export function main()not treated as safe-to-fix —isFunctionNamedMainchecksfn.parent.type === Programbut exported declarations have an intermediateExportNamedDeclarationnode, so the autofix suggestion is silently suppressed for the commonexport async function main()pattern (line 38). -
Potential false positive via
Programfilter — TheProgramvisitor filters outImportDeclaration/ExportAllDeclarationbefore runningcheckStatements. AnExportNamedDeclaration(re-export) interspersed betweencore.errorandprocess.exitCode = 1would be removed by the filter, making them appear adjacent and producing a false positive (line 152). -
Misleading suggestion message for module-level fixes — The single
replaceWithSetFailedmessageId says "return;" in the description, but module-level fixes intentionally omitreturn(which is invalid there). Two message variants would be clearer (line 140). -
SwitchCasepath lacks test coverage — TheSwitchCasevisitor exists in the implementation but has no test case exercising it (test file line 91).
Positive Highlights
- ✅ Clean parallel with the sibling
no-core-error-then-process-exitrule — consistent patterns throughout - ✅ Good low-false-positive design: only integer literals flagged, zero excluded, variables excluded
- ✅ Context-aware autofix (safe only at module level or inside
main()) is well thought out - ✅ Comprehensive test coverage for template literals, async main, and non-main helpers
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 43.1 AIC · ⌖ 5.11 AIC · ⊞ 6.6K
Comment /matt to run again
| if ( | ||
| declarator == null || | ||
| declarator.type !== AST_NODE_TYPES.VariableDeclarator || | ||
| declarator.id.type !== AST_NODE_TYPES.Identifier || |
There was a problem hiding this comment.
[/tdd] isFunctionNamedMain does not handle export async function main() {...} — in that case fn.parent.type is ExportNamedDeclaration, not Program, so the autofix suggestion is silently suppressed for exported main functions.
💡 Suggested fix + test
Add a check for the exported case:
if (fn.type === AST_NODE_TYPES.FunctionDeclaration) {
if (fn.id?.name !== "main") return false;
if (fn.parent?.type === AST_NODE_TYPES.Program) return true;
// export function main() {}
return (
fn.parent?.type === AST_NODE_TYPES.ExportNamedDeclaration &&
fn.parent.parent?.type === AST_NODE_TYPES.Program
);
}Add a test case:
{
code: `export async function main() { core.error("fatal"); process.exitCode = 1; }`,
errors: [{ messageId: "noCoreErrorThenProcessExitCode",
suggestions: [{ messageId: "replaceWithSetFailed" }] }],
}@copilot please address this.
|
|
||
| return { | ||
| BlockStatement(node: TSESTree.BlockStatement) { | ||
| checkStatements(node.body); |
There was a problem hiding this comment.
[/tdd] The Program visitor filters out ImportDeclaration and ExportAllDeclaration but not ExportNamedDeclaration. If someone writes export const x = 1; core.error("msg"); process.exitCode = 1; at the top level, the index after filtering may not align with the original body indices — the filter creates a new array, so stmts[i] and stmts[i+1] are now the correct adjacent statements within the filtered list. However, process.exitCode = 1 wrapped in ExportNamedDeclaration (e.g. export { process }) is a separate concern. More critically: if there are ExportNamedDeclarations (re-exports) interspersed, the filter removes them and two formerly non-adjacent statements may appear adjacent. This could produce false positives.
💡 Example false positive
core.error("msg");
export { helper }; // this is filtered out
process.exitCode = 1;After filtering, core.error("msg") and process.exitCode = 1 appear adjacent and get flagged, even though they are not literally adjacent in source.
Consider using a different strategy: traverse the unfiltered body but skip non-statement nodes, or look backward from the process.exitCode statement for the nearest real statement.
@copilot please address this.
| const callee = errorCall.callee as TSESTree.MemberExpression; | ||
| const objectName = sourceCode.getText(callee.object); | ||
| const replacement = enclosingFn !== null ? `${objectName}.setFailed(${args}); return;` : `${objectName}.setFailed(${args});`; | ||
| return [fixer.replaceText(current, replacement + "\n"), fixer.remove(next)]; |
There was a problem hiding this comment.
[/tdd] The autofix for the module-level (no enclosing function) case omits return;. In mcp_cli_bridge.cjs the target locations are at module top-level, and after replacing with core.setFailed(msg) only, execution continues. The PR description says the fix is core.setFailed(msg); return;, but the fixer code branches: enclosingFn !== null ? ... return; : ...setFailed(msg) — the return; is intentionally dropped for module-level. However at module top-level return is not valid JS, so this is correct, but the test outputs for the top-level case (lines 36-47 in test file) show no return; either. The inconsistency between the description ("Replace with core.setFailed(msg); return;") and the actual fix (no return at module level) may confuse users. The suggestion message should be split into two variants.
💡 Suggestion
Add a second messageId:
messages: {
replaceWithSetFailed: "Replace with `core.setFailed(msg); return;`.",
replaceWithSetFailedNoReturn: "Replace with `core.setFailed(msg)` (module-level — `return` is not valid here).",
}And use the appropriate one in the fix based on enclosingFn.
@copilot please address this.
| ], | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[/tdd] Missing test coverage for the SwitchCase visitor. The rule handles switch-case bodies via checkStatements(node.consequent), but there is no test case exercising this path. A regression could silently disable detection in switch blocks.
💡 Suggested test case
{
code: `switch (x) { case 1: core.error("fatal"); process.exitCode = 1; break; }`,
errors: [{ messageId: "noCoreErrorThenProcessExitCode", suggestions: [] }],
}(No autofix because the enclosing function is neither null nor main.)
@copilot please address this.
…code-4e52a7822d92bdee
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Summary
Adds a new ESLint rule
gh-aw-custom/no-core-error-then-process-exitcodeto theeslint-factoryplugin. The rule flags the anti-patterncore.error(msg); process.exitCode = nonzeroin GitHub Actions scripts and guides authors towardcore.setFailed(msg), which correctly marks the action as failed and allows post-action cleanup hooks to run.This is a sibling to the existing
no-core-error-then-process-exitrule, targetingprocess.exitCode = nonzeroassignments rather thanprocess.exit(1)calls. Unlikeprocess.exit(1), settingprocess.exitCodedoes not immediately halt execution — subsequent code continues running in a failed state.Changes
eslint-factory/src/rules/no-core-error-then-process-exitcode.tseslint-factory/src/rules/no-core-error-then-process-exitcode.test.tseslint-factory/src/index.tseslint-factory/eslint.config.cjswarnseverityRule Behaviour
Flagged pattern:
Preferred replacement:
Autofix scope (suggestion only): Available when the pattern appears inside a top-level
main()function (namedmainat program scope, includingexport async function main) with a single non-spread argument. For other contexts (module scope, helper functions,SwitchCasebranches, or multi-argument calls) the rule reports without a suggestion fix.Valid patterns not flagged:
core.error(msg); process.exitCode = 0— clean exit, not an error signalprocess.exitCode = 1without a precedingcore.error— unrelated assignmentcore.warning(msg); process.exitCode = 1— non-error log methodscore.error(msg); process.exitCode = code— runtime value, unknown at lint timecore.error(msg); process.exit(1)— covered by the sibling rulecore.error(msg); process.exitCode += 1— compound assignment operatorDetection
The rule uses
@typescript-eslint/utilsAST traversal acrossBlockStatement,SwitchCase, andProgramnode visitors. It resolves core aliases via the sharedCORE_ALIASESset andisCoreAliasIdentifierhelper, consistent with other rules in the plugin.