Implementation of knowledge base directory setting #2484
Conversation
| KnowledgeSubcommand::Add { path } => Self::handle_add(os, path, knowledge_base_dir).await, | ||
| KnowledgeSubcommand::Remove { path } => Self::handle_remove(os, path, knowledge_base_dir).await, | ||
| KnowledgeSubcommand::Update { path } => Self::handle_update(os, path, knowledge_base_dir).await, | ||
| KnowledgeSubcommand::Clear => Self::handle_clear(session).await, | ||
| KnowledgeSubcommand::Status => Self::handle_status().await, | ||
| KnowledgeSubcommand::Cancel { operation_id } => Self::handle_cancel(operation_id.as_deref()).await, | ||
| KnowledgeSubcommand::Status => Self::handle_status(knowledge_base_dir).await, | ||
| KnowledgeSubcommand::Cancel { operation_id } => { | ||
| Self::handle_cancel(operation_id.as_deref(), knowledge_base_dir).await | ||
| }, |
There was a problem hiding this comment.
Would it be better to pass the session in here?
There was a problem hiding this comment.
Agreed, the less parameters the better specially if more than 1 parameter has same type.
| fn get_knowledge_base_dir(agent: Option<&Agent>) -> Option<impl AsRef<Path> + use<>> { | ||
| if agent.is_none() { | ||
| return None; | ||
| } | ||
|
|
||
| agent.unwrap().tools_settings.get("knowledge")?.get("base_dir"); | ||
| match agent.unwrap().tools_settings.get("knowledge") { | ||
| Some(settings) => match serde_json::from_value::<Settings>(settings.clone()) { | ||
| Ok(settings) => Some(settings.base_dir), | ||
| Err(e) => { | ||
| error!("Failed to deserialize tool settings for execute_bash: {:?}", e); | ||
| None | ||
| }, | ||
| }, | ||
| None => None, | ||
| } | ||
| } |
There was a problem hiding this comment.
Is there a better way to share this function between the knowledge base tool and setting?
There was a problem hiding this comment.
IMO, it's fine if the tool has the responsibility to know where this needs to be extracted, careful with usage of unwrap here. So we can gracefully handle failure scenarios.
| match self { | ||
| Tool::FsRead(fs_read) => fs_read.invoke(os, stdout).await, | ||
| Tool::FsWrite(fs_write) => fs_write.invoke(os, stdout).await, | ||
| Tool::ExecuteCommand(execute_command) => execute_command.invoke(stdout).await, | ||
| Tool::UseAws(use_aws) => use_aws.invoke(os, stdout).await, | ||
| Tool::Custom(custom_tool) => custom_tool.invoke(os, stdout).await, | ||
| Tool::GhIssue(gh_issue) => gh_issue.invoke(os, stdout).await, | ||
| Tool::Knowledge(knowledge) => knowledge.invoke(os, stdout).await, | ||
| Tool::Knowledge(knowledge) => knowledge.invoke(os, stdout, agent).await, |
There was a problem hiding this comment.
Very unsure of this - is there another way for us to pass tool settings? Looks like we've started to pass the agent for requires_acceptance, so figured this would be an acceptable pattern. Again, open to feedback though.
| @@ -32,21 +33,21 @@ pub struct KnowledgeStore { | |||
|
|
|||
| impl KnowledgeStore { | |||
| /// Get singleton instance | |||
| pub async fn get_async_instance() -> Arc<Mutex<Self>> { | |||
| pub async fn get_async_instance(path: Option<impl AsRef<Path>>) -> Arc<Mutex<Self>> { | |||
There was a problem hiding this comment.
Because any invocation of this could technically create the instance, we need to always pass the path if possible.
There was a problem hiding this comment.
Yeah, that's the downside of a Lazy singleton
| @@ -1881,7 +1881,10 @@ impl ChatSession { | |||
| ev.is_accepted = true; | |||
| }); | |||
|
|
|||
| let invoke_result = tool.tool.invoke(os, &mut self.stdout).await; | |||
| let invoke_result = tool | |||
| .tool | |||
Issue #, if available: None.
Description of changes:
base_dirin the tool settings. This would allow different agents to access different knowledge bases. This would keep the knowledge bases for a particular agent clean - e.g. an oncall-helper agent would not recieve documentation about the team charter, or other unnecessary information.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.