Skip to content

refactor: migrate all subprocess call sites to the shared subprocess module - #1849

Open
iankhou wants to merge 8 commits into
mainfrom
iankhou-shell-interactions
Open

refactor: migrate all subprocess call sites to the shared subprocess module#1849
iankhou wants to merge 8 commits into
mainfrom
iankhou-shell-interactions

Conversation

@iankhou

@iankhou iankhou commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

This re-submits changes from #1758, which was merged and then reverted in #1774 because we weren't able to run Windows integ tests on it yet.

Tested in workflow: https://github.com/aws/aws-cdk-cli/actions/runs/32169009705, which ran the changes through the integ tests on both Linux and Windows using throwaway branch https://github.com/aws/aws-cdk-cli/tree/refs/heads/iankhou-windows-x-shell-interactions.

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 that pattern. 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).

Follow-ups include cdk-build-tools/integ-runner/cli-integ migrations.

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

@iankhou
iankhou deployed to automation August 18, 2026 17:39 — with GitHub Actions Active
@github-actions github-actions Bot added the p2 label Aug 18, 2026
@aws-cdk-automation
aws-cdk-automation requested a review from a team August 18, 2026 17:39
@iankhou
iankhou deployed to no-approval August 18, 2026 17:39 — with GitHub Actions Active
@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

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
  • ⚠️ 3 package(s) with unknown licenses.
See the Details below.

License Issues

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

PackageVersionLicenseIssue Type
cross-spawn^7.0.6NullUnknown License

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

PackageVersionLicenseIssue Type
cross-spawn^7.0.6NullUnknown License

packages/aws-cdk/package.json

PackageVersionLicenseIssue Type
cross-spawn^7.0.6NullUnknown License

OpenSSF Scorecard

PackageVersionScoreDetails
npm/cross-spawn ^7.0.6 UnknownUnknown
npm/cross-spawn ^7.0.6 UnknownUnknown
npm/cross-spawn ^7.0.6 UnknownUnknown

Scanned Files

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

@iankhou
iankhou deployed to run-tests August 19, 2026 13:40 — with GitHub Actions Active
@iankhou
iankhou deployed to run-tests August 19, 2026 13:40 — with GitHub Actions Active
@iankhou
iankhou deployed to run-tests August 19, 2026 13:40 — with GitHub Actions Active
@iankhou
iankhou deployed to run-tests August 19, 2026 13:40 — with GitHub Actions Active
@iankhou
iankhou deployed to run-tests August 19, 2026 13:40 — with GitHub Actions Active
@iankhou
iankhou deployed to run-tests August 19, 2026 13:40 — with GitHub Actions Active
@iankhou
iankhou deployed to run-tests August 19, 2026 13:40 — with GitHub Actions Active
@iankhou
iankhou deployed to run-tests August 19, 2026 13:40 — with GitHub Actions Active
@iankhou
iankhou deployed to run-tests August 19, 2026 13:40 — with GitHub Actions Active
@iankhou
iankhou deployed to run-tests August 19, 2026 13:40 — with GitHub Actions Active
@iankhou
iankhou deployed to run-tests August 19, 2026 13:40 — with GitHub Actions Active
@iankhou
iankhou deployed to run-tests August 19, 2026 13:40 — with GitHub Actions Active
@iankhou
iankhou deployed to run-tests August 19, 2026 13:40 — with GitHub Actions Active
@iankhou
iankhou deployed to run-tests August 19, 2026 13:40 — with GitHub Actions Active
@iankhou
iankhou deployed to run-tests August 19, 2026 13:40 — with GitHub Actions Active
@iankhou
iankhou deployed to run-tests August 19, 2026 13:40 — with GitHub Actions Active
@iankhou
iankhou deployed to run-tests August 19, 2026 13:40 — with GitHub Actions Active
@iankhou
iankhou deployed to run-tests August 19, 2026 13:40 — with GitHub Actions Active
@iankhou
iankhou deployed to run-tests August 19, 2026 13:40 — with GitHub Actions Active
@iankhou
iankhou deployed to run-tests August 19, 2026 13:40 — with GitHub Actions Active
Since the revert, @aws-cdk/private-tools is a build-time-only devDependency
consumed through the per-package generated shim (lib/private/tools.ts).
Register the subprocess tool for the CLI project and switch all direct
imports to the shim.
@iankhou
iankhou deployed to no-approval August 19, 2026 15:01 — with GitHub Actions Active
@iankhou
iankhou deployed to run-tests August 19, 2026 15:05 — with GitHub Actions Active
@iankhou
iankhou deployed to run-tests August 19, 2026 15:05 — with GitHub Actions Active
@iankhou
iankhou deployed to run-tests August 19, 2026 15:05 — with GitHub Actions Active
@iankhou
iankhou deployed to run-tests August 19, 2026 15:05 — with GitHub Actions Active
@iankhou
iankhou deployed to run-tests August 19, 2026 15:05 — with GitHub Actions Active
@codecov-commenter

codecov-commenter commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.37209% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.90%. Comparing base (a4c0282) to head (fc848fa).

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    #1849   +/-   ##
=======================================
  Coverage   90.90%   90.90%           
=======================================
  Files          80       80           
  Lines       12250    12177   -73     
  Branches     1754     1741   -13     
=======================================
- Hits        11136    11070   -66     
+ Misses       1077     1069    -8     
- Partials       37       38    +1     
Flag Coverage Δ
suite.unit 90.90% <88.37%> (+<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.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants