Fix directory context paths (#28459)

Release Notes:

- N/A

crates/agent/src/active_thread.rs

@@ -2840,10 +2840,10 @@ pub(crate) fn open_context(
             }
         }
         AssistantContext::Directory(directory_context) => {
-            let path = directory_context.project_path.clone();
+            let project_path = directory_context.project_path(cx);
             workspace.update(cx, |workspace, cx| {
                 workspace.project().update(cx, |project, cx| {
-                    if let Some(entry) = project.entry_for_path(&path, cx) {
+                    if let Some(entry) = project.entry_for_path(&project_path, cx) {
                         cx.emit(project::Event::RevealInProjectPanel(entry.id));
                     }
                 })

crates/agent/src/context.rs

@@ -1,9 +1,9 @@
-use std::{ops::Range, sync::Arc};
+use std::{ops::Range, path::Path, sync::Arc};
 
 use gpui::{App, Entity, SharedString};
 use language::{Buffer, File};
 use language_model::LanguageModelRequestMessage;
-use project::ProjectPath;
+use project::{ProjectPath, Worktree};
 use serde::{Deserialize, Serialize};
 use text::{Anchor, BufferId};
 use ui::IconName;
@@ -69,10 +69,21 @@ pub struct FileContext {
 #[derive(Debug, Clone)]
 pub struct DirectoryContext {
     pub id: ContextId,
-    pub project_path: ProjectPath,
+    pub worktree: Entity<Worktree>,
+    pub path: Arc<Path>,
+    /// Buffers of the files within the directory.
     pub context_buffers: Vec<ContextBuffer>,
 }
 
+impl DirectoryContext {
+    pub fn project_path(&self, cx: &App) -> ProjectPath {
+        ProjectPath {
+            worktree_id: self.worktree.read(cx).id(),
+            path: self.path.clone(),
+        }
+    }
+}
+
 #[derive(Debug, Clone)]
 pub struct SymbolContext {
     pub id: ContextId,
@@ -86,12 +97,11 @@ pub struct FetchedUrlContext {
     pub text: SharedString,
 }
 
-// TODO: Model<Thread> holds onto the thread even if the thread is deleted. Can either handle this
-// explicitly or have a WeakModel<Thread> and remove during snapshot.
-
 #[derive(Debug, Clone)]
 pub struct ThreadContext {
     pub id: ContextId,
+    // TODO: Entity<Thread> holds onto the thread even if the thread is deleted. Should probably be
+    // a WeakEntity and handle removal from the UI when it has dropped.
     pub thread: Entity<Thread>,
     pub text: SharedString,
 }
@@ -105,12 +115,11 @@ impl ThreadContext {
     }
 }
 
-// TODO: Model<Buffer> holds onto the buffer even if the file is deleted and closed. Should remove
-// the context from the message editor in this case.
-
 #[derive(Clone)]
 pub struct ContextBuffer {
     pub id: BufferId,
+    // TODO: Entity<Buffer> holds onto the thread even if the thread is deleted. Should probably be
+    // a WeakEntity and handle removal from the UI when it has dropped.
     pub buffer: Entity<Buffer>,
     pub file: Arc<dyn File>,
     pub version: clock::Global,

crates/agent/src/context_picker.rs

@@ -289,12 +289,14 @@ impl ContextPicker {
                 path_prefix,
             } => {
                 let context_store = self.context_store.clone();
+                let worktree_id = project_path.worktree_id;
                 let path = project_path.path.clone();
 
                 ContextMenuItem::custom_entry(
                     move |_window, cx| {
                         render_file_context_entry(
                             ElementId::NamedInteger("ctx-recent".into(), ix),
+                            worktree_id,
                             &path,
                             &path_prefix,
                             false,
@@ -466,7 +468,7 @@ fn recent_context_picker_entries(
     recent.extend(
         workspace
             .recent_navigation_history_iter(cx)
-            .filter(|(path, _)| !current_files.contains(&path.path.to_path_buf()))
+            .filter(|(path, _)| !current_files.contains(path))
             .take(4)
             .filter_map(|(project_path, _)| {
                 project

crates/agent/src/context_picker/file_context_picker.rs

@@ -189,6 +189,7 @@ impl PickerDelegate for FileContextPickerDelegate {
                 .toggle_state(selected)
                 .child(render_file_context_entry(
                     ElementId::NamedInteger("file-ctx-picker".into(), ix),
+                    WorktreeId::from_usize(mat.worktree_id),
                     &mat.path,
                     &mat.path_prefix,
                     mat.is_dir,
@@ -328,19 +329,26 @@ pub fn extract_file_name_and_directory(
 
 pub fn render_file_context_entry(
     id: ElementId,
-    path: &Path,
+    worktree_id: WorktreeId,
+    path: &Arc<Path>,
     path_prefix: &Arc<str>,
     is_directory: bool,
     context_store: WeakEntity<ContextStore>,
     cx: &App,
 ) -> Stateful<Div> {
-    let (file_name, directory) = extract_file_name_and_directory(path, path_prefix);
+    let (file_name, directory) = extract_file_name_and_directory(&path, path_prefix);
 
     let added = context_store.upgrade().and_then(|context_store| {
+        let project_path = ProjectPath {
+            worktree_id,
+            path: path.clone(),
+        };
         if is_directory {
-            context_store.read(cx).includes_directory(path)
+            context_store.read(cx).includes_directory(&project_path)
         } else {
-            context_store.read(cx).will_include_file_path(path, cx)
+            context_store
+                .read(cx)
+                .will_include_file_path(&project_path, cx)
         }
     });
 
@@ -380,8 +388,9 @@ pub fn render_file_context_entry(
                     )
                     .child(Label::new("Added").size(LabelSize::Small)),
             ),
-            FileInclusion::InDirectory(dir_name) => {
+            FileInclusion::InDirectory(directory_project_path) => {
-                let dir_name = dir_name.to_string_lossy().into_owned();
+                // TODO: Consider using worktree full_path to include worktree name.
+                let directory_path = directory_project_path.path.to_string_lossy().into_owned();
 
                 el.child(
                     h_flex()
@@ -395,7 +404,7 @@ pub fn render_file_context_entry(
                         )
                         .child(Label::new("Included").size(LabelSize::Small)),
                 )
-                .tooltip(Tooltip::text(format!("in {dir_name}")))
+                .tooltip(Tooltip::text(format!("in {directory_path}")))
             }
         })
 }

crates/agent/src/context_store.rs

@@ -1,5 +1,5 @@
 use std::ops::Range;
-use std::path::{Path, PathBuf};
+use std::path::Path;
 use std::sync::Arc;
 
 use anyhow::{Context as _, Result, anyhow};
@@ -28,7 +28,7 @@ pub struct ContextStore {
     // TODO: If an EntityId is used for all context types (like BufferId), can remove ContextId.
     next_context_id: ContextId,
     files: BTreeMap<BufferId, ContextId>,
-    directories: HashMap<PathBuf, ContextId>,
+    directories: HashMap<ProjectPath, ContextId>,
     symbols: HashMap<ContextSymbolId, ContextId>,
     symbol_buffers: HashMap<ContextSymbolId, Entity<Buffer>>,
     symbols_by_path: HashMap<ProjectPath, Vec<ContextSymbolId>>,
@@ -93,7 +93,7 @@ impl ContextStore {
             let buffer_id = this.update(cx, |_, cx| buffer.read(cx).remote_id())?;
 
             let already_included = this.update(cx, |this, cx| {
-                match this.will_include_buffer(buffer_id, &project_path.path) {
+                match this.will_include_buffer(buffer_id, &project_path) {
                     Some(FileInclusion::Direct(context_id)) => {
                         if remove_if_exists {
                             this.remove_context(context_id, cx);
@@ -159,7 +159,7 @@ impl ContextStore {
             return Task::ready(Err(anyhow!("failed to read project")));
         };
 
-        let already_included = match self.includes_directory(&project_path.path) {
+        let already_included = match self.includes_directory(&project_path) {
             Some(FileInclusion::Direct(context_id)) => {
                 if remove_if_exists {
                     self.remove_context(context_id, cx);
@@ -223,14 +223,12 @@ impl ContextStore {
                 .collect::<Vec<_>>();
 
             if context_buffers.is_empty() {
-                return Err(anyhow!(
+                let full_path = cx.update(|cx| worktree.read(cx).full_path(&project_path.path))?;
-                    "No text files found in {}",
+                return Err(anyhow!("No text files found in {}", &full_path.display()));
-                    &project_path.path.display()
-                ));
             }
 
             this.update(cx, |this, cx| {
-                this.insert_directory(project_path, context_buffers, cx);
+                this.insert_directory(worktree, project_path, context_buffers, cx);
             })?;
 
             anyhow::Ok(())
@@ -239,17 +237,20 @@ impl ContextStore {
 
     fn insert_directory(
         &mut self,
+        worktree: Entity<Worktree>,
         project_path: ProjectPath,
         context_buffers: Vec<ContextBuffer>,
         cx: &mut Context<Self>,
     ) {
         let id = self.next_context_id.post_inc();
-        self.directories.insert(project_path.path.to_path_buf(), id);
+        let path = project_path.path.clone();
+        self.directories.insert(project_path, id);
 
         self.context
             .push(AssistantContext::Directory(DirectoryContext {
                 id,
-                project_path,
+                worktree,
+                path,
                 context_buffers,
             }));
         cx.notify();
@@ -478,23 +479,31 @@ impl ContextStore {
     /// Returns whether the buffer is already included directly in the context, or if it will be
     /// included in the context via a directory. Directory inclusion is based on paths rather than
     /// buffer IDs as the directory will be re-scanned.
-    pub fn will_include_buffer(&self, buffer_id: BufferId, path: &Path) -> Option<FileInclusion> {
+    pub fn will_include_buffer(
+        &self,
+        buffer_id: BufferId,
+        project_path: &ProjectPath,
+    ) -> Option<FileInclusion> {
         if let Some(context_id) = self.files.get(&buffer_id) {
             return Some(FileInclusion::Direct(*context_id));
         }
 
-        self.will_include_file_path_via_directory(path)
+        self.will_include_file_path_via_directory(project_path)
     }
 
     /// Returns whether this file path is already included directly in the context, or if it will be
     /// included in the context via a directory.
-    pub fn will_include_file_path(&self, path: &Path, cx: &App) -> Option<FileInclusion> {
+    pub fn will_include_file_path(
+        &self,
+        project_path: &ProjectPath,
+        cx: &App,
+    ) -> Option<FileInclusion> {
         if !self.files.is_empty() {
             let found_file_context = self.context.iter().find(|context| match &context {
                 AssistantContext::File(file_context) => {
                     let buffer = file_context.context_buffer.buffer.read(cx);
-                    if let Some(file_path) = buffer_path_log_err(buffer, cx) {
+                    if let Some(context_path) = buffer.project_path(cx) {
-                        *file_path == *path
+                        &context_path == project_path
                     } else {
                         false
                     }
@@ -506,31 +515,40 @@ impl ContextStore {
             }
         }
 
-        self.will_include_file_path_via_directory(path)
+        self.will_include_file_path_via_directory(project_path)
     }
 
-    fn will_include_file_path_via_directory(&self, path: &Path) -> Option<FileInclusion> {
+    fn will_include_file_path_via_directory(
+        &self,
+        project_path: &ProjectPath,
+    ) -> Option<FileInclusion> {
         if self.directories.is_empty() {
             return None;
         }
 
-        let mut buf = path.to_path_buf();
+        let mut path_buf = project_path.path.to_path_buf();
 
-        while buf.pop() {
+        while path_buf.pop() {
-            if let Some(_) = self.directories.get(&buf) {
+            // TODO: This isn't very efficient. Consider using a better representation of the
-                return Some(FileInclusion::InDirectory(buf));
+            // directories map.
+            let directory_project_path = ProjectPath {
+                worktree_id: project_path.worktree_id,
+                path: path_buf.clone().into(),
+            };
+            if let Some(_) = self.directories.get(&directory_project_path) {
+                return Some(FileInclusion::InDirectory(directory_project_path));
             }
         }
 
         None
     }
 
-    pub fn includes_directory(&self, path: &Path) -> Option<FileInclusion> {
+    pub fn includes_directory(&self, project_path: &ProjectPath) -> Option<FileInclusion> {
-        if let Some(context_id) = self.directories.get(path) {
+        if let Some(context_id) = self.directories.get(project_path) {
             return Some(FileInclusion::Direct(*context_id));
         }
 
-        self.will_include_file_path_via_directory(path)
+        self.will_include_file_path_via_directory(project_path)
     }
 
     pub fn included_symbol(&self, symbol_id: &ContextSymbolId) -> Option<ContextId> {
@@ -564,13 +582,13 @@ impl ContextStore {
         }
     }
 
-    pub fn file_paths(&self, cx: &App) -> HashSet<PathBuf> {
+    pub fn file_paths(&self, cx: &App) -> HashSet<ProjectPath> {
         self.context
             .iter()
             .filter_map(|context| match context {
                 AssistantContext::File(file) => {
                     let buffer = file.context_buffer.buffer.read(cx);
-                    buffer_path_log_err(buffer, cx).map(|p| p.to_path_buf())
+                    buffer.project_path(cx)
                 }
                 AssistantContext::Directory(_)
                 | AssistantContext::Symbol(_)
@@ -587,7 +605,7 @@ impl ContextStore {
 
 pub enum FileInclusion {
     Direct(ContextId),
-    InDirectory(PathBuf),
+    InDirectory(ProjectPath),
 }
 
 // ContextBuffer without text.
@@ -654,19 +672,6 @@ fn collect_buffer_info_and_text(
     Ok((buffer_info, text_task))
 }
 
-pub fn buffer_path_log_err(buffer: &Buffer, cx: &App) -> Option<Arc<Path>> {
-    if let Some(file) = buffer.file() {
-        let mut path = file.path().clone();
-        if path.as_os_str().is_empty() {
-            path = file.full_path(cx).into();
-        }
-        Some(path)
-    } else {
-        log::error!("Buffer that had a path unexpectedly no longer has a path.");
-        None
-    }
-}
-
 fn to_fenced_codeblock(path: &Path, content: Rope) -> SharedString {
     let path_extension = path.extension().and_then(|ext| ext.to_str());
     let path_string = path.to_string_lossy();
@@ -742,13 +747,13 @@ pub fn refresh_context_store_text(
                     }
                 }
                 AssistantContext::Directory(directory_context) => {
+                    let directory_path = directory_context.project_path(cx);
                     let should_refresh = changed_buffers.is_empty()
                         || changed_buffers.iter().any(|buffer| {
-                            let buffer = buffer.read(cx);
+                            let Some(buffer_path) = buffer.read(cx).project_path(cx) else {
-
+                                return false;
-                            buffer_path_log_err(&buffer, cx).map_or(false, |path| {
+                            };
-                                path.starts_with(&directory_context.project_path.path)
+                            buffer_path.starts_with(&directory_path)
-                            })
                         });
 
                     if should_refresh {
@@ -835,14 +840,16 @@ fn refresh_directory_text(
     let context_buffers = future::join_all(futures);
 
     let id = directory_context.id;
-    let project_path = directory_context.project_path.clone();
+    let worktree = directory_context.worktree.clone();
+    let path = directory_context.path.clone();
     Some(cx.spawn(async move |cx| {
         let context_buffers = context_buffers.await;
         context_store
             .update(cx, |context_store, _| {
                 let new_directory_context = DirectoryContext {
                     id,
-                    project_path,
+                    worktree,
+                    path,
                     context_buffers,
                 };
                 context_store.replace_context(AssistantContext::Directory(new_directory_context));

crates/agent/src/context_strip.rs

@@ -1,3 +1,4 @@
+use std::path::Path;
 use std::rc::Rc;
 
 use collections::HashSet;
@@ -9,6 +10,7 @@ use gpui::{
 };
 use itertools::Itertools;
 use language::Buffer;
+use project::ProjectItem;
 use ui::{KeyBinding, PopoverMenu, PopoverMenuHandle, Tooltip, prelude::*};
 use workspace::{Workspace, notifications::NotifyResultExt};
 
@@ -93,26 +95,23 @@ impl ContextStrip {
         let active_buffer_entity = editor.buffer().read(cx).as_singleton()?;
         let active_buffer = active_buffer_entity.read(cx);
 
-        let path = active_buffer.file()?.full_path(cx);
+        let project_path = active_buffer.project_path(cx)?;
 
         if self
             .context_store
             .read(cx)
-            .will_include_buffer(active_buffer.remote_id(), &path)
+            .will_include_buffer(active_buffer.remote_id(), &project_path)
             .is_some()
         {
             return None;
         }
 
-        let name = match path.file_name() {
+        let file_name = active_buffer.file()?.file_name(cx);
-            Some(name) => name.to_string_lossy().into_owned().into(),
-            None => path.to_string_lossy().into_owned().into(),
-        };
 
-        let icon_path = FileIcons::get_icon(&path, cx);
+        let icon_path = FileIcons::get_icon(&Path::new(&file_name), cx);
 
         Some(SuggestedContext::File {
-            name,
+            name: file_name.to_string_lossy().into_owned().into(),
             buffer: active_buffer_entity.downgrade(),
             icon_path,
         })

crates/agent/src/ui/context_pill.rs

@@ -280,9 +280,10 @@ impl AddedContext {
             }
 
             AssistantContext::Directory(directory_context) => {
-                // TODO: handle worktree disambiguation. Maybe by storing an `Arc<dyn File>` to also
+                let full_path = directory_context
-                // handle renames?
+                    .worktree
-                let full_path = &directory_context.project_path.path;
+                    .read(cx)
+                    .full_path(&directory_context.path);
                 let full_path_string: SharedString =
                     full_path.to_string_lossy().into_owned().into();
                 let name = full_path

crates/project/src/project.rs

@@ -334,6 +334,10 @@ impl ProjectPath {
             path: Path::new("").into(),
         }
     }
+
+    pub fn starts_with(&self, other: &ProjectPath) -> bool {
+        self.worktree_id == other.worktree_id && self.path.starts_with(&other.path)
+    }
 }
 
 #[derive(Debug, Default)]

crates/worktree/src/worktree.rs

@@ -1172,6 +1172,31 @@ impl Worktree {
     pub fn is_single_file(&self) -> bool {
         self.root_dir().is_none()
     }
+
+    /// For visible worktrees, returns the path with the worktree name as the first component.
+    /// Otherwise, returns an absolute path.
+    pub fn full_path(&self, worktree_relative_path: &Path) -> PathBuf {
+        let mut full_path = PathBuf::new();
+
+        if self.is_visible() {
+            full_path.push(self.root_name());
+        } else {
+            let path = self.abs_path();
+
+            if self.is_local() && path.starts_with(home_dir().as_path()) {
+                full_path.push("~");
+                full_path.push(path.strip_prefix(home_dir().as_path()).unwrap());
+            } else {
+                full_path.push(path)
+            }
+        }
+
+        if worktree_relative_path.components().next().is_some() {
+            full_path.push(&worktree_relative_path);
+        }
+
+        full_path
+    }
 }
 
 impl LocalWorktree {
@@ -3229,27 +3254,7 @@ impl language::File for File {
     }
 
     fn full_path(&self, cx: &App) -> PathBuf {
-        let mut full_path = PathBuf::new();
+        self.worktree.read(cx).full_path(&self.path)
-        let worktree = self.worktree.read(cx);
-
-        if worktree.is_visible() {
-            full_path.push(worktree.root_name());
-        } else {
-            let path = worktree.abs_path();
-
-            if worktree.is_local() && path.starts_with(home_dir().as_path()) {
-                full_path.push("~");
-                full_path.push(path.strip_prefix(home_dir().as_path()).unwrap());
-            } else {
-                full_path.push(path)
-            }
-        }
-
-        if self.path.components().next().is_some() {
-            full_path.push(&self.path);
-        }
-
-        full_path
     }
 
     /// Returns the last component of this handle's absolute path. If this handle refers to the root