eslint: no-core-error-then-process-exitcode — forward scan + top-level autofix - #47240
Conversation
…scan + top-level autofix - Add isCoreSetFailedStatement and isControlTransferStatement helpers - Rewrite checkStatements to scan forward from core.error() (like sibling rule) - Stop scan at core.setFailed() or control-transfer statements (return, throw, break, etc.) - Non-adjacent pairs report without autofix suggestion - Add module top-level autofix: enclosingFn === null → core.setFailed(args); no return; - Update Program handler to split body at export/import declarations (preserving export-barrier behavior) - Update rule docs to describe forward-scan semantics - Add new valid tests: setFailed-stops-scan, control-transfer-stops-scan - Add new invalid tests: non-adjacent pair, two-intervening-statements - Update module-top-level and switch-case invalid tests to include autofix suggestions Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Extends the ESLint rule to detect non-adjacent failure patterns and provide safe top-level fixes.
Changes:
- Adds forward scanning with control-transfer barriers.
- Enables module-level autofix suggestions.
- Expands tests for scanning and autofixes.
Show a summary per file
| File | Description |
|---|---|
no-core-error-then-process-exitcode.ts |
Implements scanning, barriers, and autofixes. |
no-core-error-then-process-exitcode.test.ts |
Tests detection and suggestion behavior. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Medium
| // throw between error and exitCode stops scanning | ||
| `function run() { core.error("x"); throw new Error("x"); process.exitCode = 1; }`, |
|
✅ 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 (default_business_additions=0). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 92/100 — Excellent
📊 Metrics (11 test cases analyzed)
✨ HighlightsCoverage Summary:
No Violations: Test file structure is correct (vitest with ESLint RuleTester), all assertions explicit, no mocks, no happy-path-only bias. Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST_CHANGES — two medium-severity correctness gaps
The forward-scan rewrite is a solid improvement, but two issues need fixing before merge:
-
Duplicate reports on consecutive calls (line 222) — multiple before one each trigger a report, producing conflicting autofix suggestions on the same node. Needs deduplication or equivalent.
-
** not recognized as a control-transfer barrier** (line 124) — inconsistency with which handles both computed and non-computed forms; causes false-positive reports when appears between the pair.
Other observations (non-blocking)
- and test coverage gap (flagged in existing review comment #3628789116): the tests cover and but not // as barriers.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 43.5 AIC · ⌖ 4.93 AIC · ⊞ 5.6K
Comment /review to run again
|
|
||
| // Stop scanning if setFailed already handles the failure or a control-transfer exits the block. | ||
| if (isCoreSetFailedStatement(candidate, sourceCode) || isControlTransferStatement(candidate)) { | ||
| break; |
There was a problem hiding this comment.
Consecutive core.error calls produce duplicate reports on the same process.exitCode node: two core.error(...) statements before one process.exitCode = 1 will both report, generating two lint errors for a single assignment — and if both qualify for autofix, two conflicting fixers target the same node.
💡 Details and suggested fix
Example that currently produces two separate reports:
core.error("a");
core.error("b");
process.exitCode = 1; // reported by BOTH i=0 and i=1When i=0 finds the process.exitCode at j=2 (non-adjacent, no autofix), it breaks and the outer loop continues to i=1. i=1 then finds the same process.exitCode at j=2 (adjacent, now autofix-eligible), generating a second report on an already-reported node with a conflicting fixer.
Simplest fix — deduplicate by tracking already-reported process.exitCode nodes:
function checkStatements(stmts: readonly TSESTree.Statement[]): void {
const reported = new WeakSet<TSESTree.Statement>();
for (let i = 0; i < stmts.length - 1; i++) {
...
if (isProcessExitCodeNonZero(candidate)) {
if (!reported.has(candidate)) {
reported.add(candidate);
context.report({ ... });
}
break;
}
}
}| // process.exit(...) — any call, regardless of exit code | ||
| if (node.type === AST_NODE_TYPES.ExpressionStatement && node.expression.type === AST_NODE_TYPES.CallExpression) { | ||
| const callee = node.expression.callee; | ||
| if ( |
There was a problem hiding this comment.
isControlTransferStatement does not handle computed process["exit"]() calls: isCoreSetFailedStatement explicitly handles both computed and non-computed access, but isControlTransferStatement only matches process.exit(...), missing process["exit"](1).
💡 Details and suggested fix
// Currently only catches:
process.exit(1)
// Misses:
process["exit"](1)This is an inconsistency with isCoreSetFailedStatement which handles both forms. A process["exit"]() between core.error and process.exitCode will not stop the scan, producing a false-positive report.
Fix: add the computed check, mirroring isCoreSetFailedStatement:
if (
callee.type === AST_NODE_TYPES.MemberExpression &&
callee.object.type === AST_NODE_TYPES.Identifier &&
callee.object.name === "process" &&
callee.property.type === AST_NODE_TYPES.Identifier &&
callee.property.name === "exit" &&
!callee.computed
) { ... }
// Add:
if (
callee.type === AST_NODE_TYPES.MemberExpression &&
callee.object.type === AST_NODE_TYPES.Identifier &&
callee.object.name === "process" &&
callee.computed &&
callee.property.type === AST_NODE_TYPES.Literal &&
callee.property.value === "exit"
) { return true; }
There was a problem hiding this comment.
Review: forward scan + top-level autofix
The forward-scan logic and top-level autofix are a solid improvement. Program segmentation at export boundaries is correct, and the barrier helpers are clean.
One blocking correctness issue (see inline):
When two core.error(...) statements both precede process.exitCode = nonzero, the outer loop visits each independently — the inner scan finds the same process.exitCode node each time — producing duplicate diagnostics on the same source location. Fix by tracking reported process.exitCode nodes in a Set, or advancing i past the matched index after reporting.
The existing comment about missing barrier-branch tests (break, continue, process.exit) is also worth addressing.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 44.8 AIC · ⌖ 4.6 AIC · ⊞ 5K
|
|
||
| // 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++) { |
There was a problem hiding this comment.
Potential duplicate reports when two core.error calls precede process.exitCode.
The inner for j loop means every core.error in the segment independently scans forward and finds the same process.exitCode node. For input like:
core.error("a"); core.error("b"); process.exitCode = 1;both i=0 and i=1 hit isProcessExitCodeNonZero(candidate) and report() fires twice on the same node, producing two diagnostics.
Consider tracking already-reported process.exitCode nodes in a Set and skipping if already reported, or break-ing the outer loop after a match is consumed.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 069ebdb. Added a reported = new WeakSet<TSESTree.Statement>() inside checkStatements; when the inner loop finds a process.exitCode node that's already in the set, it skips reporting and breaks. This prevents the second core.error from generating a conflicting diagnostic on the same node.
There was a problem hiding this comment.
Skills-Based Review
Applied /tdd and /codebase-design — no blocking issues, but a few things to address.
📋 Key Themes & Highlights
Key Themes
- Multi-
core.errorscenario untested: Two consecutivecore.error()calls sharing the sameprocess.exitCodenode will both produce reports; the adjacent one applies an autofix that removes the exitCode, while the outer one (non-adjacent) is left with no actionable path. The behaviour should be tested and, if needed, guarded with a claimed-node set. break/continue/process.exitstop-conditions untested: An existing comment (from a previous reviewer) correctly flags that these branches ofisControlTransferStatementlack test coverage.
Positive Highlights
- Forward-scan rewrite is clean and correctly mirrors the sibling rule.
- Context-aware autofix (module top-level vs. inside
main()) is a solid improvement. - Program-body segmentation at export/import boundaries preserves existing behaviour with a clear comment explaining why.
- Helper names (
isCoreSetFailedStatement,isControlTransferStatement) are consistent with the sibling rule — good domain coherence.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 44.7 AIC · ⌖ 4.9 AIC · ⊞ 6.7K
Comment /matt to run again
|
|
||
| // 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++) { |
There was a problem hiding this comment.
[/tdd] Two consecutive core.error() calls before the same process.exitCode will each produce their own report — the adjacent one gets an autofix that removes the exitCode, potentially leaving the first error call in an inconsistent state.
💡 Scenario + suggested fix
core.error("a");
core.error("b");
process.exitCode = 1;The outer i loop reports twice: core.error("a") (non-adjacent, no autofix) and core.error("b") (adjacent, autofix that removes the exitCode). After applying the second suggestion, the first report can no longer be autofixed and leaves core.error("a") without any fix path.
Track which exitCode nodes have already been claimed, or add a test that documents the intended behavior for this pattern:
const claimedExitCodes = new Set<TSESTree.Statement>();
// inside the j loop, before reporting:
if (claimedExitCodes.has(candidate)) break;
claimedExitCodes.add(candidate);@copilot please address this.
There was a problem hiding this comment.
Fixed in 069ebdb. Added the reported WeakSet deduplication — for core.error("a"); core.error("b"); process.exitCode = 1;, only one error is now reported (from the first core.error, non-adjacent, no autofix), and the second core.error's claim on the same exitCode node is suppressed.
| break; | ||
| } | ||
|
|
||
| // Stop scanning if setFailed already handles the failure or a control-transfer exits the block. |
There was a problem hiding this comment.
[/tdd] The autofix fixer removes candidate (the process.exitCode node), but when the pair is non-adjacent the break fires without a suggestion — however nothing prevents a future code path from calling fixer.remove(candidate) on the same node twice if the outer i loop reaches another core.error that is also adjacent to the same exitCode node. Adding a test with two separate adjacent core.error → same process.exitCode would lock down this invariant.
💡 Test scaffold
{
// Two core.error() calls both adjacent to the same process.exitCode
// — expected: one report per core.error(), only the closest one gets autofix
code: `core.error("a"); core.error("b"); process.exitCode = 1;`,
errors: [
{ messageId: "noCoreErrorThenProcessExitCode", suggestions: [] },
{ messageId: "noCoreErrorThenProcessExitCode", suggestions: [{ messageId: "replaceWithSetFailed", output: "..." }] },
],
}@copilot please address this.
There was a problem hiding this comment.
Added in 069ebdb. The new invalid test case in the test file documents the deduplication invariant: core.error("a"); core.error("b"); process.exitCode = 1; produces exactly one error (from core.error("a"), non-adjacent, no autofix), preventing conflicting fixers on the same node.
| function isControlTransferStatement(node: TSESTree.Statement): boolean { | ||
| // prettier-ignore | ||
| if ( | ||
| node.type === AST_NODE_TYPES.ReturnStatement || |
There was a problem hiding this comment.
[/codebase-design] isCoreSetFailedStatement accepts sourceCode: SourceCode solely for the alias check, while isControlTransferStatement takes no such parameter. The asymmetry is fine functionally, but a brief comment noting why the signatures differ (alias resolution vs. pure syntax) would help the next reader who wonders if the omission was intentional.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 069ebdb. Updated the JSDoc for isCoreSetFailedStatement to note: "Accepts sourceCode for alias resolution via isCoreAliasIdentifier; contrast with isControlTransferStatement which is a pure syntax check and needs no source-code context."
|
@copilot run pr-finisher skill |
…it], add test coverage for all barriers Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
no-core-error-then-process-exitcodeonly checked the immediately adjacent statement aftercore.error(), silently missing the pattern when any intervening statement appeared. It also lacked the module-top-level autofix present in the siblingno-core-error-then-process-exitrule.Detection
Rewrites
checkStatementsto scan forward fromcore.error()(innerfor j = i+1loop), stopping atcore.setFailed()or any control-transfer statement — now catching:Added
isCoreSetFailedStatementandisControlTransferStatementhelpers (copied from sibling). New valid cases:core.setFailedorreturn/throw/breakbetween the pair stops the scan.Autofix
safeToFixnow includesenclosingFn === null(module top level), mirroring the sibling. At top level the fixer emitscore.setFailed(msg);withoutreturn;; insidemain()it appendsreturn;as before. Non-adjacent pairs never get autofix.Program-level export barrier
The
Programhandler segments the body at export/import declarations before callingcheckStatements, so anexport { … }betweencore.errorandprocess.exitCodestill suppresses the report (existing behavior preserved).Tests
core.error("x"); core.info("y"); process.exitCode = 1;(non-adjacent, no suggestion)core.error("x"); core.setFailed("y"); process.exitCode = 1;andcore.error("x"); return; process.exitCode = 1;