refactor: migrate all subprocess call sites to the shared subprocess module by iankhou · Pull Request #1758 · aws/aws-cdk-cli

iankhou · 2026-07-23T20:29:39Z

Summary

Follow-up to #1763, which adds the @aws-cdk/private-tools/lib/subprocess module.

This PR migrates every child_process call site in the shipped packages onto the shared module, so that no code path both escapes and executes:

Call site	Before	After
`init/os.ts`, `init/init.ts`	`spawn(argv, {shell: true})` + broken hand-rolled escaping	`run()` — no shell, `inherit-stderr` keeps npm/pip progress bars
`telemetry/library-version.ts`	`exec()` string	`run()` argv
toolkit-lib `ec2-detection.ts`	`execSync` with `2>nul`	`runSync()`, stderr discarded via stdio
toolkit-lib `cloud-assembly/exec.ts` (synth `app`)	`spawn(str, {shell: true})` + split2	`runUserCommandLine()`, line-buffered, `collect: false`
`commands/docs.ts` (`--browser`)	`exec()`	`runUserCommandLine()`
cdk-assets-lib `shell.ts`	own spawn + broken escaping copy	delegates to `run()`; spawn failures rethrow the OS error so docker's ENOENT → 'please install docker' guidance is preserved
toolkit-lib `environment.ts`	`quoteSpaces()` (space-only quoting)	allowlist-based `quoteShellPart()`

The deleted windowsEscape helpers carried an escaping bug (shellMeta.has(x) instead of has(c)) that made Windows escaping a silent no-op — this PR removes the pattern rather than fixing it. grep -rn runUserCommandLine now returns exactly the two sinks where a shell is the documented contract (synth app, --browser) — the complete trust-model surface.

Drops split2 from toolkit-lib (replaced by the module's line buffering).

Not in scope (follow-ups): cdk-build-tools/integ-runner/cli-integ migration, the eslint no-restricted-imports ban on child_process, and a Windows CI leg for the module.

Checklist

Unit tests added/updated
Manually verified: init (git/npm/metachar paths), synth (shell features, exit codes, 200k-line streaming, UTF-8), docs --browser, cdk-assets shell paths (hostile args, docker ENOENT, EPIPE)
No manual edits to generated files

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

github-actions · 2026-07-23T20:31:39Z

Dependency Review

The following issues were found:

✅ 0 vulnerable package(s)
✅ 0 package(s) with incompatible licenses
✅ 0 package(s) with invalid SPDX license definitions
⚠️ 2 package(s) with unknown licenses.

See the Details below.

License Issues

packages/@aws-cdk/cdk-assets-lib/package.json

Package	Version	License	Issue Type
cross-spawn	^7.0.6	Null	Unknown License

packages/@aws-cdk/toolkit-lib/package.json

Package	Version	License	Issue Type
cross-spawn	^7.0.6	Null	Unknown License

OpenSSF Scorecard

Package	Version	Score	Details
npm/cross-spawn	^7.0.6	Unknown	Unknown
npm/cross-spawn	^7.0.6	Unknown	Unknown

Scanned Files

packages/@aws-cdk/cdk-assets-lib/package.json
packages/@aws-cdk/toolkit-lib/package.json
yarn.lock

codecov-commenter · 2026-07-24T17:16:25Z

Codecov Report

❌ Patch coverage is 87.50000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.99%. Comparing base (53368d1) to head (b6514bf).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
packages/aws-cdk/lib/commands/init/os.ts	72.22%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1758      +/-   ##
==========================================
- Coverage   89.99%   89.99%   -0.01%     
==========================================
  Files          78       78              
  Lines       12026    11951      -75     
  Branches     1692     1679      -13     
==========================================
- Hits        10823    10755      -68     
+ Misses       1174     1166       -8     
- Partials       29       30       +1

Flag	Coverage Δ
suite.unit	`89.99% <87.50%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot

Pull request overview

Copilot reviewed 32 out of 33 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

packages/@aws-cdk/cdk-assets-lib/lib/private/shell.ts:55

When the subprocess exits non-zero but produces no stderr, the ProcessFailed message ends up with a trailing ":" (because it unconditionally appends : ${e.stderr.trim()}).

      throw new ProcessFailed(
        e.exitCode,
        e.signal,
        `${e.message}: ${e.stderr.trim()}`,
      );

github-actions · 2026-07-24T21:16:35Z

Total lines changed 1086 is greater than 1000. Please consider breaking this PR down.

…module

Copilot

Pull request overview

Copilot reviewed 27 out of 28 changed files in this pull request and generated no new comments.

iankhou · 2026-07-28T17:19:55Z

Total lines changed 1086 is greater than 1000. Please consider breaking this PR down.

New change is smaller.

ShadowCat567 · 2026-07-28T20:08:29Z

The deleted windowsEscape helpers carried an escaping bug (shellMeta.has(x) instead of has(c)) that made Windows escaping a silent no-op — this PR removes the pattern rather than fixing it.

Does this mean we still can't run tests on windows? Are customers affected by this at all or is this just testing infra?

ShadowCat567 · 2026-07-28T20:16:28Z

-  shellEventType: ShellEventType,
-): void {
+function handleShellOutput(props: HandleShellOutputProps): void {
+  const { chunk, options, shellEventType } = props;


if we're just going to expand the options back out again, what's the point of making HandleShellOutputProps?

Good point, will remove the property bag here

ShadowCat567 · 2026-07-28T20:18:46Z

- *
- * Wrapping in single quotes and escaping single quotes inside will do it for us.
- */
-function posixEscape(x: string) {


You made note of removing the windowsEscape, is posixEscape no longer needed because of the refactor?

It is also unneeded, since we use renderForDisplay in this change to subsume it. renderForDisplay uses similar logic, except it uses an allowlist for characters instead of a denylist (previous logic), which is safer.

Both windowsEscape and posixEscape are made obsolete from this PR, but windowsEscape was also buggy, which is why I mentioned it. The "logic" for both is still used in renderForDisplay.

iankhou · 2026-07-28T23:23:20Z

The deleted windowsEscape helpers carried an escaping bug (shellMeta.has(x) instead of has(c)) that made Windows escaping a silent no-op — this PR removes the pattern rather than fixing it.

Does this mean we still can't run tests on windows? Are customers affected by this at all or is this just testing infra?

It's not really related to being able to test on Windows. The original escaping bug allowed special characters like ,, |, ^, when it should not have, since it passed whole strings instead of validating individual characters. The approach that I used removes the entire helper, removing the bug as well.

We still don't have CI in Windows in this PR, but I can add it as a follow-up. I'm not certain why we don't already have it.

iankhou · 2026-07-28T23:38:58Z

The deleted windowsEscape helpers carried an escaping bug (shellMeta.has(x) instead of has(c)) that made Windows escaping a silent no-op — this PR removes the pattern rather than fixing it.

Does this mean we still can't run tests on windows? Are customers affected by this at all or is this just testing infra?

In terms of customer impact:

We fix a bug in cdk init on Windows that would have misparsed input containing special characters like &, and |, which weren't in the deny list
We put in quotes everything that doesn't fit the allowlist in DISPLAY_SAFE, so more stuff will be quoted, but it's just stuff displayed to the user

iankhou had a problem deploying to automation July 23, 2026 20:29 — with GitHub Actions Failure

iankhou temporarily deployed to automation July 23, 2026 20:29 — with GitHub Actions Inactive

github-actions Bot added the p2 label Jul 23, 2026

aws-cdk-automation requested a review from a team July 23, 2026 20:31

iankhou had a problem deploying to no-approval July 23, 2026 20:31 — with GitHub Actions Failure

iankhou had a problem deploying to no-approval July 23, 2026 20:34 — with GitHub Actions Failure

iankhou had a problem deploying to no-approval July 23, 2026 20:48 — with GitHub Actions Failure

iankhou commented Jul 23, 2026

View reviewed changes

Comment thread packages/@aws-cdk/toolkit-lib/lib/api/aws-auth/ec2-detection.ts

iankhou had a problem deploying to no-approval July 23, 2026 20:57 — with GitHub Actions Failure

iankhou temporarily deployed to no-approval July 23, 2026 21:08 — with GitHub Actions Inactive

iankhou requested a review from Copilot July 23, 2026 21:09

Copilot started reviewing on behalf of iankhou July 23, 2026 21:10 View session

iankhou temporarily deployed to run-tests July 23, 2026 21:11 — with GitHub Actions Inactive

iankhou temporarily deployed to run-tests July 24, 2026 17:03 — with GitHub Actions Inactive

Copilot AI reviewed Jul 24, 2026

View reviewed changes

Comment thread packages/@aws-cdk/toolkit-lib/lib/api/cloud-assembly/private/exec.ts Outdated

iankhou mentioned this pull request Jul 24, 2026

chore: add a shared subprocess execution module to private-tools #1763

Merged

3 tasks

iankhou added 2 commits July 28, 2026 11:15

refactor: migrate all subprocess call sites to the shared subprocess …

d973217

…module

Merge branch 'main' into iankhou-shell-interactions

13539f0

Copilot AI reviewed Jul 28, 2026

View reviewed changes

ShadowCat567 reviewed Jul 28, 2026

View reviewed changes

revert handleShellOutput to positional parameters

b6514bf

Uh oh!

Conversation

iankhou commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

github-actions Bot commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dependency Review

License Issues

packages/@aws-cdk/cdk-assets-lib/package.json

packages/@aws-cdk/toolkit-lib/package.json

OpenSSF Scorecard

Scanned Files

Uh oh!

Uh oh!

codecov-commenter commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

github-actions Bot commented Jul 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

iankhou commented Jul 28, 2026

Uh oh!

ShadowCat567 commented Jul 28, 2026

Uh oh!

ShadowCat567 Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

iankhou Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

ShadowCat567 Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

iankhou Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

iankhou commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iankhou commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

iankhou commented Jul 23, 2026 •

edited

Loading

github-actions Bot commented Jul 23, 2026 •

edited

Loading

codecov-commenter commented Jul 24, 2026 •

edited

Loading

iankhou commented Jul 28, 2026 •

edited

Loading