Skip to content

Escape URIs like the editor does and harden Dir.glob against metacharacters in paths - #4022

Open
andriytyurnikov wants to merge 1 commit into
Shopify:mainfrom
andriytyurnikov:bracket-file-fix
Open

Escape URIs like the editor does and harden Dir.glob against metacharacters in paths#4022
andriytyurnikov wants to merge 1 commit into
Shopify:mainfrom
andriytyurnikov:bracket-file-fix

Conversation

@andriytyurnikov

@andriytyurnikov andriytyurnikov commented Mar 20, 2026

Copy link
Copy Markdown

Fixes #3503. Contributes to #3639.

The problem

The server and the editor have to agree on how a file is spelled. A URI identifies a resource, so if indexing builds file:///a/%5Bid%5D.rb for a file the editor calls file:///a/[id].rb, the same file is tracked under two identities: indexed twice, reported twice, and never resolved to the document the editor has open.

There are two independent ways that agreement breaks today, and both are fixed here.

1. Our escaping did not match the editor's. URI::Generic.from_path used the RFC 2396 safe set. VS Code builds its URIs with vscode-uri, which only leaves the RFC 3986 unreserved set (A-Za-z0-9-._~) plus / unescaped. Checked against the package directly, 12 of 17 sample paths disagreed — not just on brackets, but on (, ), !, ', *, $, &, +, ,, ;, = and @. Every one of those is a standing source of the duplication tracked in #3639.

2. Paths were interpolated into glob patterns. Dir.glob reads [id] as a character class and {a,b} as an alternation, so a workspace or file path containing them silently matches nothing.

When both bite, every feature fails for the affected file: indexing, definition, references, hover, completion, diagnostics, code lens. The reporter in #3503 hit it on a generated path they could not control (/home/streetp/index[0]/example.rb); the names are also routine under routing conventions (app/models/[id].rb, {slug}.rb).

Related reports

This is one failure class with several faces. Fixing the spelling mismatch is what they have in common:

Issue Symptom
#3503 Bracketed paths fail to index
#3639 Duplicate entries when a didOpen races indexing
#3842 Duplicate definitions in Neovim, // vs /
#3285 Duplicate definitions on Windows, c%3A vs C%3A
#4024 A bracketed workspace root indexes zero files

#896 sketches the structural end state — URI identity based on components rather than on the exact string. This PR does not go there, but narrowing the encoding is a prerequisite either way: component-based equality still needs the components to agree.

Changes

URI escaping

Narrowing the safe set to [^\-_.~a-zA-Z\d/] brings the sample from 12 of 17 disagreeing to 17 of 17 matching, with every round trip intact. It also fixes from_path raising URI::InvalidComponentError for a path containing ?, which used to be treated as safe and produced a string URI::Generic.build rejected.

Glob hardening

  • Dir.glob's base: parameter in references.rb, rename.rb, addon.rb, test_style.rb and go_to_relevant_file.rb, so workspace and file paths are passed as literal paths rather than as patterns. This also works on Windows, where backslash escaping is unavailable.
  • RubyLsp.escape_glob_metacharacters for user-provided content interpolated into a pattern: require_relative content in the completion listener, and file names in go_to_relevant_file.rb. The comma is escaped too, because complete_require_relative interpolates into a brace group where an unescaped comma splits the alternation.

Expectation test harness

Adding a bracketed fixture surfaced a pre-existing bug. Seven expectation test classes built their URI as URI("file://#{@_path}") from a relative path, so file://test/fixtures/foo.rb parsed test as the URI authority and the path came out as /fixtures/foo.rb — the test/ segment silently dropped.

CodeLens appends -Itest when the path matches **/test/**/*, so with that segment missing five committed expectations asserted commands without -Itest, the opposite of what a user gets for a file genuinely under test/. Those are regenerated here; across all 324 modified lines the only changes are the restored /test/ segment and the flag it enables.

The runner also now looks expectation files up with File.file? instead of a glob, and sanitises fixture names when generating test method names.

Tests

Every test below was verified to fail when its fix is reverted, not merely to pass with it.

Test Covers
uri_test — parity, unreserved characters, ? escaping matches vscode-uri
store_test — client-encoded URI vs server-built URI the reported failure
addon_test — add-ons under [id]/{slug}/ addon.rb (escaping and glob in one path)
references_test, rename_test — bracketed workspace references.rb, rename.rb
resolve_test_commands_test — real directory, unstubbed test_style.rb test_dir expansion
go_to_relevant_file_test ×3 file name either direction, and parent directory
completion_test ×2 brackets and commas in require_relative
test/fixtures/[id].rb the expectations runner

The pre-existing test_dir tests stub Dir.glob, which bypasses the base: behaviour under test, so the new one hits the real file system. Without the sanitised method name, the fixture makes every expectation suite fail to parse with def test_hover__[id]__does_not_raise.

Deliberately not included

Windows drive letter case. vscode-uri normalises the drive to lower case regardless of input; from_path preserves whatever it is given, so indexing (upper case, from Dir.pwd) and the editor (lower case) still disagree. That is #3285, which is currently closed but only half fixed — evidence and a repro are in this comment. It is left out because two committed tests assert the current behaviour, and server.rb has case-sensitive path.start_with?(@global_state.workspace_path) guards that would have to change at the same time. Normalising only from_path would trade one Windows bug for another.

Bracketed workspace roots. configuration.rb still interpolates the workspace path, so a bracketed root indexes zero files. Converting it to base: triggers a Dir.glob bug: when top_level_directories is empty the pattern becomes {}/**/*.rb, and with base: that escapes the base and walks the file system from / — reproduced on Ruby 4.0.1 as Errno::EPERM on /Library/Application Support/com.apple.TCC. Tracked in #4024, which was auto-closed as stale and should be reopened. A follow-up branch fixes it by guarding the empty case.

// normalisation (#3842) is not addressed; this only covers URIs we build.

Test plan

  • bundle exec rake — 330 + 18,565 runs, 0 failures, 0 errors
  • bundle exec srb tc — no errors
  • bin/rubocop — no offenses
  • bin/tapioca check-shims — no duplicates
  • Escaping compared against the vscode-uri package directly — 17/17

@RoyLeviGit RoyLeviGit left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not run the lsp with this change but took a close look and ran AI diagnostics on it and it looks good to me. Thank you for solving this issue.

Comment thread lib/ruby_indexer/test/uri_test.rb Outdated
Comment thread lib/ruby_indexer/test/uri_test.rb Outdated
Comment thread lib/ruby_lsp/listeners/completion.rb Outdated

@RoyLeviGit RoyLeviGit left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as before, did not explicitly run this change but ran even more AI diagnostics on it including letting the AI suggest concerns and look at rest of repo for comparison and it can out approving PR and so will I :)

@andriytyurnikov
andriytyurnikov force-pushed the bracket-file-fix branch 2 times, most recently from 6ca1aec to 99c4d80 Compare April 27, 2026 22:31
@andriytyurnikov

Copy link
Copy Markdown
Author

@rafaelfranca - rebased successfully

@RoyLeviGit RoyLeviGit left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebase

@andriytyurnikov andriytyurnikov changed the title Fix bracket/brace filenames breaking LSP features Escape URIs like the editor does and harden Dir.glob against metacharacters in paths Jul 31, 2026
@andriytyurnikov

andriytyurnikov commented Jul 31, 2026

Copy link
Copy Markdown
Author

Rebased onto main; addon.rb conflicted with #4178. Escaping now matches vscode-uri exactly — 12 of 17 paths previously disagreed on ( ) & , + = @ ; ' ! * $, not just brackets, each a standing source of #3639 duplicates. Every hardened call site now has a regression test, verified to fail when its fix is reverted. Rationale, plus a harness fix touching committed expectations, is in the rewritten description.

…acters

Fixes Shopify#3503. Contributes to Shopify#3639.

A URI identifies a resource, so the server and the editor have to agree
on how a file is spelled. If indexing builds `file:///a/%5Bid%5D.rb` for
a file the editor calls `file:///a/[id].rb`, the same file is tracked
under two identities: indexed twice, reported twice, and never resolved
to the document the editor has open. Two independent things break that
agreement today.

`URI::Generic.from_path` used the RFC 2396 safe set. VS Code builds its
URIs with `vscode-uri`, which leaves only the RFC 3986 unreserved set
(`A-Za-z0-9-._~`) plus `/` unescaped. Compared against that package
directly, 12 of 17 sample paths disagreed, and not only on brackets:
`(`, `)`, `!`, `'`, `*`, `$`, `&`, `+`, `,`, `;`, `=` and `@` were all
left unescaped here and escaped there. Narrowing the safe set to
`[^\-_.~a-zA-Z\d/]` brings that to 17 of 17 with every round trip
intact, and fixes `from_path` raising `URI::InvalidComponentError` for a
path containing `?`, which used to be treated as safe and produced a
string `URI::Generic.build` rejected.

Separately, `Dir.glob` reads `[id]` as a character class and `{a,b}` as
an alternation, so interpolating a workspace or file path into a pattern
makes it silently match nothing. `references.rb`, `rename.rb`,
`addon.rb`, `test_style.rb` and `go_to_relevant_file.rb` now pass the
path as `base:`, which treats it literally on every platform, including
Windows, where backslash escaping is unavailable. Content that is
genuinely user input rather than a path is escaped instead, through
`RubyLsp.escape_glob_metacharacters`: `require_relative` content in the
completion listener and file names in `go_to_relevant_file.rb`. The
comma is escaped too, because `complete_require_relative` interpolates
into a brace group where an unescaped comma splits the alternation.

Adding a bracketed fixture surfaced a pre-existing bug in the test
harness. Seven expectation classes built their URI as
`URI("file://#{@_path}")` from a relative path, so
`file://test/fixtures/foo.rb` parsed `test` as the authority and the
path came out as `/fixtures/foo.rb`, dropping the `test/` segment.
`CodeLens` appends `-Itest` when the path matches `**/test/**/*`, so
with that segment missing, five committed expectations asserted commands
without `-Itest`, the opposite of what a user gets for a file genuinely
under `test/`. They are regenerated here; across all 324 modified lines
the only changes are the restored segment and the flag it enables. The
runner also looks expectations up with `File.file?` rather than a glob,
and sanitises fixture names when generating method names, without which
the new fixture yields `def test_hover__[id]__does_not_raise` and every
expectation suite fails to parse.

Every test added was verified to fail when its corresponding fix is
reverted.

The Windows drive letter case is deliberately left alone. `vscode-uri`
lower cases it regardless of input while `from_path` preserves it, which
is Shopify#3285, closed but only half fixed. Two committed tests assert the
current behaviour and `server.rb` has case-sensitive
`start_with?(@global_state.workspace_path)` guards that would have to
move at the same time, so normalising only `from_path` would trade one
Windows bug for another. Bracketed workspace roots are also out of scope
here and tracked in Shopify#4024.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Linux file system with square brackets in the directory/filename are failing to be indexed

4 participants