Skip to content

Migrate subprocess handling to execa #99

Description

@thecodedrift

Every subprocess in the CLI is spawned by hand with node:child_process, and the surrounding scaffolding — timeout, kill, stream decoding, error shaping — is re-implemented per call site. Two sites have grown full independent copies of it, and one bug has already been fixed by hand that a library would have made unrepresentable.

Raised in review on #71, on two separate threads:

Do we gain anything from switching to execa here? It's a little more feature complete, but I'd like to see if there are tradeoffs worth it WRT stdout/err capture

We should really prefer async alternatives to sync options when possible

These turn out to be the same change. execa is promise-based, so adopting it converts the sync path as a consequence rather than as a second effort.

What exists today

Ten subprocess call sites:

File Call
src/rules/scan.ts:145 spawn — ast-grep, --json=stream, read line-by-line via node:readline
src/rules/verify.ts:185 spawn — ast-grep test
src/rules/vale/run.ts:110 spawn — Vale, with hand-rolled timeout + SIGKILL + settle guard
src/rules/runtime/invoke.ts:95 spawn — runtime harness, second independent copy of the same scaffolding
src/rules/runtime/invoke.ts:108 spawn — Windows taskkill /pid /T /F tree-kill branch
src/rules/runtime/narrow.ts:43 spawn — ast-grep narrowing
src/rules/platform-binary.ts:139 spawnSync--version identity probe, in a loop over candidates
src/util/git-remote.ts:14,108 execFile — git
src/auth/token.ts:131 execFilegit ls-files

Why this is worth a dependency

The scaffolding is duplicated, and the duplication is already load-bearing. vale/run.ts and runtime/invoke.ts each independently implement timeout → kill → settle-once. invoke.ts additionally carries a Windows tree-kill branch that vale/run.ts does not, so the two disagree about what "terminate this child" means depending on which engine you are in. execa's timeout and forceKillAfterDelay cover both, cross-platform, once.

A decoding bug was already fixed by hand here. vale/run.ts accumulated stdout with chunk.toString() per Buffer, which corrupts a multi-byte UTF-8 sequence split across a chunk boundary — reachable, since Vale lints prose with curly quotes and em dashes. Worst case the corruption lands inside JSON string escaping and a clean run is reported as Vale produced output that is not JSON. It was fixed with an explicit StringDecoder per stream. Correct stream decoding is table stakes for a subprocess library; hand-rolling it is how that class of bug recurs at the next call site.

Errors are shaped per site. Each spawn invents its own failure message. execa's errors carry the command, exit code, signal, and captured stderr by construction.

It resolves the sync question without a separate migration. platform-binary.ts probes candidates with spawnSync in a loop — up to six spawns, each with a 5s timeout, blocking the event loop. Going async there ripples findSgBinary() through scan.ts, verify.ts, and runtime/narrow.ts, one of which sits inside a new Promise executor. That ripple is the same work as adopting execa, so doing them together is strictly cheaper than doing them in sequence.

Costs, stated plainly

  • A runtime dependency, not a dev one. The lib build bundles everything except node builtins, so execa and its transitive deps land in the shipped dist/index.js (currently ~490 kB). Worth measuring the delta before committing.
  • Eight call sites change, plus the findSgBinary() signature ripple through three callers and its memoization (a cached value becomes a cached promise).
  • Streaming needs care at one site. scan.ts consumes ast-grep's --json=stream incrementally through readline. execa supports line iteration, but this is the one site where the current approach is already correct and idiomatic, so it should be converted deliberately rather than mechanically.

Suggested scope

Its own PR, not folded into the Vale stack. Deliberately kept separate so the "does this earn a runtime dependency" question gets answered on its own terms rather than inside a 2000-line diff.

  • Measure the bundle-size delta first; abandon or reconsider if it is disproportionate
  • Convert vale/run.ts and runtime/invoke.ts (highest value — deletes both copies of the timeout/kill/settle scaffolding and the Windows tree-kill divergence)
  • Convert platform-binary.ts off spawnSync, and make findSgBinary() async through its three callers
  • Convert scan.ts, verify.ts, narrow.ts
  • Leave git-remote.ts / token.ts for last; they are simple execFile calls with the least to gain
  • Confirm the StringDecoder fix in vale/run.ts is genuinely subsumed rather than merely deleted

Refs #71

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions