From 52b539ec978f844508b709759d2aca3de8c0d390 Mon Sep 17 00:00:00 2001 From: hongbailingmeng <3471972616@qq.com> Date: Fri, 23 Oct 2026 17:41:16 +0800 Subject: [PATCH] fix: resolve Windows command shims for stdio MCP servers On Windows, npm installs its command-line tools as batch shims (npx.cmd, npm.cmd) rather than PE executables. ProcessBuilder delegates to Win32 CreateProcess, which appends only .exe to an extension-less command and never consults PATHEXT. A stdio server configured with command("npx") therefore fails with CreateProcess error=2 even though the same command works when typed into a shell -- the shell does the PATHEXT lookup, and CreateProcess does not. That failure never reaches the caller. No subprocess is ever spawned, so the MCP client waits out its initialization timeout instead of reporting anything. Measured on Windows 11 with the mcpfilesystem sample: 900 s (3 attempts x 5-minute timeout) of silence before a McpToolLoadingException. DefaultMcpTransportBuilder now resolves the command against PATH and PATHEXT before handing it to StdioClientTransport, mirroring shutil.which as used by the Python MCP SDK: a command that already carries a PATHEXT extension is tried as-is, anything else is tried with each extension appended, in PATHEXT order. Non-Windows platforms, and commands that are already paths, are untouched. The lookup matches case-insensitively and returns the name as spelled on disk, so the result does not depend on the case sensitivity of the filesystem it runs on. Fixes #1686. --- .../tools/mcp/DefaultMcpTransportBuilder.java | 42 +++- .../tools/mcp/WindowsExecutableResolver.java | 183 ++++++++++++++++++ .../mcp/DefaultMcpTransportBuilderTest.java | 78 ++++++++ .../mcp/WindowsExecutableResolverTest.java | 171 ++++++++++++++++ 4 files changed, 473 insertions(+), 1 deletion(-) create mode 100644 core/src/main/java/com/google/adk/tools/mcp/WindowsExecutableResolver.java create mode 100644 core/src/test/java/com/google/adk/tools/mcp/WindowsExecutableResolverTest.java diff --git a/core/src/main/java/com/google/adk/tools/mcp/DefaultMcpTransportBuilder.java b/core/src/main/java/com/google/adk/tools/mcp/DefaultMcpTransportBuilder.java index e2fdfa7b8..e95f7b0e6 100644 --- a/core/src/main/java/com/google/adk/tools/mcp/DefaultMcpTransportBuilder.java +++ b/core/src/main/java/com/google/adk/tools/mcp/DefaultMcpTransportBuilder.java @@ -18,6 +18,7 @@ import static com.google.common.base.Strings.isNullOrEmpty; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableMap; import io.modelcontextprotocol.client.transport.HttpClientSseClientTransport; import io.modelcontextprotocol.client.transport.HttpClientStreamableHttpTransport; @@ -36,6 +37,10 @@ * The default builder for creating MCP client transports. Supports StdioClientTransport based on * {@link ServerParameters}, HttpClientSseClientTransport based on {@link SseServerParameters}, and * HttpClientStreamableHttpTransport based on {@link StreamableHttpServerParameters}. + * + *

For stdio servers, the command is resolved against {@code PATH} first on Windows, so npm shims + * such as {@code npx} reach {@code ProcessBuilder} as the {@code .cmd} file they really are. See + * {@link WindowsExecutableResolver}. */ public class DefaultMcpTransportBuilder implements McpTransportBuilder { @@ -44,7 +49,7 @@ public class DefaultMcpTransportBuilder implements McpTransportBuilder { @Override public McpClientTransport build(Object connectionParams) { if (connectionParams instanceof ServerParameters serverParameters) { - return new StdioClientTransport(serverParameters, jsonMapper); + return new StdioClientTransport(resolveCommand(serverParameters), jsonMapper); } else if (connectionParams instanceof SseServerParameters sseServerParams) { return HttpClientSseClientTransport.builder(sseServerParams.url()) .sseEndpoint( @@ -89,6 +94,41 @@ public McpClientTransport build(Object connectionParams) { } } + /** + * Resolves a Windows command shim before the stdio transport hands the command to {@code + * ProcessBuilder}. + * + *

Without this, {@code command("npx")} never starts a process on Windows: the transport's + * error is swallowed by the initialization timeout, so the toolset appears to hang. See {@link + * WindowsExecutableResolver} and google/adk-java#1686. + * + *

Everywhere except Windows, and for commands that are already paths, the parameters are + * returned untouched. + */ + private static ServerParameters resolveCommand(ServerParameters serverParameters) { + String command = serverParameters.getCommand(); + String resolved = WindowsExecutableResolver.resolve(command); + if (resolved.equals(command)) { + return serverParameters; + } + return withCommand(serverParameters, resolved); + } + + /** + * Returns {@code serverParameters} with {@code command} substituted, keeping the caller's args + * and environment. + * + *

{@code ServerParameters} merges the default environment with the caller's, so round-tripping + * {@code getEnv()} reproduces the same effective environment. + */ + @VisibleForTesting + static ServerParameters withCommand(ServerParameters serverParameters, String command) { + return ServerParameters.builder(command) + .args(serverParameters.getArgs()) + .env(serverParameters.getEnv()) + .build(); + } + /** * Splits the URL into a base URI (scheme + authority) and endpoint (path + query + fragment). * Returns a null endpoint when the URL has no meaningful path or cannot be split, so the diff --git a/core/src/main/java/com/google/adk/tools/mcp/WindowsExecutableResolver.java b/core/src/main/java/com/google/adk/tools/mcp/WindowsExecutableResolver.java new file mode 100644 index 000000000..43c1e542b --- /dev/null +++ b/core/src/main/java/com/google/adk/tools/mcp/WindowsExecutableResolver.java @@ -0,0 +1,183 @@ +/* + * Copyright 2026 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.adk.tools.mcp; + +import static com.google.common.base.Strings.isNullOrEmpty; +import static java.util.Objects.requireNonNull; + +import com.google.common.collect.ImmutableList; +import java.io.File; +import java.util.List; +import java.util.Locale; +import org.jspecify.annotations.Nullable; + +/** + * Resolves a bare command name to the file Windows will actually execute. + * + *

On Windows, npm installs its command-line tools as batch shims ({@code npx.cmd}, {@code + * npm.cmd}) rather than PE executables. {@code ProcessBuilder} delegates to Win32 {@code + * CreateProcess}, which appends only {@code .exe} to an extension-less command and never consults + * {@code PATHEXT}. A stdio server configured with {@code command("npx")} therefore fails with + * {@code CreateProcess error=2} even though typing {@code npx} into a shell works — the shell does + * the {@code PATHEXT} lookup, and {@code CreateProcess} does not. + * + *

That failure is invisible to callers: no subprocess is ever spawned, so the MCP client waits + * out its initialization timeout instead of reporting the problem. Resolving the shim up front + * makes the subprocess start, which is what this class is for. See google/adk-java#1686. + * + *

The lookup mirrors {@code shutil.which} as used by the Python MCP SDK, so the Java transport + * behaves the same way on Windows as the other SDKs: a command that already ends with one of the + * {@code PATHEXT} extensions is tried as-is, and any other command is tried with each {@code + * PATHEXT} extension appended, in {@code PATHEXT} order. + */ +final class WindowsExecutableResolver { + + private static final String WINDOWS_OS_PREFIX = "windows"; + private static final String PATH_ENV = "PATH"; + private static final String PATH_EXT_ENV = "PATHEXT"; + private static final String PATH_SEPARATOR = ";"; + + /** + * Fallback for a Windows process whose {@code PATHEXT} is unset. Matches the value {@code + * shutil.which} uses. + */ + private static final String DEFAULT_PATH_EXT = ".COM;.EXE;.BAT;.CMD;.VBS;.JS;.WS;.MSC"; + + private WindowsExecutableResolver() {} + + /** + * Resolves {@code command} against the current process environment. + * + *

Returns {@code command} unchanged on non-Windows platforms, and also on Windows when no + * matching file is found — a resolution miss must not fail the build, because {@code + * ProcessBuilder} may still be able to start it (a relative path, or an entry on the current + * directory) and its own error message is the more useful one. + */ + static String resolve(String command) { + requireNonNull(command, "command"); + String resolved = + resolve( + command, + System.getProperty("os.name"), + System.getenv(PATH_ENV), + System.getenv(PATH_EXT_ENV)); + return resolved == null ? command : resolved; + } + + /** Test seam for {@link #resolve(String)}: lets the platform and lookup path be supplied. */ + static @Nullable String resolve( + @Nullable String command, + @Nullable String osName, + @Nullable String path, + @Nullable String pathExt) { + if (isNullOrEmpty(command) || !isWindows(osName) || hasPathSeparator(command)) { + return command; + } + String found = which(command, path, pathExt); + return found == null ? command : found; + } + + /** + * Returns the absolute path of the first {@code PATH} entry that contains {@code command}, or + * {@code null} when there is none. + * + *

Empty {@code PATH} entries are skipped rather than read as the current directory: resolving + * against the working directory would make the behaviour depend on where the caller happens to + * run, and a miss falls back to {@code ProcessBuilder} anyway. + */ + private static @Nullable String which( + String command, @Nullable String path, @Nullable String pathExt) { + if (isNullOrEmpty(path)) { + return null; + } + List fileNames = candidateFileNames(command, pathExt); + for (String directory : path.split(PATH_SEPARATOR)) { + if (directory.isEmpty()) { + continue; + } + File dir = new File(directory); + File[] entries = dir.listFiles(); + if (entries == null) { + continue; + } + for (String fileName : fileNames) { + File match = findEntry(entries, fileName); + if (match != null) { + return match.getAbsolutePath(); + } + } + } + return null; + } + + /** + * Finds {@code fileName} among {@code entries}, ignoring case. + * + *

The match is deliberately case-insensitive rather than left to {@link File#isFile()}: this + * class models Windows semantics, and the lookup has to give the same answer when it runs on a + * case-sensitive filesystem (tests, or a build host that is not the target). Matching against the + * directory listing also returns the name as it is spelled on disk, so the caller gets {@code + * npx.cmd} rather than the {@code npx.CMD} built from {@code PATHEXT}. + */ + private static @Nullable File findEntry(File[] entries, String fileName) { + for (File entry : entries) { + if (entry.getName().equalsIgnoreCase(fileName) && entry.isFile()) { + return entry; + } + } + return null; + } + + /** + * Returns the file names to look for, in the order {@code shutil.which} tries them: the command + * itself when it already carries a {@code PATHEXT} extension, otherwise the command with each + * extension appended. + */ + private static List candidateFileNames(String command, @Nullable String pathExt) { + List extensions = splitPathExt(pathExt); + String lowerCommand = command.toLowerCase(Locale.ROOT); + for (String extension : extensions) { + if (lowerCommand.endsWith(extension.toLowerCase(Locale.ROOT))) { + return ImmutableList.of(command); + } + } + ImmutableList.Builder fileNames = ImmutableList.builder(); + for (String extension : extensions) { + fileNames.add(command + extension); + } + return fileNames.build(); + } + + private static List splitPathExt(@Nullable String pathExt) { + String raw = isNullOrEmpty(pathExt) ? DEFAULT_PATH_EXT : pathExt; + ImmutableList.Builder extensions = ImmutableList.builder(); + for (String extension : raw.split(PATH_SEPARATOR)) { + if (!extension.isEmpty()) { + extensions.add(extension); + } + } + return extensions.build(); + } + + private static boolean isWindows(@Nullable String osName) { + return osName != null && osName.toLowerCase(Locale.ROOT).startsWith(WINDOWS_OS_PREFIX); + } + + private static boolean hasPathSeparator(String command) { + return command.indexOf('/') >= 0 || command.indexOf('\\') >= 0; + } +} diff --git a/core/src/test/java/com/google/adk/tools/mcp/DefaultMcpTransportBuilderTest.java b/core/src/test/java/com/google/adk/tools/mcp/DefaultMcpTransportBuilderTest.java index d2c263ac4..197b38b11 100644 --- a/core/src/test/java/com/google/adk/tools/mcp/DefaultMcpTransportBuilderTest.java +++ b/core/src/test/java/com/google/adk/tools/mcp/DefaultMcpTransportBuilderTest.java @@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertThrows; +import static org.junit.Assume.assumeTrue; import com.google.common.collect.ImmutableMap; import io.modelcontextprotocol.client.transport.HttpClientSseClientTransport; @@ -27,10 +28,12 @@ import io.modelcontextprotocol.client.transport.customizer.McpAsyncHttpClientRequestCustomizer; import io.modelcontextprotocol.common.McpTransportContext; import io.modelcontextprotocol.spec.McpClientTransport; +import java.io.File; import java.lang.reflect.Field; import java.net.URI; import java.net.http.HttpRequest; import java.util.HashMap; +import java.util.Locale; import java.util.Map; import org.junit.Test; import org.junit.runner.RunWith; @@ -52,6 +55,73 @@ public void build_withServerParameters_returnsStdioTransport() { assertThat(transport).isInstanceOf(StdioClientTransport.class); } + @Test + public void build_withServerParameters_preservesArgsAndEnv() throws Exception { + // Command resolution rebuilds ServerParameters; that rebuild must not drop the caller's args or + // environment, or the stdio server would start with the wrong argv. + ServerParameters params = + ServerParameters.builder("test-command") + .args("-y", "@modelcontextprotocol/server-filesystem", "/tmp") + .addEnvVar("CUSTOM_VAR", "custom-value") + .build(); + + StdioClientTransport transport = (StdioClientTransport) transportBuilder.build(params); + ServerParameters built = getServerParameters(transport); + + assertThat(built.getArgs()) + .containsExactly("-y", "@modelcontextprotocol/server-filesystem", "/tmp") + .inOrder(); + assertThat(built.getEnv()).containsEntry("CUSTOM_VAR", "custom-value"); + } + + @Test + public void withCommand_replacesCommandAndKeepsArgsAndEnv() { + // Deterministic on every platform: exercises the rebuild that resolveCommand performs on + // Windows, which the builder test below cannot reach on a non-Windows CI machine. + ServerParameters params = + ServerParameters.builder("npx") + .args("-y", "@modelcontextprotocol/server-filesystem", "/tmp") + .addEnvVar("CUSTOM_VAR", "custom-value") + .build(); + + ServerParameters rebuilt = + DefaultMcpTransportBuilder.withCommand(params, "C:\\nodejs\\npx.cmd"); + + assertThat(rebuilt.getCommand()).isEqualTo("C:\\nodejs\\npx.cmd"); + assertThat(rebuilt.getArgs()) + .containsExactly("-y", "@modelcontextprotocol/server-filesystem", "/tmp") + .inOrder(); + assertThat(rebuilt.getEnv()).containsEntry("CUSTOM_VAR", "custom-value"); + } + + @Test + public void build_withServerParameters_appliesCommandResolution() throws Exception { + // Regression test for google/adk-java#1686: the builder must hand the transport whatever the + // resolver produced, so a Windows npm shim reaches ProcessBuilder as a real file. + ServerParameters params = ServerParameters.builder("npx").build(); + + StdioClientTransport transport = (StdioClientTransport) transportBuilder.build(params); + + assertThat(getServerParameters(transport).getCommand()) + .isEqualTo(WindowsExecutableResolver.resolve("npx")); + } + + @Test + public void build_withServerParameters_onWindows_resolvesNpxShimToCmdFile() throws Exception { + assumeTrue( + "Windows-only: asserts that the npm shim is actually found on this machine", + System.getProperty("os.name", "").toLowerCase(Locale.ROOT).startsWith("windows")); + assumeTrue("Needs Node.js on PATH", WindowsExecutableResolver.resolve("npx").endsWith(".cmd")); + + ServerParameters params = ServerParameters.builder("npx").build(); + + StdioClientTransport transport = (StdioClientTransport) transportBuilder.build(params); + + String command = getServerParameters(transport).getCommand(); + assertThat(command.toLowerCase(Locale.ROOT)).endsWith("npx.cmd"); + assertThat(new File(command).isFile()).isTrue(); + } + @Test public void build_withSseServerParameters_returnsSseTransport() { SseServerParameters params = SseServerParameters.builder().url("http://localhost:1234").build(); @@ -251,6 +321,14 @@ private static URI getBaseUri(HttpClientStreamableHttpTransport transport) throw return (URI) field.get(transport); } + /** Reads back the parameters the transport will hand to {@code ProcessBuilder}. */ + private static ServerParameters getServerParameters(StdioClientTransport transport) + throws Exception { + Field field = StdioClientTransport.class.getDeclaredField("params"); + field.setAccessible(true); + return (ServerParameters) field.get(transport); + } + private static String getEndpoint(HttpClientStreamableHttpTransport transport) throws Exception { Field field = HttpClientStreamableHttpTransport.class.getDeclaredField("endpoint"); field.setAccessible(true); diff --git a/core/src/test/java/com/google/adk/tools/mcp/WindowsExecutableResolverTest.java b/core/src/test/java/com/google/adk/tools/mcp/WindowsExecutableResolverTest.java new file mode 100644 index 000000000..cb4ed8bb9 --- /dev/null +++ b/core/src/test/java/com/google/adk/tools/mcp/WindowsExecutableResolverTest.java @@ -0,0 +1,171 @@ +/* + * Copyright 2026 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.adk.tools.mcp; + +import static com.google.common.truth.Truth.assertThat; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Unit tests for {@link WindowsExecutableResolver}. */ +@RunWith(JUnit4.class) +public final class WindowsExecutableResolverTest { + + private static final String WINDOWS = "Windows 11"; + private static final String LINUX = "Linux"; + private static final String PATHEXT = ".COM;.EXE;.BAT;.CMD"; + + @Rule public final TemporaryFolder temporaryFolder = new TemporaryFolder(); + + @Test + public void resolve_onNonWindows_returnsCommandUnchanged() throws Exception { + Path dir = temporaryFolder.newFolder("bin").toPath(); + createFile(dir.resolve("npx.cmd")); + + String resolved = WindowsExecutableResolver.resolve("npx", LINUX, dir.toString(), PATHEXT); + + assertThat(resolved).isEqualTo("npx"); + } + + @Test + public void resolve_withNullCommand_returnsNull() { + assertThat(WindowsExecutableResolver.resolve(null, WINDOWS, "C:\\bin", PATHEXT)).isNull(); + } + + @Test + public void resolve_withEmptyCommand_returnsEmpty() { + assertThat(WindowsExecutableResolver.resolve("", WINDOWS, "C:\\bin", PATHEXT)).isEmpty(); + } + + @Test + public void resolve_withExplicitPath_returnsPathUnchanged() throws Exception { + Path dir = temporaryFolder.newFolder("bin").toPath(); + createFile(dir.resolve("npx.cmd")); + String explicit = dir.resolve("npx.cmd").toString(); + + assertThat(WindowsExecutableResolver.resolve(explicit, WINDOWS, dir.toString(), PATHEXT)) + .isEqualTo(explicit); + } + + @Test + public void resolve_withForwardSlashPath_returnsPathUnchanged() { + assertThat(WindowsExecutableResolver.resolve("tools/npx", WINDOWS, "C:\\bin", PATHEXT)) + .isEqualTo("tools/npx"); + } + + @Test + public void resolve_whenOnlyCmdShimExists_returnsAbsoluteShimPath() throws Exception { + // Regression test for google/adk-java#1686: npm installs npx as a batch shim, so a bare "npx" + // must resolve to npx.cmd or CreateProcess fails with error=2. + Path dir = temporaryFolder.newFolder("nodejs").toPath(); + Path shim = createFile(dir.resolve("npx.cmd")); + + String resolved = WindowsExecutableResolver.resolve("npx", WINDOWS, dir.toString(), PATHEXT); + + assertThat(resolved).isEqualTo(shim.toFile().getAbsolutePath()); + } + + @Test + public void resolve_whenExeAlsoExists_prefersPathextOrder() throws Exception { + Path dir = temporaryFolder.newFolder("bin").toPath(); + Path exe = createFile(dir.resolve("npx.exe")); + createFile(dir.resolve("npx.cmd")); + + String resolved = WindowsExecutableResolver.resolve("npx", WINDOWS, dir.toString(), PATHEXT); + + assertThat(resolved).isEqualTo(exe.toFile().getAbsolutePath()); + } + + @Test + public void resolve_whenCommandAlreadyHasExtension_returnsAbsolutePath() throws Exception { + Path dir = temporaryFolder.newFolder("bin").toPath(); + Path shim = createFile(dir.resolve("npx.cmd")); + + String resolved = + WindowsExecutableResolver.resolve("npx.cmd", WINDOWS, dir.toString(), PATHEXT); + + assertThat(resolved).isEqualTo(shim.toFile().getAbsolutePath()); + } + + @Test + public void resolve_withMultiplePathEntries_searchesThemInOrder() throws Exception { + Path empty = temporaryFolder.newFolder("empty").toPath(); + Path second = temporaryFolder.newFolder("second").toPath(); + Path shim = createFile(second.resolve("npx.cmd")); + // Windows PATH is always ';'-separated, whatever platform the test runs on. + String path = empty + ";" + second; + + String resolved = WindowsExecutableResolver.resolve("npx", WINDOWS, path, PATHEXT); + + assertThat(resolved).isEqualTo(shim.toFile().getAbsolutePath()); + } + + @Test + public void resolve_whenCommandIsNotFound_returnsCommandUnchanged() throws Exception { + Path dir = temporaryFolder.newFolder("empty").toPath(); + + String resolved = WindowsExecutableResolver.resolve("npx", WINDOWS, dir.toString(), PATHEXT); + + assertThat(resolved).isEqualTo("npx"); + } + + @Test + public void resolve_withNullPath_returnsCommandUnchanged() { + assertThat(WindowsExecutableResolver.resolve("npx", WINDOWS, null, PATHEXT)).isEqualTo("npx"); + } + + @Test + public void resolve_withoutPathext_fallsBackToDefaultExtensions() throws Exception { + Path dir = temporaryFolder.newFolder("bin").toPath(); + Path shim = createFile(dir.resolve("npx.cmd")); + + String resolved = WindowsExecutableResolver.resolve("npx", WINDOWS, dir.toString(), null); + + assertThat(resolved).isEqualTo(shim.toFile().getAbsolutePath()); + } + + @Test + public void resolve_withCustomPathext_honoursIt() throws Exception { + Path dir = temporaryFolder.newFolder("bin").toPath(); + createFile(dir.resolve("npx.cmd")); + Path custom = createFile(dir.resolve("npx.bat")); + + String resolved = WindowsExecutableResolver.resolve("npx", WINDOWS, dir.toString(), ".BAT"); + + assertThat(resolved).isEqualTo(custom.toFile().getAbsolutePath()); + } + + @Test + public void resolve_isCaseInsensitiveForOsName() throws Exception { + Path dir = temporaryFolder.newFolder("bin").toPath(); + Path shim = createFile(dir.resolve("npx.cmd")); + + String resolved = WindowsExecutableResolver.resolve("npx", "WINDOWS", dir.toString(), PATHEXT); + + assertThat(resolved).isEqualTo(shim.toFile().getAbsolutePath()); + } + + private static Path createFile(Path path) throws IOException { + return Files.createFile(path); + } +}