From 0660d8e0c8fd1595996e3b629265e19055ce9398 Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Thu, 8 Jan 2026 10:47:41 -0700 Subject: [PATCH] Make URL parsing less strict for agent generated URLs (#46330) Before this we failed to parse some file:/// links the agent would generate causing it to open in the system default app and not zed. Release Notes: - N/A --- crates/acp_thread/src/mention.rs | 131 ++++++++++++++++--------------- 1 file changed, 66 insertions(+), 65 deletions(-) diff --git a/crates/acp_thread/src/mention.rs b/crates/acp_thread/src/mention.rs index 3e2e53fb7fbdf581b45566bd747cfcbfc1c0a004..2de3525cb84ec084cfc10cd612fa8a62000b6d56 100644 --- a/crates/acp_thread/src/mention.rs +++ b/crates/acp_thread/src/mention.rs @@ -12,7 +12,7 @@ use std::{ use ui::{App, IconName, SharedString}; use url::Url; use urlencoding::decode; -use util::paths::PathStyle; +use util::{ResultExt, paths::PathStyle}; #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize, Hash)] pub enum MentionUri { @@ -53,23 +53,30 @@ pub enum MentionUri { impl MentionUri { pub fn parse(input: &str, path_style: PathStyle) -> Result { fn parse_line_range(fragment: &str) -> Result> { - let range = fragment - .strip_prefix("L") - .context("Line range must start with \"L\"")?; - let (start, end) = range - .split_once(":") - .context("Line range must use colon as separator")?; - let range = start + let range = fragment.strip_prefix("L").unwrap_or(fragment); + + let (start, end) = if let Some((start, end)) = range.split_once(":") { + (start, end) + } else if let Some((start, end)) = range.split_once("-") { + // Also handle L10-20 or L10-L20 format + (start, end.strip_prefix("L").unwrap_or(end)) + } else { + // Single line number like L1872 - treat as a range of one line + (range, range) + }; + + let start_line = start .parse::() .context("Parsing line range start")? .checked_sub(1) - .context("Line numbers should be 1-based")? - ..=end - .parse::() - .context("Parsing line range end")? - .checked_sub(1) - .context("Line numbers should be 1-based")?; - Ok(range) + .context("Line numbers should be 1-based")?; + let end_line = end + .parse::() + .context("Parsing line range end")? + .checked_sub(1) + .context("Line numbers should be 1-based")?; + + Ok(start_line..=end_line) } let url = url::Url::parse(input)?; @@ -85,7 +92,7 @@ impl MentionUri { let path = decoded.as_ref(); if let Some(fragment) = url.fragment() { - let line_range = parse_line_range(fragment)?; + let line_range = parse_line_range(fragment).log_err().unwrap_or(1..=1); if let Some(name) = single_query_param(&url, "symbol")? { Ok(Self::Symbol { name, @@ -511,58 +518,52 @@ mod tests { } #[test] - fn test_invalid_line_range_format() { - // Missing L prefix - assert!( - MentionUri::parse(uri!("file:///path/to/file.rs#10:20"), PathStyle::local()).is_err() - ); - - // Missing colon separator - assert!( - MentionUri::parse(uri!("file:///path/to/file.rs#L1020"), PathStyle::local()).is_err() - ); - - // Invalid numbers - assert!( - MentionUri::parse(uri!("file:///path/to/file.rs#L10:abc"), PathStyle::local()).is_err() - ); - assert!( - MentionUri::parse(uri!("file:///path/to/file.rs#Labc:20"), PathStyle::local()).is_err() - ); + fn test_single_line_number() { + // https://github.com/zed-industries/zed/issues/46114 + let uri = uri!("file:///path/to/file.rs#L1872"); + let parsed = MentionUri::parse(uri, PathStyle::local()).unwrap(); + match &parsed { + MentionUri::Selection { + abs_path: path, + line_range, + } => { + assert_eq!(path.as_ref().unwrap(), Path::new(path!("/path/to/file.rs"))); + assert_eq!(line_range.start(), &1871); + assert_eq!(line_range.end(), &1871); + } + _ => panic!("Expected Selection variant"), + } } #[test] - fn test_invalid_query_parameters() { - // Invalid query parameter name - assert!( - MentionUri::parse( - uri!("file:///path/to/file.rs#L10:20?invalid=test"), - PathStyle::local() - ) - .is_err() - ); - - // Too many query parameters - assert!( - MentionUri::parse( - uri!("file:///path/to/file.rs#L10:20?symbol=test&another=param"), - PathStyle::local() - ) - .is_err() - ); - } + fn test_dash_separated_line_range() { + let uri = uri!("file:///path/to/file.rs#L10-20"); + let parsed = MentionUri::parse(uri, PathStyle::local()).unwrap(); + match &parsed { + MentionUri::Selection { + abs_path: path, + line_range, + } => { + assert_eq!(path.as_ref().unwrap(), Path::new(path!("/path/to/file.rs"))); + assert_eq!(line_range.start(), &9); + assert_eq!(line_range.end(), &19); + } + _ => panic!("Expected Selection variant"), + } - #[test] - fn test_zero_based_line_numbers() { - // Test that 0-based line numbers are rejected (should be 1-based) - assert!( - MentionUri::parse(uri!("file:///path/to/file.rs#L0:10"), PathStyle::local()).is_err() - ); - assert!( - MentionUri::parse(uri!("file:///path/to/file.rs#L1:0"), PathStyle::local()).is_err() - ); - assert!( - MentionUri::parse(uri!("file:///path/to/file.rs#L0:0"), PathStyle::local()).is_err() - ); + // Also test L10-L20 format + let uri = uri!("file:///path/to/file.rs#L10-L20"); + let parsed = MentionUri::parse(uri, PathStyle::local()).unwrap(); + match &parsed { + MentionUri::Selection { + abs_path: path, + line_range, + } => { + assert_eq!(path.as_ref().unwrap(), Path::new(path!("/path/to/file.rs"))); + assert_eq!(line_range.start(), &9); + assert_eq!(line_range.end(), &19); + } + _ => panic!("Expected Selection variant"), + } } }