refactor: clean up client side code for creating schedule#6805
refactor: clean up client side code for creating schedule#6805lifeizhou-ap merged 11 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors schedule creation so the desktop UI sends a parsed Recipe object to the server (instead of a client-generated recipe_source path), centralizing recipe validation/YAML persistence on the backend and deduplicating recipe parsing helpers in the UI.
Changes:
- Update schedule creation API contract from
recipe_source: stringtorecipe: Recipeacross UI types, OpenAPI, and server route. - Move recipe file/deeplink parsing into shared
ui/desktop/src/recipe/index.tshelpers and use them from schedule/import flows. - Server now validates/sanitizes recipes and writes them to the scheduled recipes directory before scheduling.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/desktop/src/schedule.ts | Adjusts client schedule creation request payload to send a Recipe object. |
| ui/desktop/src/recipe/index.ts | Adds shared helpers for parsing recipes from files and deeplinks via API/utilities. |
| ui/desktop/src/components/schedule/ScheduleModal.tsx | Switches schedule creation flow to parse recipes locally and submit Recipe to API. |
| ui/desktop/src/components/recipes/ImportRecipeForm.tsx | Removes duplicated parsing helpers and reuses shared recipe parsing utilities. |
| ui/desktop/src/api/types.gen.ts | Updates generated API types for schedule creation to accept recipe. |
| ui/desktop/openapi.json | Updates OpenAPI schema for schedule creation request to require recipe. |
| crates/goose-server/src/routes/schedule.rs | Updates schedule creation endpoint to accept Recipe, validate it, persist YAML, and schedule the job. |
| let recipe_path = scheduled_recipes_dir.join(format!("{}.yaml", req.id)); | ||
| let yaml_content = req |
There was a problem hiding this comment.
req.id is used directly to construct a filename ({id}.yaml), which allows path separators / .. and can lead to path traversal or invalid filenames; validate/sanitize the schedule id to a safe character set and reject anything else before building recipe_path.
There was a problem hiding this comment.
added the request id validation
| fs::write(&recipe_path, yaml_content) | ||
| .map_err(|e| ErrorResponse::internal(format!("Failed to save recipe file: {}", e)))?; |
There was a problem hiding this comment.
The recipe file is written to disk before add_scheduled_job runs, so if the scheduler rejects the request (e.g., JobIdExists or cron parse error) the existing recipe file can be overwritten and/or a new orphaned file will be left behind; only persist the file after the scheduler has accepted the schedule (or write to a temp path and atomically rename/clean up on error).
There was a problem hiding this comment.
it is fine, it is an edge case and saving the files in the schedule folder is ok.
| fs::write(&recipe_path, yaml_content) | ||
| .map_err(|e| ErrorResponse::internal(format!("Failed to save recipe file: {}", e)))?; |
There was a problem hiding this comment.
std::fs::write is a blocking filesystem call inside an async Axum handler; switch to tokio::fs (or spawn_blocking) to avoid blocking the runtime under load.
| if (value.trim()) { | ||
| try { | ||
| const recipe = await parseDeepLink(value.trim()); | ||
| const recipe = await parseDeeplink(value.trim()); | ||
| if (recipe) { |
There was a problem hiding this comment.
parseDeeplink only supports goose://recipe?config=... links, but this modal still presents/validates deeplinks as goose://bot or goose://recipe; either add goose://bot support in the parser or update the modal copy/validation so users aren’t told a format will work when it won’t.
There was a problem hiding this comment.
this is only supports recipe deeplink. so it is fine
* main: docs: usage data collection (#6822) feat: platform extension migrator + code mode rename (#6611) feat: CLI flag to skip loading profile extensions (#6780) Swap canonical model from openrouter to models.dev (#6625) Hook thinking status (#6815) Fetch new skills hourly (#6814) copilot instructions: Update "No prerelease docs" instruction (#6795) refactor: centralize audience filtering before providers receive messages (#6728) update doc to remind contributors to activate hermit and document minimal npm and node version (#6727) nit: don't spit out compaction when in term mode as it fills up the screen (#6799) fix: correct tool support detection in Tetrate provider model fetching (#6808) Session manager fixes (#6809) fix(desktop): handle quoted paths with spaces in extension commands (#6430) fix: we can default gooseignore without writing it out (#6802) fix broken link (#6810) docs: add Beads MCP extension tutorial (#6792) feat(goose): add support for AWS_BEARER_TOKEN_BEDROCK environment variable (#6739)
| fs::write(&recipe_path, yaml_content) | ||
| .await | ||
| .map_err(|e| ErrorResponse::internal(format!("Failed to save recipe file: {}", e)))?; |
There was a problem hiding this comment.
fs::write(&recipe_path, ...) will overwrite an existing scheduled recipe file (and can leave an orphan file) if add_scheduled_job later fails (e.g., JobIdExists or invalid cron); consider checking for an existing job/file first and/or writing to a temp file and cleaning it up on scheduler failure.
| let is_valid = !id.is_empty() | ||
| && id | ||
| .chars() | ||
| .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_' || c == ' '); | ||
|
|
||
| if !is_valid { | ||
| return Err(ErrorResponse::bad_request( | ||
| "Schedule name must use only alphanumeric characters, hyphens, underscores, or spaces" |
There was a problem hiding this comment.
validate_schedule_id allows leading/trailing spaces, which can create hard-to-reference schedule IDs (and awkward filenames like "foo .yaml"); consider trimming id before validation and/or rejecting IDs that start/end with whitespace.
| let is_valid = !id.is_empty() | |
| && id | |
| .chars() | |
| .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_' || c == ' '); | |
| if !is_valid { | |
| return Err(ErrorResponse::bad_request( | |
| "Schedule name must use only alphanumeric characters, hyphens, underscores, or spaces" | |
| // Reject empty or all-whitespace IDs, and disallow leading/trailing whitespace. | |
| let trimmed = id.trim(); | |
| let is_valid = !trimmed.is_empty() | |
| && trimmed.len() == id.len() | |
| && id | |
| .chars() | |
| .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_' || c == ' '); | |
| if !is_valid { | |
| return Err(ErrorResponse::bad_request( | |
| "Schedule name must use only alphanumeric characters, hyphens, underscores, or spaces, and must not start or end with whitespace" |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
ui/desktop/src/components/schedule/ScheduleModal.tsx:81
- The UI text suggests
goose://botlinks are supported, butparseDeeplinkonly acceptsgoose://recipe?config=..., so users will see confusing validation errors for bot links. Either update the placeholder/error messages to only mentiongoose://recipe?config=...or extend the parser to acceptgoose://bot?...as well.
const recipe = await parseDeeplink(value.trim());
if (!recipe) throw new Error();
setParsedRecipe(recipe);
if (recipe.title) {
setScheduleIdFromTitle(recipe.title);
}
} catch {
setParsedRecipe(null);
setInternalValidationError('Invalid deep link. Please use a goose://recipe link.');
}
} else {
setParsedRecipe(null);
}
}, []);
useEffect(() => {
if (isOpen) {
| } | ||
| } | ||
| } else { | ||
| setInternalValidationError('Invalid file type: Please select a YAML file (.yaml or .yml)'); | ||
| } |
There was a problem hiding this comment.
handleBrowseFile awaits parseRecipeFromFile without handling failures; if the file read fails or parsing throws, the modal can end up with an unhandled rejection and no user-facing error. Wrap the read/parse in try/catch and set internalValidationError (and/or clear parsedRecipe) when parsing fails.
There was a problem hiding this comment.
I think this one might be valid @lifeizhou-ap if you want to take a look?
There was a problem hiding this comment.
Thanks @zanesq for the review! I've added the error handling now
| let id = id.trim(); | ||
| let is_valid = !id.is_empty() | ||
| && id |
There was a problem hiding this comment.
validate_schedule_id trims the input for validation but the untrimmed req.id is later used as the schedule ID and filename, which allows leading/trailing spaces (and can create problematic filenames, especially on Windows). Consider rejecting leading/trailing whitespace or returning/using the trimmed value when creating the job and recipe path.
| let id = id.trim(); | |
| let is_valid = !id.is_empty() | |
| && id | |
| let trimmed = id.trim(); | |
| let has_no_leading_or_trailing_whitespace = trimmed == id; | |
| let is_valid = !trimmed.is_empty() | |
| && has_no_leading_or_trailing_whitespace | |
| && trimmed |
* main: fix: fixed the broken release (#6887) feat: Streamable HTTP transport for ACP + goose-acp usage (#6741) Add Laminar for Observability (#6514) Missed a couple of places that hard code J for the newline key (#6853) fix(ui): preserve working directory when creating new chat (#6789) blog: add 5 tips for building MCP Apps that work (#6855) docs: session isolation (#6846) upgrade react and electron to latest (#6845) Fix: Small update UI settings prompt injection (#6830) Remove autogenerated .gooseignore files that don't belong in repo (#6824) Fix case-insensitive matching for builtin extension names (#6825) docs: cli newline keybinding (#6823) Update version to 1.22.0 (#6821) Refactor: move persisting extension to session outside of route (#6685) acp: load configured extensions and refactor tests (#6803)
| let scheduled_recipes_dir = get_default_scheduled_recipes_dir().map_err(|e| { | ||
| ErrorResponse::internal(format!("Failed to get scheduled recipes directory: {}", e)) | ||
| })?; | ||
|
|
||
| let recipe_path = scheduled_recipes_dir.join(format!("{}.yaml", id)); | ||
| let yaml_content = req | ||
| .recipe | ||
| .to_yaml() | ||
| .map_err(|e| ErrorResponse::internal(format!("Failed to convert recipe to YAML: {}", e)))?; | ||
| fs::write(&recipe_path, yaml_content) | ||
| .await | ||
| .map_err(|e| ErrorResponse::internal(format!("Failed to save recipe file: {}", e)))?; | ||
|
|
||
| let job = ScheduledJob { | ||
| id: req.id, | ||
| source: req.recipe_source, | ||
| id, | ||
| source: recipe_path.to_string_lossy().into_owned(), | ||
| cron: req.cron, | ||
| last_run: None, | ||
| currently_running: false, | ||
| paused: false, | ||
| current_session_id: None, | ||
| process_start_time: None, | ||
| }; | ||
|
|
||
| let scheduler = state.scheduler(); | ||
| scheduler | ||
| .add_scheduled_job(job.clone(), true) | ||
| .add_scheduled_job(job.clone(), false) | ||
| .await |
There was a problem hiding this comment.
The recipe YAML is written to {scheduled_recipes_dir}/{id}.yaml before add_scheduled_job checks for duplicate IDs, so a request with an existing id (or one that later fails cron validation) can overwrite an existing schedule’s recipe file or leave an orphaned file. Consider checking for JobIdExists before writing and/or writing to a temp file and only renaming after add_scheduled_job succeeds (and cleaning up the temp file on error).
Signed-off-by: Harrison <hcstebbins@gmail.com>
Summary
Type of Change
AI Assistance
Testing
Related Issues
Relates to #ISSUE_ID
Discussion: LINK (if any)
Screenshots/Demos (for UX changes)
Before:
After: