eslint-factory: resolve promisify()-wrapped child_process bindings in prefer-actions-exec-over-child-process - #55687
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #55687 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
Non-blocking: the resolver change is narrowly scoped and the added tests cover the new promisify paths plus the main false-positive guards.
Highlights
- I checked the new recursive binding resolution and the visited-set guard; it avoids the obvious self-reference loop and preserves the existing handle-retention exemption for non-promisified
exec/execFile. - The test matrix covers CommonJS and ESM, direct/member-based
promisifyusage, and important negatives likespawn, unrelatedexec, and files without the github-script marker. - I did not find a changed-line correctness, performance, or maintainability issue strong enough to block merge.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 4.7 AIC · ⌖ 7.89 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
A couple of simplifications here would trim the patch without changing behavior. net: -20 lines possible.
Generated by ✂️ Ponytail Reviewer for #55687 · codex · mai10 · 7.5 AIC · ⌖ 1.92 AIC · ⊞ 16.7K
Comment /ponytail to run again
| * Resolves whether `identifierName` is bound (directly or via destructuring/require/`promisify()`) | ||
| * to one of `OUTPUT_CAPTURING_METHODS` from the `child_process` module. | ||
| */ | ||
| function resolveChildProcessOutputMethodBinding(identifierName: string, scopeNode: TSESTree.Node, sourceCode: TSESLint.SourceCode, visited: Set<string> = new Set()): ResolvedChildProcessMethod | null { |
There was a problem hiding this comment.
eslint-factory/src/rules/prefer-actions-exec-over-child-process.ts:108: yagni: recursive binding resolution plus a ResolvedChildProcessMethod wrapper for one promisify() branch. A small direct lookup for promisify(exec) would keep the rule simpler.
| }); | ||
| }); | ||
|
|
||
| it("flags promisify()-wrapped child_process bindings", () => { |
There was a problem hiding this comment.
eslint-factory/src/rules/prefer-actions-exec-over-child-process.test.ts:142: shrink: the new promisify() test block is a 10-case matrix for one resolver branch. Two focused CJS/ESM examples would cover the behavior without repeating almost identical fixtures.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — two targeted issues worth addressing before merge.
📋 Key Themes & Highlights
Key Themes
- False-positive risk:
isPromisifyCallaccepts any function namedpromisifyregardless of source module, unlike the pattern used forchild_processitself. This could flag non-utilwrappers. - ESM test coverage gap: ESM invalid cases only test
exec;execFilevia promisify has no ESM coverage.
Positive Highlights
- ✅ Clean
ResolvedChildProcessMethodtype narrowing — threadingpromisified: booleanthrough the resolution chain is elegant and avoids a second pass. - ✅
visitedset for self-referential binding guard is exactly the right fix for the cycle risk. - ✅ Extracting
resolveChildProcessMemberMethod()removes a meaningful duplication and the refactor is behaviour-preserving. - ✅ Good breadth of test cases (CJS + ESM, identifier + member-expression argument shapes, negatives for
spawn, unrelated modules,fs).
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 35.6 AIC · ⌖ 10.1 AIC · ⊞ 7.6K
Comment /matt to run again
| if (!node || node.type !== AST_NODE_TYPES.CallExpression || node.arguments.length !== 1) return false; | ||
| const callee = node.callee; | ||
| if (callee.type === AST_NODE_TYPES.Identifier) return callee.name === "promisify"; | ||
| return callee.type === AST_NODE_TYPES.MemberExpression && !callee.computed && callee.property.type === AST_NODE_TYPES.Identifier && callee.property.name === "promisify"; |
There was a problem hiding this comment.
[/diagnosing-bugs] isPromisifyCall matches any identifier named "promisify" or any *.promisify(...) call without verifying the source is util — a user-defined promisify wrapping a child_process method would be flagged incorrectly.
💡 Details and suggested test
Example that would be a false positive today:
// custom promisify, nothing to do with util
function promisify(fn) { return fn; }
const execAsync = promisify(exec);
execAsync("git status"); // ← incorrectly flaggedEither add a valid case covering a non-util promisify so the broad scope is intentional and documented, or narrow the check to also verify that the callee binding comes from "util" (similar to how isChildProcessModuleBinding tracks the child_process source). A short comment in isPromisifyCall would also clarify the deliberate choice.
@copilot please address this.
| valid: [{ code: ghScript(`import { promisify } from "util"; import { spawn } from "child_process"; const spawnAsync = promisify(spawn); spawnAsync("node", ["server.js"]);`) }], | ||
| invalid: [ | ||
| { | ||
| code: ghScript(`import { promisify } from "util"; import { exec } from "child_process"; const execAsync = promisify(exec); async function f() { await execAsync("git status"); }`), |
There was a problem hiding this comment.
[/tdd] The ESM invalid cases cover only exec — there's no ESM test for execFile via util.promisify(cp.execFile) or the member-expression form import * as cp from "child_process"; util.promisify(cp.execFile). The CJS tests cover these shapes, but a parity gap in ESM leaves the import-binding path untested for execFile.
💡 Suggested addition
{
code: ghScript(`import { promisify } from "util"; import { execFile } from "child_process"; const execFileAsync = promisify(execFile); async function f() { await execFileAsync("git", ["status"]); }`),
errors: [{ messageId: "preferActionsExec", data: { method: "execFile" } }],
},This ensures the ESM import-binding path through resolveChildProcessOutputMethodBinding is exercised for all flagged methods, not just exec.
@copilot please address this.
There was a problem hiding this comment.
Pull request overview
Extends the ESLint rule to detect promisified child_process output methods.
Changes:
- Adds promisified binding resolution and cycle protection.
- Updates CJS/ESM tests and documentation.
Show a summary per file
| File | Description |
|---|---|
prefer-actions-exec-over-child-process.ts |
Adds promisified method resolution. |
prefer-actions-exec-over-child-process.test.ts |
Adds positive and negative cases. |
eslint-factory/README.md |
Documents the new behavior. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (1)
eslint-factory/src/rules/prefer-actions-exec-over-child-process.ts:210
- Promisified
execandexecFilecalls can still retain the process handle: Node attaches theChildProcessas.childon the returned Promise. Consequently,const pending = execAsync("cmd"); pending.child.kill()is now reported even though it has the same lifecycle-management need as the existing exemption and@actions/execcannot replace it. Preserve the exemption for uses that retain/access the promisified call's.childwhile still reporting awaited output captures.
// A promisified binding resolves to captured output, never to a ChildProcess handle.
if (!resolved.promisified && HANDLE_RETURNING_METHODS.has(resolved.method) && retainsCallResult(node)) return;
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
| if (callee.type === AST_NODE_TYPES.Identifier) return callee.name === "promisify"; | ||
| return callee.type === AST_NODE_TYPES.MemberExpression && !callee.computed && callee.property.type === AST_NODE_TYPES.Identifier && callee.property.name === "promisify"; |
There was a problem hiding this comment.
The implementation is correct and well-tested. The visited set prevents recursion, resolveChildProcessMemberMethod properly isolates member-resolution logic, and the promisified flag correctly suppresses the handle-retention exemption for promisified bindings.
Minor observation: isPromisifyCall identifies promisify calls by callee name alone without verifying the binding comes from util. This is safe in practice because the inner resolution still requires the wrapped argument to resolve to an actual child_process binding — so a false positive would require both a user-defined promisify function AND a child_process method in scope being passed to it.
Tests cover all key paths: self-referential guard, ESM imports, inline require(...).exec, util.promisify() member call form, and spawn exclusions. No blocking issues.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 34.3 AIC · ⌖ 9.07 AIC · ⊞ 6.2K
|
🎉 This pull request is included in a new release. Release: |
prefer-actions-exec-over-child-processresolvedchild_processmethods only through direct destructuring, imports, and member access, so a binding created bypromisify()was never recognized and the call silently passed — despite being the exact output-capturing anti-pattern the rule exists to catch (actions/setup/js/validate_secrets.cjs:252).Changes
resolveChildProcessOutputMethodBinding()now tracesconst x = promisify(<method ref>), handling both an identifier argument (recursively resolved, e.g. destructured/importedexec) and a member-expression argument (promisify(require("child_process").exec),util.promisify(cp.execFile)). A visited set guards against self-referential declarations.{ method, promisified }; the exemption for retainedChildProcesshandles is skipped whenpromisifiedis true, sincepromisify(exec)resolves to captured output rather than a handle. Without this,const { stdout } = await execAsync(...)would still be exempted as a "retained handle".resolveChildProcessMemberMethod(), reused by the call-site path and both binding paths (no behavior change to existing forms).promisifydetection — matches a barepromisify(...)call or any<obj>.promisify(...)member call with exactly one argument. Scope is unchanged otherwise:spawn/spawnSync, non-child_processsources, and files without the@actions/github-scriptmarker remain unflagged.spawn,execfrom an unrelated module,fs.readFile, no marker, self-reference); rule description andeslint-factory/README.mdupdated.validate_secrets.cjsis intentionally left unchanged — the issue asks for the call to be flagged, not fixed. It currently surfaces as one new warning.