Skip to content

fix(client): set { shell: true } in server options for "deno.{bat,cmd}"#1273

Merged
nayeemrmn merged 1 commit intodenoland:mainfrom
BinToss:fix/spawn-bat-or-cmd-on-win32-vscode1.92-or-later
Mar 12, 2025
Merged

fix(client): set { shell: true } in server options for "deno.{bat,cmd}"#1273
nayeemrmn merged 1 commit intodenoland:mainfrom
BinToss:fix/spawn-bat-or-cmd-on-win32-vscode1.92-or-later

Conversation

@BinToss
Copy link
Contributor

@BinToss BinToss commented Mar 12, 2025

Fixes "Deno Language Server client: couldn't create connection to server. Error: spawn EINVAL" #1257

I've debugged it locally to ensure it works on my Windows PC.


vscode_deno is currently broken on Win32 VSCode 1.92 (July 2024) and later. spawn's/spawnSync's EINVAL error was introduced in the Node.js 18.x, 20.x, 21.x releases lines to mitigate CVE-2024-27980. The fix was implemented at nodejs/node@69ffc6d50d.

Running a .bat or .cmd via spawn or spawnSync without assigning the shell option is prohibited in VSCode 1.92 and later.

…md}"

Fixes "Deno Language Server client: couldn't create connection to server. Error: spawn EINVAL" denoland#1257

I've debugged it locally to ensure it works on my Windows PC.
@CLAassistant
Copy link

CLAassistant commented Mar 12, 2025

CLA assistant check
All committers have signed the CLA.

Comment on lines +178 to +185
} as Executable,
debug: {
command,
// disabled for now, as this gets super chatty during development
// args: ["lsp", "-L", "debug"],
args: ["lsp"],
options: { env },
},
options: { env, shell },
} as Executable,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are the casts necessary? as Executable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was just for typed intellisense so I knew what data type was expected. Without the cast, the structs weren't typed appropriately.

If necessary, I can revert it or use narrowing and type extraction to extract the correct type from the ServerOptions union.

@nayeemrmn
Copy link
Collaborator

Thanks for the fix @BinToss! If you can sign the CLA we'll land this

@BinToss
Copy link
Contributor Author

BinToss commented Mar 12, 2025

Done

Copy link
Collaborator

@nayeemrmn nayeemrmn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@nayeemrmn nayeemrmn merged commit 75e3298 into denoland:main Mar 12, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants