-
Notifications
You must be signed in to change notification settings - Fork 2.6k
chore: move list recipes and archive recipe to goose server #4422
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…howing goosehints, etc on RecipesView
* main: (40 commits) new recipe to lint-check my code (#4416) removing a leftover syntax error (#4415) Iand/updating recipe validation workflow (#4413) Iand/updating recipe validation workflow (#4410) Fix (Ollama provider): Unsupported operation: streaming not implemented (#4303) change databricks default to claude sonnet 4 (#4405) Iand/updating recipe validation workflow (#4406) Add metrics for recipe metadata in scheduler, UI, and CLI (#4399) Iand/updating recipe validation workflow (#4403) making small updates to recipe validation workflow (#4401) Automate OpenRouter API Key Distribution for External Recipe Contributors (#3198) Enhance `convert_path_with_tilde_expansion` to handle Windows (#4390) make sure all cookbook recipes have a title and version, but no id (#4395) Nest TODO State in session data (#4361) Fast model falls back to regular (#4375) Update windows instructions (#4333) feat: linux computer control for android (termux) (#3890) feat: Added scroll state support for chat-session-list navigation (#4360) docs: typo fix (#4376) blog: goose janitor (#4131) ...
| agent: Option<AgentRef>, | ||
| pub secret_key: String, | ||
| pub scheduler: Arc<Mutex<Option<Arc<dyn SchedulerTrait>>>>, | ||
| pub recipe_file_hash_map: Arc<Mutex<HashMap<String, PathBuf>>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This in memory map maintains the mapping of recipe id (hashed based on recipe path) and recipe path. recipe id is the key of map
| </div> | ||
| )} | ||
|
|
||
| {selectedRecipe.recipe.goosehints && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @zanesq I am not sure whether this is really used in RecipeView UI. It seems not. same as profile and mcps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch I was wondering the same thing, might have been legacy functionality that we can drop now.
| * @param recipeName The name of the recipe to restore | ||
| * @param isGlobal Whether the recipe is in global or local storage | ||
| */ | ||
| export async function restoreRecipe(recipeName: string, isGlobal: boolean): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is not used at all
| * @param recipeName The name of the recipe to permanently delete | ||
| * @param isGlobal Whether the recipe is in global or local storage | ||
| */ | ||
| export async function permanentlyDeleteRecipe( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is not used at all
| * @param recipeName The name of the recipe to delete/archive | ||
| * @param isGlobal Whether the recipe is in global or local storage | ||
| */ | ||
| export async function deleteRecipe(recipeName: string, isGlobal: boolean): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is not used at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might have been accidentally dropped with the ui refactor should we add it back to the ui instead? Seems like we need a way to delete recipes still?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad we do have delete already nvm on this comment go ahead and remove it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * @param recipeName The name of the recipe to archive | ||
| * @param isGlobal Whether the recipe is in global or local storage | ||
| */ | ||
| export async function archiveRecipe(recipeName: string, isGlobal: boolean): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function is no longer required as we perform archiveRecipe via api call
|
|
||
| // Render a recipe item with error handling | ||
| const RecipeItem = ({ savedRecipe }: { savedRecipe: SavedRecipe }) => { | ||
| try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed try/catch here as the server side will ignore the yaml files that do not conform to recipe format.
|
|
||
| /** | ||
| * Delete a recipe (archives it by default for backward compatibility). | ||
| * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just all LLM generated non-sense - there was never backward compatibility here
crates/goose/Cargo.toml
Outdated
|
|
||
| arrow = "52.2" | ||
| oauth2 = "5.0.0" | ||
| xxhash-rust = { version = "0.8.12", features = ["xxh3"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need yet another hashing library, ahash should do the trick
| Ok(()) | ||
| } | ||
|
|
||
| pub fn archive(file_path: &Path) -> Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's drop archive. we have no way to restore archived recipes it seems and we call it delete. let's delete them.
| use utoipa::ToSchema; | ||
|
|
||
| #[derive(Serialize, Deserialize, Debug, Clone, ToSchema)] | ||
| pub struct RecipeManifest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this RecipeManifest in the client was a mistake. they should never be saved to disk - @jsibbison-square already build some compatibility in so we can read this client side version of recipes.
In the client we just list the recipes and don't show is_global anywhere. name is a duplicated. last_modified should not be stored either, that's just when the file got updated.
in short, we shouldn't need this at all. just load recipes from disk and if the client needs it, ad the modification date (if you need an intermediate class for that, fine, but just in routes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the list_recipe, manifest related object sas view objects to goose-server.
list_recipes is now calling goose core to get the recipe objects
If cli needs list recipes in the future, we can move list_recipes (only get the recipe objects part, no the manifest) to goose core
| } | ||
|
|
||
| fn get_all_recipes_manifests() -> Result<Vec<RecipeManifestWithPath>> { | ||
| let current_dir = std::env::current_dir()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should not use the current_dir, but use the current dir of the session
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where working directory is changed on UI, it started goosed with the working directory.
So current_dir here is the session working directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I know that is currently the case, but once we move to having multiple agents per goosed, this will no longer be the case
|
|
||
| let global_recipe_path = choose_app_strategy(APP_STRATEGY.clone()) | ||
| .map(|strategy| strategy.in_config_dir("recipes")) | ||
| .unwrap_or_else(|_| PathBuf::from("~/.config/goose/recipes")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should not hardcode .config - app_strategy has a handler to get to I think the data_dir?
| format!("{:016x}", hash) | ||
| } | ||
|
|
||
| fn load_recipes_from_path(path: &PathBuf) -> Result<Vec<RecipeManifestWithPath>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we already have read_recipe_in_dir, use that. this would be a good opportunity to unify the recipes the user sees in CLI and the desktop
| @@ -1,3 +1,4 @@ | |||
| import { listRecipes, RecipeManifestResponse } from '../api'; | |||
| import { Recipe } from './index'; | |||
| import * as yaml from 'yaml'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this entire file should now be deleted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will. There are still some codes for save and load. When we move these recipe operations to server side, then we don't need it
| <p className="text-text-muted text-sm mb-2 line-clamp-2">{manifest.recipe.description}</p> | ||
| <div className="flex items-center text-xs text-text-muted"> | ||
| <Calendar className="w-3 h-3 mr-1" /> | ||
| {recipeLastModified(manifest.lastModified)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do this locally, not worth two calls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i renamed recipeLastModified to convertToLocaleDateString. basically it convert the utc time string to local date
| @@ -0,0 +1,48 @@ | |||
| use anyhow::Result; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file should not exist
| @@ -0,0 +1,85 @@ | |||
| use std::fs; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we shouldn't need this, just read the recipes just like we always have
* main: chore: move list recipes and archive recipe to goose server (#4422) deleting a recipe and testing workflow (#4451) adding a new recipe (#4449) docs: autovisualiser extension (#4380) trying to restore functionality for api-key sending after merging a recipe (#4446) restoring a deleted recipe (#4445) testing recipe removal (#4443) updating our 3 workflows to only operate if the PR is adding/editing a recipe (#4441) [cookbook recipe] Update Wording (#4438) feat: show enabled extensions at top of extensions page (#4423) test recipe (#4436) Extensions loading indicator on desktop launch (#4412) removing trailing slash (#4433) [recipe cookbook] test recipe (#4431) [recipe cookbook] switching to SHA (#4429) [recipe cookbook] Update url build (#4427) [Recipe Cookbook] test recipe flow (#4426) [Recipe cookbook] Addressing GitHub api format issue (#4424) feat: integrate tool call icons with status indicators and daisy chaining (#4279)
Signed-off-by: Carine Bruyndoncx <bruyndoncx@gmail.com>
Signed-off-by: Matt Donovan <mattddonovan@protonmail.com>
Pull Request Description
Move list recipes and archive recipe logic from UI to goose server
What
/recipes/listroute to render the recipe files in the global recipe directory and current working directory/recipes/archiveroute to archive recipeNext PR
move load recipe to server side