From 8b764a547756b8319d1501306ce7bdb0607ed07f Mon Sep 17 00:00:00 2001 From: Cole Miller Date: Thu, 8 May 2025 16:53:04 -0400 Subject: [PATCH] Add a test for remote tool use by the agent (#30289) - Adds a new smoke test for the use of the read_file tool by the agent in an SSH project - Fixes the SSH shutdown sequence to use a timer from the app's executor instead of always using a real timer - Changes the main executor loop for tests to advance the clock automatically instead of panicking with `parked with nothing left to run` when there is a delayed task Release Notes: - N/A --- Cargo.lock | 3 + crates/assistant_tools/src/assistant_tools.rs | 3 +- .../remote_editing_collaboration_tests.rs | 8 ++- crates/gpui/src/executor.rs | 3 + crates/gpui/src/platform/test/dispatcher.rs | 9 +++ crates/project/src/project.rs | 14 ++-- crates/remote/src/ssh_session.rs | 7 +- crates/remote_server/Cargo.toml | 3 + .../remote_server/src/remote_editing_tests.rs | 67 +++++++++++++++++++ 9 files changed, 104 insertions(+), 13 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1d6316ce06..6dc7dd1ce7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -11952,6 +11952,8 @@ version = "0.1.0" dependencies = [ "anyhow", "askpass", + "assistant_tool", + "assistant_tools", "async-watch", "backtrace", "cargo_toml", @@ -11974,6 +11976,7 @@ dependencies = [ "http_client", "language", "language_extension", + "language_model", "languages", "libc", "log", diff --git a/crates/assistant_tools/src/assistant_tools.rs b/crates/assistant_tools/src/assistant_tools.rs index f9ef5749e1..8a9d4bf6fb 100644 --- a/crates/assistant_tools/src/assistant_tools.rs +++ b/crates/assistant_tools/src/assistant_tools.rs @@ -40,13 +40,12 @@ use crate::find_path_tool::FindPathTool; use crate::grep_tool::GrepTool; use crate::list_directory_tool::ListDirectoryTool; use crate::now_tool::NowTool; -use crate::read_file_tool::ReadFileTool; use crate::thinking_tool::ThinkingTool; pub use edit_file_tool::EditFileToolInput; pub use find_path_tool::FindPathToolInput; pub use open_tool::OpenTool; -pub use read_file_tool::ReadFileToolInput; +pub use read_file_tool::{ReadFileTool, ReadFileToolInput}; pub use terminal_tool::TerminalTool; pub fn init(http_client: Arc, cx: &mut App) { diff --git a/crates/collab/src/tests/remote_editing_collaboration_tests.rs b/crates/collab/src/tests/remote_editing_collaboration_tests.rs index 70fc6bd516..df5f573f35 100644 --- a/crates/collab/src/tests/remote_editing_collaboration_tests.rs +++ b/crates/collab/src/tests/remote_editing_collaboration_tests.rs @@ -581,7 +581,11 @@ async fn test_ssh_collaboration_formatting_with_prettier( } #[gpui::test] -async fn test_remote_server_debugger(cx_a: &mut TestAppContext, server_cx: &mut TestAppContext) { +async fn test_remote_server_debugger( + cx_a: &mut TestAppContext, + server_cx: &mut TestAppContext, + executor: BackgroundExecutor, +) { cx_a.update(|cx| { release_channel::init(SemanticVersion::default(), cx); command_palette_hooks::init(cx); @@ -679,7 +683,7 @@ async fn test_remote_server_debugger(cx_a: &mut TestAppContext, server_cx: &mut }); client_ssh.update(cx_a, |a, _| { - a.shutdown_processes(Some(proto::ShutdownRemoteServer {})) + a.shutdown_processes(Some(proto::ShutdownRemoteServer {}), executor) }); shutdown_session.await.unwrap(); diff --git a/crates/gpui/src/executor.rs b/crates/gpui/src/executor.rs index affca7b1b4..273a3ea503 100644 --- a/crates/gpui/src/executor.rs +++ b/crates/gpui/src/executor.rs @@ -281,6 +281,9 @@ impl BackgroundExecutor { } if !dispatcher.parking_allowed() { + if dispatcher.advance_clock_to_next_delayed() { + continue; + } let mut backtrace_message = String::new(); let mut waiting_message = String::new(); if let Some(backtrace) = dispatcher.waiting_backtrace() { diff --git a/crates/gpui/src/platform/test/dispatcher.rs b/crates/gpui/src/platform/test/dispatcher.rs index 105c97d625..16edabfa4b 100644 --- a/crates/gpui/src/platform/test/dispatcher.rs +++ b/crates/gpui/src/platform/test/dispatcher.rs @@ -89,6 +89,15 @@ impl TestDispatcher { self.state.lock().time = new_now; } + pub fn advance_clock_to_next_delayed(&self) -> bool { + let next_due_time = self.state.lock().delayed.first().map(|(time, _)| *time); + if let Some(next_due_time) = next_due_time { + self.state.lock().time = next_due_time; + return true; + } + false + } + pub fn simulate_random_delay(&self) -> impl 'static + Send + Future + use<> { struct YieldNow { pub(crate) count: usize, diff --git a/crates/project/src/project.rs b/crates/project/src/project.rs index 214ff11bc8..30b7e11cbb 100644 --- a/crates/project/src/project.rs +++ b/crates/project/src/project.rs @@ -1130,9 +1130,10 @@ impl Project { cx.on_release(Self::release), cx.on_app_quit(|this, cx| { let shutdown = this.ssh_client.take().and_then(|client| { - client - .read(cx) - .shutdown_processes(Some(proto::ShutdownRemoteServer {})) + client.read(cx).shutdown_processes( + Some(proto::ShutdownRemoteServer {}), + cx.background_executor().clone(), + ) }); cx.background_executor().spawn(async move { @@ -1472,9 +1473,10 @@ impl Project { fn release(&mut self, cx: &mut App) { if let Some(client) = self.ssh_client.take() { - let shutdown = client - .read(cx) - .shutdown_processes(Some(proto::ShutdownRemoteServer {})); + let shutdown = client.read(cx).shutdown_processes( + Some(proto::ShutdownRemoteServer {}), + cx.background_executor().clone(), + ); cx.background_spawn(async move { if let Some(shutdown) = shutdown { diff --git a/crates/remote/src/ssh_session.rs b/crates/remote/src/ssh_session.rs index 8dad4780fb..8c266554ce 100644 --- a/crates/remote/src/ssh_session.rs +++ b/crates/remote/src/ssh_session.rs @@ -18,8 +18,8 @@ use futures::{ select, select_biased, }; use gpui::{ - App, AppContext as _, AsyncApp, BorrowAppContext, Context, Entity, EventEmitter, Global, - SemanticVersion, Task, WeakEntity, + App, AppContext as _, AsyncApp, BackgroundExecutor, BorrowAppContext, Context, Entity, + EventEmitter, Global, SemanticVersion, Task, WeakEntity, }; use itertools::Itertools; use parking_lot::Mutex; @@ -683,6 +683,7 @@ impl SshRemoteClient { pub fn shutdown_processes( &self, shutdown_request: Option, + executor: BackgroundExecutor, ) -> Option + use> { let state = self.state.lock().take()?; log::info!("shutting down ssh processes"); @@ -705,7 +706,7 @@ impl SshRemoteClient { // We wait 50ms instead of waiting for a response, because // waiting for a response would require us to wait on the main thread // which we want to avoid in an `on_app_quit` callback. - smol::Timer::after(Duration::from_millis(50)).await; + executor.timer(Duration::from_millis(50)).await; } // Drop `multiplex_task` because it owns our ssh_proxy_process, which is a diff --git a/crates/remote_server/Cargo.toml b/crates/remote_server/Cargo.toml index 43b157483b..f3c85afb28 100644 --- a/crates/remote_server/Cargo.toml +++ b/crates/remote_server/Cargo.toml @@ -69,6 +69,8 @@ fork.workspace = true libc.workspace = true [dev-dependencies] +assistant_tool.workspace = true +assistant_tools.workspace = true client = { workspace = true, features = ["test-support"] } clock = { workspace = true, features = ["test-support"] } dap = { workspace = true, features = ["test-support"] } @@ -79,6 +81,7 @@ language = { workspace = true, features = ["test-support"] } node_runtime = { workspace = true, features = ["test-support"] } project = { workspace = true, features = ["test-support"] } remote = { workspace = true, features = ["test-support"] } +language_model = { workspace = true, features = ["test-support"] } lsp = { workspace = true, features=["test-support"] } unindent.workspace = true serde_json.workspace = true diff --git a/crates/remote_server/src/remote_editing_tests.rs b/crates/remote_server/src/remote_editing_tests.rs index 96863044cc..5ed462c934 100644 --- a/crates/remote_server/src/remote_editing_tests.rs +++ b/crates/remote_server/src/remote_editing_tests.rs @@ -2,8 +2,11 @@ /// The tests in this file assume that server_cx is running on Windows too. /// We neead to find a way to test Windows-Non-Windows interactions. use crate::headless_project::HeadlessProject; +use assistant_tool::Tool as _; +use assistant_tools::{ReadFileTool, ReadFileToolInput}; use client::{Client, UserStore}; use clock::FakeSystemClock; +use language_model::{LanguageModelRequest, fake_provider::FakeLanguageModel}; use extension::ExtensionHostProxy; use fs::{FakeFs, Fs}; @@ -1548,6 +1551,70 @@ async fn test_remote_git_branches(cx: &mut TestAppContext, server_cx: &mut TestA assert_eq!(server_branch.name(), "totally-new-branch"); } +#[gpui::test] +async fn test_remote_agent_fs_tool_calls(cx: &mut TestAppContext, server_cx: &mut TestAppContext) { + let fs = FakeFs::new(server_cx.executor()); + fs.insert_tree( + path!("/project"), + json!({ + "a.txt": "A", + "b.txt": "B", + }), + ) + .await; + + let (project, _headless_project) = init_test(&fs, cx, server_cx).await; + project + .update(cx, |project, cx| { + project.find_or_create_worktree(path!("/project"), true, cx) + }) + .await + .unwrap(); + + let action_log = cx.new(|_| assistant_tool::ActionLog::new(project.clone())); + let model = Arc::new(FakeLanguageModel::default()); + let request = Arc::new(LanguageModelRequest::default()); + + let input = ReadFileToolInput { + path: "project/b.txt".into(), + start_line: None, + end_line: None, + }; + let exists_result = cx.update(|cx| { + ReadFileTool::run( + Arc::new(ReadFileTool), + serde_json::to_value(input).unwrap(), + request.clone(), + project.clone(), + action_log.clone(), + model.clone(), + None, + cx, + ) + }); + let output = exists_result.output.await.unwrap().content; + assert_eq!(output, "B"); + + let input = ReadFileToolInput { + path: "project/c.txt".into(), + start_line: None, + end_line: None, + }; + let does_not_exist_result = cx.update(|cx| { + ReadFileTool::run( + Arc::new(ReadFileTool), + serde_json::to_value(input).unwrap(), + request.clone(), + project.clone(), + action_log.clone(), + model.clone(), + None, + cx, + ) + }); + does_not_exist_result.output.await.unwrap_err(); +} + pub async fn init_test( server_fs: &Arc, cx: &mut TestAppContext,