Skip to content

Commit 3df9d97

Browse files
wenytang-msCopilot
andauthored
fix: always prepend separator when appending noConfigScripts to PATH (#1637) (#1641)
* fix: always prepend separator when appending noConfigScripts to PATH EnvironmentVariableCollection.append() performs a literal string concatenation and does not insert a path separator. The previous heuristic checked process.env.PATH on the extension host and skipped the separator when that PATH already ended with one. However, the PATH used by the integrated terminal can differ from the extension host's PATH, so this check could incorrectly drop the separator and glue the noConfigScripts directory onto the last entry of the user's PATH (e.g. C:\Program Files\jreleaser\c:\Users\...\noConfigScripts). Always prepend the separator. A trailing empty PATH entry (if the user's PATH already ended with one) is harmless on both Windows and POSIX shells. Fixes #1637 * test: add unit tests for noConfigScripts PATH append helper Extract the PATH append-value computation into a vscode-free src/pathUtil.ts module so it can be unit-tested in plain Node mocha. Add test/pathUtil.test.ts covering: - Correct separator on Windows (;), Linux (:), and macOS (:). - The appended value always starts with a path separator (regression guard for #1637). - The scripts directory remains a standalone PATH entry when the user's PATH last entry has no trailing separator (the exact scenario reported in #1637, e.g. C:\Program Files\jreleaser\\). - A trailing separator on the user's PATH only produces a harmless empty entry, never glues another entry onto our scripts dir. - The scripts directory is preserved verbatim at the end of the value. All 9 tests pass under plain mocha; the file is also picked up by the existing Electron-based test runner via the **/**.test.js glob. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * style: trim verbose comments in pathUtil and tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 0db5f15 commit 3df9d97

3 files changed

Lines changed: 86 additions & 8 deletions

File tree

src/noConfigDebugInit.ts

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import * as vscode from 'vscode';
88

99
import { sendInfo, sendError } from "vscode-extension-telemetry-wrapper";
1010
import { getJavaHome } from "./utility";
11+
import { buildNoConfigPathAppendValue } from "./pathUtil";
1112

1213
/**
1314
* Registers the configuration-less debugging setup for the extension.
@@ -91,14 +92,7 @@ export async function registerNoConfigDebug(
9192
}
9293

9394
const noConfigScriptsDir = path.join(extPath, 'bundled', 'scripts', 'noConfigScripts');
94-
const pathSeparator = process.platform === 'win32' ? ';' : ':';
95-
96-
// Check if the current PATH already ends with a path separator to avoid double separators
97-
const currentPath = process.env.PATH || '';
98-
const needsSeparator = currentPath.length > 0 && !currentPath.endsWith(pathSeparator);
99-
const pathValueToAppend = needsSeparator ? `${pathSeparator}${noConfigScriptsDir}` : noConfigScriptsDir;
100-
101-
collection.append('PATH', pathValueToAppend);
95+
collection.append('PATH', buildNoConfigPathAppendValue(noConfigScriptsDir));
10296

10397
// create file system watcher for the debuggerAdapterEndpointFolder for when the communication port is written
10498
const fileSystemWatcher = vscode.workspace.createFileSystemWatcher(

src/pathUtil.ts

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
// Copyright (c) Microsoft Corporation. All rights reserved.
2+
// Licensed under the MIT license.
3+
4+
/**
5+
* Builds the value to append to PATH for the noConfigScripts directory.
6+
*
7+
* `vscode.EnvironmentVariableCollection.append()` does literal string
8+
* concatenation, so we always prepend a separator to avoid gluing our
9+
* directory onto the last entry of the user's PATH (see issue #1637).
10+
*
11+
* @param platform defaults to `process.platform`; injectable for tests.
12+
*/
13+
export function buildNoConfigPathAppendValue(
14+
scriptsDir: string,
15+
platform: NodeJS.Platform = process.platform,
16+
): string {
17+
const pathSeparator = platform === 'win32' ? ';' : ':';
18+
return `${pathSeparator}${scriptsDir}`;
19+
}

test/pathUtil.test.ts

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
// Copyright (c) Microsoft Corporation. All rights reserved.
2+
// Licensed under the MIT license.
3+
4+
import * as assert from "assert";
5+
6+
import { buildNoConfigPathAppendValue } from "../src/pathUtil";
7+
8+
// Regression tests for issue #1637.
9+
suite("buildNoConfigPathAppendValue", () => {
10+
11+
const winDir = "C:\\Users\\me\\.vscode\\extensions\\vscjava.vscode-java-debug-0.59.0\\bundled\\scripts\\noConfigScripts";
12+
const posixDir = "/home/me/.vscode/extensions/vscjava.vscode-java-debug-0.59.0/bundled/scripts/noConfigScripts";
13+
14+
test("uses ';' as separator on Windows", () => {
15+
const result = buildNoConfigPathAppendValue(winDir, "win32");
16+
assert.strictEqual(result, `;${winDir}`);
17+
});
18+
19+
test("uses ':' as separator on Linux", () => {
20+
const result = buildNoConfigPathAppendValue(posixDir, "linux");
21+
assert.strictEqual(result, `:${posixDir}`);
22+
});
23+
24+
test("uses ':' as separator on macOS", () => {
25+
const result = buildNoConfigPathAppendValue(posixDir, "darwin");
26+
assert.strictEqual(result, `:${posixDir}`);
27+
});
28+
29+
test("always starts with a path separator (Windows)", () => {
30+
const result = buildNoConfigPathAppendValue(winDir, "win32");
31+
assert.ok(result.startsWith(";"));
32+
});
33+
34+
test("always starts with a path separator (POSIX)", () => {
35+
const result = buildNoConfigPathAppendValue(posixDir, "linux");
36+
assert.ok(result.startsWith(":"));
37+
});
38+
39+
test("never collapses scriptsDir into the previous PATH entry on Windows", () => {
40+
// #1637 scenario: last user PATH entry has no trailing separator.
41+
const userPath = "C:\\foo;C:\\Program Files\\jreleaser\\";
42+
const entries = (userPath + buildNoConfigPathAppendValue(winDir, "win32")).split(";");
43+
assert.ok(entries.includes("C:\\Program Files\\jreleaser\\"));
44+
assert.ok(entries.includes(winDir));
45+
});
46+
47+
test("never collapses scriptsDir into the previous PATH entry on POSIX", () => {
48+
const userPath = "/usr/bin:/opt/jreleaser/bin";
49+
const entries = (userPath + buildNoConfigPathAppendValue(posixDir, "linux")).split(":");
50+
assert.ok(entries.includes("/opt/jreleaser/bin"));
51+
assert.ok(entries.includes(posixDir));
52+
});
53+
54+
test("yields only an empty (harmless) entry when the user's PATH already ends with a separator", () => {
55+
const userPath = "C:\\foo;C:\\bar;";
56+
const entries = (userPath + buildNoConfigPathAppendValue(winDir, "win32")).split(";");
57+
assert.ok(entries.includes(winDir));
58+
assert.ok(!entries.some((e) => e !== winDir && e.endsWith(winDir)));
59+
});
60+
61+
test("scriptsDir appears unchanged at the end of the appended value", () => {
62+
const result = buildNoConfigPathAppendValue(winDir, "win32");
63+
assert.ok(result.endsWith(winDir));
64+
});
65+
});

0 commit comments

Comments
 (0)