| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
xclear-cast please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information. @microsoft-github-policy-service agree [company="{your company}"]
Contributor License Agreement Contribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
Sorry, something went wrong.
| 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:
[verified]
Sorry, something went wrong.
| 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]
Sorry, something went wrong.
|
|
||
| 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:
[verified]
Sorry, something went wrong.
|
|
||
| '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]
Sorry, something went wrong.
| 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]
Sorry, something went wrong.
|
|
||
| // "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]
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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.