fix(eslint): patch switch fall-through false negative and hoisted function false positive in require-return-after-core-setfailed

[Copilot]

Two precision gaps in require-return-after-core-setfailed: a false negative for bare switch-case fall-through, and a false positive for trailing hoisted function declarations.

False negative — bare switch fall-through (was silently missed)

switch (x) {
  case 1:
    core.setFailed("bad");  // last in consequent → no in-list next → not flagged ❌
  case 2:
    doMore();               // runs in failed state
}

Root cause: SwitchCase visitor only called checkStatementList, which needs a next sibling. When setFailed is the last consequent statement, next is undefined and the existing fall-through logic in findContinuationOutsideBlock was never reached.

Fix: mirror the BlockStatement pattern — call checkNestedContinuation on the last consequent statement when it is a setFailed. Reuses the fall-through machinery already present in findContinuationOutsideBlock.

False positive — trailing hoisted function declaration (was spuriously flagged)

core.setFailed("bad");
function helper() {}  // hoisted; no sequential runtime effect — flagged ❌

Root cause: FunctionDeclaration was not excluded from isExecutableStatement, so it became next in checkStatementList and isControlTransfer returned false.

Fix: add FunctionDeclaration, TSInterfaceDeclaration, and TSTypeAliasDeclaration to the isExecutableStatement exclusion list alongside the existing ImportDeclaration/export/TSModuleDeclaration exclusions.

• Fixes eslint-factory: require-return-after-core-setfailed misses bare switch-case fall-through (FN) and flags trailing function declar [Content truncated due to length] #46992

[Copilot]

Pull request overview

Fixes precision gaps in the ESLint rule for execution after core.setFailed().

Changes:

• Detects bare switch-case fall-through.
• Ignores trailing hoisted/type-only declarations.
• Adds regression tests for both behaviors.

Show a summary per file

File	Description
eslint-factory/src/rules/require-return-after-core-setfailed.ts	Updates continuation detection and declaration filtering.
eslint-factory/src/rules/require-return-after-core-setfailed.test.ts	Adds switch fall-through and hoisted-function tests.

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

[gh-aw-bot]

@copilot please run the pr-finisher skill and address the unresolved inline review feedback before rerunning checks:

• Unresolved review comment: update the switch-case continuation logic so fall-through only reports when a later case contains an executable statement, not just a hoisted declaration.

Run: https://github.com/github/gh-aw/actions/runs/29826418446

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• pi.dev

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "pi.dev"

See Network Configuration for more information.

Generated by 👨‍🍳 PR Sous Chef · gpt54 13.2 AIC · ⌖ 6.32 AIC · ⊞ 7K · ◷
_{Comment /souschef to run again}

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

No analyzable test files in this PR. Changed test file is TypeScript (.test.ts), which is outside the scope of the Test Quality Sentinel (analyzes Go, JavaScript, Python, Rust). TypeScript testing is not covered by this analysis framework.

[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 #47055 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).

[github-actions]

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

[Copilot]

@copilot please run the pr-finisher skill and address the unresolved inline review feedback before rerunning checks:

• [Unresolved review comment](fix(eslint): patch switch fall-through false negative and hoisted function false positive in require-return-after-core-setfailed #47055 (comment)...

Fixed in the latest commit. findContinuationOutsideBlock now uses find(s => isExecutableStatement(...)) when scanning subsequent switch cases for a fall-through continuation, so cases whose only statements are hoisted declarations (e.g., FunctionDeclaration) are skipped and we keep looking — or return null if no executable statement is found anywhere. The regression test case switch (x) { case 1: core.setFailed("bad"); case 2: function helper() {} } now passes as valid.

[github-actions]

Skills-Based Review 🧠

Applied /diagnosing-bugs and /tdd — requesting changes on two correctness gaps that interact with each other.

📋 Key Themes & Highlights

Issues Found

1. findContinuationOutsideBlock — SwitchCase branch doesn't filter non-executable statements (line 105–106): The Program branch correctly applies isExecutableStatement, but the SwitchCase consequent branch does not. A hoisted FunctionDeclaration inside a case block can become a false continuation when searching for a next sibling after setFailed.

2. SwitchCase visitor uses raw lastStmt instead of last-executable (line 348): isCoreSetFailedStatement(lastStmt) silently returns false when the case ends with a hoisted declaration, causing the fall-through check to be skipped even when setFailed is the last executable statement. The two fixes (FP and FN) were not composed consistently.

3. Missing regression test for the interaction: No test covers case 1: core.setFailed(...); function helper() {} case 2: doMore() — which should be invalid but slips past both checkStatementList and checkNestedContinuation in the current implementation.

Positive Highlights

• ✅ Root causes are correctly diagnosed in the PR description — the fix for findContinuationOutsideBlock's fall-through scanning (lines 138–144) is sound and well-explained.
• ✅ The isExecutableStatement exclusions for TSInterfaceDeclaration and TSTypeAliasDeclaration are a nice defensive bonus.
• ✅ The new tests added are well-structured and improve coverage meaningfully for the happy paths.

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

Comments that could not be inline-anchored

eslint-factory/src/rules/require-return-after-core-setfailed.ts:105

[/diagnosing-bugs] The SwitchCase branch in findContinuationOutsideBlock does not filter non-executable statements when searching for a next sibling — so a FunctionDeclaration immediately following a nested block containing setFailed would still appear as a continuation (false positive).

The new isExecutableStatement exclusions are applied at line 144 (switch fall-through) and line 108 (Program) but not here at line 105–106.

<details>
<summary>💡 Suggested fix</summary>

Change:…

eslint-factory/src/rules/require-return-after-core-setfailed.ts:344

[/tdd] checkStatementList on node.consequent at line 345 and then checkNestedContinuation on lastStmt at line 350 may both fire for the same setFailed call when it is both inside the statement list (not the last) AND the last — but more importantly they will double-report when setFailed is not the last in-list statement but is also the last executable statement.

For example: case 1: core.setFailed("bad"); doMore(); function helper() {} — checkStatementList reports on `d…

eslint-factory/src/rules/require-return-after-core-setfailed.ts:348

[/tdd] lastStmt is the raw last element of node.consequent, but after the FP fix, the semantically relevant "last executable statement" is the last item passing isExecutableStatement. If the case ends with function helper() {}, lastStmt is a FunctionDeclaration — isCoreSetFailedStatement returns false and the fall-through check is silently skipped, even if setFailed is the last executable statement before the hoisted declaration.

No test covers: `case 1: core.setFailed("b…

[github-actions]

Review: fix(eslint) — switch fall-through FN and hoisted-function FP

Both bugs are correctly addressed.

Hoisted-declaration FP — isExecutableStatement now excludes FunctionDeclaration, TSInterfaceDeclaration, and TSTypeAliasDeclaration. Applied in both findContinuationOutsideBlock (switch case search) and the Program visitor. Correct.

Switch fall-through FN — new SwitchCase visitor: checkStatementList covers same-case continuations; checkNestedContinuation handles fall-through. No double-report risk since checkNestedContinuation is called only when setFailed is the last consequent.

findContinuationOutsideBlock now uses .find(s => isExecutableStatement(...)) instead of [0], correctly skipping hoisted declarations in subsequent cases.

Test cases are well-chosen and cover all boundary conditions. ✅

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 33.5 AIC · ⌖ 4.41 AIC · ⊞ 4.9K

[github-actions]

Review: one medium maintainability concern, logic is sound

The two bug fixes are correctly targeted and the test cases cover the new paths well. One non-blocking issue to address: the as TSESTree.ProgramStatement cast on the new find() call.

Findings summary

Medium — type-unsafe cast in findContinuationOutsideBlock (commented inline): nextCase.consequent is Statement[] but isExecutableStatement expects ProgramStatement. The cast hides a real type mismatch; widening the parameter type or inlining the checks eliminates it cleanly.

All other grumpy-coder candidates were dropped: the double-report concern is not an issue (the two code paths are exclusive when setFailed is the last statement), and the FunctionDeclaration-in-switch semantic edge case is theoretical and TypeScript-only code runs in strict mode anyway.

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

Comments that could not be inline-anchored

eslint-factory/src/rules/require-return-after-core-setfailed.ts:138

Unsafe type cast may silently break isExecutableStatement for switch-case nodes: s as TSESTree.ProgramStatement forces switch-case consequent statements (typed as TSESTree.Statement[]) into TSESTree.ProgramStatement, suppressing a real type mismatch.

<details>
<summary>💡 Details and suggested fix</summary>

TSESTree.ProgramStatement is the union of top-level statement types allowed in Program.body. Switch-case consequents are Statement[], which is broader and includes nodes …

[pelikhan]

@copilot run pr-finisher skill

[Copilot]

@copilot run pr-finisher skill

All three merge-ready conditions are satisfied locally. No new changes were needed — the fix was already pushed in the previous run. Summary below.