diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 00000000..867bd47b --- /dev/null +++ b/.gitattributes @@ -0,0 +1,3 @@ +bundled/scripts/noConfigScripts/debugpy text eol=lf +bundled/scripts/noConfigScripts/debugpy.sh text eol=lf +bundled/scripts/noConfigScripts/debugpy.fish text eol=lf diff --git a/.github/agents/reviewCritic.agent.md b/.github/agents/reviewCritic.agent.md new file mode 100644 index 00000000..a860e694 --- /dev/null +++ b/.github/agents/reviewCritic.agent.md @@ -0,0 +1,24 @@ +--- +description: "Review critic for vscode-python-debugger. Use when: reviewing a fix, checking regressions, verifying test coverage, or pressure-testing a PR before merge." +tools: [read/readFile, edit/editFiles, execute/runInTerminal, execute/getTerminalOutput, execute/sendToTerminal, search/textSearch, vscode/askQuestions, todo] +--- + +You are a high-signal review critic for **vscode-python-debugger**. + +Focus on correctness, regressions, environment mutation, debug configuration behavior, cross-platform shell integration, and missing tests. Ignore style unless it hides a real bug. + +## Review workflow + +1. Start with `git status --short` and `git diff --stat`, then read every changed file in scope. +2. Verify each behavior change has a targeted test, or explain exactly why a test is not practical. +3. Prioritize: + - PATH / Path normalization and environment merging + - no-config debugging bootstrap scripts across shells + - debug configuration resolution and workspace behavior + - Windows/macOS/Linux differences that could break launch or attach +4. Report only actionable findings with: + - severity + - affected file or scope + - why it matters + - the missing fix or missing test +5. If the diff looks sound, say so explicitly and cite the tests that support that conclusion. diff --git a/bundled/scripts/noConfigScripts/debugpy b/bundled/scripts/noConfigScripts/debugpy index def62ec5..94210f89 100755 --- a/bundled/scripts/noConfigScripts/debugpy +++ b/bundled/scripts/noConfigScripts/debugpy @@ -1,4 +1,4 @@ #! /bin/bash # Bash script -export DEBUGPY_ADAPTER_ENDPOINTS=$VSCODE_DEBUGPY_ADAPTER_ENDPOINTS -python3 $BUNDLED_DEBUGPY_PATH --listen 0 --wait-for-client $@ +export DEBUGPY_ADAPTER_ENDPOINTS="$VSCODE_DEBUGPY_ADAPTER_ENDPOINTS" +python3 "$BUNDLED_DEBUGPY_PATH" --listen 0 --wait-for-client "$@" diff --git a/bundled/scripts/noConfigScripts/debugpy.bat b/bundled/scripts/noConfigScripts/debugpy.bat index 2450cf6a..855d03ba 100755 --- a/bundled/scripts/noConfigScripts/debugpy.bat +++ b/bundled/scripts/noConfigScripts/debugpy.bat @@ -1,4 +1,4 @@ -@echo off -:: Bat script -set DEBUGPY_ADAPTER_ENDPOINTS=%VSCODE_DEBUGPY_ADAPTER_ENDPOINTS% -python %BUNDLED_DEBUGPY_PATH% --listen 0 --wait-for-client %* +@echo off +:: Bat script +set "DEBUGPY_ADAPTER_ENDPOINTS=%VSCODE_DEBUGPY_ADAPTER_ENDPOINTS%" +python "%BUNDLED_DEBUGPY_PATH%" --listen 0 --wait-for-client %* diff --git a/bundled/scripts/noConfigScripts/debugpy.fish b/bundled/scripts/noConfigScripts/debugpy.fish index 624f7202..8aaeb56f 100755 --- a/bundled/scripts/noConfigScripts/debugpy.fish +++ b/bundled/scripts/noConfigScripts/debugpy.fish @@ -1,3 +1,3 @@ # Fish script -set -x DEBUGPY_ADAPTER_ENDPOINTS $VSCODE_DEBUGPY_ADAPTER_ENDPOINTS -python3 $BUNDLED_DEBUGPY_PATH --listen 0 --wait-for-client $argv +set -x DEBUGPY_ADAPTER_ENDPOINTS "$VSCODE_DEBUGPY_ADAPTER_ENDPOINTS" +python3 "$BUNDLED_DEBUGPY_PATH" --listen 0 --wait-for-client $argv diff --git a/bundled/scripts/noConfigScripts/debugpy.ps1 b/bundled/scripts/noConfigScripts/debugpy.ps1 index 4b2ff85a..1f2e85da 100755 --- a/bundled/scripts/noConfigScripts/debugpy.ps1 +++ b/bundled/scripts/noConfigScripts/debugpy.ps1 @@ -1,8 +1,8 @@ -# PowerShell script -$env:DEBUGPY_ADAPTER_ENDPOINTS = $env:VSCODE_DEBUGPY_ADAPTER_ENDPOINTS -$os = [System.Environment]::OSVersion.Platform -if ($os -eq [System.PlatformID]::Win32NT) { - python $env:BUNDLED_DEBUGPY_PATH --listen 0 --wait-for-client $args -} else { - python3 $env:BUNDLED_DEBUGPY_PATH --listen 0 --wait-for-client $args -} \ No newline at end of file +# PowerShell script +$env:DEBUGPY_ADAPTER_ENDPOINTS = $env:VSCODE_DEBUGPY_ADAPTER_ENDPOINTS +$os = [System.Environment]::OSVersion.Platform +if ($os -eq [System.PlatformID]::Win32NT) { + python "$env:BUNDLED_DEBUGPY_PATH" --listen 0 --wait-for-client $args +} else { + python3 "$env:BUNDLED_DEBUGPY_PATH" --listen 0 --wait-for-client $args +} diff --git a/bundled/scripts/noConfigScripts/debugpy.sh b/bundled/scripts/noConfigScripts/debugpy.sh new file mode 100644 index 00000000..94210f89 --- /dev/null +++ b/bundled/scripts/noConfigScripts/debugpy.sh @@ -0,0 +1,4 @@ +#! /bin/bash +# Bash script +export DEBUGPY_ADAPTER_ENDPOINTS="$VSCODE_DEBUGPY_ADAPTER_ENDPOINTS" +python3 "$BUNDLED_DEBUGPY_PATH" --listen 0 --wait-for-client "$@" diff --git a/src/extension/common/variables/environment.ts b/src/extension/common/variables/environment.ts index ce67bb8b..ed7e0135 100644 --- a/src/extension/common/variables/environment.ts +++ b/src/extension/common/variables/environment.ts @@ -50,7 +50,7 @@ export function mergeVariables( if (!target) { return; } - const settingsNotToMerge = ['PYTHONPATH', getSearchPathEnvVarNames()[0]]; + const settingsNotToMerge = ['PYTHONPATH', ...getSearchPathEnvVarNames()]; Object.keys(source).forEach((setting) => { if (settingsNotToMerge.indexOf(setting) >= 0) { return; @@ -61,6 +61,44 @@ export function mergeVariables( }); } +export function normalizeSearchPathEnvironmentVariable( + vars: EnvironmentVariables, + variableNames = getSearchPathEnvVarNames(), +) { + if (!vars || variableNames.length <= 1) { + return vars; + } + + const [preferredName, ...aliasNames] = variableNames; + const mergedPathEntries: string[] = []; + + for (const variableName of variableNames) { + const value = vars[variableName]; + if (typeof value !== 'string' || value.length === 0) { + continue; + } + + for (const entry of value.split(path.delimiter)) { + const trimmedEntry = entry.trim(); + if (!trimmedEntry || mergedPathEntries.includes(trimmedEntry)) { + continue; + } + mergedPathEntries.push(trimmedEntry); + } + } + + if (mergedPathEntries.length === 0) { + return vars; + } + + vars[preferredName] = mergedPathEntries.join(path.delimiter); + for (const aliasName of aliasNames) { + delete vars[aliasName]; + } + + return vars; +} + export function appendPythonPath(vars: EnvironmentVariables, ...pythonPaths: string[]) { return appendPaths(vars, 'PYTHONPATH', ...pythonPaths); } diff --git a/src/extension/debugger/configuration/resolvers/helper.ts b/src/extension/debugger/configuration/resolvers/helper.ts index 6791bded..f01dd8ff 100644 --- a/src/extension/debugger/configuration/resolvers/helper.ts +++ b/src/extension/debugger/configuration/resolvers/helper.ts @@ -13,14 +13,18 @@ import * as envParser from '../../../common/variables/environment'; export async function getDebugEnvironmentVariables(args: LaunchRequestArguments): Promise { const pathVariableName = getSearchPathEnvVarNames()[0]; + const normalizedProcessEnv = { ...process.env }; + envParser.normalizeSearchPathEnvironmentVariable(normalizedProcessEnv); // Merge variables from both .env file and env json variables. const debugLaunchEnvVars: Record = args.env && Object.keys(args.env).length > 0 ? ({ ...args.env } as Record) : ({} as Record); + envParser.normalizeSearchPathEnvironmentVariable(debugLaunchEnvVars); const envFileVars = await envParser.parseFile(args.envFile, debugLaunchEnvVars); const env = envFileVars ? { ...envFileVars } : {}; + envParser.normalizeSearchPathEnvironmentVariable(env); // "overwrite: true" to ensure that debug-configuration env variable values // take precedence over env file. @@ -33,29 +37,31 @@ export async function getDebugEnvironmentVariables(args: LaunchRequestArguments) if (typeof env[pathVariableName] === 'string' && env[pathVariableName]!.length > 0) { // Now merge this path with the current system path. // We need to do this to ensure the PATH variable always has the system PATHs as well. - envParser.appendPath(env, process.env[pathVariableName]!); + envParser.appendPath(env, normalizedProcessEnv[pathVariableName]!); } if (typeof env.PYTHONPATH === 'string' && env.PYTHONPATH.length > 0) { // We didn't have a value for PATH earlier and now we do. // Now merge this path with the current system path. // We need to do this to ensure the PATH variable always has the system PATHs as well. - envParser.appendPythonPath(env, process.env.PYTHONPATH!); + envParser.appendPythonPath(env, normalizedProcessEnv.PYTHONPATH!); } if (args.console === 'internalConsole') { // For debugging, when not using any terminal, then we need to provide all env variables. // As we're spawning the process, we need to ensure all env variables are passed. // Including those from the current process (i.e. everything, not just custom vars). - envParser.mergeVariables(process.env, env); + envParser.mergeVariables(normalizedProcessEnv, env); - if (env[pathVariableName] === undefined && typeof process.env[pathVariableName] === 'string') { - env[pathVariableName] = process.env[pathVariableName]; + if (env[pathVariableName] === undefined && typeof normalizedProcessEnv[pathVariableName] === 'string') { + env[pathVariableName] = normalizedProcessEnv[pathVariableName]; } - if (env.PYTHONPATH === undefined && typeof process.env.PYTHONPATH === 'string') { - env.PYTHONPATH = process.env.PYTHONPATH; + if (env.PYTHONPATH === undefined && typeof normalizedProcessEnv.PYTHONPATH === 'string') { + env.PYTHONPATH = normalizedProcessEnv.PYTHONPATH; } } + envParser.normalizeSearchPathEnvironmentVariable(env); + if (!env.hasOwnProperty('PYTHONIOENCODING')) { env.PYTHONIOENCODING = 'UTF-8'; } diff --git a/src/test/unittest/configuration/resolvers/helper.unit.test.ts b/src/test/unittest/configuration/resolvers/helper.unit.test.ts index c2d6d483..92ff422d 100644 --- a/src/test/unittest/configuration/resolvers/helper.unit.test.ts +++ b/src/test/unittest/configuration/resolvers/helper.unit.test.ts @@ -4,12 +4,15 @@ 'use strict'; import { expect } from 'chai'; +import * as path from 'path'; import * as sinon from 'sinon'; import * as typemoq from 'typemoq'; import { TextDocument, TextEditor } from 'vscode'; import { PYTHON_LANGUAGE } from '../../../../extension/common/constants'; +import * as platform from '../../../../extension/common/platform'; import * as vscodeapi from '../../../../extension/common/vscodeapi'; -import { getProgram } from '../../../../extension/debugger/configuration/resolvers/helper'; +import { getDebugEnvironmentVariables, getProgram } from '../../../../extension/debugger/configuration/resolvers/helper'; +import { LaunchRequestArguments } from '../../../../extension/types'; suite('Debugging - Helpers', () => { let getActiveTextEditorStub: sinon.SinonStub; @@ -67,4 +70,48 @@ suite('Debugging - Helpers', () => { expect(program).to.be.equal(undefined, 'Not undefined'); }); + + test('Debug env vars should normalize duplicate Windows path keys', async () => { + sinon.stub(platform, 'getOSType').returns(platform.OSType.Windows); + + /* eslint-disable @typescript-eslint/naming-convention */ + const env = await getDebugEnvironmentVariables({ + type: 'debugpy', + name: 'Launch', + request: 'launch', + env: { + Path: 'C:\\tool-bin', + PATH: 'C:\\user-bin', + }, + } as LaunchRequestArguments); + /* eslint-enable @typescript-eslint/naming-convention */ + + expect(env.Path).to.include(`C:\\tool-bin${path.delimiter}C:\\user-bin`); + expect(env).to.not.have.property('PATH'); + }); + + test('Debug env vars should normalize Windows path keys after merging process env', async () => { + sinon.stub(platform, 'getOSType').returns(platform.OSType.Windows); + /* eslint-disable @typescript-eslint/naming-convention */ + const processEnvStub = sinon.stub(process, 'env').value({ + Path: 'C:\\system-bin', + PATH: 'C:\\legacy-system-bin', + }); + + const env = await getDebugEnvironmentVariables({ + type: 'debugpy', + name: 'Launch', + request: 'launch', + console: 'internalConsole', + env: { + Path: 'C:\\tool-bin', + }, + } as LaunchRequestArguments); + /* eslint-enable @typescript-eslint/naming-convention */ + + expect(processEnvStub).to.not.equal(undefined); + expect(env.Path).to.include(`C:\\tool-bin${path.delimiter}C:\\system-bin`); + expect(env.Path).to.include('C:\\legacy-system-bin'); + expect(env).to.not.have.property('PATH'); + }); }); diff --git a/src/test/unittest/noConfigDebugInit.unit.test.ts b/src/test/unittest/noConfigDebugInit.unit.test.ts index 2e9a7ca9..9e61f5ed 100644 --- a/src/test/unittest/noConfigDebugInit.unit.test.ts +++ b/src/test/unittest/noConfigDebugInit.unit.test.ts @@ -12,6 +12,7 @@ import { assert } from 'console'; import * as fs from 'fs'; import * as os from 'os'; import * as crypto from 'crypto'; +import { EXTENSION_ROOT_DIR_FOR_TESTS } from '../constants'; suite('setup for no-config debug scenario', function () { let envVarCollectionReplaceStub: sinon.SinonStub; @@ -225,6 +226,29 @@ suite('setup for no-config debug scenario', function () { fsExistsSyncStub.restore(); fsUnlinkSyncStub.restore(); }); + + test('should ship quoted no-config launcher scripts', () => { + const scriptDir = path.join(EXTENSION_ROOT_DIR_FOR_TESTS, 'bundled', 'scripts', 'noConfigScripts'); + const bashScript = fs.readFileSync(path.join(scriptDir, 'debugpy'), 'utf8'); + const shScript = fs.readFileSync(path.join(scriptDir, 'debugpy.sh'), 'utf8'); + const fishScript = fs.readFileSync(path.join(scriptDir, 'debugpy.fish'), 'utf8'); + const batScript = fs.readFileSync(path.join(scriptDir, 'debugpy.bat'), 'utf8'); + const ps1Script = fs.readFileSync(path.join(scriptDir, 'debugpy.ps1'), 'utf8'); + + assert(bashScript.includes('export DEBUGPY_ADAPTER_ENDPOINTS="$VSCODE_DEBUGPY_ADAPTER_ENDPOINTS"')); + assert(bashScript.includes('python3 "$BUNDLED_DEBUGPY_PATH" --listen 0 --wait-for-client "$@"')); + assert(!bashScript.includes('\r\n')); + assert(shScript.includes('export DEBUGPY_ADAPTER_ENDPOINTS="$VSCODE_DEBUGPY_ADAPTER_ENDPOINTS"')); + assert(shScript.includes('python3 "$BUNDLED_DEBUGPY_PATH" --listen 0 --wait-for-client "$@"')); + assert(!shScript.includes('\r\n')); + assert(fishScript.includes('set -x DEBUGPY_ADAPTER_ENDPOINTS "$VSCODE_DEBUGPY_ADAPTER_ENDPOINTS"')); + assert(fishScript.includes('python3 "$BUNDLED_DEBUGPY_PATH" --listen 0 --wait-for-client $argv')); + assert(!fishScript.includes('\r\n')); + assert(batScript.includes('set "DEBUGPY_ADAPTER_ENDPOINTS=%VSCODE_DEBUGPY_ADAPTER_ENDPOINTS%"')); + assert(batScript.includes('python "%BUNDLED_DEBUGPY_PATH%" --listen 0 --wait-for-client %*')); + assert(ps1Script.includes('python "$env:BUNDLED_DEBUGPY_PATH" --listen 0 --wait-for-client $args')); + assert(ps1Script.includes('python3 "$env:BUNDLED_DEBUGPY_PATH" --listen 0 --wait-for-client $args')); + }); }); function setupFileSystemWatchers(): sinon.SinonStub {