fix(lsp): Windows-correct file:// URI conversion (jdtls no-op on Windows) - #92
Merged
Merged
Conversation
…n Windows) The naive `"file://" + path` is correct on POSIX (an absolute path already starts with /, giving three slashes) but broken on Windows: C:\repo\Main.java becomes file://C:\repo\Main.java — the drive lands in the URI authority and backslashes are not separators. Inbound, the returned URI's path is /C:/repo/Main.java and a strings.HasPrefix against the C:\repo repoRoot always fails, so every LSP result is silently dropped. Net effect on native Windows: jdtls (and any LSP server) is detected and initialized, but every textDocument/didOpen + definition/references/ implementation round-trip no-ops — zero Java edges get enriched (ast_inferred -> lsp_resolved). POSIX was unaffected, so it went unnoticed. Centralise all path<->file-URI conversion in a new leaf package internal/lspuri (PathToURI / URIToAbsPath / URIToRepoRel), correct for both POSIX and Windows (drive-letter leading-slash, ToSlash/FromSlash, filepath.Rel instead of string prefix, %xx decode). Route all six previously-buggy sites through it: - internal/semantic/lsp/provider.go: pathToURI, uriToAbsPath, uriToPath - internal/semantic/lsp/resolver_helper.go: uriToAbsLocalPath - internal/mcp/diagnostics.go: pathToFileURI - internal/mcp/tools_simulate.go: normaliseEditURI The drive-letter / separator logic is unit-tested via pure helpers so the Windows path is exercised on the Linux/macOS CI runners (Windows only builds in CI). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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.
What
Centralises filesystem-path ↔
file://URI conversion in a new leaf packageinternal/lspuriand routes all six previously hand-rolled sites through it, fixing a Windows-only bug that made the LSP layer (jdtls etc.) a silent no-op.The bug
The naive
"file://" + pathis correct on POSIX — an absolute path already starts with/, sofile://+/repo/X.java=file:///repo/X.java(the required three slashes). On Windows it breaks:C:\repo\Main.java→file://C:\repo\Main.java— the drive letter lands in the URI authority and backslashes aren't URI separators, so the server can't match the document wedidOpen.file:///C:/repo/Main.java; the olduriToPathtookurl.Parse(...).Path=/C:/repo/Main.javaand didstrings.HasPrefix(absPath, repoRoot)againstC:\repo→ always false → the result is silently dropped.Net effect on native Windows: jdtls is detected and
initializesucceeds, but everydidOpen+definition/references/implementationround-trip no-ops — zero Java edges get enriched (ast_inferred→lsp_resolved), with no error logged. POSIX/macOS were unaffected, so it went unnoticed.Fix
New
internal/lspuri:PathToURI—C:\repo\Main.java→file:///C:/repo/Main.java;/repo/Main.java→file:///repo/Main.java(ToSlash, drive leading-slash,%xx-safe vianet/url).URIToAbsPath— inverse, strips the spurious leading slash before a drive letter,FromSlash, decodes%xx.URIToRepoRel— usesfilepath.Rel(case-insensitive on Windows) instead of a string prefix, so drive-casing / separators never cause a false miss.Routed through it (all had the same bug class):
internal/semantic/lsp/provider.go—pathToURI,uriToAbsPath,uriToPathinternal/semantic/lsp/resolver_helper.go—uriToAbsLocalPathinternal/mcp/diagnostics.go—pathToFileURIinternal/mcp/tools_simulate.go—normaliseEditURI(now also decodes%xxand drops the host viaurl)Tests
internal/lspuriunit tests exercise the drive-letter / separator logic via pure helpers, so the Windows behaviour is verified on the Linux/macOS CI runners (Windows only builds in CI, it doesn't run the suite).go build ./...,go vet,internal/lspuriandinternal/mcpsuites pass. (Pre-existing unrelated failureTestPassiveProvider_DialFailsNoFallbackreproduces onmainwithout this change — it needs a live LSP server.)Follow-up (not in this PR)
If jdtls still under-resolves after this, the next suspects are async readiness (no wait for jdtls workspace-import to finish before querying) and Java-project recognition (no
pom.xml/build.gradle⇒ no classpath).