[eslint-miner] eslint-factory: extend no-setfailed-then-exit-zero to catch process.exitCode = 0

[github-actions]

test body

Generated by PR Description Updater for #49740 · auto · 50.6 AIC · ⌖ 5.47 AIC · ⊞ 6.9K · ◷

[github-actions]

Hey @github-actions 👋 — thanks for this soundness improvement to the no-setfailed-then-exit-zero ESLint rule! This looks great:

✅ Comprehensive test coverage — new test cases cover all the key scenarios (adjacent detection, non-zero assignments, string literals, intervening statements, and return handling) to ensure the new message is triggered correctly.

✅ Clear implementation — the new isProcessExitCodeZero() helper function is well-documented and correctly identifies literal zero assignments to process.exitCode, while the continued scanning behavior properly handles follow-up statements.

✅ Focused scope — the change does exactly one thing: extends the rule to catch the coverage gap identified in #49721 without touching unrelated logic.

✅ Validation provided — all tests pass, linting succeeds on the target codebase, and the implementation is verified clean.

This PR is ready for review!

Generated by ✅ Contribution Check · auto · 51.3 AIC · ⌖ 4.74 AIC · ⊞ 8.7K · ◷

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

No test files were added or modified in this PR. Test Quality Sentinel skipped.

[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 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.

[github-actions]

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

[Copilot]

Pull request overview

Extends the ESLint rule to detect exit-code assignments that override core.setFailed().

Changes:

• Detects process.exitCode = 0.
• Adds diagnostics, removal suggestions, and tests.
• Continues scanning for later exit calls.

Show a summary per file

File	Description
no-setfailed-then-exit-zero.ts	Implements detection and reporting.
no-setfailed-then-exit-zero.test.ts	Adds valid and invalid cases.

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: 2
• Review effort level: Balanced

[github-actions]

The implementation is correct and well-tested.

• isProcessExitCodeZero properly matches only the literal number 0 assignment (not strings, variables, or computed properties).
• Scanning continues after flagging process.exitCode = 0 — correct, since it does not halt execution unlike process.exit(0).
• Test coverage is thorough: valid cases (standalone, non-zero, variable, return-before, string "0") and invalid cases (adjacent, non-adjacent, inside function).
• Message IDs and suggestion fix are consistent with the existing pattern.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 14.1 AIC · ⌖ 9.16 AIC · ⊞ 5.4K

[github-actions]

Skills-Based Review 🧠

Applied /tdd and /codebase-design — two minor improvements noted, no blocking issues.

📋 Key Themes & Highlights

Issues Found

• Missing test for scan-continuation (test file, line 127): The key behavioral difference between process.exitCode = 0 (non-halting) and process.exit(0) (halting) is advertised in the PR description but has no test. A combined case covering both patterns in one block would cover the continue branch explicitly.
• Fixer whitespace artifact (impl file, line 191): fixer.remove(candidate) leaves a trailing space/double-space in the fixed output, as visible in the test snapshots. Pre-existing pattern, but worth cleaning up.

Positive Highlights

• ✅ Predicate isProcessExitCodeZero is clean and precise — correctly rejects non-zero literals, string "0", and identifier rvalues.
• ✅ Existing scan logic (control-transfer stop) is untouched; the new path integrates naturally via continue.
• ✅ Test coverage is thorough for valid cases (non-zero, variable, string, return guard) and invalid cases (adjacent, non-adjacent, in-function).
• ✅ PR description clearly documents the soundness rationale and validation steps.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 31.3 AIC · ⌖ 11 AIC · ⊞ 7.1K
_{Comment /matt to run again}

[github-actions]

[/tdd] The PR description highlights that process.exitCode = 0 does not halt execution and scanning continues — but no test exercises that property.

💡 Suggested test case

Add an invalid case that confirms a later process.exit(0) is still caught after process.exitCode = 0:

// Both patterns after setFailed — both should be reported
{
  code: `core.setFailed("bad"); process.exitCode = 0; process.exit(0);`,
  errors: [
    { messageId: "noSetFailedThenExitCodeZero" },
    { messageId: "noSetFailedThenExitZero" },
  ],
},

Without this test the continue path in the implementation is exercised only by manual reasoning.

@copilot please address this.

[github-actions]

[/codebase-design] fixer.remove(candidate) removes the node's source range but leaves a trailing space, producing double-spaces visible in the test snapshots (e.g. core.setFailed("bad"); }). The process.exit(0) path uses the same strategy, so this is a pre-existing quirk — but flagging it here since the new path inherits it.

💡 A tighter removal

Replace:

return fixer.remove(candidate);

with:

return fixer.removeRange([candidate.range![0], candidate.range![1]]);
// or, to eat the leading whitespace/newline:
const src = context.getSourceCode();
const tokenBefore = src.getTokenBefore(candidate);
const start = tokenBefore ? tokenBefore.range[1] : candidate.range![0];
return fixer.removeRange([start, candidate.range![1]]);

This keeps fixer output clean in multi-statement bodies.

@copilot please address this.

[github-actions]

Verdict: Approve with a minor note

Sound, well-scoped extension of the rule with solid test coverage; one non-blocking noise concern flagged inline.

Review themes

• The new isProcessExitCodeZero matcher correctly restricts to process.exitCode = 0 (literal zero, not string/variable), matching the intent.
• The continue (vs break) after reporting exitCode = 0 is the right call for correctness (execution doesn't halt), but combined with a later process.exit(0) in the same block it can produce two diagnostics for what is effectively one failure-hiding chain — flagged as an inline comment, not blocking.
• Test coverage is good: adjacent, non-adjacent, in-function, negative cases (string literal, variable, return-before-assignment) are all covered.
• Autofix (fixer.remove) leaves a trailing space per the test snapshots — cosmetic, already implicitly accepted by the existing process.exit(0)->return; fixer pattern in this file.

🔎 Code quality review by PR Code Quality Reviewer · auto · 58.1 AIC · ⌖ 6.36 AIC · ⊞ 7.8K
_{Comment /review to run again}

[github-actions]

After reporting process.exitCode = 0, the loop continues instead of breaking, so a later process.exit(0) in the same block triggers a second, separate diagnostic for what is effectively one failure-hiding chain.

💡 Double-reporting for a single logical violation

For input like:

core.setFailed("bad");
process.exitCode = 0;
process.exit(0);

ESLint will emit both noSetFailedThenExitCodeZero (for the assignment) and noSetFailedThenExitZero (for the exit call), even though only one exit-code override matters at runtime (the process.exit(0) wins). This isn't incorrect per se, but it produces noisy duplicate errors for a single root cause and no test exercises this combination. Consider tracking whether a report was already made for this setFailed chain and suppressing the second one, or add a test documenting this is the intended (if slightly noisy) behavior.

[github-actions]

🎉 This pull request is included in a new release.

Release: v0.84.3