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); + } +}