Fix/305740 subprocess wait by rupeshkumar-555 · Pull Request #308681 · microsoft/vscode

rupeshkumar-555 · 2026-04-09T06:25:14Z

Summary\nFixes #305740 by removing synchronous subprocess-based node detection in TypeScript configuration and resolving node executable via PATH/PATHEXT with platform-aware handling.\n\n## Why\nThe sync subprocess approach can block and cause delays/hangs in some environments.\n\n## What changed\n- Updated node path resolution in configuration.electron.ts\n- Added focused unit tests in configuration.electron.test.ts for Windows/Unix and fallback behavior\n\n## Scope\nThis PR now contains only the #305740 fix changes.

Copilot

Pull request overview

This PR combines a fix for TS Server Node detection (avoiding a subprocess exec), enhancements to chat inline-reference rendering (supporting embedded code snippets), and the introduction of a new MCP server provider extension (ToolPipe), along with a few unrelated CI/CSS/config example updates.

Changes:

Replace execFileSync('node', ...)-based Node discovery in the TypeScript extension with PATH-based resolution and add unit tests.
Extend chat inline references to optionally append a fenced code block snippet (with backtick-escaping) and add coverage for snippet edge cases.
Add a new built-in toolpipe-mcp-server extension (plus MCP schema examples), and re-enable the Sessions E2E workflow trigger.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
src/vs/workbench/contrib/terminal/browser/media/xterm.css	Use theme variables for IME composition view background instead of hardcoded black.
src/vs/workbench/contrib/mcp/common/mcpConfiguration.ts	Add ToolPipe-related MCP server examples to the schema examples.
src/vs/workbench/contrib/chat/common/widget/annotations.ts	Append inline-reference snippets as fenced code blocks; infer language from label.
src/vs/workbench/contrib/chat/common/chatService/chatService.ts	Extend inline reference model with optional `snippet` and `languageId`.
src/vs/workbench/contrib/chat/test/common/widget/annotations.test.ts	Add tests for snippet rendering and fence escaping behavior.
extensions/typescript-language-features/src/configuration/configuration.electron.ts	Replace subprocess-based Node detection with PATH-based resolver.
extensions/typescript-language-features/src/test/unit/configuration.electron.test.ts	Add unit tests for PATH-based Node resolution.
extensions/toolpipe-mcp-server/tsconfig.json	New extension TS config including MCP proposed d.ts.
extensions/toolpipe-mcp-server/src/extension.ts	New MCP server definition provider extension implementation.
extensions/toolpipe-mcp-server/README.md	New extension documentation and configuration instructions.
extensions/toolpipe-mcp-server/package.nls.json	New extension localization strings for manifest/configuration.
extensions/toolpipe-mcp-server/package.json	New extension manifest contributing MCP provider and settings.
build/gulpfile.extensions.ts	Add ToolPipe extension tsconfig to the extensions compilation list.
.github/workflows/sessions-e2e.yml	Re-enable workflow trigger for Sessions E2E tests on PRs.

Copilot · 2026-04-09T06:32:50Z

+				let markdownText = `[${label}](${printUri.toString()})`;
+
+				// If the inline reference includes a code snippet, append it as a code block
+				if (item.snippet) {


item.snippet is checked with a truthiness condition, so an explicitly provided empty snippet ('') will be ignored and no code fence will be appended. This contradicts the newly added test case for empty snippets and will cause it to fail. Consider checking item.snippet !== undefined (or 'snippet' in item) instead of truthiness so empty snippets still render.

Suggested change

if (item.snippet) {

if (item.snippet !== undefined) {

Copilot · 2026-04-09T06:32:51Z

+		'psm1': 'powershell',
+		'lua': 'lua',
+		'r': 'r',
+		'R': 'r',


ext is lowercased before indexing into languageMap, so the 'R' entry is unreachable/dead code. It should be removed (or the lowercasing logic adjusted) to avoid confusion about supported extensions.

Suggested change

'R': 'r',

Copilot · 2026-04-09T06:32:51Z

+	if (!pathValue) {
+		return null;
+	}
+
+	const searchPaths = pathValue.split(pathLib.delimiter).filter(Boolean);
+	const windowsExecutableSuffixes = platform === 'win32'
+		? (getCaseInsensitiveEnvValue(env, 'PATHEXT') || '.COM;.EXE;.BAT;.CMD').split(';').filter(Boolean)
+		: [];
+
+	for (const pathEntry of searchPaths) {
+		const baseDir = pathLib.isAbsolute(pathEntry) ? pathEntry : pathLib.join(cwd, pathEntry);
+
+		if (platform === 'win32') {
+			for (const ext of windowsExecutableSuffixes) {
+				const candidate = pathLib.join(baseDir, `node${ext}`);
+				if (isExecutableFile(candidate)) {
+					return candidate;
+				}
+			}
+		}
+
+		const candidate = pathLib.join(baseDir, 'node');
+		if (isExecutableFile(candidate)) {
+			return candidate;
+		}
+	}
+


resolveNodeExecutableFromPath returns null immediately when PATH is missing, and it never checks the cwd directly. This differs from common command resolution behavior (and VS Code's findExecutable, which falls back to checking cwd), and may regress scenarios where node is resolvable without PATH. Consider adding a cwd fallback candidate (including PATHEXT handling on Windows) before returning null.

Suggested change

if (!pathValue) {

return null;

}

const searchPaths = pathValue.split(pathLib.delimiter).filter(Boolean);

const windowsExecutableSuffixes = platform === 'win32'

? (getCaseInsensitiveEnvValue(env, 'PATHEXT') || '.COM;.EXE;.BAT;.CMD').split(';').filter(Boolean)

: [];

for (const pathEntry of searchPaths) {

const baseDir = pathLib.isAbsolute(pathEntry) ? pathEntry : pathLib.join(cwd, pathEntry);

if (platform === 'win32') {

for (const ext of windowsExecutableSuffixes) {

const candidate = pathLib.join(baseDir, `node${ext}`);

if (isExecutableFile(candidate)) {

return candidate;

}

}

}

const candidate = pathLib.join(baseDir, 'node');

if (isExecutableFile(candidate)) {

return candidate;

}

}

const windowsExecutableSuffixes = platform === 'win32'

? (getCaseInsensitiveEnvValue(env, 'PATHEXT') || '.COM;.EXE;.BAT;.CMD').split(';').filter(Boolean)

: [];

if (pathValue) {

const searchPaths = pathValue.split(pathLib.delimiter).filter(Boolean);

for (const pathEntry of searchPaths) {

const baseDir = pathLib.isAbsolute(pathEntry) ? pathEntry : pathLib.join(cwd, pathEntry);

if (platform === 'win32') {

for (const ext of windowsExecutableSuffixes) {

const candidate = pathLib.join(baseDir, `node${ext}`);

if (isExecutableFile(candidate)) {

return candidate;

}

}

}

const candidate = pathLib.join(baseDir, 'node');

if (isExecutableFile(candidate)) {

return candidate;

}

}

}

if (platform === 'win32') {

for (const ext of windowsExecutableSuffixes) {

const candidate = pathLib.join(cwd, `node${ext}`);

if (isExecutableFile(candidate)) {

return candidate;

}

}

}

const candidate = pathLib.join(cwd, 'node');

if (isExecutableFile(candidate)) {

return candidate;

}

Copilot · 2026-04-09T06:32:51Z

+  "main": "./out/extension.js",
+  "activationEvents": [
+    "onStartupFinished"
+  ],
+  "contributes": {
+    "mcpServerDefinitionProviders": [
+      {
+        "id": "toolpipe",
+        "label": "%mcpServerDefinition.label%"
+      }
+    ],


This extension calls the proposed vscode.lm.registerMcpServerDefinitionProvider API (types come from vscode.proposed.mcpServerDefinitions.d.ts), but package.json does not declare the corresponding enabledApiProposals entry. Without it, activation will throw due to proposed API gating. Add "enabledApiProposals": ["mcpServerDefinitions"] to the manifest.

Copilot · 2026-04-09T06:32:52Z

+  },
+  "main": "./out/extension.js",
+  "activationEvents": [
+    "onStartupFinished"


activationEvents is set to onStartupFinished, which eagerly activates the extension even when MCP isn’t used. Since the mcpServerDefinitionProviders extension point generates onMcpCollection:<id> activation events (here: onMcpCollection:toolpipe), consider switching to that activation event to reduce startup impact and align with the MCP contribution model.

Suggested change

"onStartupFinished"

"onMcpCollection:toolpipe"

Copilot · 2026-04-09T06:32:52Z

+
+## License
+
+MIT - See LICENSE file for details


The README states "MIT - See LICENSE file for details", but no LICENSE file is added under this extension directory in the PR. Either add the referenced LICENSE file or adjust the text to point to the repository root license to avoid a broken reference.

Suggested change

MIT - See LICENSE file for details

MIT - See the repository root LICENSE file for details.

Copilot · 2026-04-09T06:32:53Z

+on:
+  pull_request:
+    branches:
+      - main
+      - 'release/*'


This change re-enables the workflow trigger on pull_request. Please confirm this is intentional for this PR/issue, since it will start running Sessions E2E on all PRs touching the listed paths and can significantly increase CI load and PR turnaround time.

Copilot · 2026-04-09T06:32:53Z

 	'extensions/search-result/tsconfig.json',
 	'extensions/simple-browser/tsconfig.json',
+	'extensions/toolpipe-mcp-server/tsconfig.json',
 	'extensions/tunnel-forwarding/tsconfig.json',
 	'extensions/typescript-language-features/web/tsconfig.json',


This PR adds a new built-in extension (toolpipe-mcp-server) to the extension compilation set, which seems unrelated to the stated goal of fixing a subprocess wait issue. If this is intentional, consider splitting into a dedicated PR to keep review/rollback risk contained and to make the bugfix easier to track.

Copilot · 2026-04-09T06:32:53Z

+  "scripts": {
+    "vscode:prepublish": "npm run compile",
+    "compile": "tsc -p ./",
+    "watch": "tsc -w -p ./",
+    "pretest": "npm run compile && npm run lint",
+    "test": "vscode-test",
+    "lint": "eslint src --ext ts"
+  },


The scripts.test command is vscode-test, but the extension doesn’t declare a dependency/devDependency on @vscode/test-electron / @vscode/test-web (or a vscode-test binary). As written, running the extension’s tests from this folder would fail. Consider either wiring this up to the repo’s existing extension test infrastructure (gulp tasks) or adding the correct devDependency and script.

Copilot · 2026-04-09T06:32:53Z

+			definitions.push(
+				new vscode.McpHttpServerDefinition(
+					'ToolPipe Developer Tools',
+					vscode.Uri.parse(remoteUrl)
+				)
+			);
+		}
+	} else if (mode === 'local') {
+		const command = config.get<string>('localCommand', 'npx');
+		const args = config.get<string[]>('localArgs', ['@cosai-labs/toolpipe-mcp-server']);
+
+		definitions.push(
+			new vscode.McpStdioServerDefinition(
+				'ToolPipe Developer Tools (Local)',
+				command,
+				args
+			)


The MCP server labels passed into McpHttpServerDefinition / McpStdioServerDefinition (e.g. "ToolPipe Developer Tools") are likely user-visible. In VS Code extensions, user-facing strings should be localized via vscode.l10n.t(...) (and corresponding package.nls.json entries if needed) rather than hard-coded literals.

TylerLeonhardt · 2026-04-09T10:24:00Z

Please provide context as to why you opened this PR

…lution - Add resolveNodeExecutableFromPath() to resolve node via PATH/PATHEXT - Remove execFileSync usage from node path detection - Add helper functions for cross-platform support (Windows PATHEXT, Unix PATH) - Add comprehensive unit tests for all platforms (Windows, Unix, fallback) - Maintain existing warning behavior when Node cannot be detected - Handles cross-platform differences in PATH environment variables Fixes: microsoft#305740

rupeshkumar-555 · 2026-04-17T01:49:58Z

Thanks for the review request. Context for this PR: issue #305740 reports that TS node path detection can block on a synchronous subprocess call. This change removes the sync subprocess path and resolves node via PATH/PATHEXT with cross-platform handling, plus focused unit tests for resolver behavior. I have now force-pushed the branch to contain only the #305740 fix files to keep the scope narrow. Please re-review the updated diff.

- Use single lstatSync (instead of existsSync + lstatSync) in defaultIsExecutableFile - Add X_OK executable permission check on non-Windows - Add cwd fallback in resolveNodeExecutableFromPath when PATH is missing or exhausted - Add unit tests for cwd fallback on Linux and win32 platforms

rupeshkumar-555 · 2026-04-17T18:02:41Z

Applied the Copilot review feedback in the latest commit:

**\defaultIsExecutableFile**: replaced \existsSync\ + \lstatSync\ with a single \lstatSync\ call, and added an \s.accessSync(candidate, fs.constants.X_OK)\ check on non-Windows so only truly executable files are matched.
**
esolveNodeExecutableFromPath**: added a \cwd\ fallback (including PATHEXT handling on win32) that runs after the PATH-based search exhausts or when PATH is missing, aligning with VS Code\u2019s \indExecutable\ behaviour.
Tests: added three new unit tests covering the cwd fallback on Linux, win32 (with PATHEXT), and the null-return path when neither PATH nor cwd contains node.

The PR now only touches \configuration.electron.ts\ and \configuration.electron.test.ts\ \u2014 fully scoped to issue #305740.

Copilot AI review requested due to automatic review settings April 9, 2026 06:25

Copilot started reviewing on behalf of rupeshkumar-555 April 9, 2026 06:26 View session

Copilot AI reviewed Apr 9, 2026

View reviewed changes

vs-code-engineering bot added the triage-needed label Apr 9, 2026

vs-code-engineering bot assigned TylerLeonhardt Apr 9, 2026

austindyoung mentioned this pull request Apr 9, 2026

[mirror] microsoft/vscode#308681 Fix/305740 subprocess wait austindyoung/vscode-driftfence-mirror-fork#1393

Open

TylerLeonhardt added info-needed Issue requires more information from poster and removed triage-needed labels Apr 9, 2026

rupeshkumar-555 force-pushed the fix/305740-subprocess-wait branch 4 times, most recently from 20791a1 to 4944d47 Compare April 15, 2026 05:13

rupeshkumar-555 force-pushed the fix/305740-subprocess-wait branch from eb2a000 to 1bb15c6 Compare April 17, 2026 01:49

rupeshkumar-555 and others added 4 commits April 17, 2026 10:48

Merge branch 'main' into fix/305740-subprocess-wait

a656866

Merge branch 'main' into fix/305740-subprocess-wait

e72e425

Merge branch 'main' into fix/305740-subprocess-wait

85bd542

	MIT - See LICENSE file for details
	MIT - See the repository root LICENSE file for details.

Conversation

rupeshkumar-555 commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

TylerLeonhardt commented Apr 9, 2026

Uh oh!

rupeshkumar-555 commented Apr 17, 2026

Uh oh!

rupeshkumar-555 commented Apr 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rupeshkumar-555 commented Apr 9, 2026 •

edited

Loading