fix(debug): normalize Windows PATH env keys#1029
Conversation
|
@xclear-cast please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
| name: 'Python Debug Test', | ||
| type: 'debugpy', | ||
| request: 'launch', | ||
| env: launchEnv, |
There was a problem hiding this comment.
Copilot generated:
-95
Test fails on non-Windows CI (Skeptic, structurally confirmed). sinon.stub(platform, 'getOSType').returns(platform.OSType.Windows) makes getSearchPathEnvVarNames() return ['Path', 'PATH'], but path.delimiter remains : on Linux/macOS — it's a Node.js runtime constant, not controlled by the stub. The production code in normalizeSearchPathEnvVar uses path.delimiter to split/join segments, so on Linux env.Path will contain : delimiters. The assertions then split by hardcoded ';', which won't match.
Fix options:
- Use
path.delimiterin assertions instead of hardcoded';' - Also stub/mock
path.delimiterto';' - Skip the test on non-Windows:
if (process.platform !== 'win32') return
[verified]
| name: 'Python Debug Test', | ||
| type: 'debugpy', | ||
| request: 'launch', | ||
| env: launchEnv, |
There was a problem hiding this comment.
Copilot generated:
-91
Partial assertions violate test precision (Rules). .to.contain(...) would still pass if env.Path contained extra duplicates, wrong delimiters, or unrelated segments — exactly the regression class this test should catch:
expect(env.Path).to.contain('C:\\Project\\.venv\\Scripts');
expect(env.Path).to.contain('C:\\Tools');Fix: Capture the exact runtime value of env.Path (via debugger or a passing run) and assert with .to.equal('<exact-full-string>'). This ensures the full merged, deduplicated, correctly-delimited value is validated.
[verified]
|
|
||
| expect(program).to.be.equal(undefined, 'Not undefined'); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Copilot generated:
-102
Missing edge case coverage (Skeptic + Architect). The test only covers the internalConsole code path (which triggers mergeVariables(process.env, env)). Consider adding:
- A non-
internalConsolemode test to verify the earlier normalization calls work independently. - A test confirming Linux/macOS (
PATH-only, single key) is unaffected by normalization. - [unverified] On Windows Node.js,
process.envis case-insensitive, soprocess.env.Path = 'X'; process.env.PATH = 'Y'may alias to the same entry. The test's process.env setup may not create the two-key scenario it appears to — theargs.envplain-object path still exercises the core logic, but the process.env interaction is less realistic than intended.
[verified]
|
|
||
| 'use strict'; | ||
|
|
||
| import * as path from 'path'; |
There was a problem hiding this comment.
Copilot generated:
,12
Readonly parameters (Rules). Both new helpers accept names: ('Path' | 'PATH')[] but never mutate it. vars in getSearchPathEnvVar is only read (.map, .find). Per the repo's immutability-by-default convention:
function getSearchPathEnvVar(vars: Readonly<EnvironmentVariables>, names: readonly ('Path' | 'PATH')[]): ...
function normalizeSearchPathEnvVar(env: EnvironmentVariables, names: readonly ('Path' | 'PATH')[]) { ...(env is intentionally mutable since the function modifies it.)
[verified]
| const currentSegments = new Set(currentValue.split(path.delimiter).filter((item) => item.length > 0)); | ||
| const alternateSegments = alternateValue | ||
| .split(path.delimiter) | ||
| .filter((item) => item.length > 0 && !currentSegments.has(item)); |
There was a problem hiding this comment.
Copilot generated:
-35
Case-sensitive path segment deduplication on a case-insensitive filesystem (Skeptic, structurally confirmed; Architect concurs). The Set.has() check uses === semantics. On Windows, C:\Windows\System32 and c:\windows\system32 refer to the same directory but would survive deduplication.
The Advocate correctly notes this matches pre-existing behavior in appendPath/appendPaths. However, the PR description claims "duplicate path entries are avoided" — case-insensitive duplicates are not. Consider normalizing with .toLowerCase() for the lookup Set while preserving original casing in the output, or document this as a known limitation.
[verified]
|
|
||
| // "overwrite: true" to ensure that debug-configuration env variable values | ||
| // take precedence over env file. | ||
| envParser.mergeVariables(debugLaunchEnvVars, env, { overwrite: true }); |
There was a problem hiding this comment.
Copilot generated:
-100
Root cause assessment — symptom treatment, not root fix (Architect + Skeptic). Five normalizeSearchPathEnvVar calls are needed because mergeVariables in environment.ts only excludes getSearchPathEnvVarNames()[0] (i.e., Path), letting PATH through on every merge. The normalization is idempotent and correct (Advocate), but fragile — any future merge/append without a following normalize silently regresses the bug (Architect).
Recommended follow-up: fix environment.ts line ~55 to settingsNotToMerge = ['PYTHONPATH', ...getSearchPathEnvVarNames()]. This one-line change would eliminate most normalize calls and protect all consumers of mergeVariables, not just the debugger. If not addressed in this PR, add a TODO comment explaining why each normalize call exists.
[verified]
Summary
Fixes #261.
Validation
pm ci --ignore-scripts to install JS dependencies after plain
pm ci hit a local native build-tools blocker in @vscode/windows-process-tree (MSB8040, missing Spectre-mitigated VC libraries)
pm run compile-tests
pm run lint
pm run format-check
pm run compile
The focused VS Code-hosted unit test passed: 1 passing.