Always quote Python executable path by fgiancane8 · Pull Request #964 · microsoft/vscode-python-debugger

fgiancane8 · 2026-02-24T19:03:11Z

On Windows, this would cause pahts like "C:\Program Files" to fail launching the Python command as the blank space was not escaped properly.

Add also a unit test to validate this change.
Please refer to #635, #351 for the initial discussion.

On Windows, this would cause pahts like "C:\Program Files\" to fail launching the Python command as the blank space was not escaped properly. Add also a unit test to validate this change. Signed-off-by: Francesco Giancane <me@fgiancane8.dev>

fgiancane8 · 2026-02-24T19:24:30Z

@rchiodo thanks for reviewing the PR! Can you comment on the Unit Tests? They were generated with Copilot so I do not know if that's the best way of testing this case. Thanks!

rchiodo · 2026-02-24T19:26:44Z

Yeah the added tests seem fine. I was wondering about the changing of the // to \ though. It seems that change is breaking a bunch of other tests.

fgiancane8 · 2026-02-24T19:33:42Z

Yeah the added tests seem fine. I was wondering about the changing of the // to \ though. It seems that change is breaking a bunch of other tests.

So I am not a TypeScript developer. By far. i have only noticed that the DAP program is now properly quoted. If you know better, I can push another change amending // to .

fgiancane8 · 2026-02-24T19:39:26Z

.replace(/\\/g, '/'); this can probably go away. I don't see it used in any other context besides command line generation.

rchiodo · 2026-02-24T19:44:44Z

.replace(/\\/g, '/'); this can probably go away. I don't see it used in any other context besides command line generation.

Yeah it's weird that it was there. I'm wondering why. The git blame makes it look like it's been that way since the beginning. Perhaps that code path is never actually used.

fgiancane8 · 2026-02-24T19:47:03Z

.replace(/\\/g, '/'); this can probably go away. I don't see it used in any other context besides command line generation.

Yeah it's weird that it was there. I'm wondering why. The git blame makes it look like it's been that way since the beginning. Perhaps that code path is never actually used.

It's only called in remoteLauncher.ts in getDebugpyLauncherArgs. If we want to be safe, I can rework my change to use a similar function without the replace part, or we can test it more aggressively removing it altogether and see if tests go through.

rchiodo · 2026-02-24T19:50:31Z

I think we should just remove it. I'm guessing it's not necessary but that it worked in remote scenarios because most of the time people don't do remote between windows machines.

rchiodo · 2026-02-24T19:52:04Z

Or maybe even better only do the conversion when the platform isn't windows. I believe there's a 'getOSType' function somewhere.

fgiancane8 · 2026-02-24T19:55:47Z

Or maybe even better only do the conversion when the platform isn't windows. I believe there's a 'getOSType' function somewhere.

Posting a change to be safe and using the non-replace version in our context, just to see if this is what's failing. If you can suggest a snippet using the getOSType i'd appreciate it :)

src/extension/debugger/adapter/factory.ts

fgiancane8 · 2026-02-24T20:25:17Z

Looks like tests are expecting the forward slashes, maybe it's best to revert the last change and just amend test cases?

rchiodo · 2026-02-24T20:43:36Z

The tests should special case expected path based on OS

fgiancane8 · 2026-02-24T21:04:40Z

The tests should special case expected path based on OS

ok, let me think about it a little bit better. will push new commits once I figure out how to do best. Thanks for all the inputs!

There is no need anymore to replace backslashes with forward slashes for paths on Windows, as the Python extension's `toCommandArgumentForPythonExt` function already handles this correctly. Signed-off-by: Francesco Giancane <me@fgiancane8.dev>

rchiodo · 2026-02-25T00:22:21Z

src/extension/common/stringUtils.ts

-    return toCommandArgumentForPythonExt(source).replace(/\\/g, '/');
+
+    let result = toCommandArgumentForPythonExt(source);
+


I don't think this is the correct change. On non windows machines it may have the wrong path separators.

The unit tests are failing only because the old code did the wrong thing. The unit tests are wrong. They need to be platform specific too.

Oh ok, I excluded that because I suggested amending previously pushed tests but we discarded that. Let me re-push it once more

Okay this leaves the 'bug' that was there before. It should be checking OS.

Ok, restored the OS-checking path as you recommended before.

fgiancane8 · 2026-02-25T00:25:43Z

I think we should just remove it. I'm guessing it's not necessary but that it worked in remote scenarios because most of the time people don't do remote between windows machines.

I am exploring this scenario. Tests are passing locally. I have to test on a Windows machine to be 100% sure but as far I can see here, 4 tests were failing previously and now the total number of tests match so I'd assume we got it right.

I'll keep you posted. Thanks!

fgiancane8 · 2026-02-25T00:42:57Z

So this is the test reportedly failing:
https://github.com/microsoft/vscode-python-debugger/blob/e0ec60c7b5ba3284553d914739e05b744ace5921/src/test/unittest/adapter/remoteLaunchers.unit.test.ts#L19C1-L23C11
As per my understanding, it is always expecting Unix-like paths:

      AssertionError: expected [ …(4) ] to deeply equal [ …(4) ]
      + expected - actual

       [
      -  "\"path\\to\\debugpy\\with spaces\""
      +  "\"path/to/debugpy/with spaces\""

The first line is generated using path.join. Should we enclose that call with fileToCommandArgumentForPythonExt or have the expected value to be platform-dependant? If we are normalising everything to Unix format, I'd say the former.

Explicitly call `fileToCommandArgumentForPythonExt` when computing a path with spaces and check results are as expected.

fgiancane8 · 2026-02-25T00:47:54Z

src/test/unittest/adapter/remoteLaunchers.unit.test.ts

        {
            testName: 'When path to debugpy contains spaces',
-            path: path.join('path', 'to', 'debugpy', 'with spaces'),
+            path: fileToCommandArgumentForPythonExt(path.join('path', 'to', 'debugpy', 'with spaces')),


Perhaps something like this?

I don't feel like this is the right way to test this as you're reusing the internal function to test itself.

Thought the same, although in the end we want just to validate that the "normalised" input is equivalent. What if we replace(//, ) the input here, simulating the same behaviour, without exposing the internals?

All in all, it seems to me that the platform-specific behaviour is in path.join

I'm not sure what you're seeing but my expectation is that fileToCommandArgumentForPythonExt will generate platform specific output. Or at least not change the input path's separator.

The unit tests should be checking that quotes are around a path, they don't really care about the separator in the paths.

I'm not sure what you're seeing but my expectation is that fileToCommandArgumentForPythonExt will generate platform specific output. Or at least not change the input path's separator.

The unit tests should be checking that quotes are around a path, they don't really care about the separator in the paths.

So yeah tests data is encoded as "path/to/something/with space" but of course Windows would generate "path\to\something\with spaces".

I kept the sane logic in the source code and updated the test code to use platform-specific data.

rchiodo · 2026-02-25T01:04:26Z

The first line is generated using path.join. Should we enclose that call with fileToCommandArgumentForPythonExt or have the expected value to be platform-dependant? If we are normalising everything to Unix format, I'd say the former.

We shouldn't be normalizing everything to unix paths in the real code, but in the unit tests is fine.

…Windows platforms.

This reverts commit 32822e0.

Use backward slashes on Windows and forward slashes on Unix-like OSes when testing correct behavior of path strings quotation.

fgiancane8 · 2026-02-25T01:44:56Z

We shouldn't be normalizing everything to unix paths in the real code, but in the unit tests is fine.

Added OS-specific logic in test data. Hopefully it's fine now!

fgiancane8

@rchiodo, I see tests passing as well on my Windows machine. Can we run the CI again?

rchiodo · 2026-02-25T17:37:54Z

There's still a linter error. If you go to a terminal and run npm run format-fix and then push the changes I think it should fix the linter problem.

fgiancane8 · 2026-02-25T17:41:37Z

There's still a linter error. If you go to a terminal and run npm run format-fix and then push the changes I think it should fix the linter problem.

Sorry, my bad. Found out later. Should be addressed now!

Always quote Python executable path

a3d46e4

On Windows, this would cause pahts like "C:\Program Files\" to fail launching the Python command as the blank space was not escaped properly. Add also a unit test to validate this change. Signed-off-by: Francesco Giancane <me@fgiancane8.dev>

rchiodo previously approved these changes Feb 24, 2026

View reviewed changes

rchiodo added the bug Issue identified by VS Code Team member as probable bug label Feb 24, 2026

rchiodo reviewed Feb 24, 2026

View reviewed changes

src/extension/debugger/adapter/factory.ts Show resolved Hide resolved

fgiancane8 dismissed rchiodo’s stale review via 1bcd055 February 24, 2026 20:09

rchiodo previously approved these changes Feb 24, 2026

View reviewed changes

fgiancane8 dismissed rchiodo’s stale review via 10d78b6 February 24, 2026 20:11

fgiancane8 force-pushed the fgiancane8-try-fixing-issues-with-vscode-python-debugger-on-windows branch from 1bcd055 to 10d78b6 Compare February 24, 2026 20:11

rchiodo previously approved these changes Feb 24, 2026

View reviewed changes

fgiancane8 dismissed rchiodo’s stale review via ae8e14f February 25, 2026 00:18

fgiancane8 force-pushed the fgiancane8-try-fixing-issues-with-vscode-python-debugger-on-windows branch from 10d78b6 to ae8e14f Compare February 25, 2026 00:18

rchiodo reviewed Feb 25, 2026

View reviewed changes

fgiancane8 force-pushed the fgiancane8-try-fixing-issues-with-vscode-python-debugger-on-windows branch from ae8e14f to 3668184 Compare February 25, 2026 00:24

stringUtils.ts: restore replace call when normalizing string

ef13e45

remoteLaunchers: ensure test is normalizing paths

32822e0

Explicitly call `fileToCommandArgumentForPythonExt` when computing a path with spaces and check results are as expected.

fgiancane8 commented Feb 25, 2026

View reviewed changes

fgiancane8 added 3 commits February 25, 2026 01:05

stringUtils.ts: replace backslashes with forward-slashes only on non-…

745998a

…Windows platforms.

Revert "remoteLaunchers: ensure test is normalizing paths"

6df15dc

This reverts commit 32822e0.

remoteLaunchers.ts: feed platform-specific data to tests

a0927d1

Use backward slashes on Windows and forward slashes on Unix-like OSes when testing correct behavior of path strings quotation.

Copy-paste error in comment.

2601ca5

fgiancane8 commented Feb 25, 2026

View reviewed changes

rchiodo previously approved these changes Feb 25, 2026

View reviewed changes

remoteLaunchers.unit.test.ts: fix formatting issues

f4c64e5

fgiancane8 dismissed rchiodo’s stale review via f4c64e5 February 25, 2026 17:40

rchiodo approved these changes Feb 25, 2026

View reviewed changes

edvilme approved these changes Feb 25, 2026

View reviewed changes

bschnurr approved these changes Feb 25, 2026

View reviewed changes

rchiodo merged commit 33697c0 into microsoft:main Feb 25, 2026
18 checks passed

		return toCommandArgumentForPythonExt(source).replace(/\\/g, '/');

		let result = toCommandArgumentForPythonExt(source);

Conversation

fgiancane8 commented Feb 24, 2026

Uh oh!

fgiancane8 commented Feb 24, 2026

Uh oh!

rchiodo commented Feb 24, 2026

Uh oh!

fgiancane8 commented Feb 24, 2026

Uh oh!

fgiancane8 commented Feb 24, 2026

Uh oh!

rchiodo commented Feb 24, 2026

Uh oh!

fgiancane8 commented Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rchiodo commented Feb 24, 2026

Uh oh!

rchiodo commented Feb 24, 2026

Uh oh!

fgiancane8 commented Feb 24, 2026

Uh oh!

Uh oh!

fgiancane8 commented Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rchiodo commented Feb 24, 2026

Uh oh!

fgiancane8 commented Feb 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fgiancane8 commented Feb 25, 2026

Uh oh!

fgiancane8 commented Feb 25, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rchiodo commented Feb 25, 2026

Uh oh!

fgiancane8 commented Feb 25, 2026

Uh oh!

fgiancane8 left a comment

Choose a reason for hiding this comment

Uh oh!

rchiodo commented Feb 25, 2026

Uh oh!

fgiancane8 commented Feb 25, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fgiancane8 commented Feb 24, 2026 •

edited

Loading

fgiancane8 commented Feb 24, 2026 •

edited

Loading