[belaboe]

Problem

On Windows, npm run sandbox:destroy always fails and leaves the sandbox stack deployed. The scheduled Windows E2E (windows-e2e.yml, twice a day) hits this on every run:

> tsx -C cdk aws-blocks/scripts/sandbox-destroy.ts
🗑️  Destroying sandbox...
Error [ERR_MODULE_NOT_FOUND]: Cannot find module 'D:\a\_temp\…\my-app\'D:\a\_temp\…\my-app\aws-blocks\index.cdk.ts''
  ⚠️  could not list sandbox stacks via 'cdk ls'; skipping bucket-emptying: spawnSync npm ENOENT
⏳ Stack deletion failed. Retrying in 60s…

That job stays green because its teardown is best-effort, so every run leaks one my-app-<hash>-runnerad-<hash> sandbox stack, each with an edge-optimized REST API. Together with the cleanup role being unable to delete my-app-* stacks (separate fix), this has filled the CI account's 120 edge REST API quota. PR E2E deploys now fail with This operation would breach the limit of 120 apis for EDGE endpoint type (for example on #525).

Causes

1. Quoting. destroySandbox and listSandboxStackNames passed --app "npm exec tsx -- -C cdk '<path>'". CDK runs that string through a shell, and on Windows that's cmd.exe, which doesn't treat ' as quoting, so the quotes end up in the path. Deploy, synth, and watch passed the path unquoted, which works on Windows but splits a path containing a space on any platform.
2. Spawning npm. listSandboxStackNames (and listProductionStackNames in destroy.ts) used execFileSync('npm' | 'npx'), which can't start the .cmd shim on Windows (ENOENT). The repo already has cross-spawn wrappers in run-command.ts for exactly this; these two calls just didn't use them.

Fix

• quoteCdkAppPath(path, platform): double quotes on win32, single quotes elsewhere (with ' escaped). Used by all five --app strings (destroy, cdk ls, synth, deploy, watch).
• runCapture() in run-command.ts: the stdout-returning counterpart to runSync(), on cross-spawn. It throws with error.stderr set, like execFileSync, so the existing warning still shows the real failure. Used for sandbox and production cdk ls.

Patch changeset for @aws-blocks/core and the umbrella.

Testing

• New unit tests: quoteCdkAppPath (Windows, POSIX, POSIX path containing ') and runCapture (stdout, non-zero exit with stderr, missing command).
• preflight-target.test.ts mocks the whole run-command module, so runCapture was added to that mock.
• @aws-blocks/core tests: 1435 pass, 0 fail.

Not verified on Windows locally. The Windows E2E is schedule- or dispatch-only; dispatched on this branch at 3b42193: https://github.com/aws-devtools-labs/aws-blocks/actions/runs/39804250617

[changeset-bot]

🦋 Changeset detected

Latest commit: 784e3b9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@aws-blocks/core	Patch
@aws-blocks/blocks	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[osama-rizk]

Fixes both Windows sandbox:destroy causes the right way: the --app path is quoted per shell, and cdk ls moves onto the repo's existing cross-spawn wrapper. I'm commenting rather than approving because the two lines that were the bug aren't held by any test, and the rebase over #968 needs care. Nothing here is blocking.

What's good

• runCapture builds on cross-spawn, not shell: true. The .cmd shim is resolved and every argument is escaped for cmd, and it keeps execFileSync's .stderr contract, so the existing "could not list" warning still shows the real failure.
• All five --app sites now go through one helper, and the POSIX '\'' escaping is correct. Production cdk ls in destroy.ts gets the same ENOENT fix.
• The changeset bumps the umbrella too. It needs to, because @aws-blocks/blocks ships its own nested @aws-blocks/core (visible in the Windows log path).
• I verified against the code and runs:
  • Clean build; all dist/scripts/*.test.js pass (608/608).
  • main's scheduled Windows run 39921882707 (today) still fails destroy 3 times with Cannot find module '...\'D:\...index.cdk.ts'' and spawnSync npm ENOENT.
  • Your dispatch 39804250617 shows Sandbox destroyed! and the production destroy succeeding, and the Windows sandbox deploy with the double-quoted path also worked.
  • On Linux, E2E Sandbox on 784e3b9 deploys, destroys, and destroys on retry through runCapture with no listing warning.

macOS / Linux regression risk: low. destroy and cdk ls were already single-quoted. deploy, synth and watch go from a bare path to a single-quoted one, which /bin/sh strips, so npm gets the same argv. A path with a space, $ or a backtick is now passed literally instead of being split or expanded, which is an improvement.

What needs work: 0 must-fix · 2 should-fix · 1 minor · 1 nit. Details are inline.

Speed / flakiness: there's no CI run-time change, since no workflow is touched. This PR's run took 81.3 min, inside the 66.6 to 96.4 min range of the baseline main runs 39812188339 and 39821845972. The reliability gain is real but indirect. Each scheduled Windows run (2 a day, always pool account 1) currently leaks one sandbox stack holding an edge-optimized REST API; today's 08:02 run leaked one. This fix stops that, which relieves the 120-API quota that PR E2E deploys in that account hit. The size of that effect on PR flakiness is my inference, not something I measured.

E2E integrity: nothing is relaxed. No test is removed, skipped or gated. The deploy --app assertion changes to the quoted form at the same strength, and a synth assertion is added. No workflow or required-check change. One caveat on the proof: the scheduled Windows job is currently green even when destroy fails. runTeardown in scripts/ci/windows-template-e2e.mjs calls runSpawn, which returns the spawnSync result and never throws, so teardownFailures stays empty; 39921882707 is green with a failed destroy. #946 fixes that. Until it lands, the Windows evidence for this PR comes from reading the logs, not the job status. #896 followed by #946 is what makes that job honest.

Merge order (trial merges on main 5231743):

• With #958 alone: merges clean.
• With #952: conflicts in destroy.ts, imports only.
• With #968: conflicts in sandbox.ts, in the imports and the whole watch block.
• In the planned order (#958, #952, #968, #896), only this PR conflicts, so the order is right. #958's --all auto-merges next to the new destroy --app line. #946 and #966 wait on this.

Open questions

1. After rebasing over #968, will the quoting move into buildSandboxWatchArgs (with an assertion, now that watch has a pure builder)?
2. Could you re-dispatch the Windows E2E on the rebased head? The last Windows proof is at 3b42193, and the rebase rewrites the same lines.

Cross-cutting: PR CI's E2E Templates (Windows) job is local-only and never runs sandbox:destroy. Windows destroy is covered only by the scheduled or dispatched windows-e2e.yml, which is why the guard tests in the first inline comment matter.

[osama-rizk]

Non-blocking (should fix): the two lines that were the actual bug aren't guarded by any test. I mutated the built code and ran all 34 dist/scripts/*.test.js files; each of these still passes 608/608:

• destroy --app here (:705) reverted to '${backendPath}', i.e. the original Windows bug;
• the cdk ls --app at :611 reverted the same way;
• both cdk ls calls (here and destroy.ts:24) reverted to execFileSync.

The new assertions only cover synth and deploy. PR CI's Windows lane is local-only, and the scheduled Windows job currently stays green on a failed destroy (until #946). So reintroducing this bug would ship unnoticed.

Fix: pull the two arg arrays into pure builders like the synth and deploy ones (e.g. buildSandboxDestroyArgs(backendPath) and buildSandboxLsArgs(backendPath)). Assert their --app value with a literal, e.g. "npm exec tsx -- -C cdk '/tmp/my app/index.cdk.ts'". For the spawn wiring, a mock.module of run-command (as preflight-target.test.ts already does) can assert that listSandboxStackNames and listProductionStackNames call runCapture('npm' | 'npx', ...).

[belaboe]

Fixed in #1014: destroy and cdk ls argv are now buildSandboxDestroyArgs / buildSandboxLsArgs with literal win32 and POSIX --app asserts, and sandbox-stack-listing.test.ts module-mocks run-command to assert both listers use runCapture and both destroys use runSync (6e7a8e38, b49c6a24). Each of your three mutations (plus re-inlining the old argv at the call sites) now fails a test, and the Windows E2E on that head destroys cleanly.

[osama-rizk]

Non-blocking (should fix): watch heads into a conflict. I trial-merged #958, #952, #968 and then this PR onto main. #968 replaces this inline cdk watch array with buildSandboxWatchArgs(...) (its line "--app", \npm exec tsx -- -C cdk ${backendPath}``), so the rebase conflicts on this whole block. If the conflict is resolved by taking #968's side, the watch quoting is silently lost, and no test would notice. Reverting the quoting here already passes every test.

Fix: when rebasing, apply quoteCdkAppPath(backendPath) inside buildSandboxWatchArgs. Add an --app assertion to #968's watch-args tests, which closes the "watch has no unit coverage" gap for free. The destroy.ts conflict with #952 is imports only.

[osama-rizk]

Non-blocking (minor): the win32 branch interpolates the path without any check, and CodeQL alert #83 (js/shell-command-constructed-from-input, medium) is open on the watch call at :334 because of it. On POSIX the '\'' escaping makes the string safe. On Windows a " in backendPath would close the quote early and let cmd.exe treat the remainder as command syntax. The input is the developer's own resolved file path, so this isn't a real attack surface. Still, " can't appear in a Windows file name, so refusing it costs one line, makes the helper total, and gives a concrete reason to dismiss the alert.

Fix: refuse " on win32, then dismiss #83 citing it.

Suggested change

	if (platform === 'win32') return `"${backendPath}"`;
	if (platform === 'win32') {
	// `"` cannot appear in a Windows file name; refusing it keeps the cmd.exe string well-formed.
	if (backendPath.includes('"')) throw new Error(`Invalid backend path (contains a double quote): ${backendPath}`);
	return `"${backendPath}"`;
	}

[belaboe]

Applied your suggestion in #1014 (6e7a8e38) with a test, a @throws note and a changeset line. I have left dismissing CodeQL #83 to a maintainer.

[osama-rizk]

Non-blocking (nit): the signal branch of runCapture (run-command.ts:53) isn't exercised. Mutating it to ignore result.signal still passes. A POSIX-only case covers it (I checked that it throws ... was terminated by signal SIGTERM locally):

it('throws when the command is killed by a signal', { skip: process.platform === 'win32' }, () => {
  assert.throws(() => runCapture(process.execPath, ['-e', 'process.kill(process.pid, "SIGTERM")']), /terminated by signal SIGTERM/);
});

Fix: add the case above.

[belaboe]

Added your SIGTERM case in #1014 (6e7a8e38); it fails when the signal branch is removed.

[belaboe]

Re open question 2 (Windows E2E on the new head): the review fixes are in #1014, and Windows E2E run 39972013006 on its head shows Sandbox destroyed on retry! (after an API Gateway 429 on the first attempt) and Production stack destroyed! in the logs.

[osama-rizk]

Since the last review: 0 resolved on this head · 1 new · 4 still open on this head. Findings 1, 3 and 4 are fixed in the stacked #1014 (I checked its buildSandboxLsArgs / buildSandboxDestroyArgs builders with literal --app asserts, the run-command module mock in sandbox-stack-listing.test.ts, the " refusal in quoteCdkAppPath, and the SIGTERM case), but none of that is on this branch yet. Finding 2 (watch quoting after #968) is still unanswered.

Head 784e3b9 hasn't changed, but main has: #952 (merged 2026-10-26), #981 and #999 now conflict with this PR in sandbox.ts, destroy.ts and sandbox-args.test.ts (git merge-tree against main db15b5e; #1014 conflicts in the same three files). One of those conflicts can silently undo the fix. Details are inline.

What's good

• The fix is still needed on main. Today's scheduled Windows E2E (run 40470860322) logs Destroy failed after retries. during sandbox:destroy teardown, and the job still ends green.
• #1014 covers the earlier test gaps properly: literal argv asserts and a check that the listers use runCapture, not just a computed string.

What needs work: 1 new should-fix (inline). Earlier findings 1/3/4 are addressed in #1014 and need folding in. Finding 2 is still open.

Speed: neutral, and that's expected for a correctness fix. The diff touches only packages/core/src/scripts and adds a handful of millisecond unit tests, with no workflow or job-graph change. So it doesn't move PR Checks wall time, before or after #963's split. The head's CI ran before #963, so it doesn't tell us anything about the current job layout. On the scheduled Windows job, teardown currently fails after about 3 min of retries (08:11:17 to 08:14:25 in run 40470860322). With the fix, teardown really destroys the stack, so that step may take a different amount of time (inferred, not measured on current main).

Regression: not mergeable as it stands. The head CI was green (all checks pass, including E2E Templates (Windows)), but against an old base. Against current main there are textual conflicts in three files, plus the semantic hazard below. POSIX behaviour is unchanged apart from escaping ' inside paths. Windows users get a working sandbox:destroy.

Coverage / blind spots: no assertion is removed or skipped, and no job moves between required and advisory. One assertion was weakened: sandbox-args.test.ts swaps the literal --app expectation for one computed with quoteCdkAppPath, the function under test. #1014 restores literal asserts, so folding #1014 in closes that gap.

Open questions

1. Will you fold #1014 into this branch when you rebase onto main, or land #1014 first?
2. After the rebase, does sandboxDestroyDeps (from #952) use buildSandboxDestroyArgs?
3. Finding 2 is still unanswered: when #968 lands, does the watch quoting move into buildSandboxWatchArgs?
4. Will you re-run the Windows E2E on the rebased head and point to the Sandbox destroyed log line?

Merge-order note: #952 is in. Track D order is still #958, #968, then this PR, and #946/#966 wait for this one.

[osama-rizk]

Non-blocking (should fix): the rebase onto current main can silently bring the Windows bug back. #952 (merged 2026-10-26) moved this destroy argv into a new exported sandboxDestroyDeps(backendPath, cdkEnv) on main (sandbox.ts:910-925 there), and its --app is still the hard-coded npm exec tsx -- -C cdk '${backendPath}' (main :917). In the trial merge, the conflict hunk on this side is only this cdkArgs array, and the runDestroyWithRetries(...) call below it merges cleanly into sandboxDestroyDeps(...). The natural way to resolve it, taking main's side, drops the quoting. sandbox:destroy on Windows then fails exactly as it does today, and the job still goes green. This PR's tests wouldn't catch that, because nothing asserts the destroy argv (finding 1). #1014's builder test would catch it only if sandboxDestroyDeps calls the builder. The same applies to the cdk ls site: main :611 is still '${backendPath}'.

Fix: when you rebase, have sandboxDestroyDeps build cdkArgs with buildSandboxDestroyArgs(backendPath) from #1014, and add a test that sandboxDestroyDeps(...) (or the module-mocked destroySandbox) passes the win32 double-quoted --app. Then check that grep -n -- "-C cdk '" packages/core/src/scripts/*.ts returns nothing.