eslint(no-core-error-then-process-exit): widen detection to non-adjacent pairs

[Copilot]

The rule only flagged core.error(msg) immediately followed by process.exit(nonzero). Any intervening statement (logging, summary, etc.) defeated detection — the anti-pattern was unchanged but invisible to the rule.

Changes

Rule logic (no-core-error-then-process-exit.ts)

• checkStatements now scans forward through all sibling statements after core.error() instead of stopping at stmts[i+1]
• Stops early (no flag) on:
  • core.setFailed(...) — failure already properly signalled
  • return / throw / break / continue — control no longer reaches the exit
• Autofix suggestion retained for adjacent pairs only; non-adjacent pairs are flagged without a suggestion to avoid a fixer that leaves orphaned intervening statements
• Added isCoreSetFailedStatement() and isControlTransferStatement() helpers

Tests (no-core-error-then-process-exit.test.ts)

New invalid cases (now flagged):

core.error("fatal"); core.info("y"); process.exit(1);           // intervening log
core.error("fatal"); core.summary.addRaw("fatal"); process.exit(1); // intervening summary

New valid cases (correctly suppressed):

core.error("x"); core.setFailed("x"); process.exit(1); // setFailed stops scan
core.error("x"); return; process.exit(1);               // control-transfer stops scan
core.error("x"); throw new Error(); process.exit(1);    // control-transfer stops scan

• Fixes eslint-factory: no-core-error-then-process-exit only checks adjacent statements — misses intervening statements (FN) #46993

[Copilot]

Pull request overview

Widens the ESLint rule to detect non-adjacent core.error() and nonzero process.exit() pairs.

Changes:

• Adds forward sibling-statement scanning with control-flow barriers.
• Limits autofix suggestions to adjacent pairs.
• Adds valid and invalid regression cases.

Show a summary per file

File	Description
eslint-factory/src/rules/no-core-error-then-process-exit.ts	Implements widened detection and scan barriers.
eslint-factory/src/rules/no-core-error-then-process-exit.test.ts	Tests intervening statements and direct barriers.

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-exit.ts:225

• The forward scan also crosses nested control-flow statements, although the linked issue requires stopping conservatively at a nested container that changes flow. This can produce definite false positives—for example, function f() { core.error("x"); if (true) { return; } process.exit(1); } is reported even though the exit is unreachable. Stop at such containers, or add fallthrough analysis before scanning beyond them.

          // Stop scanning if setFailed already handles the failure or a control-transfer exits the block.
          if (isCoreSetFailedStatement(candidate, sourceCode) || isControlTransferStatement(candidate)) {
            break;

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Medium

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR #47043 does not have the 'implementation' label and has 0 new lines of code in business logic directories (src/, lib/, pkg/, internal/, app/, core/, domain/, services/, api/).

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[github-actions]

🧪 Test Quality Sentinel Report

✅ Test Quality Score: 100/100 — Excellent

Analyzed 3 test(s): 3 design, 0 implementation, 0 violation(s).

📊 Metrics (3 tests)

Metric	Value
Analyzed	3 (Go: 0, JS: 3)
✅ Design	3 (100%)
⚠️ Implementation	0 (0%)
Edge/error coverage	3 (100%)
Duplicate clusters	0
Inflation	No (ratio 0.29:1)
🚨 Violations	0

Test	File	Classification	Issues
invalid[12] — 1 intervening stmt	no-core-error-then-process-exit.test.ts	design_test / high_value	None
invalid[13] — summary call intervening	no-core-error-then-process-exit.test.ts	design_test / high_value	None
invalid[14] — 2 intervening stmts	no-core-error-then-process-exit.test.ts	design_test / high_value	None

Verdict

✅ Passed. 0% implementation tests (threshold: 30%). All 3 new tests cover the behavioral contract for non-adjacent core.error + process.exit(1) detection.

🧪 Test quality analysis by Test Quality Sentinel · 30.2 AIC · ⌖ 7.14 AIC · ⊞ 6.9K · ◷
_{Comment /review to run again}

[github-actions]

✅ Test Quality Sentinel: 100/100. 0% implementation tests (threshold: 30%). All 3 new tests are high-value design tests covering the non-adjacent detection behavioral contract.

[github-actions]

Review: no-core-error-then-process-exit widen detection

The logic and tests are well-structured. One correctness gap already noted in an existing inline comment:

process.exit(0) is not treated as a scan barrier

The inner loop stops only on setFailed, control-transfer statements, or a nonzero process.exit. A zero-exit between core.error(...) and a later nonzero process.exit is silently skipped:

core.error("x"); process.exit(0); process.exit(1);

This would be falsely flagged as-if process.exit(0) were not there, even though the process has already terminated. Any process.exit(...) (regardless of argument) should stop the forward scan.

This matches the existing inline comment on line 116. Addressing that comment resolves this review.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 17.4 AIC · ⌖ 4.4 AIC · ⊞ 4.9K

[github-actions]

Skills-Based Review 🧠

Applied /diagnosing-bugs and /tdd — requesting changes on two correctness gaps.

📋 Key Themes & Highlights

Issues

• process.exit(0) is not a scan barrier (line 224): the inner loop does not stop when it encounters a process.exit(0), so core.error("x"); process.exit(0); process.exit(1); produces a false positive. Any process.exit call should terminate the forward scan.
• Off-by-one in outer loop (line 175): stmts.length - 1 was correct for the old adjacent check but now silently excludes the last statement from being a core.error origin. Change to stmts.length.

Positive Highlights

• ✅ Clean decomposition into isCoreSetFailedStatement and isControlTransferStatement helpers
• ✅ Good decision to suppress the autofix for non-adjacent pairs — avoids orphaned intervening statements
• ✅ Test cases directly map to the acceptance criteria in the PR description
• ✅ Consistent reuse of isCoreAliasIdentifier in the new isCoreSetFailedStatement helper

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 28.8 AIC · ⌖ 4.75 AIC · ⊞ 6.6K
_{Comment /matt to run again}

[github-actions]

[/diagnosing-bugs] process.exit(0) is not treated as a scan barrier — the forward scan continues past it and may produce a false positive on core.error("x"); process.exit(0); process.exit(1);, flagging the error even though the clean exit means execution never reaches the non-zero exit.

💡 Suggested fix

Add a helper and break the inner loop on any process.exit call, not only non-zero ones:

function isAnyProcessExitStatement(node: TSESTree.Statement): 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;
  return (
    callee.type === AST_NODE_TYPES.MemberExpression &&
    !callee.computed &&
    callee.object.type === AST_NODE_TYPES.Identifier &&
    callee.object.name === 'process' &&
    callee.property.type === AST_NODE_TYPES.Identifier &&
    callee.property.name === 'exit'
  );
}

Then in the scan loop, after the isProcessExitNonZero check, add:

if (isAnyProcessExitStatement(candidate)) break;

Pair it with a valid test case: core.error("x"); process.exit(0); process.exit(1); should be not flagged.

@copilot please address this.

[github-actions]

[/tdd] The outer loop bound stmts.length - 1 was carried over from the old adjacent-only check. With the new inner scan it is no longer necessary to stop one short — a core.error on the last statement can still have a process.exit scanned by the inner loop. The current bound silently drops the last statement as a potential error site.

💡 Suggested fix

Change the outer loop condition to iterate over all statements:

for (let i = 0; i < stmts.length; i++) {

And add a test case to verify the last-statement edge case:

// NOT flagged — core.error is last statement, no process.exit follows
valid: ['core.error("x");']

The inner loop j < stmts.length already terminates safely when i is the last index.

@copilot please address this.

[github-actions]

Two correctness issues must be fixed before merge.

Blocking findings

1. Multiple core.error before one process.exit causes duplicate reports + potential double-remove fixer corruption (line 181). The outer loop fires for every core.error ancestor, all matching the same exit node. If both suggestions are applied, fixer.remove is called twice on the same node — undefined ESLint fixer behavior. Fix: treat a second core.error as a scan barrier in the inner loop.

2. process.exit(0) is not a scan barrier (line 224). The new forward-scan skips over any zero-exit between the error and a later nonzero exit, producing a false positive report for code that already exits cleanly. This amplifies an existing concern already raised in a prior review comment. Fix: treat any process.exit(...) call — regardless of argument — as a barrier.

🔎 Code quality review by PR Code Quality Reviewer · 52.3 AIC · ⌖ 4.73 AIC · ⊞ 5.6K
_{Comment /review to run again}

[github-actions]

Duplicate reports and potential double-remove when multiple core.error calls precede one process.exit: both outer-loop iterations independently match the same exit node, and if autofixes were both applied fixer.remove(candidate) would run twice on the same AST node.

💡 Details and suggested fix

Given:

core.error("a"); core.error("b"); process.exit(1);

• Outer iteration i=0: inner scan reaches process.exit(1) as non-adjacent → reports core.error("a") with no suggestion.
• Outer iteration i=1: inner scan reaches process.exit(1) as adjacent → reports core.error("b") with a suggestion that calls fixer.remove(candidate) on the exit node.

Applying both suggestions (e.g. --fix-suggestions) calls fixer.remove twice on the same node — undefined behaviour in the ESLint fixer that can produce corrupted output.

The non-adjacent report on core.error("a") is also misleading: the actual anti-pattern is the adjacent pair at i=1.

Suggested fix: treat a second core.error as a scan barrier in the inner loop (just like setFailed), so the first error's scan stops before reaching an exit that belongs to the next error:

if (isCoreErrorStatement(candidate, sourceCode)) break; // stop at next core.error

[github-actions]

process.exit(0) (zero/success) is not treated as a scan barrier, creating a false positive: core.error("x"); process.exit(0); process.exit(1); will report the core.error as matched to the non-adjacent process.exit(1), even though the zero exit already terminates the process — this is the same concern already raised in the existing review comment (#3621115276), and this newly added scan path makes it more impactful.

💡 Details and suggested fix

The existing comment flagged this at line 116, but the new forward-scanning loop at line 181 makes the false positive easier to hit: any process.exit(0) between the error and a later nonzero exit is silently skipped over.

A process.exit() of any kind is a definitive control-flow terminator — the process exits, full stop. The scan should stop (barrier) at any process.exit(...), not only at nonzero ones.

Suggested fix: add an isAnyProcessExit helper or check in the barrier logic:

// Stop scanning if ANY process.exit is encountered (zero or nonzero)
if (isAnyProcessExit(candidate)) break;

This prevents the misleading report where an error is "associated" with an exit that is unreachable given the earlier exit.

done

[pelikhan]

@copilot run pr-finisher skill

[Copilot]

@copilot run pr-finisher skill

Fixed in the latest commit. isControlTransferStatement now includes any process.exit(...) call as a barrier (matching the pattern in require-return-after-core-setfailed.ts:20-39), so process.exit(0) and process.exit(variable) between core.error() and a later process.exit(1) stop the forward scan. Regression test cases for both barriers added.

[github-actions]

🎉 This pull request is included in a new release.

Release: v0.83.0