From 34cad6da337f9893ae29bcad6b95386ff4752ee9 Mon Sep 17 00:00:00 2001 From: Richard Feldman Date: Fri, 10 Apr 2026 11:00:25 -0400 Subject: [PATCH] agent_ui: Fix worktree naming regression when using existing branches When creating a new git worktree from an existing branch (e.g. 'main'), the worktree directory was incorrectly named after the branch instead of getting a random adjective-noun name. This was introduced in ccb9e60a62 which separated branch name from worktree directory name but used the branch name as the fallback when no explicit worktree name was provided. Additionally, if a directory with that name already existed, the rollback logic could destructively delete the pre-existing worktree. Changes: - Generate a random adjective-noun name for worktree directories when no explicit name is provided, instead of falling back to branch name - Collect existing worktree directory names and pass them to the name generator to avoid collisions - Check generated paths against known worktree paths before creating, to prevent attempting creation at an already-occupied path - Reduce random name retry count from 100 to 10 (sufficient given ~59,000 possible combinations) - Add regression test that verifies worktree directories get random names when using an existing branch --- crates/agent_ui/src/agent_panel.rs | 174 +++++++++++++++++++++++++++- crates/agent_ui/src/branch_names.rs | 2 +- 2 files changed, 173 insertions(+), 3 deletions(-) diff --git a/crates/agent_ui/src/agent_panel.rs b/crates/agent_ui/src/agent_panel.rs index 2ff4cd18a78fd53c5d540e66670d6e6c9e51aa47..0becd8efb4ecc1e0edb98255608f6ad463641979 100644 --- a/crates/agent_ui/src/agent_panel.rs +++ b/crates/agent_ui/src/agent_panel.rs @@ -2740,6 +2740,8 @@ impl AgentPanel { fn start_worktree_creations( git_repos: &[Entity], worktree_name: Option, + existing_worktree_names: &[String], + existing_worktree_paths: &HashSet, branch_name: &str, use_existing_branch: bool, start_point: Option, @@ -2756,12 +2758,21 @@ impl AgentPanel { let mut creation_infos = Vec::new(); let mut path_remapping = Vec::new(); - let worktree_name = worktree_name.unwrap_or_else(|| branch_name.to_string()); + let worktree_name = worktree_name.unwrap_or_else(|| { + let existing_refs: Vec<&str> = + existing_worktree_names.iter().map(|s| s.as_str()).collect(); + let mut rng = rand::rng(); + crate::branch_names::generate_branch_name(&existing_refs, &mut rng) + .unwrap_or_else(|| branch_name.to_string()) + }); for repo in git_repos { let (work_dir, new_path, receiver) = repo.update(cx, |repo, _cx| { let new_path = repo.path_for_new_linked_worktree(&worktree_name, worktree_directory_setting)?; + if existing_worktree_paths.contains(&new_path) { + anyhow::bail!("A worktree already exists at {}", new_path.display()); + } let target = if use_existing_branch { debug_assert!( git_repos.len() == 1, @@ -2827,7 +2838,8 @@ impl AgentPanel { return Ok(created_paths); }; - // Rollback all attempted worktrees (both successful and failed) + // Rollback all attempted worktrees (both successful and failed, + // since a failed creation may have left an orphan directory). let mut rollback_futures = Vec::new(); for (rollback_repo, rollback_path) in &repos_and_paths { let receiver = cx @@ -3037,6 +3049,8 @@ impl AgentPanel { } let mut occupied_branches = HashSet::default(); + let mut existing_worktree_names = Vec::new(); + let mut existing_worktree_paths = HashSet::default(); for result in futures::future::join_all(worktree_receivers).await { match result { Ok(Ok(worktrees)) => { @@ -3044,6 +3058,15 @@ impl AgentPanel { if let Some(branch_name) = worktree.branch_name() { occupied_branches.insert(branch_name.to_string()); } + if let Some(name) = worktree + .path + .parent() + .and_then(|p| p.file_name()) + .and_then(|n| n.to_str()) + { + existing_worktree_names.push(name.to_string()); + } + existing_worktree_paths.insert(worktree.path.clone()); } } Ok(Err(err)) => { @@ -3077,6 +3100,8 @@ impl AgentPanel { Self::start_worktree_creations( &git_repos, worktree_name, + &existing_worktree_names, + &existing_worktree_paths, &branch_name, use_existing_branch, start_point, @@ -6165,6 +6190,151 @@ mod tests { assert!(!existing_branches.contains(&resolved.0)); } + #[gpui::test] + async fn test_worktree_dir_name_is_random_when_using_existing_branch(cx: &mut TestAppContext) { + init_test(cx); + + let app_state = cx.update(|cx| { + agent::ThreadStore::init_global(cx); + language_model::LanguageModelRegistry::test(cx); + + let app_state = workspace::AppState::test(cx); + workspace::init(app_state.clone(), cx); + app_state + }); + + let fs = app_state.fs.as_fake(); + fs.insert_tree( + "/project", + json!({ + ".git": {}, + "src": { + "main.rs": "fn main() {}" + } + }), + ) + .await; + // Put the main worktree on "develop" so that "main" is NOT + // occupied by any worktree. + fs.set_branch_name(Path::new("/project/.git"), Some("develop")); + fs.insert_branches(Path::new("/project/.git"), &["main", "develop"]); + + let project = Project::test(app_state.fs.clone(), [Path::new("/project")], cx).await; + + let multi_workspace = + cx.add_window(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx)); + multi_workspace + .update(cx, |multi_workspace, _, cx| { + multi_workspace.open_sidebar(cx); + }) + .unwrap(); + + let workspace = multi_workspace + .read_with(cx, |multi_workspace, _cx| { + multi_workspace.workspace().clone() + }) + .unwrap(); + + workspace.update(cx, |workspace, _cx| { + workspace.set_random_database_id(); + }); + + cx.update(|cx| { + cx.observe_new( + |workspace: &mut Workspace, + window: Option<&mut Window>, + cx: &mut Context| { + if let Some(window) = window { + let panel = cx.new(|cx| AgentPanel::new(workspace, None, window, cx)); + workspace.add_panel(panel, window, cx); + } + }, + ) + .detach(); + }); + + let cx = &mut VisualTestContext::from_window(multi_workspace.into(), cx); + cx.run_until_parked(); + + let panel = workspace.update_in(cx, |workspace, window, cx| { + let panel = cx.new(|cx| AgentPanel::new(workspace, None, window, cx)); + workspace.add_panel(panel.clone(), window, cx); + panel + }); + + cx.run_until_parked(); + + panel.update_in(cx, |panel, window, cx| { + panel.open_external_thread_with_server( + Rc::new(StubAgentServer::default_response()), + window, + cx, + ); + }); + + cx.run_until_parked(); + + // Select "main" as an existing branch — this should NOT make the + // worktree directory named "main"; it should get a random name. + let content = vec![acp::ContentBlock::Text(acp::TextContent::new( + "Hello from test", + ))]; + panel.update_in(cx, |panel, window, cx| { + panel.handle_worktree_requested( + content, + WorktreeCreationArgs::New { + worktree_name: None, + branch_target: NewWorktreeBranchTarget::ExistingBranch { + name: "main".to_string(), + }, + }, + window, + cx, + ); + }); + + cx.run_until_parked(); + + // Find the new workspace and check its worktree path. + let new_worktree_path = multi_workspace + .read_with(cx, |multi_workspace, cx| { + let new_workspace = multi_workspace + .workspaces() + .find(|ws| ws.entity_id() != workspace.entity_id()) + .expect("a new workspace should have been created"); + + let new_project = new_workspace.read(cx).project().clone(); + let worktree = new_project + .read(cx) + .visible_worktrees(cx) + .next() + .expect("new workspace should have a worktree"); + worktree.read(cx).abs_path().to_path_buf() + }) + .unwrap(); + + // The worktree directory path should contain a random adjective-noun + // name, NOT the branch name "main". + let path_str = new_worktree_path.to_string_lossy(); + assert!( + !path_str.contains("/main/"), + "worktree directory should use a random name, not the branch name. \ + Got path: {path_str}", + ); + // Verify it looks like an adjective-noun pair (contains a hyphen in + // the directory component above the project name). + let parent = new_worktree_path + .parent() + .and_then(|p| p.file_name()) + .and_then(|n| n.to_str()) + .expect("should have a parent directory name"); + assert!( + parent.contains('-'), + "worktree parent directory should be an adjective-noun pair (e.g. 'swift-falcon'), \ + got: {parent}", + ); + } + #[gpui::test] async fn test_worktree_creation_preserves_selected_agent(cx: &mut TestAppContext) { init_test(cx); diff --git a/crates/agent_ui/src/branch_names.rs b/crates/agent_ui/src/branch_names.rs index 74a934e95d6219382312bdb49e7e28501a6a2517..460be9bb85c50ab7c4b326d3ad1485f95df3ec94 100644 --- a/crates/agent_ui/src/branch_names.rs +++ b/crates/agent_ui/src/branch_names.rs @@ -61,7 +61,7 @@ const NOUNS: &[&str] = &[ pub fn generate_branch_name(existing_branches: &[&str], rng: &mut impl Rng) -> Option { let existing: HashSet<&str> = existing_branches.iter().copied().collect(); - for _ in 0..100 { + for _ in 0..10 { let adjective = ADJECTIVES[rng.random_range(0..ADJECTIVES.len())]; let noun = NOUNS[rng.random_range(0..NOUNS.len())]; let name = format!("{adjective}-{noun}");