From 72d073d6423b0bf7e04aa87567a308d19617b7f2 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Tue, 6 Oct 2026 21:35:37 +0000 Subject: [PATCH] Fix agent file link navigation (#64748) Closes https://github.com/zed-industries/zed/issues/56633 Related to https://github.com/zed-industries/zed/issues/56067 Related to https://github.com/zed-industries/zed/issues/63968 Changes path handling to open files in the agent panel similar to what cmd-click does: regular open for files from the project, singe-file worktrees for files outside of the project Release Notes: - Fixed agent file link navigation --- crates/agent_ui/src/agent_ui.rs | 145 ++- .../src/conversation_view/thread_view.rs | 1090 +++++++++++++++-- crates/project/src/project.rs | 117 ++ .../tests/integration/project_tests.rs | 149 +++ crates/proto/proto/worktree.proto | 1 + crates/remote_server/src/headless_project.rs | 11 +- .../remote_server/src/remote_editing_tests.rs | 154 +++ crates/util/src/paths.rs | 94 +- 8 files changed, 1645 insertions(+), 116 deletions(-) diff --git a/crates/agent_ui/src/agent_ui.rs b/crates/agent_ui/src/agent_ui.rs index b18512281d4..eee943e6657 100644 --- a/crates/agent_ui/src/agent_ui.rs +++ b/crates/agent_ui/src/agent_ui.rs @@ -43,8 +43,9 @@ use std::sync::Arc; use ::ui::IconName; use agent_client_protocol::schema::{v1 as acp_v1, v2 as acp_v2}; use agent_settings::{AgentProfileId, AgentSettings}; +use anyhow::Context as _; use command_palette_hooks::CommandPaletteFilter; -use editor::{Editor, SelectionEffects, scroll::Autoscroll}; +use editor::{Editor, scroll::Autoscroll}; use feature_flags::FeatureFlagAppExt as _; use fs::Fs; use gpui::{ @@ -58,7 +59,7 @@ use language::{ use language_model::{ LanguageModel, LanguageModelId, LanguageModelProviderId, LanguageModelRegistry, }; -use project::{AgentId, DisableAiSettings}; +use project::{AgentId, DisableAiSettings, Project, ProjectPath}; use prompt_store::{self, PromptBuilder, rules_to_skills_migration}; use rope::Point; use schemars::JsonSchema; @@ -66,7 +67,11 @@ use serde::{Deserialize, Serialize}; use settings::{LanguageModelSelection, Settings as _, SettingsStore, SidebarSide}; use std::any::TypeId; use std::path::{Path, PathBuf}; -use workspace::{OpenOptions, Workspace}; +use util::{ + paths::SanitizedPath, + rel_path::{RelPath, RelPathBuf}, +}; +use workspace::Workspace; use crate::agent_configuration::ManageProfilesModal; pub use crate::agent_connection_store::{ActiveAcpConnection, AgentConnectionStore}; @@ -118,6 +123,72 @@ pub(crate) fn resolve_agent_image( None } +pub(crate) fn project_path_for_file_link( + project: &Project, + path: &Path, + cx: &App, +) -> Option { + let path_style = project.path_style(cx); + let validate_path = |project_path: ProjectPath, relative_path: &str| { + if project.get_open_buffer(&project_path, cx).is_none() + && !project.entry_for_path(&project_path, cx)?.is_file() + { + return None; + } + let worktree = project.worktree_for_id(project_path.worktree_id, cx)?; + let worktree = worktree.read(cx); + let mut prefix = RelPathBuf::new(); + for component in relative_path.split(path_style.separators_ch()) { + match component { + "" | "." => {} + ".." => { + let entry = worktree.entry_for_path(&prefix)?; + if !entry.is_dir() || entry.canonical_path.is_some() || !prefix.pop() { + return None; + } + } + component => prefix.push_component(component).ok()?, + } + } + Some(project_path) + }; + let path = SanitizedPath::new(path).as_path(); + if path_style.is_absolute(path.to_str()?) { + let project_path = project.project_path_for_absolute_path(path, cx)?; + let worktree = project.worktree_for_id(project_path.worktree_id, cx)?; + let root = worktree.read(cx).abs_path(); + let root_length = root + .to_str()? + .trim_end_matches(path_style.separators_ch()) + .len(); + let relative_path = path + .to_str()? + .get(root_length..)? + .trim_start_matches(path_style.separators_ch()); + validate_path(project_path, relative_path) + } else { + project.visible_worktrees(cx).find_map(|worktree| { + let worktree = worktree.read(cx); + [ + path.strip_prefix(worktree.root_name().as_std_path()).ok(), + Some(path), + ] + .into_iter() + .flatten() + .find_map(|relative_path| { + let path = RelPath::new(relative_path, path_style).ok()?.into_arc(); + validate_path( + ProjectPath { + worktree_id: worktree.id(), + path, + }, + relative_path.to_str()?, + ) + }) + }) + } +} + /// 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. @@ -128,39 +199,47 @@ pub(crate) fn open_abs_path_at_point( window: &mut Window, cx: &mut Context, ) { - 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 project_path = workspace.project().read_with(cx, |project, cx| { + project_path_for_file_link(project, &abs_path, cx) + }); + let project = workspace.project().downgrade(); let workspace = cx.weak_entity(); window .spawn(cx, async move |cx| { - let item = if let Some(project_path) = project_path { + let task = if let Some(project_path) = project_path { workspace .update_in(cx, |workspace, window, cx| { workspace.open_path(project_path, None, true, window, cx) - })? - .await? + }) + .ok() } 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:?}" - ); + let path = abs_path + .to_str() + .context("file link path is not valid UTF-8")?; + let Ok(task) = + project.update(cx, |project, cx| project.resolve_abs_file_link(path, cx)) + else { + return Ok(()); + }; + let Some(resolved_path) = task + .await + .with_context(|| format!("resolving agent file link {abs_path:?}"))? + else { + log::warn!( + "Could not resolve agent file link to {abs_path:?}: no matching file" + ); + return Ok(()); + }; workspace .update_in(cx, |workspace, window, cx| { - workspace.open_abs_path( - abs_path, - OpenOptions { - focus: Some(true), - ..Default::default() - }, - window, - cx, - ) - })? - .await? + workspace.open_resolved_path(resolved_path, window, cx) + }) + .ok() }; + let Some(task) = task else { + return Ok(()); + }; + let item = task.await?; let Some(point) = point else { return Ok(()); }; @@ -169,12 +248,14 @@ pub(crate) fn open_abs_path_at_point( }; editor .update_in(cx, |editor, window, cx| { - editor.change_selections( - SelectionEffects::scroll(Autoscroll::center()), - window, - cx, - |selections| selections.select_ranges([point..point]), - ); + if let Some(buffer) = editor.buffer().read(cx).as_singleton() { + let point = buffer + .read(cx) + .snapshot() + .point_from_external_input(point.row, point.column); + editor.go_to_singleton_buffer_point(point, window, cx); + editor.request_autoscroll(Autoscroll::center(), cx); + } }) .ok(); anyhow::Ok(()) diff --git a/crates/agent_ui/src/conversation_view/thread_view.rs b/crates/agent_ui/src/conversation_view/thread_view.rs index 8b0e7334e9b..8885a352dc9 100644 --- a/crates/agent_ui/src/conversation_view/thread_view.rs +++ b/crates/agent_ui/src/conversation_view/thread_view.rs @@ -1,11 +1,11 @@ use crate::{ DEFAULT_THREAD_TITLE, SelectPermissionGranularity, conversation_view::thread_search_bar::{ThreadSearchBar, ThreadSearchBarEvent}, - open_abs_path_at_point, + open_abs_path_at_point, project_path_for_file_link, thread_metadata_store::{ThreadId, ThreadMetadataStore}, }; use agent_client_protocol::schema::{v1 as acp_v1, v2 as acp_v2}; -use std::cell::RefCell; +use std::{cell::RefCell, path::Path}; use acp_thread::{ Elicitation, ElicitationEntryId, ElicitationStatus, ForegroundActivity, @@ -41,12 +41,14 @@ use language_model::{ LanguageModelProvider, LanguageModelProviderId, LanguageModelRegistry, Speed, }; use notifications::status_toast::StatusToast; +use project::ResolvedPath; use settings::{update_settings_file, update_settings_file_with_completion}; use ui::{ ButtonLike, CalloutBorderPosition, Checkbox, SpinnerLabel, SpinnerVariant, SplitButton, SplitButtonStyle, Tab, ToggleState, }; -use util::markdown::{source_position_from_fragment, split_local_url_fragment}; +use url::Url; +use util::markdown::source_position_from_fragment; use workspace::{OpenOptions, SERIALIZATION_THROTTLE_TIME}; use super::elicitation::{ @@ -13374,34 +13376,138 @@ pub(crate) fn open_link( cx: &mut App, ) { let Some(workspace) = workspace.upgrade() else { - cx.open_url(&url); return; }; let path_style = workspace.read(cx).path_style(cx); - let (relative_path, fragment) = split_local_url_fragment(&url); - if let Some(fragment) = fragment - && !relative_path.is_empty() - && !path_style.is_absolute(relative_path) - { - let project = workspace.read(cx).project().clone(); - let decoded_path = decode_path_escapes(relative_path); - let abs_path = project.update(cx, |project, cx| { - let resolve_path = |path: &str| { - let project_path = project.find_project_path(path, cx)?; - project.entry_for_path(&project_path, cx)?; - project.absolute_path(&project_path, cx) - }; - resolve_path(&decoded_path).or_else(|| resolve_path(relative_path)) - }); - if let Some(abs_path) = abs_path { - let point = fragment - .strip_prefix('L') + let path_url = url + .strip_prefix('`') + .and_then(|path| path.strip_suffix('`')) + .unwrap_or(&url); + if let Some((path, fragment)) = file_link_parts(path_url, path_style) { + if !path.is_empty() { + let fragment_point = fragment .and_then(source_position_from_fragment) - .map(|(row, _)| Point::new(row, 0)); - workspace.update(cx, |workspace, cx| { - open_abs_path_at_point(workspace, abs_path, point, window, cx); - }); + .map(|(row, column)| Point::new(row, column)); + let candidates = file_link_candidates(path, fragment_point, path_style); + let project = workspace.read(cx).project().downgrade(); + let Ok((roots, root_names)) = project.read_with(cx, |project, cx| { + project + .visible_worktrees(cx) + .filter_map(|worktree| { + let worktree = worktree.read(cx); + (!worktree.is_single_file()) + .then(|| (worktree.abs_path(), worktree.root_name().to_owned())) + }) + .unzip::<_, _, Vec<_>, Vec<_>>() + }) else { + return; + }; + let workspace = workspace.downgrade(); + window + .spawn(cx, async move |cx| { + let mut target = None; + let mut failures = Vec::new(); + 'resolve: for (path, point) in candidates { + let Ok(project_path) = project.read_with(cx, |project, cx| { + project_path_for_file_link(project, &path, cx) + }) else { + return Ok(()); + }; + if let Some(project_path) = project_path { + target = Some(( + ResolvedPath::ProjectPath { + project_path, + is_dir: false, + }, + point, + )); + break; + } + let paths = if path_style.is_absolute(&path.to_string_lossy()) + || path.starts_with("~") + { + vec![path] + } else { + roots + .iter() + .zip(&root_names) + .flat_map(|(root, root_name)| { + [ + path.strip_prefix(root_name.as_std_path()).ok(), + Some(path.as_path()), + ] + .into_iter() + .flatten() + .filter_map(move |path| { + path_style.join_path_preserving_components(root, path).ok() + }) + }) + .collect::>() + }; + for path in paths { + let Some(path_string) = path.to_str() else { + failures.push(format!("{path:?}: path is not valid UTF-8")); + continue; + }; + let Ok(task) = project.update(cx, |project, cx| { + project.resolve_abs_file_link(path_string, cx) + }) else { + return Ok(()); + }; + let resolved_path = match task.await { + Ok(Some(path)) => path, + Ok(None) => { + failures.push(format!("{path:?}: no matching file")); + continue; + } + Err(error) => { + failures.push(format!("{path:?}: {error:#}")); + continue; + } + }; + target = Some((resolved_path, point)); + break 'resolve; + } + } + let Some((target, point)) = target else { + let details = if failures.is_empty() { + "no candidate file paths".to_string() + } else { + failures.join("; ") + }; + log::warn!( + "Could not resolve agent file link {url:?} against project roots {roots:?}: {details}" + ); + return anyhow::Ok(()); + }; + let Some(task) = workspace + .update_in(cx, |workspace, window, cx| { + workspace.open_resolved_path(target, window, cx) + }) + .ok() + else { + return Ok(()); + }; + let item = task.await?; + if let Some(point) = point + && let Some(editor) = item.downcast::() + { + editor + .update_in(cx, |editor, window, cx| { + if let Some(buffer) = editor.buffer().read(cx).as_singleton() { + let point = buffer + .read(cx) + .snapshot() + .point_from_external_input(point.row, point.column); + editor.go_to_singleton_buffer_point(point, window, cx); + } + }) + .ok(); + } + Ok(()) + }) + .detach_and_log_err(cx); return; } } @@ -13512,6 +13618,76 @@ pub(crate) fn open_link( } } +fn file_link_parts(input: &str, path_style: PathStyle) -> Option<(&str, Option<&str>)> { + let (path, fragment) = input + .split_once('#') + .map_or((input, None), |(path, fragment)| (path, Some(fragment))); + if !path_style.is_absolute(path) + && let Ok(url) = Url::parse(input) + && (!url.scheme().contains('.') + || url.path().trim_matches(':').is_empty() + || !PathWithPosition::parse_str(path) + .path + .to_str() + .is_some_and(|name| name.eq_ignore_ascii_case(url.scheme()))) + { + return None; + } + Some((path, fragment)) +} + +fn file_link_candidates( + path: &str, + fragment_point: Option, + path_style: PathStyle, +) -> Vec<(PathBuf, Option)> { + if path_style.is_windows() && path_style.is_absolute(path) { + return [ + MentionUri::parse_hyperlink(path, path_style).ok(), + MentionUri::parse_hyperlink_literal(path, path_style), + ] + .into_iter() + .flatten() + .filter_map(|mention| match mention { + MentionUri::File { abs_path } => Some((abs_path, fragment_point)), + MentionUri::Selection { + abs_path: Some(abs_path), + line_range, + column, + } => Some(( + abs_path, + Some( + fragment_point.unwrap_or(Point::new(*line_range.start(), column.unwrap_or(0))), + ), + )), + _ => None, + }) + .collect(); + } + let decoded_path = decode_path_escapes(path); + let mut candidates = Vec::new(); + for path in std::iter::once(decoded_path.as_ref()).chain((decoded_path != path).then_some(path)) + { + let path = if path_style.is_windows() { + PathBuf::from(path.replace('\\', "/")) + } else { + PathBuf::from(path) + }; + candidates.push((path.clone(), fragment_point)); + let position = PathWithPosition::parse_str(&path.to_string_lossy()); + if let Some(row) = position.row.and_then(|row| row.checked_sub(1)) { + candidates.push(( + position.path, + Some(fragment_point.unwrap_or(Point::new( + row, + position.column.unwrap_or(1).saturating_sub(1), + ))), + )); + } + } + candidates +} + /// Returns the name of the leading built-in (native-category) slash command — /// e.g. `compact` for `/compact` or `/compact summarize the API work` — whether /// or not the user typed any trailing text after it. Built-in commands ignore @@ -13551,9 +13727,11 @@ fn strip_leading_command(text: &str, command_name: &str) -> String { #[cfg(test)] mod tests { use super::*; + use gpui::UpdateGlobal; use project::{FakeFs, Project}; use serde_json::json; - use std::path::Path; + use settings::{SettingsStore, SplicingVec}; + use std::sync::Once; use util::path; use workspace::MultiWorkspace; @@ -13663,6 +13841,86 @@ mod tests { assert_eq!(strip_leading_command("hello", "compact"), "hello"); } + #[test] + fn test_file_link_parts() { + for (input, expected) in [ + ("src/main.rs:2", Some(("src/main.rs:2", None))), + ("./tel:123", Some(("./tel:123", None))), + ("main.rs:2#L3", Some(("main.rs:2", Some("L3")))), + ("main.rs:2:4#3C2", Some(("main.rs:2:4", Some("3C2")))), + ("main.rs:0", Some(("main.rs:0", None))), + ("main.rs:4294967295", Some(("main.rs:4294967295", None))), + ("main.rs:4294967296", Some(("main.rs:4294967296", None))), + ("main.rs:2:4294967296", Some(("main.rs:2:4294967296", None))), + ("custom.proto:123", Some(("custom.proto:123", None))), + ("main.rs:2:3:4", None), + ("main.rs:2:", Some(("main.rs:2:", None))), + ("MAIN.RS:2:", Some(("MAIN.RS:2:", None))), + ("main.rs:-2", None), + ("tel:123", None), + ("custom:123", None), + ("com.example.viewer:", None), + ("com.example.viewer::", None), + ("file:/project/a:2", None), + ("file:///project/a%3A2", None), + ("https://example.com/main.rs:2#L3", None), + ] { + assert_eq!(file_link_parts(input, PathStyle::Unix), expected, "{input}"); + } + } + + #[test] + fn test_file_link_candidates_windows() { + for (path, expected_path, expected_point) in [ + ( + r"C:\project\main.rs#L2", + r"C:\project\main.rs", + Point::new(1, 0), + ), + ( + "/C:/project/main.rs#42", + r"C:\project\main.rs", + Point::new(41, 0), + ), + ( + "/c/project/main.rs:2#L3C4", + r"C:\project\main.rs", + Point::new(2, 3), + ), + ( + "//server/share/main.rs#42:3", + r"\\server\share\main.rs", + Point::new(41, 2), + ), + ( + "/C:/project/main.rs:2:4", + r"C:\project\main.rs", + Point::new(1, 3), + ), + ( + "/c/project/main.rs:2", + r"C:\project\main.rs", + Point::new(1, 0), + ), + ( + r"C:\project\a%20b.rs:2", + r"C:\project\a b.rs", + Point::new(1, 0), + ), + ] { + let (path, fragment) = file_link_parts(path, PathStyle::Windows).unwrap(); + let fragment_point = fragment + .and_then(source_position_from_fragment) + .map(|(row, column)| Point::new(row, column)); + let candidates = file_link_candidates(path, fragment_point, PathStyle::Windows); + assert_eq!( + candidates.first(), + Some(&(PathBuf::from(expected_path), Some(expected_point))), + "{path}" + ); + } + } + #[gpui::test] async fn test_open_link_bare_path(cx: &mut gpui::TestAppContext) { crate::test_support::init_test(cx); @@ -13723,6 +13981,235 @@ mod tests { }); } + #[gpui::test] + async fn test_open_link_relative_positions(cx: &mut gpui::TestAppContext) { + crate::test_support::init_test(cx); + cx.update(|cx| { + SettingsStore::update_global(cx, |store, cx| { + store.update_user_settings(cx, |settings| { + settings.project.worktree.file_scan_exclusions = + Some(SplicingVec::from(vec!["**/excluded.rs".to_string()])); + }); + }); + }); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/project"), + json!({ + "src": {"main.rs": "first\naéøbc\nthird\n", "a": "first\nsecond\n", "excluded.rs": "first\nsecond\n"}, + "main.rs": "first\naéøbc\nthird\n", + "project": {"src": {"main.rs": "wrong file"}}, + "x.rs": {"x.rs": "first\nsecond\n"}, + "a b.rs": "first\nsecond\n", + "a%20b.rs": "wrong file", + "literal%20space.rs": "first\nsecond\n", + "a%2Fb.rs": "first\nsecond\n", + }), + ) + .await; + fs.insert_tree(path!("/other"), json!({"main.rs": "one\ntwo\n"})) + .await; + #[cfg(not(target_os = "windows"))] + fs.insert_tree( + path!("/project"), + json!({"literal.rs": "wrong file", "literal.rs:2": "literal file"}), + ) + .await; + + let project = Project::test( + fs, + [path!("/project").as_ref(), path!("/other").as_ref()], + cx, + ) + .await; + let worktree_ids = project.read_with(cx, |project, cx| { + project + .worktrees(cx) + .map(|worktree| worktree.read(cx).id()) + .collect::>() + }); + let (multi_workspace, cx) = + cx.add_window_view(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx)); + let workspace = multi_workspace.read_with(cx, |workspace, _| workspace.workspace().clone()); + let workspace_weak = workspace.downgrade(); + + for (url, expected_path, expected_point) in [ + ( + "project/src/excluded.rs:2", + path!("/project/src/excluded.rs"), + Point::new(1, 0), + ), + ( + "src/excluded.rs:2", + path!("/project/src/excluded.rs"), + Point::new(1, 0), + ), + ( + path!("/project/main.rs#2"), + path!("/project/main.rs"), + Point::new(1, 0), + ), + ( + path!("/project/main.rs#L3"), + path!("/project/main.rs"), + Point::new(2, 0), + ), + ( + path!("/project/main.rs#2C4"), + path!("/project/main.rs"), + Point::new(1, 5), + ), + ("main.rs:2#L3", path!("/project/main.rs"), Point::new(2, 0)), + ( + "main.rs:3#L2:4", + path!("/project/main.rs"), + Point::new(1, 5), + ), + ("./src/a:2", path!("/project/src/a"), Point::new(1, 0)), + ("src/a:2:4", path!("/project/src/a"), Point::new(1, 3)), + ( + "src/main.rs:2", + path!("/project/src/main.rs"), + Point::new(1, 0), + ), + ( + "src/main.rs:2:4", + path!("/project/src/main.rs"), + Point::new(1, 5), + ), + ( + "project/src/main.rs:3", + path!("/project/src/main.rs"), + Point::new(2, 0), + ), + ( + "./src/main.rs:1:2", + path!("/project/src/main.rs"), + Point::new(0, 1), + ), + ("main.rs:2", path!("/project/main.rs"), Point::new(1, 0)), + ("main.rs:2:", path!("/project/main.rs"), Point::new(1, 0)), + ("main.rs(2,4)", path!("/project/main.rs"), Point::new(1, 5)), + ("other/main.rs:2", path!("/other/main.rs"), Point::new(1, 0)), + ("x.rs/x.rs:2", path!("/project/x.rs/x.rs"), Point::new(1, 0)), + ("a%20b.rs:2", path!("/project/a b.rs"), Point::new(1, 0)), + ( + "literal%20space.rs:2", + path!("/project/literal%20space.rs"), + Point::new(1, 0), + ), + ("a%2Fb.rs:2", path!("/project/a%2Fb.rs"), Point::new(1, 0)), + ( + "src/main.rs#L2", + path!("/project/src/main.rs"), + Point::new(1, 0), + ), + ( + "src/main.rs:2:0", + path!("/project/src/main.rs"), + Point::new(1, 0), + ), + ( + "src/main.rs:4294967295", + path!("/project/src/main.rs"), + Point::new(3, 0), + ), + #[cfg(not(target_os = "windows"))] + ( + "file:/project/literal.rs:2", + path!("/project/literal.rs:2"), + Point::new(0, 0), + ), + #[cfg(not(target_os = "windows"))] + ( + "literal.rs:2", + path!("/project/literal.rs:2"), + Point::new(0, 0), + ), + ] { + multi_workspace.update_in(cx, |_, window, cx| { + open_link(SharedString::from(url), &workspace_weak, window, cx); + }); + cx.run_until_parked(); + assert_eq!(cx.opened_url(), None, "{url}"); + 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 project path"); + assert_eq!( + project.read(cx).absolute_path(&project_path, cx).as_deref(), + Some(Path::new(expected_path)), + "{url}" + ); + item.downcast::() + .expect("file should be open in an editor") + }); + editor.update_in(cx, |editor, window, cx| { + let snapshot = editor.snapshot(window, cx); + assert_eq!( + editor.selections.newest::(&snapshot).head(), + expected_point, + "{url}" + ); + }); + project.read_with(cx, |project, cx| { + assert_eq!( + project + .worktrees(cx) + .map(|worktree| worktree.read(cx).id()) + .collect::>(), + worktree_ids, + "{url}" + ); + }); + } + } + + #[gpui::test] + async fn test_open_link_external_urls(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": ""}, "tel": "not a phone", "custom": "not a URI handler"}), + ) + .await; + #[cfg(not(target_os = "windows"))] + fs.insert_tree( + path!("/project"), + json!({"mailto:contact@example.com": "not the email handler"}), + ) + .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, |workspace, _| workspace.workspace().clone()); + + for url in [ + "https://example.com/src/main.rs:2", + "mailto:contact@example.com", + "tel:123", + "custom:123", + "com.example.viewer:", + "com.example.viewer::", + ] { + multi_workspace.update_in(cx, |_, window, cx| { + open_link(SharedString::from(url), &workspace.downgrade(), window, cx); + }); + cx.run_until_parked(); + assert_eq!(cx.opened_url().as_deref(), Some(url)); + workspace.read_with(cx, |workspace, cx| { + assert!(workspace.active_item(cx).is_none()); + }); + } + project.read_with(cx, |project, cx| { + assert_eq!(project.worktrees(cx).count(), 1); + }); + } + #[gpui::test] async fn test_open_link_percent_escape_disambiguation(cx: &mut gpui::TestAppContext) { crate::test_support::init_test(cx); @@ -13791,6 +14278,205 @@ mod tests { }); } + #[gpui::test] + async fn test_file_link_parent_traversal_uses_selected_worktree(cx: &mut gpui::TestAppContext) { + crate::test_support::init_test(cx); + let fs = FakeFs::new(cx.executor()); + fs.insert_tree(path!("/one"), json!({"two": {"src": "not a directory"}})) + .await; + fs.insert_tree( + path!("/two"), + json!({"src": {}, "file.rs": "selected file"}), + ) + .await; + let project = Project::test(fs, [path!("/one").as_ref(), path!("/two").as_ref()], cx).await; + let unindexed_buffer = project + .update(cx, |project, cx| { + let path = project + .project_path_for_absolute_path(Path::new(path!("/one/file.rs")), cx) + .unwrap(); + project.open_buffer(path, cx) + }) + .await + .unwrap(); + project.read_with(cx, |project, cx| { + let unindexed_path = project + .project_path_for_absolute_path(Path::new(path!("/one/file.rs")), cx) + .unwrap(); + assert!(project.entry_for_path(&unindexed_path, cx).is_none()); + assert_eq!( + project.get_open_buffer(&unindexed_path, cx).unwrap(), + unindexed_buffer, + ); + let expected = project + .project_path_for_absolute_path(Path::new(path!("/two/file.rs")), cx) + .unwrap(); + for path in [ + "src/../file.rs", + "two/src/../file.rs", + "two/file.rs", + path!("/two/src/../file.rs"), + ] { + assert_eq!( + project_path_for_file_link(project, Path::new(path), cx), + Some(expected.clone()), + "{path}", + ); + } + }); + } + + #[gpui::test] + async fn test_open_link_preserves_project_symlink_buffer(cx: &mut gpui::TestAppContext) { + crate::test_support::init_test(cx); + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/project"), + json!({"sub": {}, "main.rs": "other file"}), + ) + .await; + fs.insert_tree(path!("/outside"), json!({"target.rs": "original\n"})) + .await; + fs.insert_symlink( + path!("/project/link.rs"), + PathBuf::from(path!("/outside/target.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, |workspace, _| workspace.workspace().clone()); + let workspace_weak = workspace.downgrade(); + multi_workspace.update_in(cx, |_, window, cx| { + open_link(SharedString::from("link.rs"), &workspace_weak, window, cx); + }); + cx.run_until_parked(); + let original = workspace.read_with(cx, |workspace, cx| { + workspace + .active_item(cx) + .unwrap() + .downcast::() + .unwrap() + }); + original.update_in(cx, |editor, window, cx| { + editor.insert("unsaved ", window, cx) + }); + + for excluded in [false, true] { + if excluded { + cx.update(|_, cx| { + SettingsStore::update_global(cx, |store, cx| { + store.update_user_settings(cx, |settings| { + settings.project.worktree.file_scan_exclusions = + Some(SplicingVec::from(vec![ + "**/link.rs".to_string(), + "**/sub".to_string(), + ])); + }); + }); + }); + cx.run_until_parked(); + project.read_with(cx, |project, cx| { + let path = project + .project_path_for_absolute_path(Path::new(path!("/project/link.rs")), cx) + .unwrap(); + assert!(project.entry_for_path(&path, cx).is_none()); + assert!(project.get_open_buffer(&path, cx).is_some()); + }); + } + for (url, use_absolute_helper) in [ + ("link.rs", false), + ("project/link.rs", false), + (path!("/project/link.rs"), false), + (path!("/project//link.rs"), false), + (path!("/project//link.rs"), true), + (path!("/project/sub/../link.rs"), false), + ("project/sub/../link.rs#L1", false), + (path!("/project/sub/../link.rs"), true), + ] { + multi_workspace.update_in(cx, |_, window, cx| { + open_link(SharedString::from("main.rs"), &workspace_weak, window, cx); + }); + cx.run_until_parked(); + multi_workspace.update_in(cx, |_, window, cx| { + if use_absolute_helper { + workspace.update(cx, |workspace, cx| { + open_abs_path_at_point(workspace, PathBuf::from(url), None, window, cx); + }); + } else { + open_link(SharedString::from(url), &workspace_weak, window, cx); + } + }); + cx.run_until_parked(); + workspace.read_with(cx, |workspace, cx| { + let active = workspace + .active_item(cx) + .unwrap() + .downcast::() + .unwrap(); + assert_eq!(active.entity_id(), original.entity_id(), "{url}"); + assert_eq!(active.read(cx).text(cx), "unsaved original\n"); + assert_eq!(project.read(cx).worktrees(cx).count(), 1); + }); + assert_eq!(cx.opened_url(), None); + } + } + } + + #[gpui::test] + async fn test_open_link_does_not_retain_closed_project(cx: &mut gpui::TestAppContext) { + crate::test_support::init_test(cx); + + let mut previous_workspace = None; + for url in [ + format!("{}:2", path!("/outside/notes.md")), + format!( + "{}#L2", + Url::from_file_path(path!("/outside/notes.md")).unwrap() + ), + ] { + let fs = FakeFs::new(cx.executor()); + fs.insert_tree(path!("/project"), json!({"main.rs": ""})) + .await; + fs.insert_tree(path!("/outside"), json!({"notes.md": "first\nsecond\n"})) + .await; + let project = Project::test(fs, [path!("/project").as_ref()], cx).await; + let project_weak = project.downgrade(); + let (multi_workspace, window_cx) = cx.add_window_view(|window, cx| { + MultiWorkspace::test_new(project.clone(), window, cx) + }); + let multi_workspace_weak = multi_workspace.downgrade(); + let workspace = + multi_workspace.read_with(window_cx, |workspace, _| workspace.workspace().clone()); + let workspace_weak = workspace.downgrade(); + if let Some(previous_workspace) = previous_workspace.take() { + window_cx.update(|window, cx| { + open_link( + SharedString::from("missing.rs:2"), + &previous_workspace, + window, + cx, + ); + }); + assert_eq!(window_cx.opened_url(), None); + } + drop(project); + drop(workspace); + drop(multi_workspace); + + window_cx.update(|window, cx| { + open_link(SharedString::from(url.clone()), &workspace_weak, window, cx); + window.remove_window(); + }); + assert!(multi_workspace_weak.upgrade().is_none(), "{url}"); + assert!(workspace_weak.upgrade().is_none(), "{url}"); + assert!(project_weak.upgrade().is_none(), "{url}"); + window_cx.run_until_parked(); + assert_eq!(window_cx.opened_url(), None, "{url}"); + previous_workspace = Some(workspace_weak); + } + } + #[gpui::test] async fn test_open_link_out_of_project_path(cx: &mut gpui::TestAppContext) { crate::test_support::init_test(cx); @@ -13798,60 +14484,316 @@ mod tests { 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; + fs.insert_tree( + path!("/outside"), + json!({ + "notes.md": "first\naéøbc\nthird\n", + "a b.rs": "first\nsecond\n", + "a%20b.rs": "literal file", + "literal%20space.rs": "first\nsecond\n", + "x.rs": {"x.rs": "first\nsecond\n"}, + }), + ) + .await; + + fs.insert_symlink( + path!("/outside/notes-link.md"), + PathBuf::from(path!("/outside/notes.md")), + ) + .await; + #[cfg(not(target_os = "windows"))] + fs.insert_tree( + path!("/outside"), + json!({"literal.rs": "base file", "literal.rs:2": "literal file"}), + ) + .await; let project = Project::test(fs, [path!("/project").as_ref()], cx).await; + let visible_worktree_ids = project.read_with(cx, |project, cx| { + project + .visible_worktrees(cx) + .map(|worktree| worktree.read(cx).id()) + .collect::>() + }); 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 = multi_workspace.read_with(cx, |workspace, _| workspace.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" - ); - }); + let missing_file_uri = Url::from_file_path(path!("/outside/missing.md")) + .unwrap() + .to_string(); + for (url, attempted_paths) in [ + ( + "main.rs:2".to_string(), + vec![ + PathBuf::from(path!("/project/main.rs:2")), + PathBuf::from(path!("/project/main.rs")), + ], + ), + ( + "main.rs:2#L3".to_string(), + vec![ + PathBuf::from(path!("/project/main.rs:2")), + PathBuf::from(path!("/project/main.rs")), + ], + ), + ( + "crates/editor/src/inlays/inlay_hints.rs:394".to_string(), + vec![ + Path::new(path!("/project")) + .join("crates/editor/src/inlays/inlay_hints.rs:394"), + Path::new(path!("/project")) + .join("crates/editor/src/inlays") + .join("inlay_hints.rs"), + ], + ), + ( + "crates/editor/src/element.rs:608".to_string(), + vec![ + Path::new(path!("/project")).join("crates/editor/src/element.rs:608"), + Path::new(path!("/project")) + .join("crates/editor/src") + .join("element.rs"), + ], + ), + (missing_file_uri.clone(), Vec::new()), + ( + "../outside/missing.md:2".to_string(), + vec![ + Path::new(path!("/project")).join("../outside/missing.md:2"), + Path::new(path!("/project")) + .join("../outside") + .join("missing.md"), + ], + ), + ( + path!("/outside/missing.md").to_string(), + vec![PathBuf::from(path!("/outside/missing.md"))], + ), + ( + path!("/project/src/missing.md").to_string(), + vec![PathBuf::from(path!("/project/src/missing.md"))], + ), + ( + path!("/outside").to_string(), + vec![PathBuf::from(path!("/outside"))], + ), + ] { + let warnings = FileLinkWarningCapture::new(); + multi_workspace.update_in(cx, |_, window, cx| { + open_link(SharedString::from(url.clone()), &workspace_weak, window, cx); + }); + cx.run_until_parked(); + let expected_warning = if url == missing_file_uri { + format!( + "Could not resolve agent file link to {:?}: no matching file", + Path::new(path!("/outside/missing.md")) + ) + } else { + let failures = attempted_paths + .into_iter() + .map(|path| format!("{path:?}: no matching file")) + .collect::>() + .join("; "); + format!( + "Could not resolve agent file link {url:?} against project roots {:?}: {failures}", + [Path::new(path!("/project"))] + ) + }; + assert_eq!(warnings.take(), [expected_warning], "{url}"); + workspace.read_with(cx, |workspace, cx| { + assert!(workspace.active_item(cx).is_none()); + assert_eq!(workspace.notification_ids().len(), 0); + assert_eq!(project.read(cx).worktrees(cx).count(), 1); + }); + assert_eq!(cx.opened_url(), None); + assert!(cx.pending_prompt().is_none()); + } - // 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); - }); + let file_uri = url::Url::from_file_path(path!("/outside/notes.md")).unwrap(); + let literal_uri = Url::from_file_path(path!("/outside/a%20b.rs")).unwrap(); + let symlink_uri = Url::from_file_path(path!("/outside/notes-link.md")).unwrap(); + let mut opened_worktrees = HashMap::default(); + for (url, expected_path, expected_point) in [ + ( + literal_uri.to_string(), + path!("/outside/a%20b.rs"), + Point::new(0, 0), + ), + ( + format!("{}:2", path!("/outside/a%20b.rs")), + path!("/outside/a b.rs"), + Point::new(1, 0), + ), + #[cfg(not(target_os = "windows"))] + ( + path!("/outside/literal.rs").to_string(), + path!("/outside/literal.rs"), + Point::new(0, 0), + ), + #[cfg(not(target_os = "windows"))] + ( + "../outside/literal.rs:2".to_string(), + path!("/outside/literal.rs:2"), + Point::new(0, 0), + ), + ( + "../outside/notes.md:2:4".to_string(), + path!("/outside/notes.md"), + Point::new(1, 5), + ), + ( + format!("{}:3", path!("/outside/notes.md")), + path!("/outside/notes.md"), + Point::new(2, 0), + ), + ( + format!("{file_uri}#L2"), + path!("/outside/notes.md"), + Point::new(1, 0), + ), + ( + "../outside/notes.md#L3".to_string(), + path!("/outside/notes.md"), + Point::new(2, 0), + ), + ( + "../outside/a%20b.rs:2".to_string(), + path!("/outside/a b.rs"), + Point::new(1, 0), + ), + ( + format!("{}:2", path!("/outside/literal%20space.rs")), + path!("/outside/literal%20space.rs"), + Point::new(1, 0), + ), + ( + "../outside/x.rs/x.rs:2".to_string(), + path!("/outside/x.rs/x.rs"), + Point::new(1, 0), + ), + ( + format!("{symlink_uri}#L2"), + path!("/outside/notes.md"), + Point::new(1, 0), + ), + ( + "../outside/notes-link.md:3".to_string(), + path!("/outside/notes.md"), + Point::new(2, 0), + ), + ] { + multi_workspace.update_in(cx, |_, window, cx| { + open_link(SharedString::from(url.clone()), &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 project = project.read(cx); + assert_eq!( + project.absolute_path(&project_path, cx).as_deref(), + Some(Path::new(expected_path)), + "{url}" + ); + assert!(project_path.path.is_empty()); + let worktree = project + .worktree_for_id(project_path.worktree_id, cx) + .unwrap(); + let worktree = worktree.read(cx); + assert!(!worktree.is_visible()); + assert!(worktree.is_single_file()); + assert_eq!(worktree.abs_path().as_ref(), Path::new(expected_path)); + if let Some(previous_id) = opened_worktrees.insert(expected_path, worktree.id()) { + assert_eq!(previous_id, worktree.id()); + } + assert_eq!(project.worktrees(cx).count(), 1 + opened_worktrees.len()); + assert_eq!( + project + .visible_worktrees(cx) + .map(|worktree| worktree.read(cx).id()) + .collect::>(), + visible_worktree_ids + ); + assert_eq!(workspace.notification_ids().len(), 0); + 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(), + expected_point, + "{url}" + ); + }); + assert_eq!(cx.opened_url(), None); + assert!(cx.pending_prompt().is_none()); + } + } + + thread_local! { + static FILE_LINK_WARNINGS: RefCell>> = const { RefCell::new(None) }; + } + + struct FileLinkWarningCapture { + previous_level: log::LevelFilter, + } + + impl FileLinkWarningCapture { + fn new() -> Self { + static INSTALL_LOGGER: Once = Once::new(); + INSTALL_LOGGER.call_once(|| { + log::set_logger(&FileLinkTestLogger) + .expect("failed to install file-link test logger"); + }); + let previous_level = log::max_level(); + assert!(FILE_LINK_WARNINGS.replace(Some(Vec::new())).is_none()); + log::set_max_level(previous_level.max(log::LevelFilter::Warn)); + Self { previous_level } + } + + fn take(&self) -> Vec { + FILE_LINK_WARNINGS.with_borrow_mut(|warnings| { + std::mem::take(warnings.as_mut().expect("warning capture should be active")) + }) + } + } + + impl Drop for FileLinkWarningCapture { + fn drop(&mut self) { + drop(FILE_LINK_WARNINGS.take()); + log::set_max_level(self.previous_level); + } + } + + struct FileLinkTestLogger; + + impl log::Log for FileLinkTestLogger { + fn enabled(&self, metadata: &log::Metadata<'_>) -> bool { + metadata.level() <= log::max_level() + } + + fn log(&self, record: &log::Record<'_>) { + if !self.enabled(record.metadata()) { + return; + } + let captured = FILE_LINK_WARNINGS.with_borrow_mut(|warnings| { + if record.level() == log::Level::Warn + && let Some(warnings) = warnings + { + warnings.push(record.args().to_string()); + true + } else { + false + } + }); + if !captured { + eprintln!("{} {}: {}", record.level(), record.target(), record.args()); + } + } + + fn flush(&self) {} } } diff --git a/crates/project/src/project.rs b/crates/project/src/project.rs index ae264f3a0e6..b85cf191e02 100644 --- a/crates/project/src/project.rs +++ b/crates/project/src/project.rs @@ -4978,6 +4978,122 @@ impl Project { }) } + pub fn resolve_abs_file_link( + &self, + path: &str, + cx: &mut Context, + ) -> Task>> { + let resolve_task = self.resolve_abs_file_path_canonical(path, cx); + let path = if self.is_local() { + shellexpand::tilde(path).into_owned() + } else { + path.to_owned() + }; + cx.spawn(async move |project, cx| { + let Some(resolved_path) = resolve_task.await? else { + return Ok(None); + }; + let path = if path.starts_with("~") { + let Ok(task) = + project.update(cx, |project, cx| project.resolve_abs_path(&path, cx)) + else { + return Ok(None); + }; + let Some(path) = task.await.and_then(ResolvedPath::into_abs_path) else { + return Ok(Some(resolved_path)); + }; + path + } else { + path + }; + let Ok(candidate) = project.update(cx, |project, cx| { + let lexical_path = project.path_style(cx).normalize(&path); + let project_path = + project.project_path_for_absolute_path(Path::new(&lexical_path), cx)?; + let abs_path = project.absolute_path(&project_path, cx)?; + let task = project.resolve_abs_file_path_canonical(abs_path.to_str()?, cx); + Some((project_path, abs_path, task)) + }) else { + return Ok(None); + }; + let Some((project_path, abs_path, task)) = candidate else { + return Ok(Some(resolved_path)); + }; + let canonical_candidate = task + .await + .with_context(|| format!("validating file link alias {abs_path:?}")) + .log_err() + .flatten(); + let Ok(alias_unchanged) = project.read_with(cx, |project, cx| { + project.absolute_path(&project_path, cx).as_deref() == Some(abs_path.as_path()) + }) else { + return Ok(None); + }; + if alias_unchanged + && canonical_candidate + .is_some_and(|candidate| candidate.abs_path() == resolved_path.abs_path()) + { + Ok(Some(ResolvedPath::ProjectPath { + project_path, + is_dir: false, + })) + } else { + Ok(Some(resolved_path)) + } + }) + } + + pub fn resolve_abs_file_path_canonical( + &self, + path: &str, + cx: &mut Context, + ) -> Task>> { + if self.is_local() { + let expanded = PathBuf::from(shellexpand::tilde(path).into_owned()); + let fs = self.fs.clone(); + cx.background_spawn(async move { + if fs + .metadata(&expanded) + .await? + .is_none_or(|metadata| metadata.is_dir) + { + return Ok(None); + } + let path = fs.canonicalize(&expanded).await?; + let path = path + .to_str() + .context("canonical file path is not valid UTF-8")? + .to_owned(); + Ok(Some(ResolvedPath::AbsPath { + path, + is_dir: false, + })) + }) + } else if let Some(ssh_client) = self.remote_client.as_ref() { + let request = ssh_client + .read(cx) + .proto_client() + .request(proto::GetPathMetadata { + project_id: REMOTE_SERVER_PROJECT_ID, + path: path.into(), + canonicalize: true, + }); + cx.background_spawn(async move { + let response = request.await?; + if response.exists && !response.is_dir { + Ok(Some(ResolvedPath::AbsPath { + path: response.path, + is_dir: false, + })) + } else { + Ok(None) + } + }) + } else { + Task::ready(Ok(None)) + } + } + pub fn resolve_abs_path(&self, path: &str, cx: &App) -> Task> { if self.is_local() { let expanded = PathBuf::from(shellexpand::tilde(&path).into_owned()); @@ -4997,6 +5113,7 @@ impl Project { .request(proto::GetPathMetadata { project_id: REMOTE_SERVER_PROJECT_ID, path: path.into(), + canonicalize: false, }); cx.background_spawn(async move { let response = request.await.log_err()?; diff --git a/crates/project/tests/integration/project_tests.rs b/crates/project/tests/integration/project_tests.rs index 164616bc2f8..5cbbf8e115d 100644 --- a/crates/project/tests/integration/project_tests.rs +++ b/crates/project/tests/integration/project_tests.rs @@ -85,6 +85,8 @@ use std::{ task::Poll, time::Duration, }; +#[cfg(target_os = "linux")] +use std::{ffi::OsStr, os::unix::ffi::OsStrExt as _}; use sum_tree::SumTree; use task::{ResolvedTask, ShellKind, TaskContext}; use text::{Anchor, PointUtf16, ReplicaId, ToOffset, Unclipped}; @@ -239,6 +241,153 @@ async fn test_symlinks(cx: &mut gpui::TestAppContext) { }); } +#[cfg(not(windows))] +#[gpui::test] +async fn test_resolve_abs_file_link_project_aliases(cx: &mut gpui::TestAppContext) { + init_test(cx); + cx.executor().allow_parking(); + cx.update(|cx| { + SettingsStore::update_global(cx, |store, cx| { + store.update_user_settings(cx, |settings| { + settings.project.worktree.file_scan_exclusions = + Some(SplicingVec::from(vec!["**/hidden".to_string()])); + }); + }); + }); + + let directory = TempTree::new(json!({ + "project": { + "sub": {}, + "hidden": {"unopened.rs": "unindexed"}, + }, + "outside": { + "child": {}, + "target.rs": "saved", + "link.rs": "actual outside target", + }, + })); + let root = directory.path().join("project"); + let target = directory.path().join("outside/target.rs"); + os::unix::fs::symlink(&target, root.join("link.rs")).unwrap(); + os::unix::fs::symlink(root.join("sub"), root.join("hop")).unwrap(); + os::unix::fs::symlink(directory.path().join("outside/child"), root.join("escape")).unwrap(); + let outside_alias = directory.path().join("outside/alias.rs"); + os::unix::fs::symlink(&target, &outside_alias).unwrap(); + + let project = Project::test(RealFs::new(None, cx.executor()), [root.as_path()], cx).await; + let alias = project.read_with(cx, |project, cx| { + project + .project_path_for_absolute_path(&root.join("link.rs"), cx) + .unwrap() + }); + let buffer = project + .update(cx, |project, cx| project.open_buffer(alias.clone(), cx)) + .await + .unwrap(); + buffer.update(cx, |buffer, cx| { + buffer.edit([(0..0, "unsaved ")], None, cx); + }); + + for relative_path in ["hop/../link.rs", "hidden/../link.rs"] { + let path = root.join(relative_path); + let resolved = project + .update(cx, |project, cx| { + project.resolve_abs_file_link(path.to_str().unwrap(), cx) + }) + .await + .unwrap() + .unwrap(); + assert_eq!(resolved.project_path(), Some(&alias), "{relative_path}"); + let reopened = project + .update(cx, |project, cx| { + project.open_buffer(resolved.project_path().unwrap().clone(), cx) + }) + .await + .unwrap(); + assert_eq!(reopened, buffer); + buffer.read_with(cx, |buffer, _| { + assert_eq!(buffer.text(), "unsaved saved"); + assert!(buffer.is_dirty()); + }); + } + + let unindexed_path = root.join("hidden/unopened.rs"); + let unindexed_alias = project.read_with(cx, |project, cx| { + let path = project + .project_path_for_absolute_path(&unindexed_path, cx) + .unwrap(); + assert!(project.entry_for_path(&path, cx).is_none()); + assert!(project.get_open_buffer(&path, cx).is_none()); + assert!( + project + .entry_for_path(&(alias.worktree_id, rel_path("hidden")).into(), cx) + .is_none() + ); + path + }); + let resolved = project + .update(cx, |project, cx| { + project.resolve_abs_file_link(unindexed_path.to_str().unwrap(), cx) + }) + .await + .unwrap() + .unwrap(); + assert_eq!(resolved.project_path(), Some(&unindexed_alias)); + + for (input, expected) in [ + ( + root.join("escape/../link.rs"), + directory.path().join("outside/link.rs"), + ), + (outside_alias, target), + ] { + let resolved = project + .update(cx, |project, cx| { + project.resolve_abs_file_link(input.to_str().unwrap(), cx) + }) + .await + .unwrap() + .unwrap(); + assert_eq!(resolved.abs_path(), expected.to_str()); + } + let missing = root.join("missing/../link.rs"); + let resolved = project + .update(cx, |project, cx| { + project.resolve_abs_file_link(missing.to_str().unwrap(), cx) + }) + .await + .unwrap(); + assert!(resolved.is_none()); + project.read_with(cx, |project, cx| { + assert_eq!(project.worktrees(cx).count(), 1); + assert_eq!(project.get_open_buffer(&alias, cx), Some(buffer)); + }); +} + +#[cfg(target_os = "linux")] +#[gpui::test] +async fn test_resolve_abs_file_link_rejects_non_utf8_target(cx: &mut gpui::TestAppContext) { + init_test(cx); + cx.executor().allow_parking(); + let directory = TempTree::new(json!({})); + let target = directory.path().join(OsStr::from_bytes(b"\xff.rs")); + std::fs::write(&target, "actual target").unwrap(); + std::fs::write(directory.path().join("�.rs"), "replacement-character decoy").unwrap(); + let alias = directory.path().join("alias.rs"); + os::unix::fs::symlink(&target, &alias).unwrap(); + let project = Project::test(RealFs::new(None, cx.executor()), [], cx).await; + let error = project + .update(cx, |project, cx| { + project.resolve_abs_file_link(alias.to_str().unwrap(), cx) + }) + .await + .unwrap_err(); + assert_eq!(error.to_string(), "canonical file path is not valid UTF-8"); + project.read_with(cx, |project, cx| { + assert_eq!(project.worktrees(cx).count(), 0); + }); +} + #[gpui::test] async fn test_editorconfig_support(cx: &mut gpui::TestAppContext) { init_test(cx); diff --git a/crates/proto/proto/worktree.proto b/crates/proto/proto/worktree.proto index 74d126d9b5a..b84d861c9a9 100644 --- a/crates/proto/proto/worktree.proto +++ b/crates/proto/proto/worktree.proto @@ -52,6 +52,7 @@ message RemoveWorktree { message GetPathMetadata { uint64 project_id = 1; string path = 2; + bool canonicalize = 3; } message GetPathMetadataResponse { diff --git a/crates/remote_server/src/headless_project.rs b/crates/remote_server/src/headless_project.rs index d7927ae6af9..586d40e7211 100644 --- a/crates/remote_server/src/headless_project.rs +++ b/crates/remote_server/src/headless_project.rs @@ -1265,11 +1265,20 @@ impl HeadlessProject { let metadata = fs.metadata(&expanded).await?; let is_dir = metadata.map(|metadata| metadata.is_dir).unwrap_or(false); + let path = if envelope.payload.canonicalize && metadata.is_some() { + fs.canonicalize(&expanded) + .await? + .to_str() + .context("canonical file path is not valid UTF-8")? + .to_owned() + } else { + expanded.to_string_lossy().into_owned() + }; Ok(proto::GetPathMetadataResponse { exists: metadata.is_some(), is_dir, - path: expanded.to_string_lossy().into_owned(), + path, }) } diff --git a/crates/remote_server/src/remote_editing_tests.rs b/crates/remote_server/src/remote_editing_tests.rs index 403c1cdb329..3ef8e78748e 100644 --- a/crates/remote_server/src/remote_editing_tests.rs +++ b/crates/remote_server/src/remote_editing_tests.rs @@ -2871,9 +2871,17 @@ async fn test_remote_resolve_abs_path(cx: &mut TestAppContext, server_cx: &mut T "lib.rs": "fn one() -> usize { 1 }" } }, + "project2": { + "README.md": "# project 2", + }, }), ) .await; + fs.insert_symlink( + path!("/code/project1/linked"), + PathBuf::from(path!("/code/project2")), + ) + .await; let (project, _headless) = init_test(&fs, cx, server_cx).await; @@ -2903,6 +2911,152 @@ async fn test_remote_resolve_abs_path(cx: &mut TestAppContext, server_cx: &mut T }) .await; assert!(path.is_none()); + + for path in [ + path!("/code/project1/../project2/README.md"), + path!("/code/project1/linked/README.md"), + ] { + let resolved_path = project + .update(cx, |project, cx| project.resolve_abs_path(path, cx)) + .await + .expect("existing path should resolve without canonicalization"); + assert_eq!(resolved_path.abs_path(), Some(path)); + let resolved_file_path = project + .update(cx, |project, cx| project.resolve_abs_file_path(path, cx)) + .await + .expect("existing file path should resolve without canonicalization"); + assert_eq!(resolved_file_path.abs_path(), Some(path)); + } +} + +#[gpui::test] +async fn test_resolve_abs_file_path_canonical( + cx: &mut TestAppContext, + server_cx: &mut TestAppContext, +) { + let fs = FakeFs::new(server_cx.executor()); + fs.insert_tree( + path!("/code"), + json!({ + "project1": {}, + "project2": {"README.md": "# project 2"}, + }), + ) + .await; + fs.insert_symlink( + path!("/code/project1/linked"), + PathBuf::from(path!("/code/project2")), + ) + .await; + + let (remote_project, _headless) = init_test(&fs, cx, server_cx).await; + let local_project = Project::test(fs, [], cx).await; + for project in [remote_project, local_project] { + let path = path!("/code/project1/linked/README.md"); + let resolved_path = project + .update(cx, |project, cx| project.resolve_abs_path(path, cx)) + .await + .expect("existing path should resolve without canonicalization"); + assert_eq!(resolved_path.abs_path(), Some(path)); + + for path in [ + path!("/code/project1/linked"), + path!("/code/project1/linked/missing.md"), + ] { + let resolved_path = project + .update(cx, |project, cx| { + project.resolve_abs_file_path_canonical(path, cx) + }) + .await + .expect("missing files and directories should not cause resolution errors"); + assert!(resolved_path.is_none(), "{path}"); + } + + let (worktree, _) = project + .update(cx, |project, cx| { + project.find_or_create_worktree(path!("/code/project2/README.md"), false, cx) + }) + .await + .expect("canonical file worktree should open"); + worktree.read_with(cx, |worktree, _| { + assert!(!worktree.is_visible()); + assert!(worktree.is_single_file()); + }); + + for path in [ + path!("/code/project1/../project2/README.md"), + path!("/code/project1/linked/README.md"), + ] { + let resolved_path = project + .update(cx, |project, cx| { + project.resolve_abs_file_path_canonical(path, cx) + }) + .await + .expect("existing file path should canonicalize without errors") + .expect("existing path should resolve"); + assert!(resolved_path.is_file()); + assert_eq!( + resolved_path.abs_path(), + Some(path!("/code/project2/README.md")), + "{path}" + ); + let (opened_worktree, relative_path) = project + .update(cx, |project, cx| { + project.find_or_create_worktree( + resolved_path.abs_path().expect("path should be absolute"), + false, + cx, + ) + }) + .await + .expect("resolved file worktree should open"); + assert!(relative_path.is_empty()); + opened_worktree.read_with(cx, |opened_worktree, cx| { + assert_eq!(opened_worktree.id(), worktree.read(cx).id(), "{path}"); + assert_eq!(project.read(cx).worktrees(cx).count(), 1); + }); + } + + let (project_worktree, _) = project + .update(cx, |project, cx| { + project.find_or_create_worktree(path!("/code/project1"), true, cx) + }) + .await + .unwrap(); + let alias = project_worktree.read_with(cx, |worktree, _| { + ProjectPath::from((worktree.id(), rel_path("linked/README.md"))) + }); + let buffer = project + .update(cx, |project, cx| project.open_buffer(alias.clone(), cx)) + .await + .unwrap(); + buffer.update(cx, |buffer, cx| { + buffer.edit([(0..0, "unsaved ")], None, cx); + }); + + let resolved = project + .update(cx, |project, cx| { + project.resolve_abs_file_link(path!("/code/project1//linked/README.md"), cx) + }) + .await + .unwrap() + .unwrap(); + assert_eq!(resolved.project_path(), Some(&alias)); + let reopened = project + .update(cx, |project, cx| { + project.open_buffer(resolved.project_path().unwrap().clone(), cx) + }) + .await + .unwrap(); + assert_eq!(reopened, buffer); + buffer.read_with(cx, |buffer, _| { + assert_eq!(buffer.text(), "unsaved # project 2"); + assert!(buffer.is_dirty()); + }); + project.read_with(cx, |project, cx| { + assert_eq!(project.worktrees(cx).count(), 2); + }); + } } #[gpui::test(iterations = 10)] diff --git a/crates/util/src/paths.rs b/crates/util/src/paths.rs index 4e78888e33b..4e3511dc26f 100644 --- a/crates/util/src/paths.rs +++ b/crates/util/src/paths.rs @@ -632,17 +632,17 @@ impl PathWithPosition { LazyLock::new(|| Regex::new(ROW_COL_CAPTURE_REGEX).unwrap()); match SUFFIX_RE .captures(maybe_file_name_with_row_col) - .map(|caps| caps.extract()) - { - Some((_, [file_name, maybe_row, maybe_column])) => { + .and_then(|captures| { + let file_name_end = captures.iter().skip(1).flatten().next()?.end(); + let (_, [_, row, column]) = captures.extract(); + Some((file_name_end, row, column)) + }) { + Some((file_name_end, maybe_row, maybe_column)) => { let row = maybe_row.parse::().ok(); let column = maybe_column.parse::().ok(); - let (_, suffix) = trimmed.split_once(file_name).unwrap(); - let path_without_suffix = &trimmed[..trimmed.len() - suffix.len()]; - Self { - path: Path::new(path_without_suffix).to_path_buf(), + path: path.with_file_name(&maybe_file_name_with_row_col[..file_name_end]), row, column, } @@ -651,8 +651,12 @@ impl PathWithPosition { // The `ROW_COL_CAPTURE_REGEX` deals with separated digits only, // but in reality there could be `foo/bar.py:22:in` inputs which we want to match too. // The regex mentioned is not very extendable with "digit or random string" checks, so do this here instead. + let path = Path::new(s); + let Some(file_name) = path.file_name().and_then(|name| name.to_str()) else { + return Self::from_path(path.to_path_buf()); + }; let delimiter = ':'; - let mut path_parts = s + let mut path_parts = file_name .rsplitn(3, delimiter) .collect::>() .into_iter() @@ -681,7 +685,11 @@ impl PathWithPosition { } Self { - path: PathBuf::from(path_string), + path: if row.is_some() { + path.with_file_name(path_string) + } else { + path.to_path_buf() + }, row, column, } @@ -2336,6 +2344,74 @@ mod tests { ); } + #[test] + fn path_with_position_preserves_paths_without_positions() { + for input in [ + "", + "/", + "thf/", + "/test/cool/", + "/test/Ῥόδος/", + "parent//file.rs", + "parent/./file.rs", + "parent/file.rs/.", + "parent:12:3/file.rs/", + "file.rs:invalid/", + " parent/file.rs ", + r"C:\test\cool\", + r"C:\test\Ῥόδος\", + r"\\server\share\cool\", + ] { + let parsed = PathWithPosition::parse_str(input); + assert_eq!(parsed.path.as_os_str(), Path::new(input).as_os_str()); + assert_eq!(parsed.row, None, "{input}"); + assert_eq!(parsed.column, None, "{input}"); + } + } + + #[test] + fn path_with_position_preserves_repeated_filename() { + for (input, expected_path) in [ + ("file.rs/file.rs:12:3", "file.rs/file.rs"), + ("file.rs-backups/file.rs(12,3)", "file.rs-backups/file.rs"), + ("😀.rs/😀.rs:12:3", "😀.rs/😀.rs"), + ("parent/a(b).rs(12,3)", "parent/a(b).rs"), + ("parent/😀(b).rs(12,3)", "parent/😀(b).rs"), + ("parent/a(b).rs:(12,3)", "parent/a(b).rs"), + (r"src\file.rs\file.rs:12:3", r"src\file.rs\file.rs"), + ] { + assert_eq!( + PathWithPosition::parse_str(input), + PathWithPosition { + path: PathBuf::from(expected_path), + row: Some(12), + column: Some(3), + }, + "{input}", + ); + } + } + + #[test] + fn path_with_position_preserves_parent_position_like_components() { + for (input, expected_path, row) in [ + ("parent:12:3/file.rs", "parent:12:3/file.rs", None), + ("parent:12:3/file.rs:4:in", "parent:12:3/file.rs", Some(4)), + (" parent/file.rs ", " parent/file.rs ", None), + (" parent/file.rs:4:in ", " parent/file.rs", Some(4)), + ] { + assert_eq!( + PathWithPosition::parse_str(input), + PathWithPosition { + path: PathBuf::from(expected_path), + row, + column: None, + }, + "{input}", + ); + } + } + #[perf] fn path_with_position_parse_posix_path() { // Test POSIX filename edge cases