Skip to content

prefer-actions-exec-over-child-process: flag hidden async-conversion cost in sync contexts - #56985

Merged
pelikhan merged 3 commits into
mainfrom
copilot/prefer-actions-exec-over-child-process-fix
Aug 29, 2026
Merged

prefer-actions-exec-over-child-process: flag hidden async-conversion cost in sync contexts#56985
pelikhan merged 3 commits into
mainfrom
copilot/prefer-actions-exec-over-child-process-fix

Conversation

Copilot AI commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

prefer-actions-exec-over-child-process flagged every child_process output-capturing call uniformly, implying a drop-in swap to @actions/exec. But @actions/exec is Promise-only, so when the flagged call lives in a non-async function whose result is consumed (not just a side effect), the real fix requires converting that function — and every caller up the chain — to async/await. The message gave no signal of this larger blast radius.

  • Detect the sync-context case: added findEnclosingFunction() to locate the nearest enclosing function and requiresAsyncConversion(), which flags the call when that function is non-async and its result is consumed non-trivially (assigned, returned, or otherwise used), reusing the existing retainsCallResult() logic.
  • New messageId: preferActionsExecSyncContext extends the existing message with a note that migration requires converting the enclosing (non-async) function and its callers to async/await. Calls in already-async functions, or bare side-effect statements, keep the original preferActionsExec message.
  • Tests: added cases covering an already-async enclosing function (unchanged), a non-async function returning the call result, a non-async function assigning-then-consuming the result, an arrow function variant, and a bare side-effect statement (unchanged).

Example:

/// <reference types="@actions/github-script" />
const { execSync } = require("child_process");

// Before & after: same messageId (async already available)
async function f() { return execSync("git status"); }

// New: preferActionsExecSyncContext — migrating requires converting f (and its callers) to async
function f() { return execSync("git status"); }

Confirmed the new variant now fires on the real-world case cited in the issue, actions/setup/js/get_current_branch.cjs:21.

Copilot AI and others added 2 commits August 29, 2026 21:58
…ocess rule

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix prefer-actions-exec-over-child-process to handle async conversion prefer-actions-exec-over-child-process: flag hidden async-conversion cost in sync contexts Aug 29, 2026
Copilot AI requested a review from pelikhan August 29, 2026 22:00
@pelikhan
pelikhan marked this pull request as ready for review August 29, 2026 22:14
Copilot AI balanced review requested due to automatic review settings August 29, 2026 22:14
@pelikhan
pelikhan merged commit a6d4169 into main Aug 29, 2026
3 checks passed
@pelikhan
pelikhan deleted the copilot/prefer-actions-exec-over-child-process-fix branch August 29, 2026 22:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Result-consumption detection is incomplete and promisified calls can receive an incorrect migration warning.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds async-migration context to the child-process ESLint rule.

Changes:

  • Detects retained results in synchronous functions.
  • Adds a specialized diagnostic and regression tests.
File summaries
File Description
prefer-actions-exec-over-child-process.ts Adds context detection and diagnostic selection.
prefer-actions-exec-over-child-process.test.ts Tests synchronous and asynchronous contexts.
Review details

Suppressed comments (1)

eslint-factory/src/rules/prefer-actions-exec-over-child-process.ts:286

  • Exclude promisified bindings from this message variant. A non-async function can already return promisify(exec)(...) as a Promise; replacing that return with exec.getExecOutput(...) preserves its asynchronous contract and does not require converting the function or callers, but the current condition reports that it does.
          messageId: requiresAsyncConversion(node) ? "preferActionsExecSyncContext" : "preferActionsExec",
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Balanced

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

*/
function requiresAsyncConversion(node: TSESTree.CallExpression): boolean {
const enclosingFunction = findEnclosingFunction(node);
return enclosingFunction !== null && !enclosingFunction.async && retainsCallResult(node);
preferActionsExec:
"Prefer @actions/exec's exec()/getExecOutput() over child_process.{{method}}() to spawn processes in actions/github-script scripts. child_process.{{method}}() duplicates functionality already provided by the @actions/exec toolkit available in this context.",
preferActionsExecSyncContext:
"Prefer @actions/exec's exec()/getExecOutput() over child_process.{{method}}() to spawn processes in actions/github-script scripts. child_process.{{method}}() duplicates functionality already provided by the @actions/exec toolkit available in this context. @actions/exec's API is Promise-only, so migrating this call requires converting the enclosing (currently non-async) function — and every one of its callers up the chain — to async/await.",
@github-actions

Copy link
Copy Markdown
Contributor

🎉 This pull request is included in a new release.

Release: v0.87.10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

prefer-actions-exec-over-child-process: flags exec calls inside sync functions without noting the required async-conversion casc

3 participants