Skip to content

Conversation

@lifeizhou-ap
Copy link
Contributor

@lifeizhou-ap lifeizhou-ap commented Oct 6, 2025

Summary

Client side to call recipes/save and recipes/parse to import and save recipe

Client side

  • call recipes/save and recipes/parse to import and save recipe
  • Passed recipeId when start a recipe session so that the recipe file can be viewed or edit in that session
  • Removed the validation for importing and saving recipe on the client side. Validation errors are returned from server side and are shown in the toast on client side
  • Removed "title" (Not the recipe title), "name" and "save location" when saving recipe
  • Removed the "Save recipe" in Recipe session at the bottom of the menu
  • Removed recipeStorage

Server side

  • Applied recipe validation in recipes/save and recipes/parse endpoints
  • Refactored recipe validation so that both CLI and Desktop can use the same validation.
  • Removed is_global in save_recipe request
  • Removed name from list recipe response
  • Changed the way to handle backwards compatibility of required description on extension. Reason: current implementation does work well to get the error location of invalid recipe, because we load the recipe as a Value first, update the extension description to "" and parse the Value to recipe. so the validation error loses the lines where the error occurs.

Type of Change

  • Feature
  • Bug fix
  • Refactor / Code quality
  • Performance improvement
  • Documentation
  • Tests
  • Security fix
  • Build / Release
  • Other (specify below)

Testing

unit test and manual testing

* main:
  fix: check server is ready so that we can decode the recipe deeplink (#5021)
  fix: backwards compatible parsing recipe file (#5020)
* main:
  use agent manager for subagent (#4828)
  fix: improve Windows OS detection in CLI installation script (#4928)
  Make it startable from playwright and also isolate (#5016)
  Fix linux deeplinks not working (#5041)
  docs: embed more videos (#5042)
  Display extension install notes in "Add custom extension" form (#5036)
  Add support for headers in extensions deeplinks (#5034)
  chore: put test in the name (#4919)
  Add new subcommand for opening recipes in desktop app (#4970)
  Update system.md with softer subagent language (#5023)
  docs: add new goose tip (#4941)
  Fix nix flake double copy (#4976)
  Upgrade electron for macOS Tahoe compatibility (#5015)
@lifeizhou-ap lifeizhou-ap requested a review from DOsinga October 7, 2025 13:37
@lifeizhou-ap lifeizhou-ap assigned amed-xyz and unassigned amed-xyz Oct 7, 2025
}
}

fn validate_recipe(recipe: &Recipe) -> Result<(), ErrorResponse> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this report specific recipe schema validation errors like the front end was doing before this change? Ideally it has more info than just recipe schema validation failed.

Copy link
Contributor Author

@lifeizhou-ap lifeizhou-ap Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @zanesq ! At moment it will show the user which attribute is missing such as title or with line number. For save recipe for files, it is clear for the user. However, for import recipe via deeplink or new recipe, it might not make sense using the line number. We can enhance later.

Copy link
Collaborator

@zanesq zanesq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Users won't see the local recipes after upgrading. Should we add migration logic to “import” the locally saved recipes into the global store or still read from local but not allow saving there anymore?

validate_recipe_template(recipe_content, None)
}

fn validate_recipe_template(recipe_content: &str, recipe_dir: Option<String>) -> Result<Recipe> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is is a lot of intermediate functions for validation, consider inlining some

}
let recipe_file = load_recipe_file(recipe_name)?;
validate_recipe_template_from_file(&recipe_file)?;
println!("{} recipe file is valid", style("✓").green().bold());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we missing the pretty errors here?

Copy link
Contributor Author

@lifeizhou-ap lifeizhou-ap Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed because the error was print here and printed again at the top level main. Now i made a change to return the error with style

reqwest = { version = "0.12.9", features = ["json", "rustls-tls", "blocking", "multipart"], default-features = false }
tokio-util = "0.7.15"
uuid = { version = "1.11", features = ["v4"] }
serde_path_to_error = "0.1.20"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh that's cool


return {
recipe: finalRecipe,
recipeId,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, we want to get rid of useRecipeManager

Copy link
Collaborator

@amed-xyz amed-xyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good overall! Let's make sure all validations are ported over from the frontend. For instance, it's currently allowing recipes with empty instructions

@lifeizhou-ap
Copy link
Contributor Author

Looking good overall! Let's make sure all validations are ported over from the frontend. For instance, it's currently allowing recipes with empty instructions

Thanks @amed-xyz ! I did not notice the instruction is optional now :) will add this into validation!

@lifeizhou-ap
Copy link
Contributor Author

Users won't see the local recipes after upgrading. Should we add migration logic to “import” the locally saved recipes into the global store or still read from local but not allow saving there anymore?

We still load the local recipe from global and local (current directory). For existing local recipes, since we know its path, when we save it, it still saves to the original path. But for any new recipes, it will save to the global.

However, it might be helpful to have some migration to move the local to global in the future. maybe better to let user know before doing it?

@zanesq
Copy link
Collaborator

zanesq commented Oct 9, 2025

Users won't see the local recipes after upgrading. Should we add migration logic to “import” the locally saved recipes into the global store or still read from local but not allow saving there anymore?

We still load the local recipe from global and local (current directory). For existing local recipes, since we know its path, when we save it, it still saves to the original path. But for any new recipes, it will save to the global.

However, it might be helpful to have some migration to move the local to global in the future. maybe better to let user know before doing it?

Ah ok then I think we are good with this as is, thanks for clarifying!

@lifeizhou-ap lifeizhou-ap merged commit 1396d31 into main Oct 9, 2025
11 checks passed
@lifeizhou-ap lifeizhou-ap deleted the lifei/use-server-side-save-import-recipe branch October 9, 2025 04:45
@alexhancock alexhancock mentioned this pull request Oct 9, 2025
michaelneale added a commit that referenced this pull request Oct 9, 2025
* main: (170 commits)
  Applied server side call to parse and save recipe (#5022)
  feat(prompt-library): add Code Documentation Migrator intermediate prompt (#4996) (#5051)
  Add Messy Column Fixer recipe (#5062)
  Cleanup temp files (#5081)
  add openmetadata recipe (#5076)
  Fix Hacktoberfest Leaderboard (#5080)
  adding brand guidelines to AGENTS.md (#4887)
  Fix: Prevent cross-contamination of cache data across analysis modes for `analyze` tool (#5075)
  fix: remove circular reference (#5018)
  Introduced a new prompt for content amplification that integrates multi-step workflows using official Goose extensions. Closes Issue #4998 (#5050)
  Add hint for focus mode when used on file paths for `analyze` tool (#5069)
  fix: use dynamic port allocation for OAuth server (#5019)
  Art vandelay: Import & Export (#5053)
  docs: misc updates for extensions directory (#5035)
  updating recipe scanner workflows for detecting recipes from forked repos (#5056)
  feat(prompt-library): add Smart Meeting Assistant advanced prompt (#4998) (#5031)
  Allow auto focus and typing while chat is initializing (#5043)
  docs(blog): Add blog for running Goose in containerized envs  (#5052)
  fix: Add WINDOWS_CODESIGN_CERTIFICATE to nightly workflow (#5037)
  Developer `analyze` tool improvement (#5030)
  ...
tlongwell-block added a commit that referenced this pull request Oct 10, 2025
* origin/main:
  Improve Rust analysis output for `analyze` tool (#5072)
  Remove duplicate prepare_reply_context call (#5063)
  install react dev tools in development (#4979)
  Doc: Added powershell installation link to the guide (#5012)
  draft of new blog post about automating more automation (#5038)
  Subagent extension selection behavior fix (#5093)
  Add dev and alpha environment indicator (#5092)
  docs: add content carousel (#5086)
  Applied server side call to parse and save recipe (#5022)
  feat(prompt-library): add Code Documentation Migrator intermediate prompt (#4996) (#5051)
  Add Messy Column Fixer recipe (#5062)
  Cleanup temp files (#5081)
  add openmetadata recipe (#5076)
  Fix Hacktoberfest Leaderboard (#5080)
  adding brand guidelines to AGENTS.md (#4887)
  Fix: Prevent cross-contamination of cache data across analysis modes for `analyze` tool (#5075)
  fix: remove circular reference (#5018)
  Introduced a new prompt for content amplification that integrates multi-step workflows using official Goose extensions. Closes Issue #4998 (#5050)
  Add hint for focus mode when used on file paths for `analyze` tool (#5069)
  fix: use dynamic port allocation for OAuth server (#5019)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants