From 930c3bf0ecce7b435446b929c6b08331eaf063da Mon Sep 17 00:00:00 2001 From: skcd Date: Tue, 17 Dec 2024 17:36:30 +0000 Subject: [PATCH 1/2] [sidecar] fix absolute path issues --- sidecar/src/agentic/tool/session/service.rs | 6 +- sidecar/src/agentic/tool/session/session.rs | 157 ++------------------ 2 files changed, 13 insertions(+), 150 deletions(-) diff --git a/sidecar/src/agentic/tool/session/service.rs b/sidecar/src/agentic/tool/session/service.rs index c716e6cd..3b01c925 100644 --- a/sidecar/src/agentic/tool/session/service.rs +++ b/sidecar/src/agentic/tool/session/service.rs @@ -465,8 +465,7 @@ impl SessionService { tool_box.clone(), tool_agent.clone(), user_message.to_owned(), - false, - true, + root_directory.to_owned(), message_properties.clone(), ) .await; @@ -631,8 +630,7 @@ impl SessionService { tool_box.clone(), tool_agent.clone(), user_message.to_owned(), - false, // is not part of test genertaion - false, + root_directory.to_owned(), message_properties.clone(), ) .await?; diff --git a/sidecar/src/agentic/tool/session/session.rs b/sidecar/src/agentic/tool/session/session.rs index c40207ec..4cb4fa33 100644 --- a/sidecar/src/agentic/tool/session/session.rs +++ b/sidecar/src/agentic/tool/session/session.rs @@ -2169,36 +2169,9 @@ The Github Issue we are trying to solve is: tool_box: Arc, tool_agent: ToolUseAgent, original_user_message: String, - is_test_generation: bool, - is_swe_bench: bool, + root_directory: String, message_properties: SymbolEventMessageProperties, ) -> Result { - // we want to send a new event only when we are not going to ask for the followup questions - // we might have generated a new exchange id over here if we are going to be working - // on top of any tool which does not require user feedback - // let exchange_id = if !matches!(tool_type, ToolType::AskFollowupQuestions) - // && !matches!(tool_type, ToolType::AttemptCompletion) - // { - // let new_exchange_id = tool_box - // .create_new_exchange( - // message_properties.root_request_id().to_owned(), - // message_properties.clone(), - // ) - // .await?; - // message_properties = message_properties.set_request_id(new_exchange_id.to_owned()); - // let session_id = message_properties.root_request_id().to_owned(); - // let exchange_id = message_properties.request_id_str().to_owned(); - // let _ = message_properties - // .ui_sender() - // .send(UIEventWithID::tool_output_type_found( - // session_id.to_owned(), - // exchange_id.to_owned(), - // tool_type.clone(), - // )); - // new_exchange_id - // } else { - // message_properties.request_id_str().to_owned() - // }; let exchange_id = message_properties.request_id_str().to_owned(); match tool_input_partial { ToolInputPartial::TestRunner(test_runner) => { @@ -2278,7 +2251,15 @@ The Github Issue we are trying to solve is: // figure out what to do over here } ToolInputPartial::CodeEditing(code_editing) => { - let fs_file_path = code_editing.fs_file_path().to_owned(); + let mut fs_file_path = code_editing.fs_file_path().to_owned(); + if !std::path::Path::new(&fs_file_path).is_absolute() { + // we need to join the file path here with the root directory + println!("fixing relative file path"); + fs_file_path = std::path::Path::new(&root_directory) + .join(fs_file_path) + .to_string_lossy() + .to_string(); + } println!("Code editing: {}", fs_file_path); let file_contents = tool_box .file_open(fs_file_path.to_owned(), message_properties.clone()) @@ -2294,123 +2275,7 @@ The Github Issue we are trying to solve is: // if the file is very very large then we chunk it up and use search and replace // on individual chunks instead - let _ = if file_contents.lines().into_iter().collect::>().len() - >= 1300 - // if we are not in swe_bench mode, never try to be extra, go with - // the standard search and replace flow - && !is_swe_bench - { - let first_part_lines = file_contents - .to_owned() - .lines() - .into_iter() - .enumerate() - .filter_map(|(idx, line)| { - if idx <= 750 { - Some(line.to_owned()) - } else { - None - } - }) - .collect::>() - .join("\n"); - let second_part_lines = file_contents - .to_owned() - .lines() - .into_iter() - .enumerate() - .filter_map(|(idx, line)| { - if idx > 750 { - Some(line.to_owned()) - } else { - None - } - }) - .collect::>() - .join("\n"); - let first_range = - Range::new(Position::new(0, 0, 0), Position::new(10_000, 0, 0)); - let second_range = - Range::new(Position::new(0, 0, 0), Position::new(10_000, 0, 0)); - - // First half of the file has been edited - let symbol_to_edit = SymbolToEdit::new( - fs_file_path.to_owned(), - first_range, - fs_file_path.to_owned(), - vec![instruction.clone()], - false, - false, // is_new - false, - "".to_owned(), - None, - false, - None, - false, - None, - vec![], // previous_user_queries - None, - ); - - let symbol_identifier = SymbolIdentifier::new_symbol(&fs_file_path); - - let first_part_edited = tool_box - .code_editing_with_search_and_replace( - &symbol_to_edit, - &fs_file_path, - &first_part_lines, - &first_range, - "".to_owned(), - instruction.clone(), - &symbol_identifier, - None, - None, - message_properties.clone(), - ) - .await?; // big expectations but can also fail, we should handle it properly - - // Editing second half of the file - let symbol_to_edit = SymbolToEdit::new( - fs_file_path.to_owned(), - second_range, - fs_file_path.to_owned(), - vec![instruction.clone()], - false, - false, // is_new - false, - "".to_owned(), - None, - false, - None, - false, - None, - vec![], // previous_user_queries - None, - ); - - let symbol_identifier = SymbolIdentifier::new_symbol(&fs_file_path); - - let second_part_edited = tool_box - .code_editing_with_search_and_replace( - &symbol_to_edit, - &fs_file_path, - &second_part_lines, - &second_range, - "".to_owned(), - format!(r#"{} -This is part of the file which might not contain the method in full, if thats the case do not generate any edits"#, instruction.clone()), - &symbol_identifier, - None, - None, - message_properties.clone(), - ) - .await?; // big expectations but can also fail, we should handle it properly - format!( - r#"{} -{}"#, - first_part_edited, second_part_edited - ) - } else { + let _ = { let default_range = // very large end position Range::new(Position::new(0, 0, 0), Position::new(10_000, 0, 0)); From 14d0d91e4a585be0b2f072e1c62c06db04734b84 Mon Sep 17 00:00:00 2001 From: skcd Date: Tue, 17 Dec 2024 20:42:02 +0000 Subject: [PATCH 2/2] [sidecar] fix path problems --- reproduce_error.py | 19 +++ sidecar/src/agentic/tool/session/service.rs | 8 +- sidecar/src/agentic/tool/session/session.rs | 128 -------------------- 3 files changed, 21 insertions(+), 134 deletions(-) create mode 100644 reproduce_error.py diff --git a/reproduce_error.py b/reproduce_error.py new file mode 100644 index 00000000..246fbe59 --- /dev/null +++ b/reproduce_error.py @@ -0,0 +1,19 @@ +import subprocess +import sys + +def main(): + # Try to list available tools in zed + try: + result = subprocess.run(['cargo', 'run', '--bin', 'sidecar', '--', 'tools', 'list'], + cwd='sidecar', + capture_output=True, + text=True) + print("Current tools available:") + print(result.stdout) + print("\nError output:") + print(result.stderr) + except Exception as e: + print(f"Error running sidecar: {e}") + +if __name__ == "__main__": + main() \ No newline at end of file diff --git a/sidecar/src/agentic/tool/session/service.rs b/sidecar/src/agentic/tool/session/service.rs index 3b01c925..8f686874 100644 --- a/sidecar/src/agentic/tool/session/service.rs +++ b/sidecar/src/agentic/tool/session/service.rs @@ -399,7 +399,7 @@ impl SessionService { // we should ideally get this information from the vscode-server side setting let tool_agent = ToolUseAgent::new( llm_broker.clone(), - root_directory, + root_directory.to_owned(), std::env::consts::OS.to_owned(), shell.to_owned(), Some(repo_name), @@ -463,8 +463,6 @@ impl SessionService { tool_type.clone(), tool_input_partial, tool_box.clone(), - tool_agent.clone(), - user_message.to_owned(), root_directory.to_owned(), message_properties.clone(), ) @@ -562,7 +560,7 @@ impl SessionService { // we should ideally get this information from the vscode-server side setting let tool_agent = ToolUseAgent::new( llm_broker.clone(), - root_directory, + root_directory.to_owned(), std::env::consts::OS.to_owned(), shell.to_owned(), None, @@ -628,8 +626,6 @@ impl SessionService { tool_type.clone(), tool_input_partial, tool_box.clone(), - tool_agent.clone(), - user_message.to_owned(), root_directory.to_owned(), message_properties.clone(), ) diff --git a/sidecar/src/agentic/tool/session/session.rs b/sidecar/src/agentic/tool/session/session.rs index 4cb4fa33..1f373414 100644 --- a/sidecar/src/agentic/tool/session/session.rs +++ b/sidecar/src/agentic/tool/session/session.rs @@ -704,62 +704,6 @@ impl Session { .find(|exchange| &exchange.exchange_id == exchange_id) } - fn decay_messages( - &self, - // both of these have the same length - exchanges: &[Exchange], - mut conversation_messages: Vec, - ) -> Vec { - // The algorithm we use for decay is the following: - // - When using any tool which is of map type query -> List - // we keep the tool output for the map tool type as long as there is not - // a mutation (code_edit) or we use another map type tool - // - This allows us to keep the token usage small while still retaining - let mut previous_map_tool_indices = vec![]; - for (idx, exchange) in exchanges.into_iter().enumerate() { - match &exchange.exchange_type { - ExchangeType::AgentChat(agent_chat) => match &agent_chat.reply { - ExchangeReplyAgent::Tool(tool_input) => { - let input_tool_type = &tool_input.tool_type; - // We are have an input tool over here - // map_tool_type || mutation_tool_type - // [T T T T M I I C T M I I I M M T T] - // if we get a C which is code-edit - // then we remove the output of the previous map test - // and if we have multiple M steps we remove all of them - // until we get a C (code-edit) - if input_tool_type.is_map_type() { - previous_map_tool_indices.push(idx); - } - if input_tool_type.is_code_edit_type() { - // rest all the running map types over here sinc we - // have started started code editing - previous_map_tool_indices.into_iter().for_each( - |map_tool_input_index| { - // the tool output is generally immediately - // after the current tool input index - let role = conversation_messages[map_tool_input_index + 1] - .role() - .clone(); - conversation_messages[map_tool_input_index + 1] = - SessionChatMessage::new( - role, - "... truncated output".to_owned(), - vec![], - ); - }, - ); - previous_map_tool_indices = vec![]; - } - } - _ => {} - }, - _ => {} - } - } - conversation_messages - } - /// Finds the exchange we are interested in and mutates the previous queries /// and the current query pub fn plan_iteration( @@ -2110,65 +2054,11 @@ impl Session { self } - async fn handle_critique( - mut self, - tool_agent: ToolUseAgent, - tool_box: Arc, - original_user_message: String, - message_properties: SymbolEventMessageProperties, - ) -> Result { - // figure out what to do over here given the state of the session - let mut converted_messages = vec![]; - for previous_message in self.exchanges.iter() { - converted_messages.push( - previous_message - .to_conversation_message(tool_box.tools().clone()) - .await, - ); - } - - // decay the content of the messages depending on the decay condition - // so we can keep the context smaller and more relevant - converted_messages = self.decay_messages(self.exchanges.as_slice(), converted_messages); - let input = - ToolUseAgentInput::new(converted_messages, vec![], None, message_properties.clone()); - let critique = tool_agent.invoke_critique(input).await?; - // reset all the exchanges since we are going to start a new - println!("session::handle_critique::starting_new"); - let exchange_message = format!( - r#"When trying to solve this issue before we ran into a wrong approach, the patch was reviewed by a senior engineer who had the following helpful feedback to share: -{} - -We have also reset the repository state and discarded all the changes which you did before. This is to help you start on a fresh plate. - -The Github Issue we are trying to solve is: -{original_user_message}"#, - critique - ); - self.exchanges = vec![]; - self.exchanges.push(Exchange::human_chat( - "critique".to_owned(), - exchange_message, - UserContext::default(), - self.project_labels.to_vec(), - self.repo_ref.clone(), - )); - - // reset the repository status by running git stash - let _ = tool_box - .use_terminal_command("git stash", message_properties) - .await; - self.save_to_storage().await?; - Ok(self) - } - pub async fn invoke_tool( mut self, tool_type: ToolType, tool_input_partial: ToolInputPartial, tool_box: Arc, - tool_agent: ToolUseAgent, - original_user_message: String, root_directory: String, message_properties: SymbolEventMessageProperties, ) -> Result { @@ -2220,24 +2110,6 @@ The Github Issue we are trying to solve is: formatted_output, // truncated UserContext::default(), ); - - // The test running is a terminal condition, if we have an exit code 0 - // we are good, otherwise the cirtique will take a look - // we only do this when we are not generating test cases - if !is_test_generation { - if test_runner_output.exit_code() != 0 { - return self - .handle_critique( - tool_agent, - tool_box, - original_user_message, - message_properties, - ) - .await; - } else if test_runner_output.exit_code() == 0 { - return Err(SymbolError::TestCaseIsPassing); - } - } } ToolInputPartial::AskFollowupQuestions(_followup_question) => { // this waits for the user-feedback so we do not need to react or