diff --git a/crates/acp_thread/src/mention.rs b/crates/acp_thread/src/mention.rs index b5e6ab90ab..93e7b4d846 100644 --- a/crates/acp_thread/src/mention.rs +++ b/crates/acp_thread/src/mention.rs @@ -83,33 +83,6 @@ impl MentionUri { .and_then(|input| input.strip_suffix('`')) .unwrap_or(input); - fn parse_line_range(fragment: &str) -> Result> { - 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")?; - 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 parse_column = |input: Option| -> Option { input?.parse::().ok()?.checked_sub(1) }; let validate_query_params = |url: &Url, allowed: &[&str]| -> Result<()> { @@ -121,37 +94,6 @@ impl MentionUri { Ok(()) }; - let parse_absolute_path = |input: &str| -> Result { - let (path_input, fragment) = input - .split_once('#') - .map_or((input, None), |(path, fragment)| (path, Some(fragment))); - - if let Some(fragment) = fragment.and_then(|fragment| parse_line_range(fragment).ok()) { - return Ok(MentionUri::Selection { - abs_path: Some(path_input.into()), - line_range: fragment, - column: None, - }); - } - - let path_with_position = PathWithPosition::parse_str(path_input); - let abs_path = path_with_position.path; - if let Some(row) = path_with_position.row { - let line = row - .checked_sub(1) - .context("Line numbers should be 1-based")?; - Ok(MentionUri::Selection { - abs_path: Some(abs_path), - line_range: line..=line, - column: path_with_position - .column - .map(|column| column.saturating_sub(1)), - }) - } else { - Ok(MentionUri::File { abs_path }) - } - }; - if is_absolute(input, path_style) && !input.contains("://") { return parse_absolute_path(input) .with_context(|| format!("Invalid absolute path mention URI: {input}")); @@ -168,7 +110,10 @@ impl MentionUri { }; let decoded = decode(trimmed).unwrap_or(Cow::Borrowed(trimmed)); let normalized: Cow = if path_style.is_windows() { - Cow::Owned(decoded.replace('/', "\\")) + match to_native_windows_path(&decoded) { + Some(native) => Cow::Owned(native), + None => decoded, + } } else { decoded }; @@ -337,6 +282,56 @@ impl MentionUri { } } + /// Parses a hyperlink target from agent-authored Markdown. + /// + /// Unlike [`MentionUri::parse`] — which stays strict so canonical mention + /// URIs round-trip verbatim — bare path targets are normalized first: + /// percent escapes are decoded (see [`decode_path_escapes`]) and + /// Windows-compatible spellings like `/C:/foo` or `/c/foo` become native + /// paths (see [`to_native_windows_path`]). + pub fn parse_hyperlink(input: &str, path_style: PathStyle) -> Result { + if let Some(target) = bare_path_target(input, path_style) { + return parse_hyperlink_path(target, path_style, DecodePercentEscapes::Yes) + .with_context(|| format!("Invalid hyperlink path target: {input}")); + } + Self::parse(input, path_style) + } + + /// Returns the literal (un-decoded) interpretation of a bare-path + /// hyperlink target, for files whose names literally contain an escape + /// sequence (e.g. `a%20b.rs`). Returns `None` when this wouldn't differ + /// from [`MentionUri::parse_hyperlink`], including for URLs, whose + /// escapes are unambiguous. + pub fn parse_hyperlink_literal(input: &str, path_style: PathStyle) -> Option { + let target = bare_path_target(input, path_style)?; + let (path_input, _) = split_path_fragment(target); + if !matches!(decode_path_escapes(path_input), Cow::Owned(_)) { + return None; + } + parse_hyperlink_path(target, path_style, DecodePercentEscapes::No).ok() + } + + /// The absolute path this mention refers to, if it refers to one. + pub fn abs_path(&self) -> Option<&Path> { + match self { + MentionUri::File { abs_path } + | MentionUri::Directory { abs_path } + | MentionUri::Symbol { abs_path, .. } => Some(abs_path), + MentionUri::Selection { abs_path, .. } => abs_path.as_deref(), + MentionUri::Skill { + skill_file_path, .. + } => Some(skill_file_path), + MentionUri::PastedImage { .. } + | MentionUri::Thread { .. } + | MentionUri::Rule { .. } + | MentionUri::Diagnostics { .. } + | MentionUri::Fetch { .. } + | MentionUri::TerminalSelection { .. } + | MentionUri::GitDiff { .. } + | MentionUri::MergeConflict { .. } => None, + } + } + pub fn name(&self) -> String { match self { MentionUri::File { abs_path, .. } | MentionUri::Directory { abs_path, .. } => abs_path @@ -599,6 +594,217 @@ impl fmt::Display for MentionLink<'_> { } } +#[derive(Clone, Copy, PartialEq, Eq)] +enum DecodePercentEscapes { + Yes, + No, +} + +fn parse_line_range(fragment: &str) -> Result> { + 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")?; + 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) +} + +/// Returns the mention target as a bare absolute path (not a URL), with the +/// backticks agents sometimes add stripped. +fn bare_path_target(input: &str, path_style: PathStyle) -> Option<&str> { + let input = input + .strip_prefix('`') + .and_then(|input| input.strip_suffix('`')) + .unwrap_or(input); + (is_absolute(input, path_style) && !input.contains("://")).then_some(input) +} + +fn split_path_fragment(input: &str) -> (&str, Option<&str>) { + input + .split_once('#') + .map_or((input, None), |(path, fragment)| (path, Some(fragment))) +} + +fn parse_absolute_path(input: &str) -> Result { + let (path_input, fragment) = split_path_fragment(input); + absolute_path_mention(path_input, fragment) +} + +/// Like [`parse_absolute_path`], but normalizes hyperlink spellings first. +fn parse_hyperlink_path( + input: &str, + path_style: PathStyle, + decode_escapes: DecodePercentEscapes, +) -> Result { + let (path_input, fragment) = split_path_fragment(input); + let path_input = normalize_path_mention(path_input, path_style, decode_escapes); + absolute_path_mention(&path_input, fragment) +} + +fn absolute_path_mention(path_input: &str, fragment: Option<&str>) -> Result { + if let Some(fragment) = fragment.and_then(|fragment| parse_line_range(fragment).ok()) { + return Ok(MentionUri::Selection { + abs_path: Some(path_input.into()), + line_range: fragment, + column: None, + }); + } + + let path_with_position = PathWithPosition::parse_str(path_input); + let abs_path = path_with_position.path; + if let Some(row) = path_with_position.row { + let line = row + .checked_sub(1) + .context("Line numbers should be 1-based")?; + Ok(MentionUri::Selection { + abs_path: Some(abs_path), + line_range: line..=line, + column: path_with_position + .column + .map(|column| column.saturating_sub(1)), + }) + } else { + Ok(MentionUri::File { abs_path }) + } +} + +fn normalize_path_mention( + input: &str, + path_style: PathStyle, + decode_escapes: DecodePercentEscapes, +) -> Cow<'_, str> { + let decoded = match decode_escapes { + DecodePercentEscapes::Yes => decode_path_escapes(input), + DecodePercentEscapes::No => Cow::Borrowed(input), + }; + if !path_style.is_windows() { + return decoded; + } + match to_native_windows_path(&decoded) { + Some(native) => Cow::Owned(native), + None => decoded, + } +} + +/// Decodes percent escapes in a path, leaving separator escapes (`%2F`, +/// `%5C`) encoded so decoding can't change which directories the path +/// traverses. Invalid sequences and non-UTF-8 results leave the input +/// unchanged. Returns `Cow::Owned` iff decoding changed the input +/// (`parse_hyperlink_literal` relies on this). +fn decode_path_escapes(input: &str) -> Cow<'_, str> { + fn hex_digit(byte: u8) -> Option { + match byte { + b'0'..=b'9' => Some(byte - b'0'), + b'a'..=b'f' => Some(byte - b'a' + 10), + b'A'..=b'F' => Some(byte - b'A' + 10), + _ => None, + } + } + + if !input.contains('%') { + return Cow::Borrowed(input); + } + let bytes = input.as_bytes(); + let mut decoded = Vec::with_capacity(bytes.len()); + let mut index = 0; + while index < bytes.len() { + if bytes[index] == b'%' + && let Some(high) = bytes.get(index + 1).copied().and_then(hex_digit) + && let Some(low) = bytes.get(index + 2).copied().and_then(hex_digit) + { + let byte = (high << 4) | low; + if byte != b'/' && byte != b'\\' { + decoded.push(byte); + index += 3; + continue; + } + } + decoded.push(bytes[index]); + index += 1; + } + if decoded == bytes { + return Cow::Borrowed(input); + } + match String::from_utf8(decoded) { + Ok(decoded) => Cow::Owned(decoded), + Err(_) => Cow::Borrowed(input), + } +} + +/// Converts Windows-compatible path spellings into a native Windows path, +/// normalizing separators to backslashes and drive letters to uppercase so +/// parsed paths compare equal to worktree paths. Returns `None` when the +/// input needs no changes. +fn to_native_windows_path(path: &str) -> Option { + fn join_drive(drive: char, rest: &str) -> String { + format!( + "{}:\\{}", + drive.to_ascii_uppercase(), + rest.replace('/', "\\") + ) + } + + if let Some(rest) = path.strip_prefix('/') { + // URL-style path with a leading slash before the drive: `/C:/foo`. + let mut chars = rest.chars(); + if let (Some(drive), Some(':'), Some('/' | '\\')) = + (chars.next(), chars.next(), chars.next()) + && drive.is_ascii_alphabetic() + { + return Some(join_drive(drive, chars.as_str())); + } + + // MSYS/Git Bash style: `/c/foo`. Lowercase-only, since that's what + // those shells emit and uppercase risks misreading real directories. + let mut chars = rest.chars(); + if let (Some(drive), Some('/' | '\\')) = (chars.next(), chars.next()) + && drive.is_ascii_lowercase() + { + return Some(join_drive(drive, chars.as_str())); + } + } + + // A native path with a drive prefix: uppercase the drive and normalize + // separators, e.g. `c:/foo` or `c:\foo`. + let mut chars = path.chars(); + if let (Some(drive), Some(':')) = (chars.next(), chars.next()) + && drive.is_ascii_alphabetic() + { + if drive.is_ascii_uppercase() && !path.contains('/') { + return None; + } + return Some(format!( + "{}:{}", + drive.to_ascii_uppercase(), + chars.as_str().replace('/', "\\") + )); + } + + if path.contains('/') { + return Some(path.replace('/', "\\")); + } + + None +} + fn default_include_errors() -> bool { true } @@ -727,6 +933,298 @@ mod tests { } } + #[test] + fn test_parse_file_uri_with_spaces() { + let parsed = + MentionUri::parse("file:///C:/path%20with%20space/file.rs", PathStyle::Windows) + .unwrap(); + match parsed { + MentionUri::File { abs_path } => { + assert_eq!(abs_path, PathBuf::from("C:\\path with space\\file.rs")); + } + other => panic!("Expected File variant, got {other:?}"), + } + assert_eq!( + MentionUri::File { + abs_path: PathBuf::from("C:\\path with space\\file.rs") + } + .to_uri() + .to_string(), + "file:///C:/path%20with%20space/file.rs" + ); + } + + #[test] + fn test_parse_windows_drive_path_with_leading_slash_and_line() { + let parsed = MentionUri::parse_hyperlink( + "/C:/Projects/Example Workspace/Cargo.toml:2", + PathStyle::Windows, + ) + .unwrap(); + match parsed { + MentionUri::Selection { + abs_path: Some(abs_path), + line_range, + .. + } => { + assert_eq!( + abs_path, + PathBuf::from("C:\\Projects\\Example Workspace\\Cargo.toml") + ); + assert_eq!(line_range, 1..=1); + } + other => panic!("Expected Selection variant, got {other:?}"), + } + } + + #[test] + fn test_parse_windows_path_with_percent_escaped_spaces_and_line() { + let parsed = MentionUri::parse_hyperlink( + "C:\\Projects\\Example%20Workspace\\path\\to\\filename.ext:42", + PathStyle::Windows, + ) + .unwrap(); + match parsed { + MentionUri::Selection { + abs_path: Some(abs_path), + line_range, + .. + } => { + assert_eq!( + abs_path, + PathBuf::from("C:\\Projects\\Example Workspace\\path\\to\\filename.ext") + ); + assert_eq!(line_range, 41..=41); + } + other => panic!("Expected Selection variant, got {other:?}"), + } + } + + #[test] + fn test_parse_windows_compat_path_with_spaces() { + let parsed = MentionUri::parse_hyperlink( + "/c/Projects/Example Workspace/AGENTS.md", + PathStyle::Windows, + ) + .unwrap(); + match parsed { + MentionUri::File { abs_path } => { + assert_eq!( + abs_path, + PathBuf::from("C:\\Projects\\Example Workspace\\AGENTS.md") + ); + } + other => panic!("Expected File variant, got {other:?}"), + } + } + + #[test] + fn test_parse_windows_drive_path_with_leading_slash_and_fragment_line() { + let parsed = + MentionUri::parse_hyperlink("/C:/Projects/Cargo.toml#L4", PathStyle::Windows).unwrap(); + match parsed { + MentionUri::Selection { + abs_path: Some(abs_path), + line_range, + .. + } => { + assert_eq!(abs_path, PathBuf::from("C:\\Projects\\Cargo.toml")); + assert_eq!(line_range, 3..=3); + } + other => panic!("Expected Selection variant, got {other:?}"), + } + } + + #[test] + fn test_windows_drive_path_with_leading_slash_round_trips() { + let parsed = MentionUri::parse_hyperlink("/C:/dir/file.rs", PathStyle::Windows).unwrap(); + assert_eq!( + parsed, + MentionUri::File { + abs_path: PathBuf::from("C:\\dir\\file.rs") + } + ); + let uri = parsed.to_uri().to_string(); + assert_eq!(uri, "file:///C:/dir/file.rs"); + assert_eq!(MentionUri::parse(&uri, PathStyle::Windows).unwrap(), parsed); + } + + #[test] + fn test_parse_windows_unc_path() { + let parsed = + MentionUri::parse_hyperlink("//server/share/dir/file.rs", PathStyle::Windows).unwrap(); + match parsed { + MentionUri::File { abs_path } => { + assert_eq!(abs_path, PathBuf::from("\\\\server\\share\\dir\\file.rs")); + } + other => panic!("Expected File variant, got {other:?}"), + } + } + + #[test] + fn test_parse_windows_drive_letters_are_uppercased() { + for input in [ + "file:///c:/foo/bar.rs", + "/c:/foo/bar.rs", + "/c/foo/bar.rs", + "c:\\foo\\bar.rs", + "c:/foo/bar.rs", + ] { + let parsed = MentionUri::parse_hyperlink(input, PathStyle::Windows).unwrap(); + assert_eq!( + parsed, + MentionUri::File { + abs_path: PathBuf::from("C:\\foo\\bar.rs") + }, + "input: {input}" + ); + } + } + + #[test] + fn test_msys_style_paths_require_lowercase_drive() { + // Uppercase `/C/foo` is more likely a real directory than a drive. + let parsed = MentionUri::parse_hyperlink("/C/Users/readme.md", PathStyle::Windows).unwrap(); + match parsed { + MentionUri::File { abs_path } => { + assert_eq!(abs_path, PathBuf::from("\\C\\Users\\readme.md")); + } + other => panic!("Expected File variant, got {other:?}"), + } + } + + #[test] + fn test_posix_paths_are_not_rewritten_as_windows_drives() { + let parsed = + MentionUri::parse_hyperlink("/c/Projects/AGENTS.md", PathStyle::Posix).unwrap(); + match parsed { + MentionUri::File { abs_path } => { + assert_eq!(abs_path, PathBuf::from("/c/Projects/AGENTS.md")); + } + other => panic!("Expected File variant, got {other:?}"), + } + } + + #[test] + fn test_hyperlink_percent_escapes_are_decoded() { + let parsed = MentionUri::parse_hyperlink("/tmp/a%20b.rs", PathStyle::Posix).unwrap(); + assert_eq!( + parsed, + MentionUri::File { + abs_path: PathBuf::from("/tmp/a b.rs") + } + ); + + // Invalid escape sequences pass through unchanged. + let parsed = + MentionUri::parse_hyperlink("C:\\dir\\100%_done.txt", PathStyle::Windows).unwrap(); + assert_eq!( + parsed, + MentionUri::File { + abs_path: PathBuf::from("C:\\dir\\100%_done.txt") + } + ); + + // Separator escapes stay encoded (no introduced path traversal). + let parsed = MentionUri::parse_hyperlink("/tmp/a%2Fb.rs", PathStyle::Posix).unwrap(); + assert_eq!( + parsed, + MentionUri::File { + abs_path: PathBuf::from("/tmp/a%2Fb.rs") + } + ); + let parsed = + MentionUri::parse_hyperlink("/tmp/..%2F..%2Fsecret", PathStyle::Posix).unwrap(); + assert_eq!( + parsed, + MentionUri::File { + abs_path: PathBuf::from("/tmp/..%2F..%2Fsecret") + } + ); + } + + #[test] + fn test_parse_keeps_bare_path_targets_verbatim() { + let parsed = MentionUri::parse("/tmp/a%20b.rs", PathStyle::Posix).unwrap(); + assert_eq!( + parsed, + MentionUri::File { + abs_path: PathBuf::from("/tmp/a%20b.rs") + } + ); + + let parsed = MentionUri::parse("/c/Projects/AGENTS.md", PathStyle::Windows).unwrap(); + assert_eq!( + parsed, + MentionUri::File { + abs_path: PathBuf::from("/c/Projects/AGENTS.md") + } + ); + } + + #[test] + fn test_parse_hyperlink_literal_keeps_percent_escapes() { + let literal = + MentionUri::parse_hyperlink_literal("/tmp/a%20b.rs", PathStyle::Posix).unwrap(); + assert_eq!( + literal, + MentionUri::File { + abs_path: PathBuf::from("/tmp/a%20b.rs") + } + ); + + // Line suffixes still parse. + let literal = + MentionUri::parse_hyperlink_literal("/tmp/a%20b.rs:42", PathStyle::Posix).unwrap(); + assert_eq!( + literal, + MentionUri::Selection { + abs_path: Some(PathBuf::from("/tmp/a%20b.rs")), + line_range: 41..=41, + column: None, + } + ); + + // Windows normalization still applies. + let literal = + MentionUri::parse_hyperlink_literal("/C:/dir/a%20b.rs", PathStyle::Windows).unwrap(); + assert_eq!( + literal, + MentionUri::File { + abs_path: PathBuf::from("C:\\dir\\a%20b.rs") + } + ); + } + + #[test] + fn test_parse_hyperlink_literal_returns_none_when_unambiguous() { + // No percent escapes: identical to `parse_hyperlink`. + assert_eq!( + MentionUri::parse_hyperlink_literal("/tmp/a b.rs", PathStyle::Posix), + None + ); + // Invalid escape sequences are also left alone by `parse_hyperlink`. + assert_eq!( + MentionUri::parse_hyperlink_literal("/tmp/100%_done.txt", PathStyle::Posix), + None + ); + // Separator escapes are never decoded, so they're not ambiguous. + assert_eq!( + MentionUri::parse_hyperlink_literal("/tmp/a%2Fb.rs", PathStyle::Posix), + None + ); + // URLs are spec-encoded, not ambiguous. + assert_eq!( + MentionUri::parse_hyperlink_literal("file:///tmp/a%20b.rs", PathStyle::Posix), + None + ); + // Relative paths are not bare-path mentions. + assert_eq!( + MentionUri::parse_hyperlink_literal("tmp/a%20b.rs", PathStyle::Posix), + None + ); + } + #[test] fn test_to_directory_uri_without_slash() { let uri = MentionUri::Directory { diff --git a/crates/agent/src/tools/find_path_tool.rs b/crates/agent/src/tools/find_path_tool.rs index 481f1433fb..eb08000d3a 100644 --- a/crates/agent/src/tools/find_path_tool.rs +++ b/crates/agent/src/tools/find_path_tool.rs @@ -1,4 +1,5 @@ use crate::{AgentTool, ToolCallEventStream, ToolInput}; +use acp_thread::MentionUri; use agent_client_protocol::schema::v1 as acp; use anyhow::{Result, anyhow}; use futures::FutureExt as _; @@ -155,10 +156,13 @@ impl AgentTool for FindPathTool { paginated_matches .iter() .map(|path| { + let uri = MentionUri::File { + abs_path: path.clone(), + }; acp::ToolCallContent::Content(acp::Content::new( acp::ContentBlock::ResourceLink(acp::ResourceLink::new( path.to_string_lossy(), - format!("file://{}", path.display()), + uri.to_uri().to_string(), )), )) }) diff --git a/crates/agent/src/tools/grep_tool.rs b/crates/agent/src/tools/grep_tool.rs index 748642b3cd..5af92b2e45 100644 --- a/crates/agent/src/tools/grep_tool.rs +++ b/crates/agent/src/tools/grep_tool.rs @@ -1,4 +1,5 @@ use crate::{AgentTool, ToolCallEventStream, ToolInput}; +use acp_thread::MentionUri; use agent_client_protocol::schema::v1 as acp; use anyhow::Result; use futures::{FutureExt as _, StreamExt}; @@ -327,10 +328,15 @@ impl AgentTool for GrepTool { output.push_str("\n```\n"); if let Some(abs_path) = &abs_path { + let uri = MentionUri::Selection { + abs_path: Some(abs_path.clone()), + line_range: range.start.row..=end_row, + column: None, + }; content.push(acp::ToolCallContent::Content(acp::Content::new( acp::ContentBlock::ResourceLink(acp::ResourceLink::new( format!("{}#{}", path.display(), line_label), - format!("file://{}#{}", abs_path.display(), line_label), + uri.to_uri().to_string(), )), ))); locations.push( @@ -393,6 +399,7 @@ mod tests { use project::{FakeFs, Project}; use serde_json::json; use settings::SettingsStore; + use std::path::PathBuf; use unindent::Unindent; use util::path; @@ -611,21 +618,30 @@ mod tests { .collect::>(); assert_eq!(links.len(), 2, "expected one resource link per match"); - let alpha_uri = format!("file://{}#L1", path!("/root/src/alpha.txt")); + let selection_uri = |abs_path: &str| { + MentionUri::Selection { + abs_path: Some(PathBuf::from(abs_path)), + line_range: 0..=0, + column: None, + } + .to_uri() + .to_string() + }; + + let alpha_uri = selection_uri(path!("/root/src/alpha.txt")); assert!( links.iter().any(|link| { - link.name.replace('\\', "/") == "root/src/alpha.txt#L1" - && link.uri.replace('\\', "/") == alpha_uri.replace('\\', "/") + link.name.replace('\\', "/") == "root/src/alpha.txt#L1" && link.uri == alpha_uri }), "missing clickable link for alpha.txt, got: {links:?}" ); - let beta_uri = format!("file://{}#L1", path!("/root/beta.txt")); + let beta_uri = selection_uri(path!("/root/beta.txt")); assert!( - links.iter().any(|link| { - link.name.replace('\\', "/") == "root/beta.txt#L1" - && link.uri.replace('\\', "/") == beta_uri.replace('\\', "/") - }), + links + .iter() + .any(|link| link.name.replace('\\', "/") == "root/beta.txt#L1" + && link.uri == beta_uri), "missing clickable link for beta.txt, got: {links:?}" ); diff --git a/crates/agent_ui/src/agent_ui.rs b/crates/agent_ui/src/agent_ui.rs index 1949a5965a..ccaf59f72b 100644 --- a/crates/agent_ui/src/agent_ui.rs +++ b/crates/agent_ui/src/agent_ui.rs @@ -65,7 +65,7 @@ use serde::{Deserialize, Serialize}; use settings::{LanguageModelSelection, Settings as _, SettingsStore, SidebarSide}; use std::any::TypeId; use std::path::{Path, PathBuf}; -use workspace::Workspace; +use workspace::{OpenOptions, Workspace}; use crate::agent_configuration::ManageProfilesModal; pub use crate::agent_connection_store::{ActiveAcpConnection, AgentConnectionStore}; @@ -118,40 +118,68 @@ pub(crate) fn resolve_agent_image( None } +/// Opens `abs_path` in the workspace, moving the cursor to `point` when one +/// is given. Paths outside every worktree are only opened when a file exists +/// there, so broken agent links don't create empty buffers. pub(crate) fn open_abs_path_at_point( workspace: &mut Workspace, abs_path: PathBuf, - point: Point, + point: Option, window: &mut Window, cx: &mut Context, -) -> bool { - let project = workspace.project(); - let Some(path) = project.update(cx, |project, cx| project.find_project_path(abs_path, cx)) - else { - return false; - }; - - let item = workspace.open_path(path, None, true, window, cx); +) { + let project_path = workspace + .project() + .update(cx, |project, cx| project.find_project_path(&abs_path, cx)); + let fs = workspace.project().read(cx).fs().clone(); + let workspace = cx.weak_entity(); window .spawn(cx, async move |cx| { - let Some(editor) = item.await?.downcast::() else { + let item = if let Some(project_path) = project_path { + workspace + .update_in(cx, |workspace, window, cx| { + workspace.open_path(project_path, None, true, window, cx) + })? + .await? + } else { + let metadata = fs.metadata(&abs_path).await?; + anyhow::ensure!( + metadata.is_some_and(|metadata| !metadata.is_dir), + "no file found at path {abs_path:?}" + ); + workspace + .update_in(cx, |workspace, window, cx| { + workspace.open_abs_path( + abs_path, + OpenOptions { + focus: Some(true), + ..Default::default() + }, + window, + cx, + ) + })? + .await? + }; + let Some(point) = point else { + return Ok(()); + }; + let Some(editor) = item.downcast::() else { return Ok(()); }; - let range = point..point; editor .update_in(cx, |editor, window, cx| { editor.change_selections( SelectionEffects::scroll(Autoscroll::center()), window, cx, - |selections| selections.select_ranges([range]), + |selections| selections.select_ranges([point..point]), ); }) .ok(); anyhow::Ok(()) }) .detach_and_log_err(cx); - true } pub const DEFAULT_THREAD_TITLE: &str = "New Agent Thread"; diff --git a/crates/agent_ui/src/conversation_view/thread_view.rs b/crates/agent_ui/src/conversation_view/thread_view.rs index 42a2221988..2d7c6b6ca4 100644 --- a/crates/agent_ui/src/conversation_view/thread_view.rs +++ b/crates/agent_ui/src/conversation_view/thread_view.rs @@ -11931,19 +11931,31 @@ pub(crate) fn open_link( return; }; - if let Some(mention) = MentionUri::parse(&url, workspace.read(cx).path_style(cx)).log_err() { + let path_style = workspace.read(cx).path_style(cx); + if let Some(mention) = MentionUri::parse_hyperlink(&url, path_style).log_err() { + // Percent escapes in bare paths are ambiguous: prefer the decoded + // interpretation, falling back to the literal one (e.g. a file + // actually named `a%20b.rs`) only when the decoded path doesn't + // resolve in the project but the literal one does. + let resolves_in_project = |mention: &MentionUri, cx: &App| { + mention.abs_path().is_some_and(|abs_path| { + let project = workspace.read(cx).project().read(cx); + project + .find_project_path(abs_path, cx) + .is_some_and(|path| project.entry_for_path(&path, cx).is_some()) + }) + }; + let mention = match MentionUri::parse_hyperlink_literal(&url, path_style) { + Some(literal) + if !resolves_in_project(&mention, cx) && resolves_in_project(&literal, cx) => + { + literal + } + _ => mention, + }; workspace.update(cx, |workspace, cx| match mention { MentionUri::File { abs_path } => { - let project = workspace.project(); - let Some(path) = - project.update(cx, |project, cx| project.find_project_path(abs_path, cx)) - else { - return; - }; - - workspace - .open_path(path, None, true, window, cx) - .detach_and_log_err(cx); + open_abs_path_at_point(workspace, abs_path, None, window, cx); } MentionUri::PastedImage { .. } => {} MentionUri::Directory { abs_path } => { @@ -11967,7 +11979,7 @@ pub(crate) fn open_link( open_abs_path_at_point( workspace, path, - Point::new(*line_range.start(), 0), + Some(Point::new(*line_range.start(), 0)), window, cx, ); @@ -11980,7 +11992,7 @@ pub(crate) fn open_link( open_abs_path_at_point( workspace, path, - Point::new(*line_range.start(), column.unwrap_or(0)), + Some(Point::new(*line_range.start(), column.unwrap_or(0))), window, cx, ); @@ -12067,6 +12079,7 @@ mod tests { use super::*; use project::{FakeFs, Project}; use serde_json::json; + use std::path::Path; use util::path; use workspace::MultiWorkspace; @@ -12171,6 +12184,118 @@ mod tests { assert!(*active.path == *"src/main.rs"); }); } + + #[gpui::test] + async fn test_open_link_percent_escape_disambiguation(cx: &mut gpui::TestAppContext) { + crate::test_support::init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/project"), + json!({ + "a%20b.rs": "literal", + "a b.rs": "decoded", + "c d.rs": "", + "e%20f.rs": "", + }), + ) + .await; + + let project = Project::test(fs, [path!("/project").as_ref()], cx).await; + let (multi_workspace, cx) = + cx.add_window_view(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx)); + let workspace = multi_workspace.read_with(cx, |mw, _| mw.workspace().clone()); + let workspace_weak = workspace.downgrade(); + + let open_link_and_active_path = |url: String, cx: &mut gpui::VisualTestContext| { + multi_workspace.update_in(cx, |_, window, cx| { + open_link(url.into(), &workspace_weak, window, cx); + }); + cx.run_until_parked(); + workspace.read_with(cx, |workspace, cx| { + workspace + .active_item(cx) + .and_then(|item| item.project_path(cx)) + .expect("file should be open") + .path + }) + }; + + // Both interpretations exist: the decoded one wins. + let path = open_link_and_active_path(path!("/project/a%20b.rs").to_string(), cx); + assert_eq!(*path, *"a b.rs"); + + // Only the decoded file exists. + let path = open_link_and_active_path(path!("/project/c%20d.rs").to_string(), cx); + assert_eq!(*path, *"c d.rs"); + + // Only the literally-named file exists: fall back to it. + let path = open_link_and_active_path(path!("/project/e%20f.rs").to_string(), cx); + assert_eq!(*path, *"e%20f.rs"); + } + + #[gpui::test] + async fn test_open_link_out_of_project_path(cx: &mut gpui::TestAppContext) { + crate::test_support::init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree(path!("/project"), json!({"src": {"main.rs": ""}})) + .await; + fs.insert_tree(path!("/outside"), json!({"notes.md": "one\ntwo\nthree\n"})) + .await; + + let project = Project::test(fs, [path!("/project").as_ref()], cx).await; + let (multi_workspace, cx) = + cx.add_window_view(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx)); + let workspace = multi_workspace.read_with(cx, |mw, _| mw.workspace().clone()); + let workspace_weak = workspace.downgrade(); + + // A nonexistent out-of-project path opens nothing, not even an + // empty buffer. + multi_workspace.update_in(cx, |_, window, cx| { + open_link( + path!("/outside/missing.md").to_string().into(), + &workspace_weak, + window, + cx, + ); + }); + cx.run_until_parked(); + workspace.read_with(cx, |workspace, cx| { + assert!( + workspace.active_item(cx).is_none(), + "nothing should open for a nonexistent path" + ); + }); + + // An existing out-of-project file opens at the linked line. + multi_workspace.update_in(cx, |_, window, cx| { + open_link( + format!("{}:2", path!("/outside/notes.md")).into(), + &workspace_weak, + window, + cx, + ); + }); + cx.run_until_parked(); + let editor = workspace.read_with(cx, |workspace, cx| { + let item = workspace.active_item(cx).expect("file should be open"); + let project_path = item.project_path(cx).expect("item should have a path"); + let abs_path = workspace + .project() + .read(cx) + .absolute_path(&project_path, cx); + assert_eq!( + abs_path.as_deref(), + Some(Path::new(path!("/outside/notes.md"))) + ); + item.downcast::().expect("should be an editor") + }); + editor.update_in(cx, |editor, window, cx| { + let snapshot = editor.snapshot(window, cx); + assert_eq!(editor.selections.newest::(&snapshot).head().row, 1); + }); + } } const FAST_MODE_WARNING_NAMESPACE: &str = "fast-mode-warning-dismissed"; diff --git a/crates/agent_ui/src/ui/mention_crease.rs b/crates/agent_ui/src/ui/mention_crease.rs index 97f45526cd..6959b5bc4c 100644 --- a/crates/agent_ui/src/ui/mention_crease.rs +++ b/crates/agent_ui/src/ui/mention_crease.rs @@ -160,14 +160,14 @@ fn open_mention_uri( workspace.update(cx, |workspace, cx| match mention_uri { MentionUri::File { abs_path } => { - open_file(workspace, abs_path, None, window, cx); + open_abs_path_at_point(workspace, abs_path, None, window, cx); } MentionUri::Symbol { abs_path, line_range, .. } => { - open_file( + open_abs_path_at_point( workspace, abs_path, Some(Point::new(*line_range.start(), 0)), @@ -180,7 +180,7 @@ fn open_mention_uri( line_range, column, } => { - open_file( + open_abs_path_at_point( workspace, abs_path, Some(Point::new(*line_range.start(), column.unwrap_or(0))), @@ -339,41 +339,6 @@ fn open_skill_content_buffer( workspace.add_item(pane, Box::new(editor), None, true, true, window, cx); } -fn open_file( - workspace: &mut Workspace, - abs_path: PathBuf, - point: Option, - window: &mut Window, - cx: &mut Context, -) { - if let Some(point) = point { - if open_abs_path_at_point(workspace, abs_path.clone(), point, window, cx) { - return; - } - } - - let project = workspace.project(); - if let Some(project_path) = - project.update(cx, |project, cx| project.find_project_path(&abs_path, cx)) - { - workspace - .open_path(project_path, None, true, window, cx) - .detach_and_log_err(cx); - } else if abs_path.exists() { - workspace - .open_abs_path( - abs_path, - OpenOptions { - focus: Some(true), - ..Default::default() - }, - window, - cx, - ) - .detach_and_log_err(cx); - } -} - fn reveal_in_project_panel( workspace: &mut Workspace, abs_path: PathBuf,