feat: Add stdio transport support for Metals MCP server#8266
Open
feat: Add stdio transport support for Metals MCP server#8266
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds stdio transport for MCP, a transport-agnostic MCP tooling trait, a stdio MCP server implementation, file-based logging helpers, MCP exception/messages, stdio-aware startup wiring (useStdio flag), and comprehensive stdio integration tests. Changes
Sequence DiagramsequenceDiagram
participant Client as MCP Client
participant StdioServer as MetalsMcpStdioServer
participant Tools as MetalsMcpTools
participant Engine as McpQueryEngine
participant LSP as ProjectMetalsLspService
Client->>StdioServer: Initialize (stdio transport)
StdioServer->>Tools: run()
Tools->>Tools: buildCapabilities()
Tools->>Tools: registerAllTools(asyncServer)
StdioServer-->>Client: ServerCapabilities
Client->>StdioServer: CallTool (e.g., compile-file)
StdioServer->>Tools: withErrorHandling(call)
Tools->>Engine: perform query/compile
Engine->>LSP: request LSP info
LSP-->>Engine: LSP response
Engine-->>Tools: operation result
Tools-->>StdioServer: CallToolResult
StdioServer-->>Client: Tool response
Client->>StdioServer: Shutdown
StdioServer->>Tools: cancel()
Tools-->>Client: closed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.OpenGrep is compatible with Semgrep configurations. Add an |
tgodzik
requested changes
Mar 18, 2026
metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpStdioServer.scala
Show resolved
Hide resolved
Member
Author
|
@tgodzik looks better now and seems to pass the tests |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
metals-mcp/src/main/scala/scala/meta/metals/McpMain.scala (1)
264-283:⚠️ Potential issue | 🟡 MinorReject or warn on
--portwhen--transport stdiois selected.
runServercomputesuseStdioand then passesconfig.port, but stdio mode does not use port-based transport. Silent acceptance of--portis confusing.💡 Proposed fix
private def runServer(config: Config): Unit = { + if (config.transport == Transport.Stdio && config.port.nonEmpty) { + System.err.println("Error: --port is only supported with --transport http") + sys.exit(1) + } + val workspace = AbsolutePath( config.workspace.get.toNIO.toAbsolutePath().normalize() )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metals-mcp/src/main/scala/scala/meta/metals/McpMain.scala` around lines 264 - 283, Detect the conflicting flags when computing useStdio (config.transport == Transport.Stdio): if useStdio is true and a port was provided in config (config.port is set/non-zero), either fail fast with a clear error (throw/exit) or log a warning and ignore the port before constructing StandaloneMcpService; update the validation in McpMain.runServer (where useStdio and config.port are read and where StandaloneMcpService is instantiated) to perform this check and ensure the port value is not passed through to StandaloneMcpService when stdio is chosen.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala`:
- Around line 76-77: Change the eager initialization of client in the
MetalsMcpTools trait to a lazy val so clientName (which is provided by the
concrete classes MetalsMcpServer and MetalsMcpStdioServer) is initialized first;
specifically replace the current protected val client =
Client.allClients.find(_.names.contains(clientName)).getOrElse(NoClient) with a
lazy val, ensuring downstream logic such as MetalsMcpServer.cancel() and its
shouldCleanUpServerEntry check uses the correctly resolved client instance.
- Around line 1214-1242: The current withErrorHandling only catches synchronous
throws during invocation of f but ignores asynchronous failures from the
returned Mono; change it to call f(exchange, arguments) inside a Mono.defer (or
keep the try for immediate exceptions) and then attach a reactive error handler
like .onErrorResume to the returned Mono to convert any Throwable into a
CallToolResult error via CallToolResult.builder().content(createContent(s"Error:
" + e.getMessage)).isError(true).build(); also remove arguments.toJson() from
all logs and instead log a short non-sensitive message (e.g., "Error while
processing request" and the exception message/stack) using scribe.warn so no raw
request payloads (arguments) are printed. Ensure you reference
withErrorHandling, f, McpAsyncServerExchange, CallToolResult, and createContent
when making the changes.
- Around line 305-339: The code for the "compile-module" tool only inspects
diagnostics and never triggers a build; to fix it, invoke the compilation API
for the found target before reading diagnostics: after matching case
Some(target) call the appropriate compilations method (e.g.,
compilations.compile or compilations.compileTargets/compileTarget with
target.id) and await its completion, then gather diagnostics via inModuleErrors,
diagnostics.allDiagnostics and upstreamModulesErros to produce the result;
update the block that builds CallToolResult (inside the Future in
withErrorHandling) to run the compilation for target.id first and only then
compute result/content based on post-compilation diagnostics.
- Around line 936-953: The current handler for languageClient.applyEdit ignores
the ApplyWorkspaceEditResponse and always returns a successful CallToolResult;
change the map/handler to inspect the ApplyWorkspaceEditResponse (use
ApplyWorkspaceEditResponse.isApplied() and getFailureReason()) returned by
languageClient.applyEdit, and if isApplied() is false return a CallToolResult
with isError(true) and content created via createContent including the failure
reason and path, otherwise return the existing success CallToolResult (content
"$path was formatted", isError(false)); update the closure where formattedText,
path, createContent and CallToolResult.builder() are used to perform this
conditional logic.
In
`@metals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scala`:
- Around line 284-333: startMcpServer currently catches and logs exceptions but
leaves isMcpServerRunning set to true and returns a successful Future; change
startMcpServer so that any exception from the register(...).run() path resets
isMcpServerRunning to false and allows the Future to complete with failure
(i.e., rethrow or return a failed Future) so callers see the error; specifically
update startMcpServer (and the Future recover/transform block) to call
isMcpServerRunning.set(false) on error and propagate the exception instead of
swallowing it.
In `@tests/slow/src/test/scala/tests/feature/McpStdioCompileToolsSuite.scala`:
- Around line 59-117: Each test currently only performs shutdown,
client.cleanup(), and RecursivelyDelete(workspacePath) on the success path;
change each test ("stdio-compile-file-works", "stdio-compile-module-works",
"stdio-compile-full-works") so that shutdown, client.cleanup(), and
RecursivelyDelete(workspacePath) run regardless of whether initialize or the
compile call fails by using a Future cleanup hook (e.g.,
transformWith/andThen/guarantee) or by chaining a .transformWith { case _ =>
perform shutdown and cleanup and return original result } around the
initialization/compile sequence; reference the existing symbols
client.initialize, client.compileFile / client.compileModule /
client.compileFull, client.shutdown, client.cleanup, and RecursivelyDelete to
locate and modify the code.
In `@tests/unit/src/main/scala/tests/mcp/TestMcpStdioClient.scala`:
- Around line 50-62: The test uses a bare "java" in buildServerParameters()
which relies on PATH; instead construct an absolute JVM executable from
System.getProperty("java.home") (e.g.,
Paths.get(System.getProperty("java.home"), "bin", "java").toString()) and pass
that string to ServerParameters.builder(...) so the child process uses the same
JRE as the test JVM; update buildServerParameters() to compute javaExec and use
it in ServerParameters.builder(...) (handle platform-specific executable name if
needed).
- Around line 103-107: shutdown() currently calls client.closeGracefully() which
can throw synchronously if _client wasn't assigned (e.g., initialize() failed);
make shutdown() resilient by first checking that _client/client is
non-null/defined or wrapping the call in a try/catch so it never throws
synchronously, then return the existing Future/Promise path (convert to Scala
Future and recover NonFatal as before). Ensure you reference and guard the same
identifiers used in the file (_client or client, shutdown(), initialize(), and
client.closeGracefully()) so cleanup becomes best-effort and cannot cause a
second failure.
---
Outside diff comments:
In `@metals-mcp/src/main/scala/scala/meta/metals/McpMain.scala`:
- Around line 264-283: Detect the conflicting flags when computing useStdio
(config.transport == Transport.Stdio): if useStdio is true and a port was
provided in config (config.port is set/non-zero), either fail fast with a clear
error (throw/exit) or log a warning and ignore the port before constructing
StandaloneMcpService; update the validation in McpMain.runServer (where useStdio
and config.port are read and where StandaloneMcpService is instantiated) to
perform this check and ensure the port value is not passed through to
StandaloneMcpService when stdio is chosen.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d4149d81-4e00-48b8-b1e9-f854a6375291
📒 Files selected for processing (14)
metals-mcp/src/main/scala/scala/meta/internal/metals/mcp/McpLanguageClient.scalametals-mcp/src/main/scala/scala/meta/internal/metals/mcp/StandaloneMcpService.scalametals-mcp/src/main/scala/scala/meta/metals/McpMain.scalametals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scalametals/src/main/scala/scala/meta/internal/metals/logging/MetalsLogger.scalametals/src/main/scala/scala/meta/internal/metals/mcp/McpExceptions.scalametals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpServer.scalametals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpStdioServer.scalametals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scalaproject/TestGroups.scalatests/slow/src/test/scala/tests/feature/McpStdioCompileToolsSuite.scalatests/slow/src/test/scala/tests/feature/McpStdioSuite.scalatests/slow/src/test/scala/tests/feature/StandaloneMcpSuite.scalatests/unit/src/main/scala/tests/mcp/TestMcpStdioClient.scala
metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scala
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala (2)
75-76:⚠️ Potential issue | 🔴 CriticalUse
lazy valto deferclientinitialization until after constructor completion.The
clientfield is initialized during trait construction, butclientNameis an abstract member that concrete classes implement as a constructor parameter. In Scala 2 trait initialization,clientNameremains uninitialized (null) when this line executes, causingClient.allClients.find(_.names.contains(null))to always returnNoClient.This breaks client-specific configuration and cleanup paths in subclasses like
MetalsMcpServer.Proposed fix
- protected val client = + protected lazy val client = Client.allClients.find(_.names.contains(clientName)).getOrElse(NoClient),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala` around lines 75 - 76, The field `client` in `MetalsMcpTools` is being initialized too early using `protected val client = Client.allClients.find(_.names.contains(clientName)).getOrElse(NoClient)` while `clientName` is abstract and not yet set; change this to `protected lazy val client` so initialization is deferred until after constructor completion (ensuring the lookup against `Client.allClients` uses the concrete `clientName`), keeping the rest of the expression the same and preserving references to `Client.allClients`, `clientName`, and `NoClient`.
304-340:⚠️ Potential issue | 🔴 Critical
compile-moduledoes not actually compile the module.The tool finds the target module but only inspects existing diagnostics via
inModuleErrors()andupstreamModulesErros(). Without a call tocompilations.cascadeCompile()(as done increateCompileTool()at line 158-159), the tool can report "Compilation successful." even though nothing was built.Proposed fix
new AsyncToolSpecification( tool, withErrorHandling { (exchange, arguments) => val module = arguments.getAs[String]("module") - Future { - (buildTargets.allScala ++ buildTargets.allJava).find( - _.displayName == module - ) match { - case Some(target) => - val result = inModuleErrors(target.id) + (buildTargets.allScala ++ buildTargets.allJava).find( + _.displayName == module + ) match { + case Some(target) => + compilations + .cascadeCompile(List(target.id)) + .map { _ => + val result = inModuleErrors(target.id) .map { diagnosticsOutput => // ... existing diagnostic logic ... } .orElse(upstreamModulesErros(target.id, "module")) .getOrElse("Compilation successful.") - CallToolResult - .builder() - .content(createContent(result)) - .isError(false) - .build() - case None => + CallToolResult + .builder() + .content(createContent(result)) + .isError(false) + .build() + } + .toMono + case None => + Future.successful( CallToolResult .builder() .content(createContent(s"Error: Module not found: $module")) .isError(true) .build() - } - }.toMono + ).toMono + } }, ),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala` around lines 304 - 340, The compile-module handler currently only reads diagnostics (inModuleErrors, upstreamModulesErros) but never triggers a build; update the handler inside withErrorHandling so it invokes compilations.cascadeCompile(target.id) (the same API used by createCompileTool) and waits for its result before collecting diagnostics; then use the post-compile diagnostics (inModuleErrors and upstreamModulesErros) to determine the CallToolResult content and error flag instead of assuming "Compilation successful." This change should be applied in the lambda that handles the module lookup (the block that matches Some(target) and builds the CallToolResult).
🧹 Nitpick comments (3)
tests/unit/src/main/scala/tests/mcp/TestMcpStdioClient.scala (2)
269-279: Temp directory may leak ifcleanup()is not called.The
tempDiris created unconditionally in the constructor (line 46), but deletion only happens ifcleanup()is explicitly invoked. If tests fail or don't callcleanup(), the temp directory accumulates.Consider either:
- Making
tempDirlazy so it's only created when actually needed, or- Adding a finalizer/try-with-resources pattern for the test client lifecycle
This is a minor concern for test code but worth noting for CI hygiene.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/src/main/scala/tests/mcp/TestMcpStdioClient.scala` around lines 269 - 279, The test client currently creates tempDir eagerly in the constructor (tempDir) and only deletes it when cleanup() is explicitly called, risking leaked temp dirs; change the lifecycle so the temp dir is created lazily or is always cleaned up automatically: either make tempDir a lazy val (so it is only created on first use) or implement AutoCloseable/close on the test client and call cleanup() from close(), then use scala.util.Using or try-with-resources in tests to ensure cleanup() is invoked; update references to tempDir, the constructor, and the cleanup() method accordingly so tests cannot leave the temp directory behind.
72-80: Potential resource leak ifcreateFreshClient()is called multiple times.If
createFreshClient()is invoked more than once (e.g., for retry logic or test reset), the previous_transportand_clientare overwritten without being closed, potentially leaking subprocess handles and transport resources.Consider closing existing resources before creating new ones:
Proposed fix
private def createFreshClient(): McpAsyncClient = { + // Close existing resources if any + Option(_client).foreach { c => + try { c.closeGracefully().toFuture().get() } catch { case NonFatal(_) => } + } val params = buildServerParameters() _transport = new StdioClientTransport(params, jsonMapper) _client = McpClient🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/src/main/scala/tests/mcp/TestMcpStdioClient.scala` around lines 72 - 80, createFreshClient currently overwrites _transport and _client and can leak resources; before creating new ones in createFreshClient(), check if existing _client and _transport are non-null and close or shut them down (call their close()/shutdown()/stop() methods as appropriate) and swallow/log any exceptions, then proceed to instantiate new StdioClientTransport(params, jsonMapper) and McpClient.async(...). Ensure you reference and close the previous _client and _transport fields before assigning new instances created by StdioClientTransport and McpClient.async(...).metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala (1)
1195-1214: Fire-and-forget.subscribe()calls may silently drop tool registration errors.Each
addTool(...).subscribe()call subscribes to the Mono without any error handling. If tool registration fails (e.g., due to schema validation errors), the exception is silently swallowed.Consider adding error logging:
Proposed fix
protected def registerAllTools( asyncServer: io.modelcontextprotocol.server.McpAsyncServer ): Unit = { - asyncServer.addTool(createFileCompileTool()).subscribe() - asyncServer.addTool(createCompileModuleTool()).subscribe() + def registerTool(tool: AsyncToolSpecification): Unit = + asyncServer.addTool(tool).subscribe( + _ => (), + err => scribe.error(s"Failed to register tool: ${err.getMessage}", err) + ) + registerTool(createFileCompileTool()) + registerTool(createCompileModuleTool()) // ... remaining tools ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala` around lines 1195 - 1214, The registerAllTools method currently calls asyncServer.addTool(...).subscribe() for each tool which will silently swallow errors; wrap each subscription with error handling (or aggregate the Monos and subscribe once) so failures are logged and surfaced: for each call to asyncServer.addTool(...) in registerAllTools, attach onError logging (e.g., doOnError or subscribe(onSuccess, onError)) that logs the tool name and exception via the Metals logger, or collect all returned Monos from createFileCompileTool/createCompileModuleTool/createCompileTool/.../createListScalafixRulesTool and use a combined reactive operator (e.g., Mono.when or Flux.mergeSequential) with a single subscribe that logs errors; ensure you reference the tool creation methods (createFileCompileTool, createCompileModuleTool, createCompileTool, createTestTool, createGlobSearchTool, createTypedGlobSearchTool, createInspectTool, createGetDocsTool, createGetUsagesTool, importBuildTool, createFindDepTool, createListModulesTool, createFormatTool, createGenerateScalafixRuleTool, createRunScalafixRuleTool, createListScalafixRulesTool) when adding the error handling so failures are not silently dropped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala`:
- Around line 1282-1292: The getOptAs method currently throws
IncorrectArgumentTypeException on a type mismatch (via .orElse(throw ...)) which
contradicts its Option-returning contract; change getOptAs (not getAs) to return
None for any cast failure by removing the thrown exception and returning
Try(value.asInstanceOf[T]).toOption (or otherwise catching ClassCastException
and returning None) so callers get None on invalid types; if throwing behavior
was intentional, instead rename getOptAs to something like getOptAsOrThrow and
leave the exception logic in place.
---
Duplicate comments:
In `@metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala`:
- Around line 75-76: The field `client` in `MetalsMcpTools` is being initialized
too early using `protected val client =
Client.allClients.find(_.names.contains(clientName)).getOrElse(NoClient)` while
`clientName` is abstract and not yet set; change this to `protected lazy val
client` so initialization is deferred until after constructor completion
(ensuring the lookup against `Client.allClients` uses the concrete
`clientName`), keeping the rest of the expression the same and preserving
references to `Client.allClients`, `clientName`, and `NoClient`.
- Around line 304-340: The compile-module handler currently only reads
diagnostics (inModuleErrors, upstreamModulesErros) but never triggers a build;
update the handler inside withErrorHandling so it invokes
compilations.cascadeCompile(target.id) (the same API used by createCompileTool)
and waits for its result before collecting diagnostics; then use the
post-compile diagnostics (inModuleErrors and upstreamModulesErros) to determine
the CallToolResult content and error flag instead of assuming "Compilation
successful." This change should be applied in the lambda that handles the module
lookup (the block that matches Some(target) and builds the CallToolResult).
---
Nitpick comments:
In `@metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala`:
- Around line 1195-1214: The registerAllTools method currently calls
asyncServer.addTool(...).subscribe() for each tool which will silently swallow
errors; wrap each subscription with error handling (or aggregate the Monos and
subscribe once) so failures are logged and surfaced: for each call to
asyncServer.addTool(...) in registerAllTools, attach onError logging (e.g.,
doOnError or subscribe(onSuccess, onError)) that logs the tool name and
exception via the Metals logger, or collect all returned Monos from
createFileCompileTool/createCompileModuleTool/createCompileTool/.../createListScalafixRulesTool
and use a combined reactive operator (e.g., Mono.when or Flux.mergeSequential)
with a single subscribe that logs errors; ensure you reference the tool creation
methods (createFileCompileTool, createCompileModuleTool, createCompileTool,
createTestTool, createGlobSearchTool, createTypedGlobSearchTool,
createInspectTool, createGetDocsTool, createGetUsagesTool, importBuildTool,
createFindDepTool, createListModulesTool, createFormatTool,
createGenerateScalafixRuleTool, createRunScalafixRuleTool,
createListScalafixRulesTool) when adding the error handling so failures are not
silently dropped.
In `@tests/unit/src/main/scala/tests/mcp/TestMcpStdioClient.scala`:
- Around line 269-279: The test client currently creates tempDir eagerly in the
constructor (tempDir) and only deletes it when cleanup() is explicitly called,
risking leaked temp dirs; change the lifecycle so the temp dir is created lazily
or is always cleaned up automatically: either make tempDir a lazy val (so it is
only created on first use) or implement AutoCloseable/close on the test client
and call cleanup() from close(), then use scala.util.Using or try-with-resources
in tests to ensure cleanup() is invoked; update references to tempDir, the
constructor, and the cleanup() method accordingly so tests cannot leave the temp
directory behind.
- Around line 72-80: createFreshClient currently overwrites _transport and
_client and can leak resources; before creating new ones in createFreshClient(),
check if existing _client and _transport are non-null and close or shut them
down (call their close()/shutdown()/stop() methods as appropriate) and
swallow/log any exceptions, then proceed to instantiate new
StdioClientTransport(params, jsonMapper) and McpClient.async(...). Ensure you
reference and close the previous _client and _transport fields before assigning
new instances created by StdioClientTransport and McpClient.async(...).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a5189320-9134-4705-a918-00684ad8c40a
📒 Files selected for processing (5)
metals-mcp/src/main/scala/scala/meta/metals/McpMain.scalametals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scalametals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scalatests/slow/src/test/scala/tests/feature/McpStdioCompileToolsSuite.scalatests/unit/src/main/scala/tests/mcp/TestMcpStdioClient.scala
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/slow/src/test/scala/tests/feature/McpStdioCompileToolsSuite.scala
- metals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scala
metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala (1)
304-338:⚠️ Potential issue | 🔴 Critical
compile-modulestill skips the actual compile.This branch still only reads cached diagnostics, so it can return
Compilation successful.for a dirty target that has not been rebuilt yet.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala` around lines 304 - 338, The handler for the "compile-module" branch currently only reads cached diagnostics (using inModuleErrors and upstreamModulesErros) and can wrongly return "Compilation successful." for a dirty target; instead, before composing the CallToolResult (the block that builds content via CallToolResult.builder().content(createContent(...)).isError(...)), invoke the project's compile/build API for the found target (use target.id) and await the compile result, then use that compile outcome to decide isError and the message; after the compile completes, read inModuleErrors(target.id) and upstreamModulesErros(target.id, "module") to include real diagnostics if compilation failed or produced warnings. Ensure you replace the current short-circuit that only checks cached diagnostics with this compile invocation and subsequent diagnostic aggregation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala`:
- Around line 867-877: The current list-modules implementation filters
buildTargets.allBuildTargetIds through buildTargets.jvmTarget, which drops
non-JVM targets; change it to include all relevant module IDs (e.g., use
buildTargets.allScala ++ buildTargets.allJava or map allBuildTargetIds to their
displayName without calling buildTargets.jvmTarget) so the module list matches
what compile-module accepts. Update the code that builds modules (replace the
flatMap(buildTargets.jvmTarget(_).map(_.displayName)) usage) to collect
displayName for all applicable build targets and keep the
CallToolResult.builder().content(...) construction the same.
- Around line 1288-1299: The getAsList[T] method currently silently drops
non-matching elements in the JList[_] branch by using collect { case value: T =>
value }, which allows mixed-type arrays like ["class", 1] to become partial
results; update the JList[_] handling so it validates every element's runtime
type against T (using the provided ClassTag or pattern matching) and if any
element does not match, throw a clear argument error (e.g.,
IllegalArgumentException or a MissingArgumentException with the key) instead of
filtering; ensure the exception includes the key and offending value(s) to aid
debugging and keep behavior consistent with the String/json branch that fails on
invalid input.
In
`@metals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scala`:
- Around line 287-328: The two MCP server constructors are being passed an extra
callback layer by wrapping focusedDocument with `() =>`, producing `() => (() =>
Option[AbsolutePath])`; pass the existing focusedDocument value directly where
the constructors expect a `() => Option[AbsolutePath] — update the arguments to
MetalsMcpStdioServer and MetalsMcpServer (the parameter currently wrapped) to
use focusedDocument without the extra `() =>` wrapper at both call sites.
---
Duplicate comments:
In `@metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala`:
- Around line 304-338: The handler for the "compile-module" branch currently
only reads cached diagnostics (using inModuleErrors and upstreamModulesErros)
and can wrongly return "Compilation successful." for a dirty target; instead,
before composing the CallToolResult (the block that builds content via
CallToolResult.builder().content(createContent(...)).isError(...)), invoke the
project's compile/build API for the found target (use target.id) and await the
compile result, then use that compile outcome to decide isError and the message;
after the compile completes, read inModuleErrors(target.id) and
upstreamModulesErros(target.id, "module") to include real diagnostics if
compilation failed or produced warnings. Ensure you replace the current
short-circuit that only checks cached diagnostics with this compile invocation
and subsequent diagnostic aggregation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b11b10a-c7c2-466a-b450-cc69ec0ab207
📒 Files selected for processing (2)
metals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scalametals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala
metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scala
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (2)
metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala (2)
1290-1310:⚠️ Potential issue | 🟡 Minor
getAsListsilently drops non-matching array elements.The
JList[_]branch at line 1299-1300 usescollect { case value: T => value }, which silently filters out elements that don't match typeT. A mixed-type payload like["class", 1]would becomeList("class")instead of raising an error, potentially masking input mistakes.Proposed fix to validate all elements
case value: JList[_] => - value.asScala.collect { case value: T => value }.toList + val converted = value.asScala.map { + case v: T => v + case other => + throw new IncorrectArgumentTypeException( + key, + s"expected ${ct.runtimeClass.getSimpleName}, got ${other.getClass.getSimpleName}", + ) + }.toList + converted🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala` around lines 1290 - 1310, The getAsList[T] method currently silently drops non-matching elements in the JList branch by using collect { case value: T => value }, which masks mixed-type inputs; update the JList[_] branch in getAsList to validate every element is an instance of T (using the implicit ClassTag ct/runtimeClass) and if any element fails, throw IncorrectArgumentTypeException(key, s"Array[${ct.runtimeClass.getSimpleName}]"), otherwise return the full list of elements cast to T; ensure the MissingArgumentException handling remains unchanged.
868-883:⚠️ Potential issue | 🟡 Minor
list-modulesfilters to JVM targets only, butcompile-moduleaccepts all Scala/Java targets.The
list-modulestool usesbuildTargets.jvmTarget(_).map(_.displayName)which excludes Scala.js and Scala Native targets. However,compile-module(lines 307-308) searches(buildTargets.allScala ++ buildTargets.allJava). This inconsistency means users can't discover non-JVM modules vialist-moduleseven though they can compile them.Proposed fix to align with compile-module
withErrorHandling { (_, _) => Future { val modules = - buildTargets.allBuildTargetIds.flatMap( - buildTargets.jvmTarget(_).map(_.displayName) - ) + (buildTargets.allScala ++ buildTargets.allJava) + .map(_.displayName) + .distinct CallToolResult .builder() .content(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala` around lines 868 - 883, The list-modules tool currently collects modules using buildTargets.jvmTarget(...).map(_.displayName), which excludes Scala.js/Native targets; change it to mirror compile-module's selection by collecting (buildTargets.allScala ++ buildTargets.allJava).map(_.displayName) (or the equivalent method used by compile-module) and use those display names when building the CallToolResult content so list-modules shows the same set of compile-able modules as compile-module.
🧹 Nitpick comments (2)
tests/unit/src/main/scala/tests/mcp/TestMcpStdioClient.scala (1)
47-47: UnusedtempDirfield.The
tempDiris created at construction but is never used by the client (theworkspacePathparameter is passed to the server separately). Thecleanup()method deletes it, but since nothing writes to it, this is wasted I/O.Consider removing both the field and the
cleanup()method, or usingtempDiras the actual workspace for tests that need an isolated directory.Proposed fix to remove unused tempDir
- private val tempDir = Files.createTempDirectory("metals-mcp-stdio-test") - private var _client: McpAsyncClient = _ private var _transport: StdioClientTransport = _And remove the cleanup method:
- /** Clean up temp directory */ - def cleanup(): Unit = { - try { - Files - .walk(tempDir) - .sorted(Comparator.reverseOrder[Path]()) - .forEach(Files.delete) - } catch { - case NonFatal(_) => - } - }Also applies to: 270-280
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/src/main/scala/tests/mcp/TestMcpStdioClient.scala` at line 47, The tempDir field in TestMcpStdioClient is created but never used and only deleted in cleanup(); remove the unused private val tempDir and the cleanup() method, or alternatively repurpose tempDir by passing tempDir to the server as the workspace (replace usages of workspacePath with tempDir where isolation is required) so the directory is actually used; update any tests that relied on cleanup() accordingly and remove related deletion logic to avoid redundant I/O.metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala (1)
358-373: Typo in method name:upstreamModulesErros→upstreamModulesErrors.This method is called from multiple tools (
createFileCompileToolat line 254,createCompileModuleToolat line 324). Consider fixing the typo for readability.Proposed fix
- protected def upstreamModulesErros( + protected def upstreamModulesErrors( buildTarget: BuildTargetIdentifier, fileOrModule: String, ): Option[String] = {Also update call sites at lines 254 and 324.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala` around lines 358 - 373, Rename the typo'd method upstreamModulesErros to upstreamModulesErrors in MetalsMcpTools (method signature and all internal references), and update every call site that uses upstreamModulesErros—notably the invocations inside createFileCompileTool and createCompileModuleTool—to call upstreamModulesErrors instead; ensure the method visibility and behavior remain unchanged apart from the name so callers compile cleanly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala`:
- Around line 1290-1310: The getAsList[T] method currently silently drops
non-matching elements in the JList branch by using collect { case value: T =>
value }, which masks mixed-type inputs; update the JList[_] branch in getAsList
to validate every element is an instance of T (using the implicit ClassTag
ct/runtimeClass) and if any element fails, throw
IncorrectArgumentTypeException(key, s"Array[${ct.runtimeClass.getSimpleName}]"),
otherwise return the full list of elements cast to T; ensure the
MissingArgumentException handling remains unchanged.
- Around line 868-883: The list-modules tool currently collects modules using
buildTargets.jvmTarget(...).map(_.displayName), which excludes Scala.js/Native
targets; change it to mirror compile-module's selection by collecting
(buildTargets.allScala ++ buildTargets.allJava).map(_.displayName) (or the
equivalent method used by compile-module) and use those display names when
building the CallToolResult content so list-modules shows the same set of
compile-able modules as compile-module.
---
Nitpick comments:
In `@metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala`:
- Around line 358-373: Rename the typo'd method upstreamModulesErros to
upstreamModulesErrors in MetalsMcpTools (method signature and all internal
references), and update every call site that uses upstreamModulesErros—notably
the invocations inside createFileCompileTool and createCompileModuleTool—to call
upstreamModulesErrors instead; ensure the method visibility and behavior remain
unchanged apart from the name so callers compile cleanly.
In `@tests/unit/src/main/scala/tests/mcp/TestMcpStdioClient.scala`:
- Line 47: The tempDir field in TestMcpStdioClient is created but never used and
only deleted in cleanup(); remove the unused private val tempDir and the
cleanup() method, or alternatively repurpose tempDir by passing tempDir to the
server as the workspace (replace usages of workspacePath with tempDir where
isolation is required) so the directory is actually used; update any tests that
relied on cleanup() accordingly and remove related deletion logic to avoid
redundant I/O.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e56504e6-8dde-478b-9536-c1f4107b2d07
📒 Files selected for processing (3)
metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpStdioServer.scalametals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scalatests/unit/src/main/scala/tests/mcp/TestMcpStdioClient.scala
✅ Files skipped from review due to trivial changes (1)
- metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpStdioServer.scala
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/unit/src/main/scala/tests/mcp/TestMcpStdioClient.scala (2)
46-47: Either usetempDirfor isolation or drop it.Each client allocates a temp directory here, but the subprocess is still started against
workspacePathand nothing else consumestempDirexcept cleanup. If this directory is meant to back stdio logs or scratch state, thread it into the server setup; otherwise removing it would simplify the lifecycle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/src/main/scala/tests/mcp/TestMcpStdioClient.scala` around lines 46 - 47, TestMcpStdioClient defines a tempDir (private val tempDir) that is never used; either remove the tempDir declaration and its cleanup to simplify lifecycle, or thread it into the server/client setup (e.g., pass tempDir as the workspace/log/scratch path when constructing the MCP stdio server or client in TestMcpStdioClient) and ensure the test uses that directory for subprocess workingDir or log output and still deletes it in teardown; update any setup methods that create the subprocess to accept and use tempDir.
76-95: Avoid makingcallTooltext-only at the transport boundary.This helper strips the SDK response down to
List[String]by collecting onlyTextContent. The MCP specification and Java SDK support tool failures viaisErrormetadata inCallToolResult, non-text payloads (image, audio, embedded resources), andstructuredContentfor structured results. The current implementation discards all of this: callers cannot inspect failure indicators, non-text responses, or structured output. Returning the fullCallToolResultor failing explicitly whenisErroris true would preserve test visibility into the full range of tool responses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/src/main/scala/tests/mcp/TestMcpStdioClient.scala` around lines 76 - 95, The helper callTool currently converts the SDK CallToolResult into a List[String] by collecting only TextContent, which drops isError metadata, structuredContent and non-text payloads; change callTool to return the full CallToolResult (or fail when result.isError) instead of List[String] so tests can inspect errors and non-text/structured responses—update the method signature of callTool, stop mapping client.callTool(...).toFuture().toScala to extract TextContent, and return the original CallToolResult produced by client.callTool (references: callTool, CallToolRequest, client.callTool, CallToolResult, TextContent).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/src/main/scala/tests/mcp/TestMcpStdioClient.scala`:
- Around line 255-259: Files.walk(tempDir) returns a Stream[Path] that must be
closed; change the deletion block to open the stream into a local variable and
ensure it is closed (e.g., acquire the stream from Files.walk(tempDir), call
stream.sorted(Comparator.reverseOrder[Path]()).forEach(Files.delete) inside a
try/finally and call stream.close() in finally or use a try-with-resources
equivalent) so the stream opened by Files.walk is closed before attempting to
delete the temp tree.
---
Nitpick comments:
In `@tests/unit/src/main/scala/tests/mcp/TestMcpStdioClient.scala`:
- Around line 46-47: TestMcpStdioClient defines a tempDir (private val tempDir)
that is never used; either remove the tempDir declaration and its cleanup to
simplify lifecycle, or thread it into the server/client setup (e.g., pass
tempDir as the workspace/log/scratch path when constructing the MCP stdio server
or client in TestMcpStdioClient) and ensure the test uses that directory for
subprocess workingDir or log output and still deletes it in teardown; update any
setup methods that create the subprocess to accept and use tempDir.
- Around line 76-95: The helper callTool currently converts the SDK
CallToolResult into a List[String] by collecting only TextContent, which drops
isError metadata, structuredContent and non-text payloads; change callTool to
return the full CallToolResult (or fail when result.isError) instead of
List[String] so tests can inspect errors and non-text/structured
responses—update the method signature of callTool, stop mapping
client.callTool(...).toFuture().toScala to extract TextContent, and return the
original CallToolResult produced by client.callTool (references: callTool,
CallToolRequest, client.callTool, CallToolResult, TextContent).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40576815-f437-40b1-af68-6c644057dc44
📒 Files selected for processing (1)
tests/unit/src/main/scala/tests/mcp/TestMcpStdioClient.scala
tgodzik
requested changes
Mar 20, 2026
Contributor
tgodzik
left a comment
tgodzik
left a comment
There was a problem hiding this comment.
Great work! I have some further comments, but nothing blocking. This already looks very good.
|
|
||
| def configureFileLogging(logfile: AbsolutePath): Unit = { | ||
| Files.createDirectories(logfile.toNIO.getParent) | ||
| configureFileLogging(List(logfile)) |
Contributor
There was a problem hiding this comment.
Suggested change
| configureFileLogging(List(logfile)) | |
| redirectSystemOut(logfile) |
wouldn't be enough? Looks like the other added method has duplicated code from other parts, unless that's on purpose?
Member
Author
There was a problem hiding this comment.
Problem is redirectSystemOut redirects the entire output, where StdioServerTransportProvider actually talks over System.out. This would break stdio mcp. I tried working around it such that redirectSystemOut redirects current stdout but hands over a copy we could still reuse with StdioServerTransportProvider. Unfortunately mcp sdk doesn't have a constructor that would allow passing custom output stream to StdioServerTransportProvider. If I missed something let me know, otherwise we'll have to live with this approach
metals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scala
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala
Show resolved
Hide resolved
| _ <- client.shutdown().recover { case NonFatal(_) => () } | ||
| _ = client.cleanup() | ||
| } yield { | ||
| try RecursivelyDelete(workspacePath) |
Contributor
There was a problem hiding this comment.
The code here seems duplicated, any chance to reuse the existing utilities we have?
| import tests.mcp.TestMcpStdioClient | ||
|
|
||
| class McpStdioCompileToolsSuite | ||
| extends BaseLspSuite("mcp-stdio-compile-tools") { |
Contributor
There was a problem hiding this comment.
Might not make sense to use BaseLspSuite here as we are not actually testing LSP
| import tests.SbtBuildLayout | ||
| import tests.mcp.TestMcpStdioClient | ||
|
|
||
| class McpStdioSuite extends BaseLspSuite("mcp-stdio-suite") { |
Contributor
There was a problem hiding this comment.
Same here, might not make sense to use BaseLspSuite here as we are not actually testing LSP
| private val objectMapper = new ObjectMapper() | ||
| private val jsonMapper = new JacksonMcpJsonMapper(objectMapper) | ||
|
|
||
| private val tempDir = Files.createTempDirectory("metals-mcp-stdio-test") |
| case NonFatal(_) => () | ||
| } | ||
|
|
||
| def listModules(): Future[String] = { |
Contributor
There was a problem hiding this comment.
Could we join this with TestMcpClient, some methods are duplicated between both
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR follows up on #8156 by adding stdio MCP transport
Summary by CodeRabbit
New Features
Bug Fixes
Tests