Index workspaces whose path contains glob metacharacters

lib/ruby_indexer/lib/ruby_indexer/configuration.rb

@@ -39,7 +39,15 @@ def initialize
 
       # We start the included patterns with only the non excluded directories so that we can avoid paying the price of
       # traversing large directories that don't include Ruby files like `node_modules`
-      @included_patterns = ["{#{top_level_directories.join(",")}}/**/*.rb", "*.rb"] #: Array[String]
+      #
+      # The empty case has to be handled separately. Joining an empty list yields `{}/**/*.rb`, and `Dir.glob` mishandles
+      # an empty brace group when it is given a `base:`, escaping the base and walking the file system from the root
+      indexable_directories = top_level_directories
+      @included_patterns = if indexable_directories.empty?
+        ["*.rb"]
+      else
+        ["{#{indexable_directories.join(",")}}/**/*.rb", "*.rb"]
+      end #: Array[String]
       @excluded_magic_comments = [
         "frozen_string_literal:",
         "typed:",
@@ -68,7 +76,12 @@ def indexable_uris
       uris = @included_patterns.flat_map do |pattern|
         load_path_entry = nil #: String?
 
-        Dir.glob(File.join(@workspace_path, pattern), flags).map! do |path|
+        # The workspace path is passed as `base:` rather than interpolated into the pattern, so that a path containing
+        # glob metacharacters such as `[id]` or `{slug}` is treated as a literal directory rather than as a character
+        # class or an alternation
+        Dir.glob(pattern, flags, base: @workspace_path).map! do |relative_path|
+          path = File.join(@workspace_path, relative_path)
+
           # All entries for the same pattern match the same $LOAD_PATH entry. Since searching the $LOAD_PATH for every
           # entry is expensive, we memoize it until we find a path that doesn't belong to that $LOAD_PATH. This happens
           # on repositories that define multiple gems, like Rails. All frameworks are defined inside the current
@@ -81,23 +94,27 @@ def indexable_uris
         end
       end
 
-      # If the patterns are relative, we make it relative to the workspace path. If they are absolute, then we shouldn't
-      # concatenate anything
-      excluded_patterns = @excluded_patterns.map do |pattern|
-        if File.absolute_path?(pattern)
-          pattern
-        else
-          File.join(@workspace_path, pattern)
-        end
+      # Absolute patterns are matched against the absolute path. Relative ones are matched against the path relative to
+      # the workspace, rather than concatenating the workspace path onto the pattern, because a workspace path holding
+      # glob metacharacters would turn into a character class or an alternation and silently match nothing
+      absolute_excluded_patterns, relative_excluded_patterns = @excluded_patterns.partition do |pattern|
+        File.absolute_path?(pattern)
       end
 
+      # The workspace path originates from a client supplied URI, which may carry a trailing slash, so normalize it
+      # before using it as a prefix
+      workspace_prefix = "#{@workspace_path.delete_suffix("/")}/"
+
       # Remove user specified patterns
       bundle_path = Bundler.settings["path"]&.gsub(/[\\]+/, "/")
       uris.reject! do |indexable|
         path = indexable.full_path #: as !nil
         next false if test_files_ignored_from_exclusion?(path, bundle_path)
 
-        excluded_patterns.any? { |pattern| File.fnmatch?(pattern, path, flags) }
+        next true if absolute_excluded_patterns.any? { |pattern| File.fnmatch?(pattern, path, flags) }
+
+        relative_path = path.delete_prefix(workspace_prefix)
+        relative_excluded_patterns.any? { |pattern| File.fnmatch?(pattern, relative_path, flags) }
       end
 
       # Add default gems to the list of files to be indexed
@@ -151,8 +168,8 @@ def indexable_uris
         uris.concat(
           spec.require_paths.flat_map do |require_path|
             load_path_entry = File.join(spec.full_gem_path, require_path)
-            Dir.glob(File.join(load_path_entry, "**", "*.rb")).map! do |path|
-              URI::Generic.from_path(path: path, load_path_entry: load_path_entry)
+            Dir.glob(File.join("**", "*.rb"), base: load_path_entry).map! do |relative_path|
+              URI::Generic.from_path(path: File.join(load_path_entry, relative_path), load_path_entry: load_path_entry)
             end
           end,
         )
@@ -265,9 +282,14 @@ def test_files_ignored_from_exclusion?(path, bundle_path)
     def top_level_directories
       excluded_directories = ["tmp", "node_modules", "sorbet"]
 
-      Dir.glob("#{Dir.pwd}/*").filter_map do |path|
-        dir_name = File.basename(path)
-        next unless File.directory?(path) && !excluded_directories.include?(dir_name)
+      # The working directory is passed as `base:` so that it is treated literally. Interpolating it into the pattern
+      # makes a directory such as `[id]` a character class, which matches nothing and leaves the project with no
+      # included patterns at all
+      current_directory = Dir.pwd
+
+      Dir.glob("*", base: current_directory).filter_map do |dir_name|
+        next unless File.directory?(File.join(current_directory, dir_name))
+        next if excluded_directories.include?(dir_name)
 
         dir_name
       end

lib/ruby_indexer/lib/ruby_indexer/uri.rb

@@ -15,10 +15,14 @@ class Generic
     class << self
       #: (path: String, ?fragment: String?, ?scheme: String, ?load_path_entry: String?) -> URI::Generic
       def from_path(path:, fragment: nil, scheme: "file", load_path_entry: nil)
-        # This unsafe regex is the same one used in the URI::RFC2396_REGEXP class with the exception of the fact that we
-        # do not include colon as a safe character. VS Code URIs always escape colons and we need to ensure we do the
-        # same to avoid inconsistencies in our URIs, which are used to identify resources
-        unsafe_regex = %r{[^\-_.!~*'()a-zA-Z\d;/?@&=+$,\[\]]}
+        # Our URIs are used to identify resources, so they must be escaped exactly the way the editor escapes them.
+        # Otherwise the same file ends up tracked under two different URIs: one built here during indexing and one
+        # received from the editor, which results in duplicate index entries (see Shopify/ruby-lsp#3639).
+        #
+        # VS Code's `vscode-uri` only leaves the RFC 3986 unreserved characters unescaped (`A-Za-z0-9-._~`), plus the
+        # forward slash in paths. Everything else, including characters that RFC 2396 considers safe such as `(`, `)`,
+        # `!`, `'`, `*`, `$`, `&`, `+`, `,`, `;`, `=`, `@`, `[`, `]` and `:`, is percent-encoded
+        unsafe_regex = %r{[^\-_.~a-zA-Z\d/]}
 
         # On Windows, if the path begins with the disk name, we need to add a leading slash to make it a valid URI
         escaped_path = if /^[A-Z]:/i.match?(path)

lib/ruby_indexer/test/configuration_test.rb

@@ -188,6 +188,88 @@ def test_includes_top_level_files
       end
     end
 
+    # Routing conventions such as Rails' or Next.js' dynamic segments produce directory names like `[id]` or `{slug}`,
+    # and generated paths may contain them anywhere. They are `Dir.glob` metacharacters, so interpolating the workspace
+    # path into a pattern makes it match nothing at all and the whole project indexes empty
+    def test_indexable_uris_with_glob_metacharacters_in_the_workspace_path
+      Dir.mktmpdir do |dir|
+        workspace = File.join(dir, "[id]", "{slug}")
+        FileUtils.mkdir_p(File.join(workspace, "app"))
+        FileUtils.touch(File.join(workspace, "app", "nested.rb"))
+        FileUtils.touch(File.join(workspace, "top_level.rb"))
+
+        # `top_level_directories` reads `Dir.pwd`, so the working directory has to be the bracketed workspace for the
+        # included patterns to be built from it
+        Dir.chdir(workspace) do
+          config = Configuration.new
+          config.workspace_path = workspace
+
+          paths = config.indexable_uris.map(&:full_path)
+          assert_includes(paths, File.join(workspace, "app", "nested.rb"))
+          assert_includes(paths, File.join(workspace, "top_level.rb"))
+        end
+      end
+    end
+
+    def test_excluded_patterns_apply_when_the_workspace_path_has_glob_metacharacters
+      Dir.mktmpdir do |dir|
+        workspace = File.join(dir, "[id]", "{slug}")
+        FileUtils.mkdir_p(File.join(workspace, "ignore"))
+        FileUtils.touch(File.join(workspace, "ignore", "excluded.rb"))
+        FileUtils.touch(File.join(workspace, "kept.rb"))
+
+        Dir.chdir(workspace) do
+          config = Configuration.new
+          config.workspace_path = workspace
+          config.apply_config({ "excluded_patterns" => ["ignore/**/*.rb"] })
+
+          paths = config.indexable_uris.map(&:full_path)
+          assert_includes(paths, File.join(workspace, "kept.rb"))
+          refute_includes(paths, File.join(workspace, "ignore", "excluded.rb"))
+        end
+      end
+    end
+
+    # The workspace path comes from a client supplied URI, which may include a trailing slash. Exclusion patterns are
+    # matched against the workspace relative path, so the prefix has to be normalized before it is stripped
+    def test_excluded_patterns_apply_when_the_workspace_path_has_a_trailing_slash
+      Dir.mktmpdir do |dir|
+        FileUtils.mkdir_p(File.join(dir, "ignore"))
+        FileUtils.touch(File.join(dir, "ignore", "excluded.rb"))
+        FileUtils.touch(File.join(dir, "kept.rb"))
+
+        @config.workspace_path = "#{dir}/"
+        # The included pattern has to actually reach the file, otherwise the exclusion below is never exercised and the
+        # assertion passes for the wrong reason
+        @config.apply_config({ "included_patterns" => ["**/*.rb"], "excluded_patterns" => ["ignore/**/*.rb"] })
+
+        paths = @config.indexable_uris.map(&:full_path)
+        assert_includes(paths, File.join(dir, "kept.rb"))
+        refute_includes(paths, File.join(dir, "ignore", "excluded.rb"))
+      end
+    end
+
+    # A workspace with no indexable subdirectories makes `top_level_directories` return an empty array, so the included
+    # pattern degenerates to `{}/**/*.rb`. `Dir.glob` mishandles that with a `base:` argument, escaping the base and
+    # walking the file system from the root, so the empty case must never reach the glob
+    def test_indexable_uris_in_a_workspace_without_indexable_subdirectories
+      Dir.mktmpdir do |dir|
+        FileUtils.touch(File.join(dir, "only.rb"))
+
+        Dir.chdir(dir) do
+          config = Configuration.new
+          config.workspace_path = dir
+
+          paths = config.indexable_uris.map(&:full_path)
+          assert_includes(paths, File.join(dir, "only.rb"))
+          assert(
+            paths.compact.none? { |path| path.start_with?("/Library", "/System") },
+            "expected the glob to stay within the workspace and known gem paths",
+          )
+        end
+      end
+    end
+
     def test_transitive_dependencies_for_non_dev_gems_are_not_excluded
       Dir.mktmpdir do |dir|
         Dir.chdir(dir) do

lib/ruby_indexer/test/uri_test.rb

@@ -81,5 +81,100 @@ def test_from_path_with_unicode_characters
       assert_equal(path, uri.to_standardized_path)
       assert_equal("file:///path/with/unicode/%E6%96%87%E4%BB%B6.rb", uri.to_s)
     end
+
+    def test_from_path_with_brackets
+      uri = URI::Generic.from_path(path: "/some/path/[id].rb")
+      assert_equal("file:///some/path/%5Bid%5D.rb", uri.to_s)
+    end
+
+    def test_from_path_with_braces
+      uri = URI::Generic.from_path(path: "/some/path/{slug}.rb")
+      assert_equal("file:///some/path/%7Bslug%7D.rb", uri.to_s)
+    end
+
+    def test_round_trip_with_brackets
+      path = "/some/path/[id].rb"
+      uri = URI::Generic.from_path(path: path)
+      assert_equal(path, uri.to_standardized_path)
+    end
+
+    def test_round_trip_with_braces
+      path = "/some/path/{slug}.rb"
+      uri = URI::Generic.from_path(path: path)
+      assert_equal(path, uri.to_standardized_path)
+    end
+
+    def test_from_path_with_parentheses
+      uri = URI::Generic.from_path(path: "/some/path/(id).rb")
+      assert_equal("file:///some/path/%28id%29.rb", uri.to_s)
+    end
+
+    def test_round_trip_with_parentheses
+      path = "/some/path/(id).rb"
+      uri = URI::Generic.from_path(path: path)
+      assert_equal(path, uri.to_standardized_path)
+    end
+
+    def test_round_trip_with_spaces_inside_brackets
+      path = "/some/path/[id page].rb"
+      uri = URI::Generic.from_path(path: path)
+      assert_equal(path, uri.to_standardized_path)
+      assert_equal("file:///some/path/%5Bid%20page%5D.rb", uri.to_s)
+    end
+
+    def test_round_trip_with_spaces_inside_braces
+      path = "/some/path/{slug name}.rb"
+      uri = URI::Generic.from_path(path: path)
+      assert_equal(path, uri.to_standardized_path)
+      assert_equal("file:///some/path/%7Bslug%20name%7D.rb", uri.to_s)
+    end
+
+    def test_round_trip_with_spaces_inside_parentheses
+      path = "/some/path/file (copy).rb"
+      uri = URI::Generic.from_path(path: path)
+      assert_equal(path, uri.to_standardized_path)
+      assert_equal("file:///some/path/file%20%28copy%29.rb", uri.to_s)
+    end
+
+    # The editor and the server must escape paths identically, otherwise the same file is tracked under two different
+    # URIs and the index ends up with duplicate entries (Shopify/ruby-lsp#3639). These expectations were captured from
+    # the `vscode-uri` package, which is what VS Code uses to build the URIs it sends us
+    def test_from_path_escapes_every_character_the_editor_escapes
+      {
+        "/some/path/[id].rb" => "file:///some/path/%5Bid%5D.rb",
+        "/some/path/{slug}.rb" => "file:///some/path/%7Bslug%7D.rb",
+        "/some/path/(id).rb" => "file:///some/path/%28id%29.rb",
+        "/some/path/a&b.rb" => "file:///some/path/a%26b.rb",
+        "/some/path/a,b.rb" => "file:///some/path/a%2Cb.rb",
+        "/some/path/a+b.rb" => "file:///some/path/a%2Bb.rb",
+        "/some/path/a=b.rb" => "file:///some/path/a%3Db.rb",
+        "/some/path/a@b.rb" => "file:///some/path/a%40b.rb",
+        "/some/path/a;b.rb" => "file:///some/path/a%3Bb.rb",
+        "/some/path/a!b.rb" => "file:///some/path/a%21b.rb",
+        "/some/path/a*b.rb" => "file:///some/path/a%2Ab.rb",
+        "/some/path/a$b.rb" => "file:///some/path/a%24b.rb",
+        "/some/path/it's.rb" => "file:///some/path/it%27s.rb",
+        "/some/path/with space.rb" => "file:///some/path/with%20space.rb",
+      }.each do |path, expected|
+        uri = URI::Generic.from_path(path: path)
+        assert_equal(expected, uri.to_s)
+        assert_equal(path, uri.to_standardized_path)
+      end
+    end
+
+    def test_from_path_does_not_escape_unreserved_characters
+      path = "/some/path/a-b_c.d~e.rb"
+      uri = URI::Generic.from_path(path: path)
+      assert_equal("file:///some/path/a-b_c.d~e.rb", uri.to_s)
+      assert_equal(path, uri.to_standardized_path)
+    end
+
+    def test_from_path_with_question_mark
+      # A question mark used to be treated as safe, which made `URI::Generic.build` reject the escaped path for not
+      # being a valid absolute path component
+      uri = URI::Generic.from_path(path: "/some/path/what?.rb")
+      assert_equal("file:///some/path/what%3F.rb", uri.to_s)
+      assert_equal("/some/path/what?.rb", uri.to_standardized_path)
+    end
   end
 end

lib/ruby_lsp/addon.rb

@@ -62,7 +62,9 @@ def load_addons(global_state, outgoing_queue, include_project_addons: true)
             nil
           end
 
-          project_addons = Dir.glob("#{global_state.workspace_path}/**/ruby_lsp/**/addon.rb")
+          project_addons = Dir.glob("**/ruby_lsp/**/addon.rb", base: global_state.workspace_path).map! do |relative_path|
+            File.join(global_state.workspace_path, relative_path)
+          end
           project_addons.reject! do |path|
             (bundle_path && path.start_with?(bundle_path)) || gem_installation_path?(path)
           end

lib/ruby_lsp/listeners/completion.rb

@@ -457,10 +457,11 @@ def complete_require_relative(node)
         # if the path is not a directory, glob all possible next characters
         # for example ../somethi| (where | is the cursor position)
         # should find files for ../somethi*/
+        escaped_content = RubyLsp.escape_glob_metacharacters(content)
         path_query = if content.end_with?("/") || content.empty?
-          "#{content}**/*.rb"
+          "#{escaped_content}**/*.rb"
         else
-          "{#{content}*/**/*.rb,**/#{content}*.rb}"
+          "{#{escaped_content}*/**/*.rb,**/#{escaped_content}*.rb}"
         end
 
         Dir.glob(path_query, File::FNM_PATHNAME | File::FNM_EXTGLOB, base: origin_dir).sort!.each do |path|

lib/ruby_lsp/listeners/test_style.rb

@@ -35,9 +35,10 @@ def resolve_test_commands(items)
               if children.empty?
                 full_files.concat(
                   Dir.glob(
-                    "#{path}/**/{*_test,test_*,*_spec}.rb",
+                    "**/{*_test,test_*,*_spec}.rb",
                     File::Constants::FNM_EXTGLOB | File::Constants::FNM_PATHNAME,
-                  ).map! { |p| Shellwords.escape(p) },
+                    base: path,
+                  ).map! { |p| Shellwords.escape(File.join(path, p)) },
                 )
               end
             elsif tags.include?("test_file")

lib/ruby_lsp/requests/go_to_relevant_file.rb

@@ -36,9 +36,10 @@ def perform
       #: -> Array[String]
       def find_relevant_paths
         patterns = relevant_filename_patterns
+        root = search_root
 
         candidate_paths = patterns.flat_map do |pattern|
-          Dir.glob(File.join(search_root, "**", pattern))
+          Dir.glob(File.join("**", pattern), base: root).map! { |p| File.join(root, p) }
         end
 
         return [] if candidate_paths.empty?
@@ -89,22 +90,25 @@ def relevant_filename_patterns
           # Test file -> find implementation
           base = input_basename.gsub(TEST_PATTERN, "")
           parent_dir = File.basename(File.dirname(@path))
+          escaped_base = RubyLsp.escape_glob_metacharacters(base)
+          escaped_parent_dir = RubyLsp.escape_glob_metacharacters(parent_dir)
 
           # If test file is in a directory matching the implementation name
           # (e.g., go_to_relevant_file/test_go_to_relevant_file_a.rb)
           # return patterns for both the base file name and the parent directory name
           if base.include?(parent_dir) && base != parent_dir
-            ["#{base}#{extension}", "#{parent_dir}#{extension}"]
+            ["#{escaped_base}#{extension}", "#{escaped_parent_dir}#{extension}"]
           else
-            ["#{base}#{extension}"]
+            ["#{escaped_base}#{extension}"]
           end
         else
           # Implementation file -> find tests (including in matching directory)
+          escaped_basename = RubyLsp.escape_glob_metacharacters(input_basename)
           [
-            "{#{TEST_PREFIX_GLOB}}#{input_basename}#{extension}",
-            "#{input_basename}{#{TEST_SUFFIX_GLOB}}#{extension}",
-            "#{input_basename}/{#{TEST_PREFIX_GLOB}}*#{extension}",
-            "#{input_basename}/*{#{TEST_SUFFIX_GLOB}}#{extension}",
+            "{#{TEST_PREFIX_GLOB}}#{escaped_basename}#{extension}",
+            "#{escaped_basename}{#{TEST_SUFFIX_GLOB}}#{extension}",
+            "#{escaped_basename}/{#{TEST_PREFIX_GLOB}}*#{extension}",
+            "#{escaped_basename}/*{#{TEST_SUFFIX_GLOB}}#{extension}",
           ]
         end
       end

lib/ruby_lsp/requests/references.rb

@@ -60,7 +60,8 @@ def perform
         reference_target = create_reference_target(target, node_context)
         return @locations unless reference_target
 
-        Dir.glob(File.join(@global_state.workspace_path, "**/*.rb")).each do |path|
+        Dir.glob("**/*.rb", base: @global_state.workspace_path).each do |relative_path|
+          path = File.join(@global_state.workspace_path, relative_path)
           uri = URI::Generic.from_path(path: path)
           # If the document is being managed by the client, then we should use whatever is present in the store instead
           # of reading from disk