Yehowshua/ZIm
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

assistant edit tool: Fix editing files in context (#26751)

Browse source 
When the user attached context in the thread, the editor model request
would fail because its tool use wouldn't be removed properly leading to
an API error.

Also, after an edit, we'd keep the old file snapshot in the context.
This would make the model think that the edits didn't apply and make it
go in a loop.

Release Notes:

- N/A
This commit is contained in:
Agus Zubiaga 2025-03-14 17:07:43 -03:00 • committed by GitHub
parent ba8b9ec2c7
commit 1bf1c7223f
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
19 changed files with 239 additions and 57 deletions
Download patch file Download diff file Expand all files Collapse all files

51
crates/assistant2/src/active_thread.rs
View file

@ -22,10 +22,13 @@ use ui::Color;
use ui::{prelude::*, Disclosure, KeyBinding};
use util::ResultExt as _;
use crate::context_store::{refresh_context_store_text, ContextStore};
pub struct ActiveThread {
language_registry: Arc<LanguageRegistry>,
thread_store: Entity<ThreadStore>,
thread: Entity<Thread>,
context_store: Entity<ContextStore>,
save_thread_task: Option<Task<()>>,
messages: Vec<MessageId>,
list_state: ListState,
@ -46,6 +49,7 @@ impl ActiveThread {
thread: Entity<Thread>,
thread_store: Entity<ThreadStore>,
language_registry: Arc<LanguageRegistry>,
context_store: Entity<ContextStore>,
window: &mut Window,
cx: &mut Context<Self>,
) -> Self {
@ -58,6 +62,7 @@ impl ActiveThread {
language_registry,
thread_store,
thread: thread.clone(),
context_store,
save_thread_task: None,
messages: Vec::new(),
rendered_messages_by_id: HashMap::default(),
@ -350,11 +355,51 @@ impl ActiveThread {
}
if self.thread.read(cx).all_tools_finished() {
let pending_refresh_buffers = self.thread.update(cx, |thread, cx| {
thread.action_log().update(cx, |action_log, _cx| {
action_log.take_pending_refresh_buffers()
})
});
let context_update_task = if !pending_refresh_buffers.is_empty() {
let refresh_task = refresh_context_store_text(
self.context_store.clone(),
&pending_refresh_buffers,
cx,
);
cx.spawn(|this, mut cx| async move {
let updated_context_ids = refresh_task.await;
this.update(&mut cx, |this, cx| {
this.context_store.read_with(cx, |context_store, cx| {
context_store
.context()
.iter()
.filter(|context| {
updated_context_ids.contains(&context.id())
})
.flat_map(|context| context.snapshot(cx))
.collect()
})
})
})
} else {
Task::ready(anyhow::Ok(Vec::new()))
};
let model_registry = LanguageModelRegistry::read_global(cx);
if let Some(model) = model_registry.active_model() {
self.thread.update(cx, |thread, cx| {
thread.send_tool_results_to_model(model, cx);
});
cx.spawn(|this, mut cx| async move {
let updated_context = context_update_task.await?;
this.update(&mut cx, |this, cx| {
this.thread.update(cx, |thread, cx| {
thread.send_tool_results_to_model(model, updated_context, cx);
});
})
})
.detach();
}
}
}

15
crates/assistant2/src/assistant_panel.rs
View file

@ -155,10 +155,14 @@ impl AssistantPanel {
let workspace = workspace.weak_handle();
let weak_self = cx.entity().downgrade();
let message_editor_context_store =
cx.new(|_cx| crate::context_store::ContextStore::new(workspace.clone()));
let message_editor = cx.new(|cx| {
MessageEditor::new(
fs.clone(),
workspace.clone(),
message_editor_context_store.clone(),
thread_store.downgrade(),
thread.clone(),
window,
@ -174,6 +178,7 @@ impl AssistantPanel {
thread.clone(),
thread_store.clone(),
language_registry.clone(),
message_editor_context_store.clone(),
window,
cx,
)
@ -242,11 +247,16 @@ impl AssistantPanel {
.update(cx, |this, cx| this.create_thread(cx));
self.active_view = ActiveView::Thread;
let message_editor_context_store =
cx.new(|_cx| crate::context_store::ContextStore::new(self.workspace.clone()));
self.thread = cx.new(|cx| {
ActiveThread::new(
thread.clone(),
self.thread_store.clone(),
self.language_registry.clone(),
message_editor_context_store.clone(),
window,
cx,
)
@ -255,6 +265,7 @@ impl AssistantPanel {
MessageEditor::new(
self.fs.clone(),
self.workspace.clone(),
message_editor_context_store,
self.thread_store.downgrade(),
thread,
window,
@ -375,11 +386,14 @@ impl AssistantPanel {
let thread = open_thread_task.await?;
this.update_in(&mut cx, |this, window, cx| {
this.active_view = ActiveView::Thread;
let message_editor_context_store =
cx.new(|_cx| crate::context_store::ContextStore::new(this.workspace.clone()));
this.thread = cx.new(|cx| {
ActiveThread::new(
thread.clone(),
this.thread_store.clone(),
this.language_registry.clone(),
message_editor_context_store.clone(),
window,
cx,
)
@ -388,6 +402,7 @@ impl AssistantPanel {
MessageEditor::new(
this.fs.clone(),
this.workspace.clone(),
message_editor_context_store,
this.thread_store.downgrade(),
thread,
window,

65
crates/assistant2/src/context_store.rs
View file

@ -9,6 +9,7 @@ use language::Buffer;
use project::{ProjectPath, Worktree};
use rope::Rope;
use text::BufferId;
use util::maybe;
use workspace::Workspace;
use crate::context::{
@ -531,35 +532,59 @@ fn collect_files_in_path(worktree: &Worktree, path: &Path) -> Vec<Arc<Path>> {
pub fn refresh_context_store_text(
context_store: Entity<ContextStore>,
changed_buffers: &HashSet<Entity<Buffer>>,
cx: &App,
) -> impl Future<Output = ()> {
) -> impl Future<Output = Vec<ContextId>> {
let mut tasks = Vec::new();
for context in &context_store.read(cx).context {
match context {
AssistantContext::File(file_context) => {
let context_store = context_store.clone();
if let Some(task) = refresh_file_text(context_store, file_context, cx) {
tasks.push(task);
let id = context.id();
let task = maybe!({
match context {
AssistantContext::File(file_context) => {
if changed_buffers.is_empty()
|| changed_buffers.contains(&file_context.context_buffer.buffer)
{
let context_store = context_store.clone();
return refresh_file_text(context_store, file_context, cx);
}
}
}
AssistantContext::Directory(directory_context) => {
let context_store = context_store.clone();
if let Some(task) = refresh_directory_text(context_store, directory_context, cx) {
tasks.push(task);
AssistantContext::Directory(directory_context) => {
let should_refresh = changed_buffers.is_empty()
|| changed_buffers.iter().any(|buffer| {
let buffer = buffer.read(cx);
buffer_path_log_err(&buffer)
.map_or(false, |path| path.starts_with(&directory_context.path))
});
if should_refresh {
let context_store = context_store.clone();
return refresh_directory_text(context_store, directory_context, cx);
}
}
AssistantContext::Thread(thread_context) => {
if changed_buffers.is_empty() {
let context_store = context_store.clone();
return Some(refresh_thread_text(context_store, thread_context, cx));
}
}
// Intentionally omit refreshing fetched URLs as it doesn't seem all that useful,
// and doing the caching properly could be tricky (unless it's already handled by
// the HttpClient?).
AssistantContext::FetchedUrl(_) => {}
}
AssistantContext::Thread(thread_context) => {
let context_store = context_store.clone();
tasks.push(refresh_thread_text(context_store, thread_context, cx));
}
// Intentionally omit refreshing fetched URLs as it doesn't seem all that useful,
// and doing the caching properly could be tricky (unless it's already handled by
// the HttpClient?).
AssistantContext::FetchedUrl(_) => {}
None
});
if let Some(task) = task {
tasks.push(task.map(move |_| id));
}
}
future::join_all(tasks).map(|_| ())
future::join_all(tasks)
}
fn refresh_file_text(

6
crates/assistant2/src/message_editor.rs
View file

@ -1,5 +1,6 @@
use std::sync::Arc;
use collections::HashSet;
use editor::actions::MoveUp;
use editor::{Editor, EditorElement, EditorEvent, EditorStyle};
use file_icons::FileIcons;
@ -51,13 +52,13 @@ impl MessageEditor {
pub fn new(
fs: Arc<dyn Fs>,
workspace: WeakEntity<Workspace>,
context_store: Entity<ContextStore>,
thread_store: WeakEntity<ThreadStore>,
thread: Entity<Thread>,
window: &mut Window,
cx: &mut Context<Self>,
) -> Self {
let tools = thread.read(cx).tools().clone();
let context_store = cx.new(|_cx| ContextStore::new(workspace.clone()));
let context_picker_menu_handle = PopoverMenuHandle::default();
let inline_context_picker_menu_handle = PopoverMenuHandle::default();
let model_selector_menu_handle = PopoverMenuHandle::default();
@ -200,7 +201,8 @@ impl MessageEditor {
text
});
let refresh_task = refresh_context_store_text(self.context_store.clone(), cx);
let refresh_task =
refresh_context_store_text(self.context_store.clone(), &HashSet::default(), cx);
let thread = self.thread.clone();
let context_store = self.context_store.clone();

24
crates/assistant2/src/thread.rs
View file

@ -2,7 +2,7 @@ use std::io::Write;
use std::sync::Arc;
use anyhow::{Context as _, Result};
use assistant_tool::ToolWorkingSet;
use assistant_tool::{ActionLog, ToolWorkingSet};
use chrono::{DateTime, Utc};
use collections::{BTreeMap, HashMap, HashSet};
use futures::future::Shared;
@ -104,6 +104,7 @@ pub struct Thread {
prompt_builder: Arc<PromptBuilder>,
tools: Arc<ToolWorkingSet>,
tool_use: ToolUseState,
action_log: Entity<ActionLog>,
scripting_session: Entity<ScriptingSession>,
scripting_tool_use: ToolUseState,
initial_project_snapshot: Shared<Task<Option<Arc<ProjectSnapshot>>>>,
@ -134,6 +135,7 @@ impl Thread {
tool_use: ToolUseState::new(),
scripting_session: cx.new(|cx| ScriptingSession::new(project.clone(), cx)),
scripting_tool_use: ToolUseState::new(),
action_log: cx.new(|_| ActionLog::new()),
initial_project_snapshot: {
let project_snapshot = Self::project_snapshot(project, cx);
cx.foreground_executor()
@ -191,6 +193,7 @@ impl Thread {
prompt_builder,
tools,
tool_use,
action_log: cx.new(|_| ActionLog::new()),
scripting_session,
scripting_tool_use,
initial_project_snapshot: Task::ready(serialized.initial_project_snapshot).shared(),
@ -750,7 +753,13 @@ impl Thread {
for tool_use in pending_tool_uses {
if let Some(tool) = self.tools.tool(&tool_use.name, cx) {
let task = tool.run(tool_use.input, &request.messages, self.project.clone(), cx);
let task = tool.run(
tool_use.input,
&request.messages,
self.project.clone(),
self.action_log.clone(),
cx,
);
self.insert_tool_output(tool_use.id.clone(), task, cx);
}
@ -857,8 +866,15 @@ impl Thread {
pub fn send_tool_results_to_model(
&mut self,
model: Arc<dyn LanguageModel>,
updated_context: Vec<ContextSnapshot>,
cx: &mut Context<Self>,
) {
self.context.extend(
updated_context
.into_iter()
.map(|context| (context.id, context)),
);
// Insert a user message to contain the tool results.
self.insert_user_message(
// TODO: Sending up a user message without any content results in the model sending back
@ -1057,6 +1073,10 @@ impl Thread {
Ok(String::from_utf8_lossy(&markdown).to_string())
}
pub fn action_log(&self) -> &Entity<ActionLog> {
&self.action_log
}
pub fn cumulative_token_usage(&self) -> TokenUsage {
self.cumulative_token_usage.clone()
}

4
crates/assistant2/src/tool_use.rs
View file

@ -226,12 +226,12 @@ impl ToolUseState {
output: Result<String>,
) -> Option<PendingToolUse> {
match output {
Ok(output) => {
Ok(tool_result) => {
self.tool_results.insert(
tool_use_id.clone(),
LanguageModelToolResult {
tool_use_id: tool_use_id.clone(),
content: output.into(),
content: tool_result.into(),
is_error: false,
},
);