feat(mcp): glob-search over all modules - #8776
Conversation
📝 WalkthroughWalkthroughMCP glob searches now use path-independent name matching across workspace and dependency scopes. Results are ranked, deduplicated, capped, and marked partial when searches are incomplete or truncated. Configuration, MCP interfaces, formatting, tests, and documentation were updated. ChangesMCP search behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant MetalsMcpTools
participant McpQueryEngine
participant McpSymbolSearch
MCPClient->>MetalsMcpTools: Send glob-search query
MetalsMcpTools->>McpQueryEngine: Call globSearch
McpQueryEngine->>McpSymbolSearch: Run workspace name search
McpSymbolSearch-->>McpQueryEngine: Return results and budget state
McpQueryEngine-->>MetalsMcpTools: Return normalized GlobSearchResult
MetalsMcpTools-->>MCPClient: Return formatted result and partial notice
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
| "type": "string", | ||
| "description": "Substring of the symbol to search for" | ||
| }, | ||
| "fileInFocus": { |
There was a problem hiding this comment.
Removed. It's radical, but I haven't found any real use-case for it. Please correct me if I'm wrong.
|
I tried not to make overly verbose comments, but the logic is pretty complex and defined in many places. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala`:
- Around line 480-483: Update the descriptions in MetalsMcpTools.scala at lines
480-483 and 513 so the documented name-boundary matching rule explicitly applies
to symbols, while accurately describing workspace-package matching as
case-insensitive substring matching; keep both untyped and typed tool
descriptions consistent.
In `@metals/src/main/scala/scala/meta/internal/metals/MetalsServerConfig.scala`:
- Around line 195-200: Update intProperty to reject empty property values and
safely handle NumberFormatException from out-of-range digit strings, returning
default for both malformed and unparseable values while preserving valid integer
parsing.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ebbdda8-3b38-4eb7-b2ec-28f56366b434
📒 Files selected for processing (15)
docs/features/mcp.mddocs/integrations/new-editor.mdmetals/src/main/scala/scala/meta/internal/metals/MetalsServerConfig.scalametals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scalametals/src/main/scala/scala/meta/internal/metals/WorkspaceSymbolProvider.scalametals/src/main/scala/scala/meta/internal/metals/mcp/McpPrinter.scalametals/src/main/scala/scala/meta/internal/metals/mcp/McpQueryEngine.scalametals/src/main/scala/scala/meta/internal/metals/mcp/McpSymbolSearch.scalametals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scalaproject/TestGroups.scalatests/slow/src/test/scala/tests/feature/McpStdioSuite.scalatests/unit/src/main/scala/tests/mcp/TestMcpStdioClient.scalatests/unit/src/test/scala/scala/meta/internal/metals/mcp/tests/McpGlobSearchNormalizeSuite.scalatests/unit/src/test/scala/tests/mcp/McpGlobSearchCapLspSuite.scalatests/unit/src/test/scala/tests/mcp/McpQueryLspSuite.scala
| """|Search symbols by name across all modules and their dependency classpaths. | ||
| |Matches the last part of a fully qualified name at name boundaries, ignoring | ||
| |case, so `idWordProbe` does not find `MidWordProbe`. | ||
| |Use `inspect` for details of a result.""".stripMargin |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the workspace-package matching rule.
The descriptions claim that all matches use name boundaries. McpQueryEngine.globSearch documents that workspace packages use case-insensitive substring matching. This gives different results for packages and symbols.
metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala#L480-L483: State that the name-boundary rule applies to symbols, or apply it to workspace packages.metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala#L513-L513: Make the typed tool description use the same corrected rule.
📍 Affects 1 file
metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala#L480-L483(this comment)metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala#L513-L513
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@metals/src/main/scala/scala/meta/internal/metals/mcp/MetalsMcpTools.scala`
around lines 480 - 483, Update the descriptions in MetalsMcpTools.scala at lines
480-483 and 513 so the documented name-boundary matching rule explicitly applies
to symbols, while accurately describing workspace-package matching as
case-insensitive substring matching; keep both untyped and typed tool
descriptions consistent.
| def intProperty(key: String, default: Int): Int = | ||
| sys.props | ||
| .get(key) | ||
| .filter(_.forall(Character.isDigit(_))) | ||
| .map(_.toInt) | ||
| .getOrElse(default) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate MetalsServerConfig.scala:"
fd -a 'MetalsServerConfig\.scala$' . | sed 's#^\./##'
file="$(fd 'MetalsServerConfig\.scala$' . | head -n 1)"
echo "File: $file"
wc -l "$file"
echo "Outline:"
ast-grep outline "$file" --view compact || true
echo "Relevant intProperty usage/definition:"
rg -n "def intProperty|sys\.props|intProperty\(" "$file" -C 8
echo "Relevant lines:"
cat -n "$file" | sed -n '170,220p'
echo "Programmatic Scala property behavior via Scala REPL availability:"
if command -v scala >/dev/null 2>&1; then
scala -e 'println("forall empty:", "".forall(java.lang.Character.isDigit(_: Char))); println("String#toInt fails throws NumberFormatException", true ); try { println("toInt empty:", "".toInt) } catch { case e: java.lang.NumberFormatException => println("catch NumberFormatException empty", e.getMessage) }; try { println("toInt 2147483648:", "2147483648".toInt) } catch { case e: java.lang.NumberFormatException => println("catch NumberFormatException 2147483648", e.getMessage) }'
elif command -v scalac >/dev/null 2>&1; then
echo "scala runtime unavailable; scalac available"
else
echo "No Scala runtime/compiler available"
fiRepository: scalameta/metals
Length of output: 6779
🌐 Web query:
Scala String.forall empty string behavior String.toInt NumberFormatException
💡 Result:
In Scala, the behavior of String.forall and String.toInt are well-defined but often lead to confusion regarding empty strings and exception handling [1][2][3]. String.forall and Empty Strings The forall method on a String returns true if the string is empty [4][5]. This is because it follows the principle of vacuous truth in formal logic: if a collection is empty, then any predicate holds true for all of its (non-existent) elements [1][6]. If you need forall to return false for an empty string (for example, when validating that a string contains only digits), you must explicitly check for emptiness [1][2]: str.nonEmpty && str.forall(_.isDigit) String.toInt and NumberFormatException The toInt method on a String (provided by StringOps) attempts to parse the string as an integer [7][3]. If the string is not a valid representation of an integer (such as "abc" or an empty string ""), it throws a java.lang.NumberFormatException [7][3][8]. To avoid exceptions, it is recommended to use the toIntOption method (available since Scala 2.13), which returns an Option[Int]—Some(value) if successful, or None if the conversion fails [3][8]. Summary of best practices: - For validation: Always consider if you want to permit empty strings when using forall [1][2]. - For parsing: Prefer.toIntOption over.toInt to safely handle malformed input without relying on exception handling for control flow [9][8].
Citations:
- 1: https://stackoverflow.com/questions/15553945/scala-variable-with-empty-string-returning-true-when-checking-if-all-are-digits
- 2: https://devsolus.com/scala-checking-string-for-only-digits-behaving-strange/
- 3: https://www.baeldung.com/scala/convert-to-from-string
- 4: https://github.com/scala/scala/blob/v2.13.4/src/library/scala/collection/StringOps.scala
- 5: https://github.com/scala/scala3/blob/3.8.3/library/src/scala/collection/StringOps.scala
- 6: https://users.scala-lang.org/t/forall-semantics/3303
- 7: https://alvinalexander.com/scala/how-cast-string-to-int-in-scala-string-int-conversion/
- 8: https://softwaremill.com/fast-number-parsing-in-scala/
- 9: https://stackoverflow.com/questions/22021589/string-to-int-in-scala
Prevent malformed integer properties from failing MetalsServerConfig construction.
An empty property value passes String.forall, and any all-digit value outside the Int range also reaches toInt and throws. Reject empty strings and handle conversion failures before returning the default value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@metals/src/main/scala/scala/meta/internal/metals/MetalsServerConfig.scala`
around lines 195 - 200, Update intProperty to reject empty property values and
safely handle NumberFormatException from out-of-range digit strings, returning
default for both malformed and unparseable values while preserving valid integer
parsing.
There was a problem hiding this comment.
This is intentional, I've just consolidated it into one method. Worth a follow-up though.
There was a problem hiding this comment.
@matejcerny Understood. The consolidation preserves the existing behavior. The empty-value and integer-overflow handling can be addressed in a follow-up.
Would you like me to open a GitHub issue for that follow-up?
You are interacting with an AI system.
1 participant
Fixes the issue with the
glob-search: #8769glob-searchandtyped-glob-searchnow search all modulesfileInFocusparameter is removed-Dmetals.max-mcp-search-results, default100)Summary by CodeRabbit
New Features
Documentation