From d5cf9be162d031bd0e6ca692235d1aae881b2cac Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Thu, 18 Sep 2025 11:38:59 +0200 Subject: [PATCH] acp: Fix behavior of read_text_file for ACP agents (#38401) We were incorrectly handling the line number as well as stripping out line breaks when returning portions of files. It also makes sure following is updated even when we load a snapshot from cache, which wasn't the case before. We also are able to load the text via a range in the snapshot, rather than allocating a string for the entire file and then another after iterating over lines in the file. Release Notes: - acp: Fix incorrect behavior when ACP agents requested to read portions of files. --- crates/acp_thread/src/acp_thread.rs | 140 +++++++++++++++++++++------- 1 file changed, 106 insertions(+), 34 deletions(-) diff --git a/crates/acp_thread/src/acp_thread.rs b/crates/acp_thread/src/acp_thread.rs index 68e5266f06aa8bddfaa252bdc1cf5b21891c7f10..f2327ca70b104de12f44d74aacd1a5a2bb1eca3b 100644 --- a/crates/acp_thread/src/acp_thread.rs +++ b/crates/acp_thread/src/acp_thread.rs @@ -1781,6 +1781,9 @@ impl AcpThread { reuse_shared_snapshot: bool, cx: &mut Context, ) -> Task> { + // Args are 1-based, move to 0-based + let line = line.unwrap_or_default().saturating_sub(1); + let limit = limit.unwrap_or(u32::MAX); let project = self.project.clone(); let action_log = self.action_log.clone(); cx.spawn(async move |this, cx| { @@ -1808,44 +1811,37 @@ impl AcpThread { action_log.update(cx, |action_log, cx| { action_log.buffer_read(buffer.clone(), cx); })?; - project.update(cx, |project, cx| { - let position = buffer - .read(cx) - .snapshot() - .anchor_before(Point::new(line.unwrap_or_default(), 0)); - project.set_agent_location( - Some(AgentLocation { - buffer: buffer.downgrade(), - position, - }), - cx, - ); - })?; - buffer.update(cx, |buffer, _| buffer.snapshot())? + let snapshot = buffer.update(cx, |buffer, _| buffer.snapshot())?; + this.update(cx, |this, _| { + this.shared_buffers.insert(buffer.clone(), snapshot.clone()); + })?; + snapshot }; - this.update(cx, |this, _| { - let text = snapshot.text(); - this.shared_buffers.insert(buffer.clone(), snapshot); - if line.is_none() && limit.is_none() { - return Ok(text); - } - let limit = limit.unwrap_or(u32::MAX) as usize; - let Some(line) = line else { - return Ok(text.lines().take(limit).collect::()); - }; + let max_point = snapshot.max_point(); + if line >= max_point.row { + anyhow::bail!( + "Attempting to read beyond the end of the file, line {}:{}", + max_point.row + 1, + max_point.column + ); + } - let count = text.lines().count(); - if count < line as usize { - anyhow::bail!("There are only {} lines", count); - } - Ok(text - .lines() - .skip(line as usize + 1) - .take(limit) - .collect::()) - })? + let start = snapshot.anchor_before(Point::new(line, 0)); + let end = snapshot.anchor_before(Point::new(line.saturating_add(limit), 0)); + + project.update(cx, |project, cx| { + project.set_agent_location( + Some(AgentLocation { + buffer: buffer.downgrade(), + position: start, + }), + cx, + ); + })?; + + Ok(snapshot.text_for_range(start..end).collect::()) }) } @@ -2391,6 +2387,82 @@ mod tests { request.await.unwrap(); } + #[gpui::test] + async fn test_reading_from_line(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree(path!("/tmp"), json!({"foo": "one\ntwo\nthree\nfour\n"})) + .await; + let project = Project::test(fs.clone(), [], cx).await; + project + .update(cx, |project, cx| { + project.find_or_create_worktree(path!("/tmp/foo"), true, cx) + }) + .await + .unwrap(); + + let connection = Rc::new(FakeAgentConnection::new()); + + let thread = cx + .update(|cx| connection.new_thread(project, Path::new(path!("/tmp")), cx)) + .await + .unwrap(); + + // Whole file + let content = thread + .update(cx, |thread, cx| { + thread.read_text_file(path!("/tmp/foo").into(), None, None, false, cx) + }) + .await + .unwrap(); + + assert_eq!(content, "one\ntwo\nthree\nfour\n"); + + // Only start line + let content = thread + .update(cx, |thread, cx| { + thread.read_text_file(path!("/tmp/foo").into(), Some(3), None, false, cx) + }) + .await + .unwrap(); + + assert_eq!(content, "three\nfour\n"); + + // Only limit + let content = thread + .update(cx, |thread, cx| { + thread.read_text_file(path!("/tmp/foo").into(), None, Some(2), false, cx) + }) + .await + .unwrap(); + + assert_eq!(content, "one\ntwo\n"); + + // Range + let content = thread + .update(cx, |thread, cx| { + thread.read_text_file(path!("/tmp/foo").into(), Some(2), Some(2), false, cx) + }) + .await + .unwrap(); + + assert_eq!(content, "two\nthree\n"); + + // Invalid + let err = thread + .update(cx, |thread, cx| { + thread.read_text_file(path!("/tmp/foo").into(), Some(5), Some(2), false, cx) + }) + .await + .unwrap_err(); + + assert_eq!( + err.to_string(), + "Attempting to read beyond the end of the file, line 5:0" + ); + } + #[gpui::test] async fn test_succeeding_canceled_toolcall(cx: &mut TestAppContext) { init_test(cx);