fix(windows): quote paths in JDTLS and Lombok arguments #151

UrielCuriel · 2025-12-16T20:05:15Z

When the user path contains spaces (e.g. C:\Users\User Name), the Java process fails to start with a ClassNotFoundException because the paths in arguments are split by the shell/process.

This change:

Quotes the path for -Dosgi.sharedConfiguration.area in JDTLS launch args on Windows.
Quotes the path for -javaagent (Lombok) on Windows.
Adds unit tests to verify path escaping behavior across platforms.

#150

cla-bot · 2025-12-16T20:05:19Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Uriel Curiel.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

Copilot

Pull request overview

This PR fixes a critical bug where Java LSP server (JDTLS) fails to start on Windows when user paths contain spaces (e.g., C:\Users\User Name). The fix adds proper quoting for paths passed as command-line arguments on Windows.

Key changes:

Added escape_path_if_needed function to quote paths on Windows in the -Dosgi.sharedConfiguration.area JDTLS argument
Added format_lombok_agent_arg function to quote Lombok jar paths on Windows in the -javaagent argument
Added unit tests to verify path quoting behavior across different platforms (Windows, macOS, Linux)

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
src/jdtls.rs	Adds `escape_path_if_needed` helper function and applies it to the shared configuration path argument; includes tests for Windows and Unix platforms
src/java.rs	Adds `format_lombok_agent_arg` helper function to properly quote the Lombok agent path on Windows; includes tests for Windows and Unix platforms

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/jdtls.rs

src/java.rs

Copilot · 2025-12-16T20:09:37Z

src/jdtls.rs

+    #[test]
+    fn test_escape_path_if_needed_windows() {
+        let path = "C:\\Users\\User Name\\Projects\\zed-extension-java".to_string();
+        let escaped = escape_path_if_needed(path.clone(), Os::Windows);
+        assert_eq!(escaped, format!("\"{}\"", path));
+    }


Consider adding a test case that verifies paths without spaces are not affected (i.e., they should still be quoted on Windows for consistency, which is what the current implementation does). This would document that the function always quotes on Windows, regardless of whether spaces are present.

Copilot · 2025-12-16T20:09:37Z

src/java.rs

+    #[test]
+    fn test_format_lombok_agent_arg_windows() {
+        let path = "C:\\Users\\User Name\\lombok.jar";
+        let arg = format_lombok_agent_arg(path, Os::Windows);
+        assert_eq!(arg, format!("-javaagent:\"{path}\""));
+    }


Consider adding a test case for paths without spaces on Windows to verify they are also quoted (documenting the current behavior). This would make it clear that the function quotes all paths on Windows, not just those with spaces.

Copilot · 2025-12-16T20:09:37Z

src/jdtls.rs

+fn escape_path_if_needed(path: String, os: Os) -> String {
+    if os == Os::Windows {
+        format!("\"{}\"", path)
+    } else {
+        path
+    }
+}


The function name escape_path_if_needed may be misleading as it quotes the path rather than escaping special characters. Consider renaming to quote_path_on_windows or wrap_path_in_quotes_if_needed to better reflect the actual behavior.

tartarughina · 2025-12-16T20:33:42Z

src/java.rs

            let canonical_lombok_jar_path = path_to_string(current_dir.join(lombok_jar_path))?;

-            Some(format!("-javaagent:{canonical_lombok_jar_path}"))
+            Some(format_lombok_agent_arg(
+                &canonical_lombok_jar_path,
+                zed::current_platform().0,
+            ))


Rather than introducing a new function specific for Lombok, consider to update path_to_string to handle instances where a space exists within the path.
This would make the change system agnostic as the same could happen on Linux and Mac.

tartarughina · 2025-12-16T20:34:28Z

src/jdtls.rs

        format!(
            "-Dosgi.sharedConfiguration.area={}",
-            path_to_string(shared_config_path)?
+            escape_path_if_needed(path_to_string(shared_config_path)?, current_platform().0)


The benefit of changing path_to_string are clear here for example

tartarughina · 2025-12-16T20:35:37Z

src/jdtls.rs

+fn escape_path_if_needed(path: String, os: Os) -> String {
+    if os == Os::Windows {
+        format!("\"{}\"", path)
+    } else {
+        path
+    }
+}


If you want to reuse this function for the suggested change to path_to_string please move it to util.rs as that's the proper place where to have all utility functions.

tartarughina · 2025-12-16T20:38:00Z

src/jdtls.rs

+#[cfg(test)]
+mod tests {
+    use super::*;
+    use zed_extension_api::Os;
+
+    #[test]
+    fn test_escape_path_if_needed_windows() {
+        let path = "C:\\Users\\User Name\\Projects\\zed-extension-java".to_string();
+        let escaped = escape_path_if_needed(path.clone(), Os::Windows);
+        assert_eq!(escaped, format!("\"{}\"", path));
+    }


Thanks for adding the tests. Let's move them to util.rs following above comments and change them to wrap the path in double quotes if the string contains a space.

cla-bot · 2025-12-16T23:27:32Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Uriel Curiel.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

tartarughina

Thanks for quickly addressing. It is good by me.

playdohface

Hey, thanks for this great catch and the contribution! Generally, this solution is fine for me but I have a few suggestions for your consideration:

Copilot has a point here that "escape" will commonly be understood as adding an escape character before certain characters, and the more intuitive name would be "quote_path_if_needed".
But I think we can actually just quote all paths regardless of whether they contain a space or not. This will have the benefit of being simpler, marginally more performant (though this is negligible here) but most importantly making it immediately obvious should the quoting be problematic at some point (which I don't think it would but I am wrong a lot). Then we can just rename path_to_string to path_to_quoted_string to make it very obvious what is happening and maybe have a single test asserting that paths do indeed get quoted with a double-quote when passing through this method.

In any case, you'll have to run cargo fmt and cargo clippy and make the CLA-Bot happy (your local git needs to be set up with an E-Mail and you have to sign the Zed license agreement) so we can land this one.

tartarughina · 2025-12-18T11:34:54Z

Hey, thanks for this great catch and the contribution! Generally, this solution is fine for me but I have a few suggestions for your consideration:

Copilot has a point here that "escape" will commonly be understood as adding an escape character before certain characters, and the more intuitive name would be "quote_path_if_needed".

But I think we can actually just quote all paths regardless of whether they contain a space or not. This will have the benefit of being simpler, marginally more performant (though this is negligible here) but most importantly making it immediately obvious should the quoting be problematic at some point (which I don't think it would but I am wrong a lot). Then we can just rename path_to_string to path_to_quoted_string to make it very obvious what is happening and maybe have a single test asserting that paths do indeed get quoted with a double-quote when passing through this method.

In any case, you'll have to run cargo fmt and cargo clippy and make the CLA-Bot happy (your local git needs to be set up with an E-Mail and you have to sign the Zed license agreement) so we can land this one.

As pointed out by @playdohface let's remove the check for spaces.
It should indeed not cause any harm. And let's rename the function with his suggested name.
Overall it is going to be a much simpler solution.

And yes, be sure to run all formatting, clippy checks and sign the Zed agreement.

cla-bot · 2025-12-18T18:22:02Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Uriel Curiel.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

UrielCuriel · 2025-12-18T18:24:23Z

@playdohface Thanks for the feedback! I have a few questions about the renaming:

Should I rename path_to_string → path_to_quoted_string in this PR, or will you handle that separately? I want to avoid breaking anything since it's used in multiple places throughout the codebase.

Also, if we're renaming it, would it make sense to inline the quote_path() helper function directly into path_to_quoted_string to simplify further? Something like:

pub fn path_to_quoted_string<P: AsRef<Path>>(path: P) -> zed::Result<String> {
    path.as_ref()
        .to_path_buf()
        .into_os_string()
        .into_string()
        .map(|s| format!("\"{}\"", s))
        .map_err(|_| PATH_TO_STR_ERROR. to_string())
}

This would eliminate one function and make it even more obvious what's happening.

Regarding cargo fmt and cargo clippy: I did run both commands, but they modified some files I hadn't touched in this PR. I didn't include those changes to keep the PR focused, but I'm happy to include them in the next commit.

Let me know and I'll update accordingly. In the meantime, I'll sign the CLA. Thanks!

When the user path contains spaces (e.g. `C:\Users\User Name`), the Java process fails to start with a `ClassNotFoundException` because the paths in arguments are split by the shell/process. This change: - Quotes the path for `-Dosgi.sharedConfiguration.area` in JDTLS launch args on Windows. - Quotes the path for `-javaagent` (Lombok) on Windows. - Adds unit tests to verify path escaping behavior across platforms. Closes zed-extensions#150

- Quote paths centrally in path_to_string and drop OS-specific helpers - Pass unescaped JDTLS shared config path and simplify Lombok -javaagent arg formatting - Move the path escaping helper and its tests into the shared utilities `src/util.rs`

- Rename escape_path_if_needed to quote_path - Remove space-checking logic, always quote paths - Update tests to reflect simplified behavior

cla-bot · 2025-12-18T18:49:44Z

We require contributors to sign our Contributor License Agreement, and we don't have @UrielCuriel on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'.

UrielCuriel · 2025-12-18T18:59:35Z

@cla-bot check

cla-bot · 2025-12-18T18:59:38Z

The cla-bot has been summoned, and re-checked this pull request!

tartarughina · 2025-12-18T19:03:33Z

Yes rename it in this PR and update any reference. I also like the suggested change to add that map inline. Overall much cleaner and contained

- Rename path_to_string to path_to_quoted_string to reflect its dual purpose - Merge internal quoting logic directly into the function and remove the quote_path helper - Update all call sites across debugger, java, jdk, and jdtls modules - Ensure consistent string representation for paths used in command-line arguments

playdohface

Looks good to me now, thanks!

Copilot AI review requested due to automatic review settings December 16, 2025 20:05

Copilot started reviewing on behalf of UrielCuriel December 16, 2025 20:05 View session

Copilot AI reviewed Dec 16, 2025

View reviewed changes

tartarughina self-assigned this Dec 16, 2025

tartarughina requested changes Dec 16, 2025

View reviewed changes

tartarughina previously approved these changes Dec 16, 2025

View reviewed changes

playdohface reviewed Dec 18, 2025

View reviewed changes

UrielCuriel dismissed tartarughina’s stale review via bb0cd41 December 18, 2025 18:21

UrielCuriel added 3 commits December 18, 2025 12:49

Normalize path quoting

f9bc117

- Quote paths centrally in path_to_string and drop OS-specific helpers - Pass unescaped JDTLS shared config path and simplify Lombok -javaagent arg formatting - Move the path escaping helper and its tests into the shared utilities `src/util.rs`

refactor: simplify path quoting to always add quotes

efa89d3

- Rename escape_path_if_needed to quote_path - Remove space-checking logic, always quote paths - Update tests to reflect simplified behavior

UrielCuriel force-pushed the fix-windows-unquoted-path branch from bb0cd41 to efa89d3 Compare December 18, 2025 18:49

cla-bot bot added the cla-signed label Dec 18, 2025

playdohface approved these changes Dec 18, 2025

View reviewed changes

Merge branch 'main' into fix-windows-unquoted-path

9086f1d

playdohface enabled auto-merge (squash) December 19, 2025 09:57

fix(windows): quote paths in JDTLS and Lombok arguments #151

Are you sure you want to change the base?

fix(windows): quote paths in JDTLS and Lombok arguments #151

Conversation

UrielCuriel commented Dec 16, 2025

Uh oh!

cla-bot bot commented Dec 16, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

tartarughina Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

tartarughina Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

tartarughina Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

tartarughina Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

cla-bot bot commented Dec 16, 2025

Uh oh!

tartarughina left a comment

Choose a reason for hiding this comment

Uh oh!

playdohface left a comment

Choose a reason for hiding this comment

Uh oh!

tartarughina commented Dec 18, 2025

Uh oh!

cla-bot bot commented Dec 18, 2025

Uh oh!

UrielCuriel commented Dec 18, 2025

Uh oh!

cla-bot bot commented Dec 18, 2025

Uh oh!

UrielCuriel commented Dec 18, 2025

Uh oh!

cla-bot bot commented Dec 18, 2025

Uh oh!

tartarughina commented Dec 18, 2025

Uh oh!

playdohface left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants