fix(eslint-factory): suppress unsafe autofix for no-core-error-then-process-exit inside helper functions#46553
Conversation
… for non-main helper functions
The suggestion fix replacing `core.error(msg); process.exit(nonzero)` with
`core.setFailed(msg); return;` was unsafe inside helper functions: `return`
only exits the helper, not the process, silently converting a hard abort into
a fall-through that returns `undefined` to the caller.
Changes:
- Add `getImmediateEnclosingFunction` to find the innermost enclosing function
- Add `isFunctionNamedMain` to identify `main()` entrypoints
- Suppress the autofix suggestion when the pair is inside any function that is
NOT named `main` (i.e., report-only with `suggest: []` for helpers)
- Soften the diagnostic message to acknowledge standalone-node contexts
- Update test: `function run()` now expects no suggestions
- Add tests: value-returning helper → no autofix; `async function main()` and
`const main = async () => {}` entrypoints → autofix retained
Closes #46540
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ 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 (0 additions detected in src/, lib/, pkg/, internal/, app/, core/, domain/, services/, api/). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Prevents unsafe ESLint autofix suggestions inside helper functions while retaining fixes for top-level and main contexts.
Changes:
- Detects the nearest enclosing function and named
mainfunctions. - Suppresses unsafe suggestions in non-entrypoint helpers.
- Adds regression tests and clarifies diagnostics.
Show a summary per file
| File | Description |
|---|---|
eslint-factory/src/rules/no-core-error-then-process-exit.ts |
Adds context-aware suggestion filtering. |
eslint-factory/src/rules/no-core-error-then-process-exit.test.ts |
Tests safe and unsafe contexts. |
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
| function isFunctionNamedMain(fn: FunctionNode): boolean { | ||
| if (fn.type === AST_NODE_TYPES.FunctionDeclaration) { | ||
| return fn.id?.name === "main"; | ||
| } | ||
| // FunctionExpression or ArrowFunctionExpression assigned to a variable named `main` | ||
| const parent = fn.parent; | ||
| return ( | ||
| parent != null && | ||
| parent.type === AST_NODE_TYPES.VariableDeclarator && | ||
| parent.id.type === AST_NODE_TYPES.Identifier && | ||
| parent.id.name === "main" | ||
| ); | ||
| } |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is correct and well-structured. The ancestor walk to find the innermost enclosing function is efficient, isFunctionNamedMain correctly covers all three main declaration styles (including export const main = ... since the ArrowFunctionExpression's parent is still a VariableDeclarator), and the safeToFix flag is cleanly reused inside the fixer instead of duplicating the walk. Test coverage addresses the core acceptance criteria. No blocking issues.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 10.8 AIC · ⌖ 4.33 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (4 test cases)
Test Classification:
Summary✅ Excellent test quality. All 4 new test cases are design tests that verify the rule's core behavioral contract: distinguishing safe autofix contexts (main entrypoint) from unsafe contexts (helper functions). The tests cover critical edge cases:
No test inflation, no duplicates, no forbidden patterns. The rule implementation changes (73 lines) are well-exercised by the test additions (19 lines). Verdict✅ APPROVE. Test Quality Score: 100/100. Implementation tests: 0% (threshold: 30%). All new tests are high-confidence design tests. 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.
|
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 27.6 AIC · ⌖ 4.67 AIC · ⊞ 6.7K
Comment /matt to run again
| * Returns true when `fn` is a conventional module entrypoint named `main`: | ||
| * - `function main() {}` / `async function main() {}` | ||
| * - `const main = function() {}` / `const main = async () => {}` | ||
| */ |
There was a problem hiding this comment.
[/diagnosing-bugs] isFunctionNamedMain does not verify that main is at module top-level — a nested function named main inside another function will incorrectly receive the autofix, where return only exits the inner main, not the outer function.
💡 Reproducer & suggested fix
Reproducer (currently gets unsafe autofix):
function outer() {
async function main() {
core.error("x");
process.exit(1); // ← autofix replaces with `return;`, but outer() continues
}
main();
}Fix: after confirming the function is named main, also check that its own parent is a Program node (or a VariableDeclaration at Program level):
function isFunctionNamedMain(fn: FunctionNode): boolean {
if (fn.type === AST_NODE_TYPES.FunctionDeclaration) {
if (fn.id?.name !== "main") return false;
return fn.parent?.type === AST_NODE_TYPES.Program;
}
const parent = fn.parent;
if (
parent?.type === AST_NODE_TYPES.VariableDeclarator &&
parent.id.type === AST_NODE_TYPES.Identifier &&
parent.id.name === "main"
) {
// Walk up: VariableDeclarator → VariableDeclaration → must be Program
return parent.parent?.parent?.type === AST_NODE_TYPES.Program;
}
return false;
}Add a corresponding test case:
{
// nested main() is NOT a safe entrypoint — no autofix
code: `function outer() { async function main() { core.error("x"); process.exit(1); } }`,
errors: [{ messageId: "noCoreErrorThenProcessExit", suggestions: [] }],
},@copilot please address this.
There was a problem hiding this comment.
Fixed in dbd04a0. isFunctionNamedMain now checks fn.parent?.type === AST_NODE_TYPES.Program for FunctionDeclaration, and for variable-assigned forms walks VariableDeclarator → VariableDeclaration and confirms varDecl.parent?.type === AST_NODE_TYPES.Program. Both nested forms now produce suggestions: [].
| errors: [{ messageId: "noCoreErrorThenProcessExit", suggestions: [{ messageId: "replaceWithSetFailed", output: "core.setFailed(`ERROR: ${message}`);\n " }] }], | ||
| }, | ||
| { | ||
| // pair inside a non-main function: no autofix because `return` only exits the helper, |
There was a problem hiding this comment.
[/tdd] No test covers the module top-level case (pair outside any function, enclosingFn === null), which is the other path that enables the autofix.
💡 Suggested test
{
// module top-level: no enclosing function, autofix is safe
code: `core.error("fatal"); process.exit(1);`,
errors: [{ messageId: "noCoreErrorThenProcessExit", suggestions: [{ messageId: "replaceWithSetFailed", output: `core.setFailed("fatal");\n` }] }],
},Without this, a future regression that accidentally suppresses top-level autofixes would go undetected.
@copilot please address this.
There was a problem hiding this comment.
Added in 7149980: an explicit test case labeled // module top-level: no enclosing function (enclosingFn === null), autofix is safe is now the first invalid case in the suite, directly documenting the enclosingFn === null path. The isInsideFunction variable was also inlined with a clarifying comment.
There was a problem hiding this comment.
REQUEST_CHANGES — one critical scope-check bug blocks merge
The fix correctly identifies the core problem (unconditional autofix inside helper functions) but introduces an incomplete guard: isFunctionNamedMain checks the variable name but not the variable scope, so a locally-nested const main = async () => {} inside another function still receives an unsafe autofix suggestion.
Findings detail
Critical — isFunctionNamedMain is scope-unaware (inline comment on line 77 of the rule): any VariableDeclarator whose id is "main" anywhere in the AST passes the check — including one nested 3 levels deep inside another function. The same gap exists for function main() declarations, which have no Program-parent guard either. Fix: verify declaration.parent.type === AST_NODE_TYPES.Program before returning true.
Medium — No test for the nested-main false-positive. The bug is invisible to CI. Suggested test case added in the inline comment on line 76 of the test file.
Low — isInsideFunction inside the fix closure is now always enclosingFn !== null, which is always true when safeToFix is true and we are in a main function. The variable name no longer matches its meaning and will mislead future readers.
🔎 Code quality review by PR Code Quality Reviewer · 38.5 AIC · ⌖ 4.71 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
eslint-factory/src/rules/no-core-error-then-process-exit.ts:77
Nested const main passes the scope check regardless of nesting depth, producing an unsafe autofix: isFunctionNamedMain only verifies the variable is named main but never checks the VariableDeclarator is at module top-level. A locally-scoped const main = async () => { core.error("x"); process.exit(1); } inside another function will receive the autofix suggestion, yet return from that inner arrow does NOT exit the process.
<details>
<summary>💡 Suggested fix</summary>
After find…
eslint-factory/src/rules/no-core-error-then-process-exit.test.ts:76
No test for the nested-main false-positive: the test suite doesn't cover const main = async () => {} inside another function. Given the scope-check gap in isFunctionNamedMain, this means the regression where a nested main receives an unsafe autofix is completely undetected.
<details>
<summary>💡 Suggested test case to add</summary>
{
// nested const main inside a factory function — NOT an entrypoint; no autofix
code: `function createRunner() { const main = async () => { …
</details>
|
@copilot run pr-finisher skill |
…larations only Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot please run the Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in dbd04a0: |
…ixer logic comment Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All remaining feedback addressed (7149980):
Local validation: |
The
no-core-error-then-process-exitsuggestion fix unconditionally replacedcore.error(msg); process.exit(nonzero)withcore.setFailed(msg); return;— even inside helper functions wherereturnonly exits the helper, letting callers continue withundefinedinstead of aborting the process.Root cause
Changes
getImmediateEnclosingFunction— new helper to find the innermost enclosing function node via ancestor walkisFunctionNamedMain— identifiesmain()entrypoints (function main,async function main,const main = () => {})suggest: [](report-only) when the flagged pair is inside any function that is notmain; the fix is only offered at module top-level or directly insidemain()wherereturncorrectly terminates the entry pathprocess.exitalways "bypasses the lifecycle"; acknowledges that in standalonenodescriptsprocess.exit(nonzero)does fail the stepSafe vs. unsafe autofix contexts
async function main()/const main = async () => {}run,requireEnvVar, etc.)Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.