From 1c1f2d6f4c61874943be4c61cc354a424ea4e8d6 Mon Sep 17 00:00:00 2001 From: Richard Feldman Date: Mon, 13 Apr 2026 16:12:36 -0400 Subject: [PATCH] agent_ui: Fix worktree naming regression when using existing branches (#53669) 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. 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 - Collect existing worktree directory names to avoid collisions - Check generated paths against known worktree paths before creating - Reduce random name retry count from 100 to 10 - Add regression test Release Notes: - Fixed a regression where creating a git worktree from an existing branch would name the worktree directory after the branch (instead of generating a random name) --- crates/agent_ui/src/agent_panel.rs | 192 +++++++++++++++++++++++++++- crates/agent_ui/src/branch_names.rs | 2 +- 2 files changed, 186 insertions(+), 8 deletions(-) diff --git a/crates/agent_ui/src/agent_panel.rs b/crates/agent_ui/src/agent_panel.rs index 43f59f1df084ab8cf27ac55d4553e9744a6946e6..08e70c5cd5b9adccc86d441a873b0c03dc9eed4a 100644 --- a/crates/agent_ui/src/agent_panel.rs +++ b/crates/agent_ui/src/agent_panel.rs @@ -3102,11 +3102,11 @@ impl AgentPanel { branch_target: &NewWorktreeBranchTarget, existing_branches: &HashSet, occupied_branches: &HashSet, + rng: &mut impl rand::Rng, ) -> Result<(String, bool, Option)> { - let generate_branch_name = || -> Result { + let mut generate_branch_name = || { let refs: Vec<&str> = existing_branches.iter().map(|s| s.as_str()).collect(); - let mut rng = rand::rng(); - crate::branch_names::generate_branch_name(&refs, &mut rng) + crate::branch_names::generate_branch_name(&refs, rng) .ok_or_else(|| anyhow!("Failed to generate a unique branch name")) }; @@ -3134,10 +3134,13 @@ 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, worktree_directory_setting: &str, + rng: &mut impl rand::Rng, cx: &mut Context, ) -> Result<( Vec<( @@ -3150,12 +3153,20 @@ 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(); + crate::branch_names::generate_branch_name(&existing_refs, 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, @@ -3221,7 +3232,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 @@ -3431,6 +3443,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)) => { @@ -3438,6 +3452,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)) => { @@ -3447,11 +3470,14 @@ impl AgentPanel { } } + let mut rng = rand::rng(); + let (branch_name, use_existing_branch, start_point) = match Self::resolve_worktree_branch_target( &branch_target, &existing_branches, &occupied_branches, + &mut rng, ) { Ok(target) => target, Err(err) => { @@ -3471,10 +3497,13 @@ impl AgentPanel { Self::start_worktree_creations( &git_repos, worktree_name, + &existing_worktree_names, + &existing_worktree_paths, &branch_name, use_existing_branch, start_point, &worktree_directory_setting, + &mut rng, cx, ) }) { @@ -5240,6 +5269,7 @@ mod tests { use fs::FakeFs; use gpui::{TestAppContext, VisualTestContext}; use project::Project; + use rand::rngs::StdRng; use serde_json::json; use std::path::Path; use std::time::Instant; @@ -6569,8 +6599,8 @@ mod tests { ); } - #[test] - fn test_resolve_worktree_branch_target() { + #[gpui::test(iterations = 10)] + fn test_resolve_worktree_branch_target(mut rng: StdRng) { let existing_branches = HashSet::from_iter([ "main".to_string(), "feature".to_string(), @@ -6584,6 +6614,7 @@ mod tests { }, &existing_branches, &HashSet::from_iter(["main".to_string()]), + &mut rng, ) .unwrap(); assert_eq!( @@ -6597,6 +6628,7 @@ mod tests { }, &existing_branches, &HashSet::default(), + &mut rng, ) .unwrap(); assert_eq!(resolved, ("feature".to_string(), true, None)); @@ -6607,6 +6639,7 @@ mod tests { }, &existing_branches, &HashSet::from_iter(["main".to_string()]), + &mut rng, ) .unwrap(); assert_eq!(resolved.1, false); @@ -6616,6 +6649,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}");