From da003e08d714e0b4ec5176fc19e5a43f09f068bc Mon Sep 17 00:00:00 2001 From: charlie Date: Fri, 26 Dec 2025 13:51:19 -0600 Subject: [PATCH 1/4] fix(language_server): handle invalid root URI gracefully When LSP clients send an invalid rootUri (e.g., 'file://' with no path), to_file_path() returns None. Previously this caused a panic via unwrap(). Now build_boxed() returns Option>, allowing worker to skip tools that fail to initialize. Server responds with capabilities instead of crashing. --- .../src/formatter/server_formatter.rs | 25 +++++++++------- .../src/formatter/tester.rs | 1 + .../src/linter/server_linter.rs | 30 ++++++++++++------- .../oxc_language_server/src/linter/tester.rs | 1 + crates/oxc_language_server/src/tests.rs | 8 ++--- crates/oxc_language_server/src/tool.rs | 3 +- crates/oxc_language_server/src/worker.rs | 16 ++++------ 7 files changed, 47 insertions(+), 37 deletions(-) diff --git a/crates/oxc_language_server/src/formatter/server_formatter.rs b/crates/oxc_language_server/src/formatter/server_formatter.rs index ad90d15486880..5655d78b84fff 100644 --- a/crates/oxc_language_server/src/formatter/server_formatter.rs +++ b/crates/oxc_language_server/src/formatter/server_formatter.rs @@ -21,9 +21,7 @@ use crate::{ pub struct ServerFormatterBuilder; impl ServerFormatterBuilder { - /// # Panics - /// Panics if the root URI cannot be converted to a file path. - pub fn build(root_uri: &Uri, options: serde_json::Value) -> ServerFormatter { + pub fn build(root_uri: &Uri, options: serde_json::Value) -> Option { let options = match serde_json::from_value::(options) { Ok(opts) => opts, Err(err) => { @@ -34,7 +32,7 @@ impl ServerFormatterBuilder { } }; - let root_path = root_uri.to_file_path().unwrap(); + let root_path = root_uri.to_file_path()?; let oxfmtrc = Self::get_config(&root_path, options.config_path.as_ref()); let (format_options, oxfmt_options) = Self::get_options(oxfmtrc); @@ -49,7 +47,7 @@ impl ServerFormatterBuilder { } }; - ServerFormatter::new(format_options, gitignore_glob) + Some(ServerFormatter::new(format_options, gitignore_glob)) } } @@ -62,8 +60,8 @@ impl ToolBuilder for ServerFormatterBuilder { capabilities.document_formatting_provider = Some(tower_lsp_server::ls_types::OneOf::Left(true)); } - fn build_boxed(&self, root_uri: &Uri, options: serde_json::Value) -> Box { - Box::new(ServerFormatterBuilder::build(root_uri, options)) + fn build_boxed(&self, root_uri: &Uri, options: serde_json::Value) -> Option> { + ServerFormatterBuilder::build(root_uri, options).map(|f| Box::new(f) as Box) } } @@ -162,8 +160,6 @@ impl Tool for ServerFormatter { fn name(&self) -> &'static str { "formatter" } - /// # Panics - /// Panics if the root URI cannot be converted to a file path. fn handle_configuration_change( &self, root_uri: &Uri, @@ -196,7 +192,11 @@ impl Tool for ServerFormatter { return ToolRestartChanges { tool: None, watch_patterns: None }; } - let new_formatter = ServerFormatterBuilder::build(root_uri, new_options_json.clone()); + let Some(new_formatter) = ServerFormatterBuilder::build(root_uri, new_options_json.clone()) + else { + warn!("Failed to build formatter: root URI cannot be converted to file path"); + return ToolRestartChanges { tool: None, watch_patterns: None }; + }; let watch_patterns = new_formatter.get_watcher_patterns(new_options_json); ToolRestartChanges { tool: Some(Box::new(new_formatter)), @@ -230,7 +230,10 @@ impl Tool for ServerFormatter { ) -> ToolRestartChanges { // TODO: Check if the changed file is actually a config file - let new_formatter = ServerFormatterBuilder::build(root_uri, options); + let Some(new_formatter) = ServerFormatterBuilder::build(root_uri, options) else { + warn!("Failed to build formatter: root URI cannot be converted to file path"); + return ToolRestartChanges { tool: None, watch_patterns: None }; + }; ToolRestartChanges { tool: Some(Box::new(new_formatter)), diff --git a/crates/oxc_language_server/src/formatter/tester.rs b/crates/oxc_language_server/src/formatter/tester.rs index 585d250ea5b1f..c6472199d9290 100644 --- a/crates/oxc_language_server/src/formatter/tester.rs +++ b/crates/oxc_language_server/src/formatter/tester.rs @@ -59,6 +59,7 @@ impl Tester<'_> { &Self::get_root_uri(self.relative_root_dir), self.options.clone(), ) + .expect("test requires valid root URI") } pub fn get_root_uri(relative_root_dir: &str) -> Uri { diff --git a/crates/oxc_language_server/src/linter/server_linter.rs b/crates/oxc_language_server/src/linter/server_linter.rs index 03bf7aa1a3dc1..63c0c7232e32b 100644 --- a/crates/oxc_language_server/src/linter/server_linter.rs +++ b/crates/oxc_language_server/src/linter/server_linter.rs @@ -41,9 +41,12 @@ use crate::{ pub struct ServerLinterBuilder; impl ServerLinterBuilder { + /// Build a new `ServerLinter` for the given root URI and options. + /// /// # Panics - /// Panics if the root URI cannot be converted to a file path. - pub fn build(root_uri: &Uri, options: serde_json::Value) -> ServerLinter { + /// + /// Panics if an empty `ConfigStoreBuilder` fails to build, which should never happen. + pub fn build(root_uri: &Uri, options: serde_json::Value) -> Option { let options = match serde_json::from_value::(options) { Ok(opts) => opts, Err(e) => { @@ -53,7 +56,7 @@ impl ServerLinterBuilder { LSPLintOptions::default() } }; - let root_path = root_uri.to_file_path().unwrap(); + let root_path = root_uri.to_file_path()?; let mut nested_ignore_patterns = Vec::new(); let (nested_configs, mut extended_paths) = Self::create_nested_configs(&root_path, &options, &mut nested_ignore_patterns); @@ -133,14 +136,14 @@ impl ServerLinterBuilder { }, ); - ServerLinter::new( + Some(ServerLinter::new( options.run, root_path.to_path_buf(), isolated_linter, LintIgnoreMatcher::new(&base_patterns, &root_path, nested_ignore_patterns), Self::create_ignore_glob(&root_path), extended_paths, - ) + )) } } @@ -213,8 +216,8 @@ impl ToolBuilder for ServerLinterBuilder { Some(DiagnosticServerCapabilities::Options(DiagnosticOptions::default())) }; } - fn build_boxed(&self, root_uri: &Uri, options: serde_json::Value) -> Box { - Box::new(ServerLinterBuilder::build(root_uri, options)) + fn build_boxed(&self, root_uri: &Uri, options: serde_json::Value) -> Option> { + ServerLinterBuilder::build(root_uri, options).map(|l| Box::new(l) as Box) } } @@ -324,8 +327,6 @@ impl Tool for ServerLinter { ToolShutdownChanges { uris_to_clear_diagnostics: Some(self.get_cached_uris()) } } - /// # Panics - /// Panics if the root URI cannot be converted to a file path. fn handle_configuration_change( &self, root_uri: &Uri, @@ -357,7 +358,11 @@ impl Tool for ServerLinter { } // get the cached files before refreshing the linter, and revalidate them after - let new_linter = ServerLinterBuilder::build(root_uri, new_options_json.clone()); + let Some(new_linter) = ServerLinterBuilder::build(root_uri, new_options_json.clone()) + else { + warn!("Failed to build linter: root URI cannot be converted to file path"); + return ToolRestartChanges { tool: None, watch_patterns: None }; + }; let patterns = { if old_option.config_path == new_options.config_path @@ -412,7 +417,10 @@ impl Tool for ServerLinter { options: serde_json::Value, ) -> ToolRestartChanges { // TODO: Check if the changed file is actually a config file (including extended paths) - let new_linter = ServerLinterBuilder::build(root_uri, options); + let Some(new_linter) = ServerLinterBuilder::build(root_uri, options) else { + warn!("Failed to build linter: root URI cannot be converted to file path"); + return ToolRestartChanges { tool: None, watch_patterns: None }; + }; ToolRestartChanges { tool: Some(Box::new(new_linter)), diff --git a/crates/oxc_language_server/src/linter/tester.rs b/crates/oxc_language_server/src/linter/tester.rs index 4908ec2b33e96..80d5a2817dd28 100644 --- a/crates/oxc_language_server/src/linter/tester.rs +++ b/crates/oxc_language_server/src/linter/tester.rs @@ -188,6 +188,7 @@ impl Tester<'_> { &Self::get_root_uri(self.relative_root_dir), self.options.clone(), ) + .expect("test requires valid root URI") } pub fn get_root_uri(relative_root_dir: &str) -> Uri { diff --git a/crates/oxc_language_server/src/tests.rs b/crates/oxc_language_server/src/tests.rs index c2d0cdc9484f2..ed3252baac60d 100644 --- a/crates/oxc_language_server/src/tests.rs +++ b/crates/oxc_language_server/src/tests.rs @@ -13,8 +13,8 @@ use crate::{Tool, ToolBuilder, ToolRestartChanges, backend::Backend, tool::Diagn pub struct FakeToolBuilder; impl ToolBuilder for FakeToolBuilder { - fn build_boxed(&self, _root_uri: &Uri, _options: serde_json::Value) -> Box { - Box::new(FakeTool) + fn build_boxed(&self, _root_uri: &Uri, _options: serde_json::Value) -> Option> { + Some(Box::new(FakeTool)) } } @@ -59,7 +59,7 @@ impl Tool for FakeTool { ) -> ToolRestartChanges { if new_options_json.as_u64() == Some(1) || new_options_json.as_u64() == Some(3) { return ToolRestartChanges { - tool: Some(FakeToolBuilder.build_boxed(root_uri, new_options_json)), + tool: FakeToolBuilder.build_boxed(root_uri, new_options_json), watch_patterns: None, }; } @@ -90,7 +90,7 @@ impl Tool for FakeTool { ) -> ToolRestartChanges { if changed_uri.as_str().ends_with("tool.config") { return ToolRestartChanges { - tool: Some(FakeToolBuilder.build_boxed(root_uri, options)), + tool: FakeToolBuilder.build_boxed(root_uri, options), watch_patterns: None, }; } diff --git a/crates/oxc_language_server/src/tool.rs b/crates/oxc_language_server/src/tool.rs index f2235fb4f7a19..7530c6e562585 100644 --- a/crates/oxc_language_server/src/tool.rs +++ b/crates/oxc_language_server/src/tool.rs @@ -18,7 +18,8 @@ pub trait ToolBuilder: Send + Sync { } /// Build a boxed instance of the tool for the given root URI and options. - fn build_boxed(&self, root_uri: &Uri, options: serde_json::Value) -> Box; + /// Returns `None` if the tool cannot be built for the given root URI and options. + fn build_boxed(&self, root_uri: &Uri, options: serde_json::Value) -> Option>; } pub type DiagnosticResult = Vec<(Uri, Vec)>; diff --git a/crates/oxc_language_server/src/worker.rs b/crates/oxc_language_server/src/worker.rs index 87d4c5fd25ca0..ac869a872f9b5 100644 --- a/crates/oxc_language_server/src/worker.rs +++ b/crates/oxc_language_server/src/worker.rs @@ -44,25 +44,21 @@ impl WorkspaceWorker { } /// Check if the worker is responsible for the given URI - /// A worker is responsible for a URI if the URI is a file URI and is located within the root URI of the worker + /// A worker is responsible for a URI if the URI starts with the root URI of the worker /// e.g. root URI: file:///path/to/root /// responsible for: file:///path/to/root/file.js /// not responsible for: file:///path/to/other/file.js - /// - /// # Panics - /// Panics if the root URI cannot be converted to a file path. pub fn is_responsible_for_uri(&self, uri: &Uri) -> bool { - if let Some(path) = uri.to_file_path() { - return path.starts_with(self.root_uri.to_file_path().unwrap()); - } - false + uri.as_str().starts_with(self.root_uri.as_str()) } /// Start all programs (linter, formatter) for the worker. /// This should be called after the client has sent the workspace configuration. pub async fn start_worker(&self, options: serde_json::Value, tools: &[Box]) { - *self.tools.write().await = - tools.iter().map(|tool| tool.build_boxed(&self.root_uri, options.clone())).collect(); + *self.tools.write().await = tools + .iter() + .filter_map(|tool| tool.build_boxed(&self.root_uri, options.clone())) + .collect(); *self.options.lock().await = Some(options); } From 21b8766bc4d03bf0552030c86b7beb38ee4c5fff Mon Sep 17 00:00:00 2001 From: charlie Date: Sat, 27 Dec 2025 16:27:51 -0600 Subject: [PATCH 2/4] fix(language_server): use file path comparison in is_responsible_for_uri String prefix matching incorrectly matched file:///path/test_workspace_second against root file:///path/test_workspace. Path::starts_with respects directory boundaries. --- crates/oxc_language_server/src/worker.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/crates/oxc_language_server/src/worker.rs b/crates/oxc_language_server/src/worker.rs index ac869a872f9b5..54355db0b5b67 100644 --- a/crates/oxc_language_server/src/worker.rs +++ b/crates/oxc_language_server/src/worker.rs @@ -44,12 +44,18 @@ impl WorkspaceWorker { } /// Check if the worker is responsible for the given URI - /// A worker is responsible for a URI if the URI starts with the root URI of the worker + /// A worker is responsible for a URI if the URI is a file URI and is located within the root URI of the worker /// e.g. root URI: file:///path/to/root /// responsible for: file:///path/to/root/file.js /// not responsible for: file:///path/to/other/file.js pub fn is_responsible_for_uri(&self, uri: &Uri) -> bool { - uri.as_str().starts_with(self.root_uri.as_str()) + let Some(path) = uri.to_file_path() else { + return false; + }; + let Some(root_path) = self.root_uri.to_file_path() else { + return false; + }; + path.starts_with(root_path) } /// Start all programs (linter, formatter) for the worker. From 5b139b4c2fc9feaf82a618e5c41d8a388bba93a0 Mon Sep 17 00:00:00 2001 From: charlie Date: Sat, 27 Dec 2025 16:30:00 -0600 Subject: [PATCH 3/4] test(language_server): add regression test for root_second prefix collision --- crates/oxc_language_server/src/worker.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/oxc_language_server/src/worker.rs b/crates/oxc_language_server/src/worker.rs index 54355db0b5b67..a6675c7e5d0bf 100644 --- a/crates/oxc_language_server/src/worker.rs +++ b/crates/oxc_language_server/src/worker.rs @@ -430,6 +430,11 @@ mod tests { !worker .is_responsible_for_uri(&Uri::from_str("file:///path/to/other/file.js").unwrap()) ); + assert!( + !worker.is_responsible_for_uri( + &Uri::from_str("file:///path/to/root_second/file.js").unwrap() + ) + ); } #[tokio::test] From 9df98a7fa702bc1f0ae48967dc64b6b3b56a3af2 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Sat, 27 Dec 2025 22:39:02 +0000 Subject: [PATCH 4/4] [autofix.ci] apply automated fixes --- crates/oxc_language_server/src/worker.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/crates/oxc_language_server/src/worker.rs b/crates/oxc_language_server/src/worker.rs index a6675c7e5d0bf..e5dddc5bb0453 100644 --- a/crates/oxc_language_server/src/worker.rs +++ b/crates/oxc_language_server/src/worker.rs @@ -430,11 +430,9 @@ mod tests { !worker .is_responsible_for_uri(&Uri::from_str("file:///path/to/other/file.js").unwrap()) ); - assert!( - !worker.is_responsible_for_uri( - &Uri::from_str("file:///path/to/root_second/file.js").unwrap() - ) - ); + assert!(!worker.is_responsible_for_uri( + &Uri::from_str("file:///path/to/root_second/file.js").unwrap() + )); } #[tokio::test]