eslint-factory: detect aliased/destructured/computed core.setFailed in require-return-after-core-setfailed#45334
Conversation
…iled in require-return-after-core-setfailed
- Detect core["setFailed"](...) (computed member with string literal)
- Detect aliased object: const c = core; c.setFailed(...)
- Detect destructured binding: const { setFailed } = core; setFailed(...)
- Detect renamed destructure: const { setFailed: sf } = core; sf(...)
- Exclude locally-shadowed setFailed bindings (function/parameter/non-core const)
- Add 7 new test cases covering all new forms and shadow exclusion
Co-authored-by: pelikhan <4175913+pelikhan@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 #45334 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Extends the ESLint rule to detect computed, aliased, and destructured core.setFailed calls.
Changes:
- Adds scope-aware alias and destructuring detection.
- Adds tests for primary detection forms and suggestions.
- Preserves existing cross-block continuation checks.
Show a summary per file
| File | Description |
|---|---|
require-return-after-core-setfailed.ts |
Implements expanded detection. |
require-return-after-core-setfailed.test.ts |
Tests new call forms. |
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
| }); | ||
| }); | ||
|
|
||
| it("valid: locally-shadowed setFailed bindings are not flagged", () => { |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 87/100 — Excellent
📊 Metrics (7 tests)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The implementation is correct and well-tested. The scope-chain walk pattern for alias/destructure detection is sound, the re-assignment guard properly rejects mutable bindings, and shadow exclusion via def.type !== "Variable" correctly handles local function and parameter shadows. Test coverage spans all three new detection forms plus negative/shadow cases. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 13 AIC · ⌖ 5.13 AIC · ⊞ 4.8K
There was a problem hiding this comment.
Skills-Based Review
Applied /tdd and /codebase-design — leaving as COMMENT with three actionable suggestions.
Key Themes
- Code duplication:
isCoreAliasIdentifierandisDestructuredSetFailedIdentifierrepeat identical scope-chain traversal boilerplate. A sharedresolveVariableDefhelper would remove ~20 lines of duplication (see inline comment). - Missing regression tests: the re-assignment guard (
isWrite() && !ref.init) and thecoreObjalias inCORE_ALIASESare untested (see inline comments).
Positive Highlights
- Scope-aware detection correctly mirrors the
require-await-core-summary-writepattern. - Conservative false-positive guards (def-type check, re-assignment rejection, origin-identity guard).
- Comprehensive happy-path and shadowing tests covering all three new forms.
checkStatementListparametrization is a clean seam improvement.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 30.8 AIC · ⌖ 4.67 AIC · ⊞ 6.6K
Comment /matt to run again
| /** | ||
| * Checks whether an Identifier is the destructured `setFailed` binding from | ||
| * a core-like object (e.g., `const { setFailed } = core` or | ||
| * `const { setFailed: sf } = core` where `sf` is the identifier). |
There was a problem hiding this comment.
[/codebase-design] isCoreAliasIdentifier and isDestructuredSetFailedIdentifier repeat the same scope-chain walk (lines ~179–193 vs ~210–224). Extracting a shared helper would make each function shallower and easier to test in isolation.
💡 Suggested refactor
function resolveVariableDef(identifier: TSESTree.Identifier): TSESTree.VariableDeclarator | null {
let scope: TSESLint.Scope.Scope | null = sourceCode.getScope(identifier);
while (scope !== null) {
const variable = scope.set.get(identifier.name);
if (variable !== undefined) {
if (variable.defs.length !== 1) return null;
const def = variable.defs[0];
if (def.type !== "Variable") return null;
if (variable.references.some(ref => ref.isWrite() && !ref.init)) return null;
const declarator = def.node as TSESTree.VariableDeclarator;
return declarator.init ? declarator : null;
}
scope = scope.upper;
}
return null;
}Both helpers then call resolveVariableDef and only differ in what they assert about the resulting declarator.
@copilot please address this.
| code: `const c = core; function f() { c["setFailed"]("bad"); doMore(); keepGoing(); }`, | ||
| errors: [{ messageId: "missingReturnAfterSetFailed", suggestions: [{ messageId: "addReturn", output: `const c = core; function f() { c["setFailed"]("bad"); return; doMore(); keepGoing(); }` }] }], | ||
| }, | ||
| ], |
There was a problem hiding this comment.
[/tdd] The re-assigned binding guard (variable.references.some(ref => ref.isWrite() && !ref.init)) is untested. A test with let c = core; c = other; would confirm the rule does NOT flag it.
💡 Suggested test
it("valid: re-assigned alias is not flagged", () => {
ruleTester.run("require-return-after-core-setfailed", requireReturnAfterCoreSetFailedRule, {
valid: [
// c is re-assigned after initialisation — not a reliable core alias
`let c = core; c = other; function f() { c.setFailed("bad"); doMore(); }`,
],
invalid: [],
});
});Without this test, a future refactor that accidentally drops the re-assignment guard would pass all existing tests.
@copilot please address this.
| return obj.type === AST_NODE_TYPES.Identifier && obj.name === "core" && prop.type === AST_NODE_TYPES.Identifier && prop.name === "setFailed"; | ||
| // Known @actions/core binding names (same allow-list as require-await-core-summary-write). | ||
| // Only exact known aliases are matched — broad prefix matching (e.g. `/^core/i`) would | ||
| // silently flag unrelated objects that happen to start with "core". |
There was a problem hiding this comment.
[/tdd] coreObj is in CORE_ALIASES but has no test exercising it. If it is a live alias used in the codebase, add a test; if it is aspirational, remove it to avoid false coverage confidence.
@copilot please address this.
There was a problem hiding this comment.
Two correctness gaps and two maintainability issues to fix before merge
Blocking:
isDestructuredSetFailedIdentifiernever callsisCoreAliasIdentifieron its init —const c = core; const { setFailed } = c; setFailed("bad"); doMore();is a silent false negative that contradicts the stated goal of this PR.isCoreAliasIdentifieris single-depth only —const d = c; d.setFailed(...)is also silently missed.
Non-blocking but should be addressed:
3. CORE_ALIASES is a mutable copy-pasted Set — should be a shared frozen constant with require-await-core-summary-write to prevent silent divergence.
4. coreObj patterns are completely untested — if the entry were removed from CORE_ALIASES, no test would fail.
Passing findings
The core pattern (computed core["setFailed"], scope-walk for aliases, ObjectPattern destructuring, shadow exclusion) is sound. The scope-chain iteration with re-assignment rejection is correct. The checkStatementList predicate injection is a clean refactor.
🔎 Code quality review by PR Code Quality Reviewer · 52.8 AIC · ⌖ 4.9 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
eslint-factory/src/rules/require-return-after-core-setfailed.ts:190
False negative: isDestructuredSetFailedIdentifier never resolves aliased inits. const c = core; const { setFailed } = c; setFailed('bad'); doMore(); will not be flagged — the init is c, which passes isCoreLikeIdentifier only for literal "core"/"coreObj", not for aliases.
<details>
<summary>💡 Suggested fix</summary>
// In isDestructuredSetFailedIdentifier, replace:
if (declarator.id.type === AST_NODE_TYPES.ObjectPattern &&
declarator.init.type === AST_NODE_TY…
</details>
<details><summary>eslint-factory/src/rules/require-return-after-core-setfailed.ts:165</summary>
**Single-depth alias only: chained aliases are silently missed.** `const c = core; const d = c; d.setFailed('bad'); doMore();` will not be flagged because `isCoreAliasIdentifier` checks `isCoreLikeIdentifier(declarator.init.name)` but `c` is not in `CORE_ALIASES`.
<details>
<summary>💡 Suggested fix</summary>
```typescript
function isCoreAliasIdentifier(identifier: TSESTree.Identifier): boolean {
// ... (existing scope walk) ...
const initName = declarator.init.name;
// Recurse: allow c…
</details>
<details><summary>eslint-factory/src/rules/require-return-after-core-setfailed.ts:124</summary>
**`CORE_ALIASES` is a mutable module-level `Set` — and is copy-pasted rather than shared.** `const` only prevents reassignment; `.add()`/`.delete()` still work. Any test or external code that mutates it affects all subsequent evaluations in the same process. Separately, the comment says 'same allow-list as `require-await-core-summary-write`' but the `Set` is copied, not imported — the two rules will silently diverge as soon as one is updated.
<details>
<summary>💡 Suggested fix</summary>
Extr…
</details>
<details><summary>eslint-factory/src/rules/require-return-after-core-setfailed.test.ts:29</summary>
**Zero test coverage for `coreObj` — the second alias in `CORE_ALIASES` is dead from a test perspective.** Every added test uses `core`; none exercises `coreObj.setFailed(...)`, `coreObj['setFailed'](...)`, aliased `coreObj`, or destructured `coreObj`. If the `coreObj` entry were silently removed from `CORE_ALIASES`, no test would fail.
<details>
<summary>💡 Suggested fix</summary>
Add at minimum one valid and one invalid test case using `coreObj`:
```typescript
it('invalid: coreObj.setFaile…
</details>
pelikhan
deleted the
copilot/eslint-factory-require-return-after-core-setfailed
branch
require-return-after-core-setfailedonly matched the literalcore.setFailed(...)form, silently missing three escape patterns where execution continues after failure is declared.New detection forms
core["setFailed"](msg)— string-literal property checkconst c = core; c.setFailed(msg)— scope-walks to find single-assignmentconst/letaliases of core-like identifiers; rejects re-assigned bindingsconst { setFailed } = core; setFailed(msg)— ObjectPattern analysis; also handles renamed destructure (const { setFailed: sf } = core; sf(msg))All three forms participate in the same cross-block continuation and suggestion (
addReturn) logic as the direct form.Locally-shadowed
setFailedbindings (function setFailed() {}, non-coreconst setFailed = ...) are excluded viadef.type !== "Variable"and origin-identity guards.Implementation notes
isCoreSetFailedStatementis moved intocreate(context)as a scope-aware closure (same pattern asrequire-await-core-summary-write).CORE_ALIASES/isCoreLikeIdentifierare shared at the top level for consistency.checkStatementListnow accepts the check function as a parameter.