fix(workflows): validate YAML on import so misplaced keys aren't silently dropped (#6274) - #6647
Open
DPS0340 wants to merge 1 commit into
Open
fix(workflows): validate YAML on import so misplaced keys aren't silently dropped (#6274)#6647DPS0340 wants to merge 1 commit into
DPS0340 wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6274.
What was wrong
Importing a workflow YAML where
with:is a sibling ofprovider:instead of nested inside it saved successfully and then ran with zero parameters — no warning, no validation error. Users hit it asHttpProvider._notify() missing 2 required positional argumentsorMessage is requiredat runtime, with nothing pointing at the YAML.The import handler in
workflow-builder-widget.tsxcalledparseWorkflowYamlStringToJSON, which is a bareyaml.parse()— its own comment says// todo: use zod schema to parse and have type safety. Nothing validated the shape.Meanwhile every consumer reads
provider.with:_parse_steps→_step.get("provider", {}).get("with")(keep/parser/parser.py)_parse_actions→provider.get("with", {})(same file)getV2StepOrV2Action→actionOrStep.provider?.withSo a step-level
with:is unreachable everywhere. I verified this directly rather than assuming — parsing the reported YAML gives:The fix
Use the validating parser that already exists in the codebase.
parseWorkflowYamlToJSONappliesYamlWorkflowDefinitionSchema, whose step schema is.strict(), so an unrecognised key at step level is rejected:The existing
catchalready routes toshowErrorToast, so the user now gets an actionable message at import time instead of a broken workflow.No new helper and no new convention:
parseWorkflowYamlToJSONandfromZodErrorare both already used together inkeep-ui/scripts/validate-workflow-examples.ts, andzod-validation-erroris already a dependency.Scope
Only the import path in
BuilderWorkflowYAMLImport. I deliberately did not touch the backend parser or add a migration — those are worth doing but are separate changes with wider blast radius, and this is the one that stops the silent data loss at the point of entry.I also left
parseWorkflowYamlStringToJSONin place; it has other callers, and changing it would affect paths I haven't verified.Tests
keep-ui/widgets/workflow-builder/__tests__/workflow-import-validation.test.ts— pins the import handler's validation:with:sibling ofprovider:withwith:nestedprovider.withisundefinedwhile the params sit at step level — shows why accepting it is harmfulPlus two cases in the existing
parseWorkflowYamlToJSON.test.tscovering the schema itself.The second row of each pair is a guard rail: a change that simply rejected more would break them.
Worth noting — that guard rail already earned its place. My first version of the "accepts nested" fixture failed because it was missing a required
description, not because of the misplacement. Without the positive case I'd have shipped a test that proved nothing.Verification
npx jest widgets/workflow-builder entities/workflows: 161 passed / 18 suites, up from 158 before.npx tsc --noEmitreports no errors in the touched files.