Fix spawn calls for node LTS versions

[msujew]

What it does

Closes #13613

1. Updates node-pty to 0.11.0-beta24, as our currently used version uses a spawn call that leads to errors on current LTS versions of node
2. Updates our child_process#spawn calls to use the { shell: true } options. This circumvents the changes for the LTS versions.

How to test

Confirm that the CI is green on Windows

Otherwise, perform a manual smoke test on Windows with focus on the terminal functionality.

Follow-ups

Review checklist

• As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• As a reviewer, I agree to behave in accordance with the review guidelines

[tsmaeder]

@msujew I guess this one's not ready for review with the PR checks failing?

[msujew]

@tsmaeder Nope, there seem to be some pretty major changes in node-pty (on Unix, it still works as expected on Windows) in more current versions related to the error behavior. I.e. spawning a process on a non-existing file doesn't throw anymore but instead just prints to the console and exits with exit code 1. I'm currently working on consolidating those changes with our expectations.

[msujew]

@tsmaeder No idea what Microsoft has done there (note that I'm exclusively working on fixing breaking changes due to the updated version of node-pty), I can't even reproduce the error the CI gets on Windows, since the process just freezes. I've tried fixing this for the past 4 hours, I'm done for the week :)

[msujew]

@planger Can I ask you to take a look at the playwright test errors (or can we just merge independent of these errors)? I'm not too deep in there and it would be good to have this fix for tomorrows release.

[msujew]

@tsmaeder I found an earlier version of node-pty that doesn't have the erroneous install script, while still using the old (non-breaking) behavior. This seems to satisfy all our tests (except for the Playwright tests, which is probably alright for now). Asking nicely for a review so we can have this in tomorrows release :)

[planger]

@msujew Sure, I'm just quite busy the next few days. So I suggest to just skip the failing tests for now and please create an issue for re-enabling them with a fix. I'm happy to look into it next week. Thank you very much!

[tsmaeder]

Did you find a description of the fix that explains what adding "shell: true" is the right workaund? I haven't been able to find a description of what nodejs actually changed.

[msujew]

Did you find a description of the fix that explains what adding "shell: true" is the right workaund? I haven't been able to find a description of what nodejs actually changed.

Not really. The changelog just states that they fixed an issue when not using shell: true, see here. The consequence is that by not using shell: true the spawn call will just fail with EINVAL. See also other projects that suffer from this issue like forcedotcom/cli#2843.

[tsmaeder]

I think this one is a good description: nodejs/node#52681 (comment). I guess always using "shell: true" won't hurt, usually. As for when we construct the process execution ourselves, we could differentiate between scrips and executables.

[msujew]

@planger False alarm. It wasn't a false positive, but I actually broke terminals for Unix. I've fixed it again and the tests are green now, so no need for action on your side :)

[planger]

@msujew Oh, ok great that you've fixed it and that the tests actually helped revealing a real issue 💯 Thank you!