From 3b01a08c50b744e06f4efb8ef9d0eaa26948da35 Mon Sep 17 00:00:00 2001 From: Toocky Date: Sat, 11 Apr 2026 10:28:19 -0600 Subject: [PATCH 01/19] Improve workflow dependency resolution error surfacing When the resolution loop stalls, report every unresolved step individually with its kind, ref, ID, and the specific unresolved child/parent step IDs it is waiting for, instead of a single generic 'failed to resolve workflow step dependencies' error pointing at an arbitrary step. --- .../automation/service/workflow_converter.go | 45 ++++++++++++++----- 1 file changed, 33 insertions(+), 12 deletions(-) diff --git a/server/automation/service/workflow_converter.go b/server/automation/service/workflow_converter.go index 06f3dbd305..f492ccfafc 100644 --- a/server/automation/service/workflow_converter.go +++ b/server/automation/service/workflow_converter.go @@ -44,10 +44,9 @@ func Convert(wfService *workflow, wf *types.Workflow) (*wfexec.Graph, types.Work // Converts workflow definition to wf execution graph func (svc workflowConverter) makeGraph(def *types.Workflow) (*wfexec.Graph, types.WorkflowIssueSet) { var ( - g = wfexec.NewGraph() - wfii = types.WorkflowIssueSet{} - IDs = make(map[uint64]int) - lastResStep *types.WorkflowStep + g = wfexec.NewGraph() + wfii = types.WorkflowIssueSet{} + IDs = make(map[uint64]int) ) // Basic step verification @@ -80,10 +79,8 @@ func (svc workflowConverter) makeGraph(def *types.Workflow) (*wfexec.Graph, type for g.Len() < len(ss) { progress := false - lastResStep = nil for _, step := range ss { - lastResStep = step if g.StepByID(step.ID) != nil { // resolved continue @@ -118,13 +115,37 @@ func (svc workflowConverter) makeGraph(def *types.Workflow) (*wfexec.Graph, type } if !progress { - var culprit = make(map[string]int) - if lastResStep != nil { - culprit = map[string]int{"step": IDs[lastResStep.ID]} - } + // nothing resolved for 1 cycle — report every unresolved step + // individually with details about what it is waiting for. + for _, step := range def.Steps { + if g.StepByID(step.ID) != nil { + continue + } + + var ( + unresolvedChildren []uint64 + unresolvedParents []uint64 + ) - // nothing resolved for 1 cycle - wfii = wfii.Append(fmt.Errorf("failed to resolve workflow step dependencies"), culprit) + for _, path := range def.Paths { + if path.ParentID == step.ID && g.StepByID(path.ChildID) == nil { + unresolvedChildren = append(unresolvedChildren, path.ChildID) + } + if path.ChildID == step.ID && g.StepByID(path.ParentID) == nil { + unresolvedParents = append(unresolvedParents, path.ParentID) + } + } + + culprit := map[string]int{"step": IDs[step.ID]} + wfii = wfii.Append(fmt.Errorf( + "failed to resolve workflow step dependencies for %s step (ref: %q, ID: %d): waiting for unresolved child step(s) [IDs: %v] and/or unresolved parent step(s) [IDs: %v]", + step.Kind, + step.Ref, + step.ID, + unresolvedChildren, + unresolvedParents, + ), culprit) + } break } } From 19c7a63720b103027205ff3ae106885ae9ebbdbe Mon Sep 17 00:00:00 2001 From: Toocky Date: Sat, 11 Apr 2026 10:33:47 -0600 Subject: [PATCH 02/19] update gitignore --- .gitignore | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index faf7cc95a3..64af6d0ab9 100644 --- a/.gitignore +++ b/.gitignore @@ -9,4 +9,6 @@ *.sw* .run node_modules -ignore \ No newline at end of file +ignore +.kilo* +.claude* From c12e6cf0b9b7e1d0f6299ebfae7beeb997bcf2fe Mon Sep 17 00:00:00 2001 From: Toocky Date: Sat, 11 Apr 2026 12:20:32 -0600 Subject: [PATCH 03/19] Extend workflow error step with structured title/body/severity Issue #1905: error step now supports optional title, body and severity arguments alongside the legacy message argument. Fields are attached to the returned KindAutomation error via the errors package meta bag under the workflow.error.* namespace. The errors package http.go is updated to preserve full error details (message + meta) for KindAutomation errors regardless of mask state, since workflow error steps are user-authored and explicitly safe to surface to the client. Backwards compatible: workflows configured with only 'message' produce identical wire output. Frontend rendering changes land separately. --- .../automation/service/workflow_converter.go | 116 +++++++++++++++--- server/pkg/errors/http.go | 9 +- 2 files changed, 107 insertions(+), 18 deletions(-) diff --git a/server/automation/service/workflow_converter.go b/server/automation/service/workflow_converter.go index f492ccfafc..861ed34426 100644 --- a/server/automation/service/workflow_converter.go +++ b/server/automation/service/workflow_converter.go @@ -422,37 +422,102 @@ func (svc workflowConverter) convFunctionStep(g *wfexec.Graph, s *types.Workflow // creates error step // -// Expects ZERO outgoing paths and +// Expects ZERO outgoing paths. +// +// Supported arguments: +// - message (required, string) backwards-compatible flat error message +// - title (optional, string) short headline for rich toast rendering +// - body (optional, string) long-form body, overrides message for display +// - severity (optional, string) one of "error" (default), "warning", "info" +// +// title/body/severity are attached to the resulting error's meta under +// the "workflow.error.*" namespace and surfaced to the frontend via the +// errors package's JSON marshaller. The error's flat message stays +// equal to body (or message as fallback) so older clients still render +// a sensible string. func (svc workflowConverter) convErrorStep(s *types.WorkflowStep) (wfexec.Step, error) { const ( - argName = "message" + argMessage = "message" + argTitle = "title" + argBody = "body" + argSeverity = "severity" + + metaTitle = "workflow.error.title" + metaBody = "workflow.error.body" + metaSeverity = "workflow.error.severity" + + severityError = "error" + severityWarning = "warning" + severityInfo = "info" ) var ( args = types.ExprSet(s.Arguments) ) + // resolve a single string argument either from the evaluated scope or + // from a literal value on the unevaluated arg set; returns "" if absent. + resolveStr := func(result *expr.Vars, name string) string { + if result != nil && result.Has(name) { + if str, err := expr.NewString(expr.Must(result.Select(name))); err == nil { + return str.GetValue() + } + } + if a := args.GetByTarget(name); a != nil { + if aux, is := a.Value.(string); is { + return aux + } + } + return "" + } + return wfexec.NewGenericStep(func(ctx context.Context, r *wfexec.ExecRequest) (wfexec.ExecResponse, error) { - var ( - msg string - result, err = args.Eval(ctx, r.Scope) - ) + result, err := args.Eval(ctx, r.Scope) if err != nil { return nil, err } - if result.Has(argName) { - str, _ := expr.NewString(expr.Must(result.Select(argName))) - msg = str.GetValue() - } else { - if aux, is := args.GetByTarget(argName).Value.(string); is { - msg = aux - } else { - msg = "ERROR" - } + var ( + message = resolveStr(result, argMessage) + title = resolveStr(result, argTitle) + body = resolveStr(result, argBody) + severity = resolveStr(result, argSeverity) + ) + + // normalise severity to a known value; default to error + switch severity { + case severityWarning, severityInfo, severityError: + // keep as-is + default: + severity = severityError + } + + // pick the flat message: prefer body, fall back to message, + // fall back to a literal "ERROR" so we never emit an empty error + flat := body + if flat == "" { + flat = message + } + if flat == "" { + flat = "ERROR" + } + + e := errors.Automation("%s", flat) + + // only attach meta when at least one rich field is set; this + // keeps the wire format identical to the legacy behaviour for + // workflows that only configure `message`. + if title != "" { + e = e.Apply(errors.Meta(metaTitle, title)) + } + if body != "" { + e = e.Apply(errors.Meta(metaBody, body)) + } + if title != "" || body != "" || severity != severityError { + e = e.Apply(errors.Meta(metaSeverity, severity)) } - return nil, errors.Automation("%s", msg) + return nil, e }), nil } @@ -810,9 +875,26 @@ func verifyStep(s *types.WorkflowStep, in, out types.WorkflowPathSet) types.Work checks = append(checks, gatewayCheck(zero(arguments), zero(results))...) case types.WorkflowStepKindError: + // Error step supports four optional string arguments: + // message (legacy, still required for backwards compatibility) + // title (optional, rich toast headline) + // body (optional, rich toast body — overrides message for display) + // severity (optional, "error"|"warning"|"info") checks = append(checks, requiredArg("message", expr.String{}), - count(0, 1, arguments), + checkArg("title", expr.String{}), + checkArg("body", expr.String{}), + checkArg("severity", expr.String{}), + count(1, 4, arguments), + func() error { + allowed := map[string]bool{"message": true, "title": true, "body": true, "severity": true} + for _, a := range s.Arguments { + if !allowed[a.Target] { + return errors.Internal("%s step does not accept argument %q (allowed: message, title, body, severity)", s.Kind, a.Target) + } + } + return nil + }, zero(results), last, ) diff --git a/server/pkg/errors/http.go b/server/pkg/errors/http.go index 0270750fe6..90b5b82b1c 100644 --- a/server/pkg/errors/http.go +++ b/server/pkg/errors/http.go @@ -139,7 +139,14 @@ func writeHttpJSON(ctx context.Context, w io.Writer, err error, mask bool) { return } - if se, is := err.(interface{ Safe() bool }); !is || !se.Safe() || mask { + // Automation (workflow) errors are user-authored via workflow error + // steps and are explicitly safe to surface in full (message + meta) + // to clients regardless of mask state. This allows the workflow error + // step to carry structured fields (title, body, severity, ...) through + // to the frontend. + if e, isAutomationErr := err.(*Error); isAutomationErr && e.kind == KindAutomation { + // preserve full error + } else if se, is := err.(interface{ Safe() bool }); !is || !se.Safe() || mask { // trim error details when not debugging or error is not safe or maske err = errors.New(err.Error()) } From 371528bf7f7205ddae19820bfc1e0809fe12aae8 Mon Sep 17 00:00:00 2001 From: Toocky Date: Sat, 11 Apr 2026 12:22:20 -0600 Subject: [PATCH 04/19] Add title/body/severity fields to workflow error step configurator Issue #1905: expose the new rich error step arguments (title, body, severity) alongside the legacy message field in the error step configurator. The canvas node label now prefers title > body > message when building its preview. Config.arguments is normalised on load so all four targets are always present (with empty exprs), keeping v-model bindings stable even for legacy workflows that only have message. --- .../src/components/Configurator/Error.vue | 166 +++++++++++++++--- .../en/corteza-webapp-workflow/general.yaml | 17 ++ 2 files changed, 155 insertions(+), 28 deletions(-) diff --git a/client/web/workflow/src/components/Configurator/Error.vue b/client/web/workflow/src/components/Configurator/Error.vue index 0a4124c2b9..2f032f335e 100644 --- a/client/web/workflow/src/components/Configurator/Error.vue +++ b/client/web/workflow/src/components/Configurator/Error.vue @@ -15,17 +15,62 @@ + + + + + + + + + + + + + + + + @@ -60,6 +105,13 @@ import base from './base' import ExpressionEditor from '../ExpressionEditor.vue' +const ALLOWED_TARGETS = ['message', 'title', 'body', 'severity'] +const SEVERITY_VALUES = ['error', 'warning', 'info'] + +function makeArg (target, expr = '') { + return { target, type: 'String', expr } +} + export default { components: { ExpressionEditor, @@ -71,53 +123,111 @@ export default { return { expressionEditor: { currentExpression: undefined, + currentField: undefined, }, } }, + computed: { + messageArg () { + return this.findOrCreateArg('message') + }, + titleArg () { + return this.findOrCreateArg('title') + }, + bodyArg () { + return this.findOrCreateArg('body') + }, + severityArg () { + return this.findOrCreateArg('severity') + }, + + severityValue: { + get () { + // severity is stored as a quoted string expression; parse it back + const raw = (this.severityArg.expr || '').trim() + if (!raw) return 'error' + const m = raw.match(/^["'](error|warning|info)["']$/) + return m ? m[1] : 'error' + }, + set (v) { + if (!SEVERITY_VALUES.includes(v)) v = 'error' + const arg = this.severityArg + this.$set(arg, 'expr', `"${v}"`) + }, + }, + + severityOptions () { + return [ + { value: 'error', text: this.$t('general:error-step.severity.options.error') }, + { value: 'warning', text: this.$t('general:error-step.severity.options.warning') }, + { value: 'info', text: this.$t('general:error-step.severity.options.info') }, + ] + }, + }, + created () { - let args = [{ - target: 'message', - type: 'String', - expr: '', - }] - - if (this.item.config.arguments && this.item.config.arguments.length) { - args = this.item.config.arguments.map(({ target, type, value, expr }) => { - return { - target, - type, - expr: expr || (value ? `"${value}"` : ''), - } - }) - } + // Normalise config.arguments: keep only allowed targets, ensure all four + // exist (even if empty) so v-model bindings are stable. + const existing = Array.isArray(this.item.config.arguments) ? this.item.config.arguments : [] + const byTarget = {} + existing.forEach(({ target, type, value, expr }) => { + if (!ALLOWED_TARGETS.includes(target)) return + byTarget[target] = { + target, + type: type || 'String', + expr: expr || (value ? `"${value}"` : ''), + } + }) + const args = ALLOWED_TARGETS.map(t => byTarget[t] || makeArg(t)) this.$set(this.item.config, 'arguments', args) }, methods: { - valueChanged (value) { + findOrCreateArg (target) { + const args = this.item.config.arguments || [] + let arg = args.find(a => a.target === target) + if (!arg) { + arg = makeArg(target) + args.push(arg) + this.$set(this.item.config, 'arguments', args) + } + return arg + }, + + onFieldInput () { + const preview = (this.titleArg.expr || this.bodyArg.expr || this.messageArg.expr || '').trim() this.$emit('update-default-value', { - value: `Stop workflow with error: ${value}`, + value: preview ? `Stop workflow with error: ${preview}` : 'Stop workflow with error', force: !this.item.node.value, }) this.$root.$emit('change-detected') }, - openInEditor () { - this.expressionEditor.currentExpression = this.item.config.arguments[0].expr + onSeverityChange () { + this.$root.$emit('change-detected') }, - saveExpression () { - const { currentExpression } = this.expressionEditor - this.$set(this.item.config.arguments[0], 'expr', currentExpression) - this.$root.$emit('change-detected') + openInEditor (field) { + const arg = this.findOrCreateArg(field) + this.expressionEditor.currentField = field + this.expressionEditor.currentExpression = arg.expr || '' + }, + saveExpression () { + const { currentExpression, currentField } = this.expressionEditor + if (currentField) { + const arg = this.findOrCreateArg(currentField) + this.$set(arg, 'expr', currentExpression) + this.$root.$emit('change-detected') + } this.resetExpression() }, resetExpression () { this.expressionEditor.currentExpression = undefined + this.expressionEditor.currentField = undefined }, }, } diff --git a/locale/en/corteza-webapp-workflow/general.yaml b/locale/en/corteza-webapp-workflow/general.yaml index 6b25c06f74..d5094d5c98 100644 --- a/locale/en/corteza-webapp-workflow/general.yaml +++ b/locale/en/corteza-webapp-workflow/general.yaml @@ -5,6 +5,23 @@ enable: Enable disable: Disable error: Error error-expression: Error message (expression) +error-step: + message: + label: Message (legacy, required) + description: Plain fallback message shown by older clients and as the flat error string. + title: + label: Title + description: Short headline shown in rich toast notifications. Optional. + body: + label: Body + description: Longer body text shown to the end user. Overrides message for display when set. Optional. + severity: + label: Severity + description: Controls the toast variant and icon in the end-user UI. + options: + error: Error + warning: Warning + info: Info export: Export general: General handle: Handle From 449ae128a23f1d4d90a341e48db7636143f76883 Mon Sep 17 00:00:00 2001 From: Toocky Date: Sat, 11 Apr 2026 12:23:40 -0600 Subject: [PATCH 05/19] Surface structured workflow error step output in shared toast mixin Issue #1905: detect the namespaced workflow.error.{title,body,severity} meta keys attached by the backend error step and render a rich toast with title/body/variant when present. The detection probes a handful of error-shape variants (direct reject, axios wrap, nested response.data.error) and returns null for anything that does not look like a workflow error step payload. When null, or when the rich rendering branch throws, the handler falls through to the existing legacy plain-toast path - so non-workflow errors and legacy workflow errors render exactly as they do today. --- lib/vue/src/mixins/toast.js | 60 +++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/lib/vue/src/mixins/toast.js b/lib/vue/src/mixins/toast.js index 27de040d46..6e7c294179 100644 --- a/lib/vue/src/mixins/toast.js +++ b/lib/vue/src/mixins/toast.js @@ -32,6 +32,42 @@ export default { return err.message }, + // extractWorkflowErrorMeta looks for the namespaced "workflow.error.*" + // meta keys attached by the backend workflow error step. It probes the + // handful of shapes the error can arrive in (direct reject, axios wrap, + // nested response.data.error) and returns a normalised object, or null + // when the error is not a workflow error step error. + // + // This is purely additive: when it returns null, toastErrorHandler + // falls through to the existing plain-toast behaviour. + extractWorkflowErrorMeta (err) { + if (!err || typeof err !== 'object') return null + + // candidate meta bags, in priority order + const candidates = [ + err.meta, + err.response && err.response.data && err.response.data.error && err.response.data.error.meta, + err.data && err.data.error && err.data.error.meta, + err.error && err.error.meta, + ] + + for (const meta of candidates) { + if (!meta || typeof meta !== 'object') continue + const title = meta['workflow.error.title'] + const body = meta['workflow.error.body'] + const severity = meta['workflow.error.severity'] + if (title || body || severity) { + return { + title: title || '', + body: body || '', + severity: severity || 'error', + } + } + } + + return null + }, + toastErrorHandler (opt) { if (typeof opt === 'string') { opt = { title: opt } @@ -40,6 +76,30 @@ export default { const { prefix, title } = opt return (err = {}) => { + // Rich workflow error step path. Guarded by a try/catch so that + // any unexpected toast library or context issue silently degrades + // to the legacy plain-toast rendering below. + try { + const wfErr = this.extractWorkflowErrorMeta(err) + if (wfErr) { + const variant = wfErr.severity === 'warning' + ? 'warning' + : wfErr.severity === 'info' + ? 'info' + : 'danger' + + // Prefer the rich body; fall back to the flat error message + // (which the backend also sets to body when available). + const body = wfErr.body || err.message || '' + const toastTitle = wfErr.title || title || '' + + this.toast(body, { title: toastTitle, variant, solid: true }) + return err.message + } + } catch (_) { + // fall through to legacy rendering + } + let toastMsg = '' let toastTitle = title From 0e23d13ebe8e69aefe169d0c5ef8a2ae1bd1c811 Mon Sep 17 00:00:00 2001 From: Toocky Date: Sat, 11 Apr 2026 12:26:09 -0600 Subject: [PATCH 06/19] Add parameter context to workflow step verification errors Issue #1725: workflow validation errors previously omitted which argument or result caused the failure, making them hard to debug. - parseExpressionsAs now wraps each parser/type/test failure with the category (argument vs result), the target name, the failing expression (truncated to 80 chars), and the lifecycle stage. - requiredArg includes the accepted type(s) when reporting a missing argument so designers know what to put in the field. - checkArg includes the list of accepted types when the value type is wrong, instead of just naming the unexpected type. - workflowStepDefConv wrapper messages now lead with the step kind and ref so the offending step is identifiable at a glance. All existing test fixtures continue to work since the wrapped errors remain *errors.Internal under the hood. --- .../automation/service/workflow_converter.go | 80 ++++++++++++++++--- 1 file changed, 69 insertions(+), 11 deletions(-) diff --git a/server/automation/service/workflow_converter.go b/server/automation/service/workflow_converter.go index 861ed34426..e3ecb8c045 100644 --- a/server/automation/service/workflow_converter.go +++ b/server/automation/service/workflow_converter.go @@ -183,12 +183,12 @@ func (svc workflowConverter) makeGraph(def *types.Workflow) (*wfexec.Graph, type // // if this func returns nil for step and error, assume unresolved dependencies func (svc workflowConverter) workflowStepDefConv(g *wfexec.Graph, def *types.Workflow, s *types.WorkflowStep, in, out []*types.WorkflowPath) (bool, error) { - if err := svc.parseExpressions(s.Arguments...); err != nil { - return false, errors.Internal("failed to parse step arguments expressions for %s: %s", s.Kind, err).Wrap(err) + if err := svc.parseExpressionsAs("argument", s.Arguments...); err != nil { + return false, errors.Internal("invalid %s step (ref: %q): %s", s.Kind, s.Ref, err).Wrap(err) } - if err := svc.parseExpressions(s.Results...); err != nil { - return false, errors.Internal("failed to parse step results expressions for %s: %s", s.Kind, err).Wrap(err) + if err := svc.parseExpressionsAs("result", s.Results...); err != nil { + return false, errors.Internal("invalid %s step (ref: %q): %s", s.Kind, s.Ref, err).Wrap(err) } conv, err := func() (wfexec.Step, error) { @@ -685,21 +685,40 @@ func (svc workflowConverter) convExecWorkflowStep(wf *types.Workflow, s *types.W } func (svc workflowConverter) parseExpressions(ee ...*types.Expr) (err error) { - for _, e := range ee { + return svc.parseExpressionsAs("", ee...) +} + +// parseExpressionsAs is parseExpressions with a human-readable category +// (e.g. "argument" or "result") used to enrich error messages so failures +// say which target failed and why instead of leaking a bare parser error. +// +// Issue #1725: previously a parse error returned bare "syntax error near +// X" with no indication of which step argument it referred to. Now the +// error includes the target name, the failing expression (truncated) +// and which lifecycle stage failed (parse / type / test parse). +func (svc workflowConverter) parseExpressionsAs(category string, ee ...*types.Expr) (err error) { + wrap := func(target, stage string, inner error) error { + excerpt := exprExcerpt(ee, target) + if category == "" { + return fmt.Errorf("%s %q (%s): %w", stage, target, excerpt, inner) + } + return fmt.Errorf("%s %q %s (%s): %w", category, target, stage, excerpt, inner) + } + for _, e := range ee { if len(strings.TrimSpace(e.Expr)) > 0 { if err = svc.parser.ParseEvaluators(e); err != nil { - return + return wrap(e.Target, "failed to parse expression", err) } } if err = e.SetType(exprTypeSetter(svc.reg, e)); err != nil { - return err + return wrap(e.Target, "failed to resolve type", err) } - for _, t := range e.Tests { + for ti, t := range e.Tests { if err = svc.parser.ParseEvaluators(t); err != nil { - return + return wrap(e.Target, fmt.Sprintf("failed to parse test #%d expression", ti+1), err) } } } @@ -707,6 +726,28 @@ func (svc workflowConverter) parseExpressions(ee ...*types.Expr) (err error) { return nil } +// exprExcerpt returns a short, single-line snippet of the expression for +// the named target, suitable for embedding in an error message. Long +// expressions are truncated with an ellipsis to keep messages readable. +func exprExcerpt(ee []*types.Expr, target string) string { + for _, e := range ee { + if e.Target != target { + continue + } + s := strings.TrimSpace(e.Expr) + s = strings.ReplaceAll(s, "\n", " ") + const max = 80 + if len(s) > max { + s = s[:max] + "…" + } + if s == "" { + return "" + } + return s + } + return "" +} + func verifyStep(s *types.WorkflowStep, in, out types.WorkflowPathSet) types.WorkflowIssueSet { const ( arguments = "argument" @@ -805,7 +846,14 @@ func verifyStep(s *types.WorkflowStep, in, out types.WorkflowPathSet) types.Work } } - return errors.Internal("unexpected type %q for argument %q on step type %q", msgArg.Type, argName, s.Kind) + accepted := make([]string, 0, len(tt)) + for _, typ := range tt { + accepted = append(accepted, typ.Type()) + } + return errors.Internal( + "%s step argument %q has unexpected type %q (accepted: %s)", + s.Kind, argName, msgArg.Type, strings.Join(accepted, ", "), + ) } } @@ -813,7 +861,17 @@ func verifyStep(s *types.WorkflowStep, in, out types.WorkflowPathSet) types.Work requiredArg = func(argName string, tt ...expr.Type) func() error { return func() error { if msgArg := types.ExprSet(s.Arguments).GetByTarget(argName); msgArg == nil { - return errors.Internal("%s step expects to have '%s' argument", s.Kind, argName) + accepted := make([]string, 0, len(tt)) + for _, typ := range tt { + accepted = append(accepted, typ.Type()) + } + if len(accepted) > 0 { + return errors.Internal( + "%s step is missing required argument %q (expected type: %s)", + s.Kind, argName, strings.Join(accepted, " or "), + ) + } + return errors.Internal("%s step is missing required argument %q", s.Kind, argName) } return checkArg(argName, tt...)() From ae446b1d606171261eef84ea2905919235cfdd7d Mon Sep 17 00:00:00 2001 From: Toocky Date: Sat, 11 Apr 2026 12:28:09 -0600 Subject: [PATCH 07/19] Handle workflow save failures gracefully Issue #687: the workflow save handler previously crashed with 'Cannot read property workflowID of undefined' when the backend was unreachable, and masked trigger-update failures behind a misleading 'Make sure all trigger steps are properly configured' message. The handler now: - Catches trigger create/update/delete failures separately and surfaces the underlying error rather than the configure-triggers red herring. - Wraps each API stage so a network failure short-circuits with a clear 'Unable to reach the server' toast. - Defensively guards against an unexpected workflowCreate / workflowUpdate response shape so saved.workflowID never throws. - Distinguishes network errors (axios timeouts, ENOTFOUND, etc.) from backend-returned errors and renders the appropriate message. --- .../workflow/src/views/Workflow/Editor.vue | 92 +++++++++++++++---- .../corteza-webapp-workflow/notification.yaml | 2 + 2 files changed, 75 insertions(+), 19 deletions(-) diff --git a/client/web/workflow/src/views/Workflow/Editor.vue b/client/web/workflow/src/views/Workflow/Editor.vue index 5a55894a7b..9e929552c4 100644 --- a/client/web/workflow/src/views/Workflow/Editor.vue +++ b/client/web/workflow/src/views/Workflow/Editor.vue @@ -121,21 +121,55 @@ export default { }, saveWorkflow: throttle(async function (wf) { - try { - this.processingSave = true + // Issue #687: previously this handler would swallow trigger-update + // failures behind a misleading "configure triggers" message and + // could crash on `undefined.workflowID` if the workflow API call + // rejected (e.g. backend unreachable). It now distinguishes + // network failures from validation errors and surfaces the real + // underlying error to the user. + this.processingSave = true + + const isNetworkError = e => { + if (!e) return false + // axios populates `code` for network errors and leaves + // `response` undefined; corteza API client rejects with the + // raw response.data.error string for backend-returned errors, + // so the absence of any backend payload is the signal. + if (typeof e === 'string') return false + if (e.response) return false + if (e.message && /Network Error|ECONN|ETIMEDOUT|Failed to fetch/i.test(e.message)) return true + // axios timeout / connection refused has no response field + if (e.code && /ECONN|ETIMEDOUT|ENETUNREACH|ENOTFOUND/.test(e.code)) return true + return false + } + + const reportSaveError = e => { + if (isNetworkError(e)) { + this.toastDanger( + this.$t('notification:failed-save-network'), + this.$t('notification:failed-save'), + ) + return + } + // Defer to the shared handler — it knows how to render workflow + // error step rich payloads and falls back to plain toasts. + this.toastErrorHandler(this.$t('notification:failed-save'))(e) + } + try { const isNew = wf.workflowID === '0' const { triggers = [] } = wf // Firstly handle trigger updates // Delete triggers of steps that were deleted - await Promise.all(this.triggers.filter(({ triggerID }) => { - return !triggers.find(t => triggerID === t.triggerID) - }).map(({ triggerID }) => { - return this.$AutomationAPI.triggerDelete({ triggerID }) - }), - ).then(async () => { + try { + await Promise.all(this.triggers.filter(({ triggerID }) => { + return !triggers.find(t => triggerID === t.triggerID) + }).map(({ triggerID }) => { + return this.$AutomationAPI.triggerDelete({ triggerID }) + })) + await Promise.all(triggers.map(t => { // Update triggers that already have an ID if (t.triggerID) { @@ -152,25 +186,45 @@ export default { ownedBy: this.userID, }) } - })).catch(() => { - throw new Error(this.$t('notification:configure-triggers')) - }) - }) + })) + } catch (e) { + // Surface the actual trigger error instead of the generic + // "configure triggers" message that used to mask it. + reportSaveError(e) + this.processingSave = false + return + } // Secondly handle workflow updates - if (isNew) { - wf = await this.$AutomationAPI.workflowCreate(wf) - } else { - wf = await this.$AutomationAPI.workflowUpdate(wf) + let saved + try { + saved = isNew + ? await this.$AutomationAPI.workflowCreate(wf) + : await this.$AutomationAPI.workflowUpdate(wf) + } catch (e) { + reportSaveError(e) + this.processingSave = false + return + } + + if (!saved || !saved.workflowID) { + // Defensive: API returned an unexpected shape. Don't crash on + // saved.workflowID later down the chain. + this.toastDanger( + this.$t('notification:failed-save-unexpected-response'), + this.$t('notification:failed-save'), + ) + this.processingSave = false + return } // Lastly update all of the bits - await this.fetchTriggers(wf.workflowID) + await this.fetchTriggers(saved.workflowID) this.changeDetected = false window.onbeforeunload = null - this.workflow = new automation.Workflow(wf) + this.workflow = new automation.Workflow(saved) this.toastSuccess(this.$t('notification:update.success')) if (isNew) { @@ -178,7 +232,7 @@ export default { this.$router.push({ name: 'workflow.edit', params: { workflowID: this.workflow.workflowID } }) } } catch (e) { - this.toastErrorHandler(this.$t('notification:failed-save'))(e) + reportSaveError(e) } this.processingSave = false diff --git a/locale/en/corteza-webapp-workflow/notification.yaml b/locale/en/corteza-webapp-workflow/notification.yaml index a48a25db78..a3b2ab363d 100644 --- a/locale/en/corteza-webapp-workflow/notification.yaml +++ b/locale/en/corteza-webapp-workflow/notification.yaml @@ -9,6 +9,8 @@ error: " - ERROR: " 'failed-fetch-workflows': Failed to fetch workflows 'failed-load-file': Failed to load file 'failed-save': Save failed +'failed-save-network': Unable to reach the server. Check your connection and try again. +'failed-save-unexpected-response': The server returned an unexpected response. The workflow may not have been saved. 'failed-test': Test failed 'fetch-types-failed': Failed to fetch types general: From f65bab1c37944faa279c1c7057429527ff8ee87a Mon Sep 17 00:00:00 2001 From: Toocky Date: Sat, 11 Apr 2026 12:44:30 -0600 Subject: [PATCH 08/19] Address branch review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Backend - errors/http.go bypass is now narrowed to KindAutomation errors that carry an explicit workflow.error.safe = true meta flag (set only by convErrorStep). Generic automation errors from corredor etc. keep going through the normal masking path. Flag constant lives in errors.MetaWorkflowErrorSafe so both sides share one source of truth. - exprExcerpt now truncates on rune boundaries so non-ASCII identifiers and literals can never split mid-codepoint in error messages. - Dead parseExpressions wrapper removed; only parseExpressionsAs remains. Frontend - Workflow save handler: isNetworkError now requires *positive* evidence (axios err.code, err.request && !err.response, or an explicit Network Error message). Plain JS errors thrown from inside .then() handlers no longer get misreported as "server unreachable". processingSave is cleared in finally so an unexpected sync throw can never leave the spinner stuck. - Error step configurator: severity dropdown only renders when the severity expression is a literal we can round-trip; otherwise a raw text input is shown with a "reset to literal" affordance so custom expressions (vars.level etc.) are never clobbered on re-render. Node label preview strips literal quotes for a cleaner canvas label. - toast mixin: added a comment explaining that body/title are workflow-author controlled and that BootstrapVue toasts render them as plain text — switching to HTML rendering here without sanitising would introduce a stored-XSS surface. - Removed dead notification:configure-triggers locale key. --- .../src/components/Configurator/Error.vue | 52 ++++++++++++++++++- .../workflow/src/views/Workflow/Editor.vue | 34 ++++++------ lib/vue/src/mixins/toast.js | 7 +++ .../en/corteza-webapp-workflow/general.yaml | 1 + .../corteza-webapp-workflow/notification.yaml | 1 - .../automation/service/workflow_converter.go | 39 +++++++------- server/pkg/errors/http.go | 23 +++++--- server/pkg/errors/meta.go | 9 ++++ 8 files changed, 121 insertions(+), 45 deletions(-) diff --git a/client/web/workflow/src/components/Configurator/Error.vue b/client/web/workflow/src/components/Configurator/Error.vue index 2f032f335e..1507e5f456 100644 --- a/client/web/workflow/src/components/Configurator/Error.vue +++ b/client/web/workflow/src/components/Configurator/Error.vue @@ -52,10 +52,32 @@ label-class="text-primary" > +
+ + + + +
@@ -142,9 +164,19 @@ export default { return this.findOrCreateArg('severity') }, + // severityIsLiteral: true when the severity expression is either + // empty or a plain quoted literal we can round-trip through the + // dropdown. When it's a real expression (e.g. `vars.level`), the + // configurator falls back to a raw text input so we never clobber + // the author's work. + severityIsLiteral () { + const raw = (this.severityArg.expr || '').trim() + if (!raw) return true + return /^["'](error|warning|info)["']$/.test(raw) + }, + severityValue: { get () { - // severity is stored as a quoted string expression; parse it back const raw = (this.severityArg.expr || '').trim() if (!raw) return 'error' const m = raw.match(/^["'](error|warning|info)["']$/) @@ -196,8 +228,19 @@ export default { return arg }, + // stripLiteralQuotes returns the unquoted content of a simple + // quoted string expression (e.g. "foo" -> foo) so the canvas node + // label preview reads cleanly. Non-literal expressions are passed + // through unchanged. + stripLiteralQuotes (s) { + const t = (s || '').trim() + const m = t.match(/^(["'])((?:[^\\]|\\.)*)\1$/) + return m ? m[2] : t + }, + onFieldInput () { - const preview = (this.titleArg.expr || this.bodyArg.expr || this.messageArg.expr || '').trim() + const pick = this.titleArg.expr || this.bodyArg.expr || this.messageArg.expr || '' + const preview = this.stripLiteralQuotes(pick) this.$emit('update-default-value', { value: preview ? `Stop workflow with error: ${preview}` : 'Stop workflow with error', force: !this.item.node.value, @@ -209,6 +252,11 @@ export default { this.$root.$emit('change-detected') }, + resetSeverityToLiteral () { + this.$set(this.severityArg, 'expr', '"error"') + this.$root.$emit('change-detected') + }, + openInEditor (field) { const arg = this.findOrCreateArg(field) this.expressionEditor.currentField = field diff --git a/client/web/workflow/src/views/Workflow/Editor.vue b/client/web/workflow/src/views/Workflow/Editor.vue index 9e929552c4..ab61a70989 100644 --- a/client/web/workflow/src/views/Workflow/Editor.vue +++ b/client/web/workflow/src/views/Workflow/Editor.vue @@ -129,17 +129,22 @@ export default { // underlying error to the user. this.processingSave = true + // isNetworkError requires *positive* evidence of a transport-layer + // failure. A plain JS error thrown from inside a .then() handler + // must not be misreported as "server unreachable", so we only + // match on known axios/fetch signals: + // - axios error code indicates network/timeout (ECONNREFUSED, + // ETIMEDOUT, ENETUNREACH, ENOTFOUND) + // - axios marks the request as sent but with no response + // (err.request set, err.response absent) which is the + // canonical "server did not reply" case + // - the error message explicitly says so + // Anything else falls through as a generic error. const isNetworkError = e => { - if (!e) return false - // axios populates `code` for network errors and leaves - // `response` undefined; corteza API client rejects with the - // raw response.data.error string for backend-returned errors, - // so the absence of any backend payload is the signal. - if (typeof e === 'string') return false - if (e.response) return false - if (e.message && /Network Error|ECONN|ETIMEDOUT|Failed to fetch/i.test(e.message)) return true - // axios timeout / connection refused has no response field - if (e.code && /ECONN|ETIMEDOUT|ENETUNREACH|ENOTFOUND/.test(e.code)) return true + if (!e || typeof e === 'string') return false + if (e.code && /^E(CONN|TIMED|NETUN|NOTFO)/i.test(e.code)) return true + if (e.request && !e.response) return true + if (typeof e.message === 'string' && /Network Error|Failed to fetch/i.test(e.message)) return true return false } @@ -191,7 +196,6 @@ export default { // Surface the actual trigger error instead of the generic // "configure triggers" message that used to mask it. reportSaveError(e) - this.processingSave = false return } @@ -203,7 +207,6 @@ export default { : await this.$AutomationAPI.workflowUpdate(wf) } catch (e) { reportSaveError(e) - this.processingSave = false return } @@ -214,7 +217,6 @@ export default { this.$t('notification:failed-save-unexpected-response'), this.$t('notification:failed-save'), ) - this.processingSave = false return } @@ -233,9 +235,11 @@ export default { } } catch (e) { reportSaveError(e) + } finally { + // Guarantee the processing flag is cleared even if an unexpected + // synchronous error escapes the try block above. + this.processingSave = false } - - this.processingSave = false }, 500), deleteWorkflow () { diff --git a/lib/vue/src/mixins/toast.js b/lib/vue/src/mixins/toast.js index 6e7c294179..e41702935f 100644 --- a/lib/vue/src/mixins/toast.js +++ b/lib/vue/src/mixins/toast.js @@ -90,6 +90,13 @@ export default { // Prefer the rich body; fall back to the flat error message // (which the backend also sets to body when available). + // + // NOTE: body/title originate from workflow-author-controlled + // meta fields. BootstrapVue's $bvToast.toast(content, opts) + // renders both body and title as plain text (no v-html), + // which is what we want here — do not switch to an HTML + // variant without sanitising, as that would expose the + // workflow editor as a stored-XSS surface. const body = wfErr.body || err.message || '' const toastTitle = wfErr.title || title || '' diff --git a/locale/en/corteza-webapp-workflow/general.yaml b/locale/en/corteza-webapp-workflow/general.yaml index d5094d5c98..d6996bee42 100644 --- a/locale/en/corteza-webapp-workflow/general.yaml +++ b/locale/en/corteza-webapp-workflow/general.yaml @@ -18,6 +18,7 @@ error-step: severity: label: Severity description: Controls the toast variant and icon in the end-user UI. + reset-to-literal: Reset to a literal value (error, warning, info) options: error: Error warning: Warning diff --git a/locale/en/corteza-webapp-workflow/notification.yaml b/locale/en/corteza-webapp-workflow/notification.yaml index a3b2ab363d..a0c0d9a92f 100644 --- a/locale/en/corteza-webapp-workflow/notification.yaml +++ b/locale/en/corteza-webapp-workflow/notification.yaml @@ -1,4 +1,3 @@ -'configure-triggers': Make sure all trigger steps are properly configured 'confirm-unsaved-changes': You have unsaved changes, are you sure you want to exit? error: " - ERROR: " 'event-type-not-found': Event type not found diff --git a/server/automation/service/workflow_converter.go b/server/automation/service/workflow_converter.go index e3ecb8c045..78b2f7cd12 100644 --- a/server/automation/service/workflow_converter.go +++ b/server/automation/service/workflow_converter.go @@ -502,11 +502,13 @@ func (svc workflowConverter) convErrorStep(s *types.WorkflowStep) (wfexec.Step, flat = "ERROR" } - e := errors.Automation("%s", flat) + // Tag the error as a user-authored workflow error step so that + // errors.ServeHTTPWithCode preserves its full payload (message + + // meta) across the wire even under production masking. This is + // a narrow, opt-in bypass — generic KindAutomation errors from + // elsewhere are not affected. + e := errors.Automation("%s", flat).Apply(errors.Meta(errors.MetaWorkflowErrorSafe, true)) - // only attach meta when at least one rich field is set; this - // keeps the wire format identical to the legacy behaviour for - // workflows that only configure `message`. if title != "" { e = e.Apply(errors.Meta(metaTitle, title)) } @@ -684,18 +686,13 @@ func (svc workflowConverter) convExecWorkflowStep(wf *types.Workflow, s *types.W }), nil } -func (svc workflowConverter) parseExpressions(ee ...*types.Expr) (err error) { - return svc.parseExpressionsAs("", ee...) -} - -// parseExpressionsAs is parseExpressions with a human-readable category -// (e.g. "argument" or "result") used to enrich error messages so failures -// say which target failed and why instead of leaking a bare parser error. +// parseExpressionsAs parses argument/result expressions and enriches any +// failure with a human-readable category (e.g. "argument" or "result"), +// the target name, the failing expression excerpt, and which lifecycle +// stage (parse / type / test parse) produced the error. // -// Issue #1725: previously a parse error returned bare "syntax error near -// X" with no indication of which step argument it referred to. Now the -// error includes the target name, the failing expression (truncated) -// and which lifecycle stage failed (parse / type / test parse). +// Issue #1725: previously failures leaked bare parser output with no +// indication of which step field was at fault. func (svc workflowConverter) parseExpressionsAs(category string, ee ...*types.Expr) (err error) { wrap := func(target, stage string, inner error) error { excerpt := exprExcerpt(ee, target) @@ -729,20 +726,24 @@ func (svc workflowConverter) parseExpressionsAs(category string, ee ...*types.Ex // exprExcerpt returns a short, single-line snippet of the expression for // the named target, suitable for embedding in an error message. Long // expressions are truncated with an ellipsis to keep messages readable. +// +// Truncation is rune-aware so that non-ASCII identifiers and string +// literals are never split mid-codepoint. func exprExcerpt(ee []*types.Expr, target string) string { + const maxRunes = 80 for _, e := range ee { if e.Target != target { continue } s := strings.TrimSpace(e.Expr) s = strings.ReplaceAll(s, "\n", " ") - const max = 80 - if len(s) > max { - s = s[:max] + "…" - } if s == "" { return "" } + runes := []rune(s) + if len(runes) > maxRunes { + return string(runes[:maxRunes]) + "…" + } return s } return "" diff --git a/server/pkg/errors/http.go b/server/pkg/errors/http.go index 90b5b82b1c..bbd3237aa2 100644 --- a/server/pkg/errors/http.go +++ b/server/pkg/errors/http.go @@ -139,15 +139,22 @@ func writeHttpJSON(ctx context.Context, w io.Writer, err error, mask bool) { return } - // Automation (workflow) errors are user-authored via workflow error - // steps and are explicitly safe to surface in full (message + meta) - // to clients regardless of mask state. This allows the workflow error - // step to carry structured fields (title, body, severity, ...) through - // to the frontend. - if e, isAutomationErr := err.(*Error); isAutomationErr && e.kind == KindAutomation { - // preserve full error + // Preserve the full error (message + meta) when: + // - it is a KindAutomation error AND + // - it carries the explicit workflow.error.safe = true meta flag + // + // The flag is set only by convErrorStep for user-authored workflow + // error steps. This keeps the bypass surgical: generic automation + // errors from elsewhere (e.g. corredor wrapper errors) still go + // through the normal masking path. + if e, isErr := err.(*Error); isErr && e.kind == KindAutomation && e.meta != nil { + if safe, _ := e.meta[MetaWorkflowErrorSafe].(bool); safe { + // preserve full error (skip masking) + } else if se, is := err.(interface{ Safe() bool }); !is || !se.Safe() || mask { + err = errors.New(err.Error()) + } } else if se, is := err.(interface{ Safe() bool }); !is || !se.Safe() || mask { - // trim error details when not debugging or error is not safe or maske + // trim error details when not debugging or error is not safe or masked err = errors.New(err.Error()) } diff --git a/server/pkg/errors/meta.go b/server/pkg/errors/meta.go index 507a901270..24045b23a9 100644 --- a/server/pkg/errors/meta.go +++ b/server/pkg/errors/meta.go @@ -12,6 +12,15 @@ type ( meta map[interface{}]interface{} ) +// Well-known meta keys recognised by the errors package itself. +// +// MetaWorkflowErrorSafe is set on KindAutomation errors that originate +// from a user-authored workflow error step. When present and truthy, +// ServeHTTPWithCode preserves the full error (message + meta) in the +// HTTP response even in masked/production mode, so that structured +// fields like workflow.error.title/body/severity reach the frontend. +const MetaWorkflowErrorSafe = "workflow.error.safe" + // StringKeys returns max length of (string) keys and slice of all strings func (m meta) StringKeys() (int, []string) { var ( From 0d7f12fac1dcf6a41b1dabba2806f66e650ec5c3 Mon Sep 17 00:00:00 2001 From: Toocky Date: Sat, 11 Apr 2026 13:28:14 -0600 Subject: [PATCH 09/19] Fix error handler vars disappearing from scope after first normal step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit State.results was doing double duty: it held both the previous step's output values AND the error handler's variable-name mapping (error -> myErr, errorMessage -> myMsg, errorStepID -> myID). The error-handler case merged the mapping into results, but the normal-step case blew results away with a plain assignment — so the first non-error step after an error handler registration clobbered the mapping, and any later error left errorMessage / error / errorStepID undefined in the downstream scope. This matched the reported "works for a while then stops working" and "Log Message can't find errorMessage" symptoms, and explains why the bug clustered around subworkflows and iterators (both of which have multi-step paths after the error handler where a normal step is guaranteed to execute before an error surfaces). Fix: split the two concerns onto separate State fields. - Add State.errHandlerResults holding only the name mapping. - Carry it across states in State.Next() alongside errHandler, same lifecycle (set together, cleared together). - setErrorHandlerResultsToScope now reads errHandlerResults. - Normal step path leaves the mapping untouched. - errHandlerResults is cleared alongside errHandler when a handler activates, so a subsequent handler cannot inherit stale mappings. --- server/pkg/wfexec/session.go | 20 ++++++++++++++++---- server/pkg/wfexec/state.go | 31 ++++++++++++++++++++++++------- 2 files changed, 40 insertions(+), 11 deletions(-) diff --git a/server/pkg/wfexec/session.go b/server/pkg/wfexec/session.go index 7795b446b1..cbc656ac62 100644 --- a/server/pkg/wfexec/session.go +++ b/server/pkg/wfexec/session.go @@ -721,15 +721,19 @@ func (s *Session) exec(ctx context.Context, log *zap.Logger, st *State) (nxt []* zap.Error(st.err), ) - err = setErrorHandlerResultsToScope(scope, st.results, st.err, st.step.ID()) + err = setErrorHandlerResultsToScope(scope, st.errHandlerResults, st.err, st.step.ID()) if err != nil { return nil, err } // copy error handler & disable it on state to prevent inf. loop - // in case of another error in the error-handling branch + // in case of another error in the error-handling branch. + // errHandlerResults is cleared alongside errHandler so the two + // stay in lockstep — a disabled handler must not leave its name + // mapping behind for a later handler to accidentally reuse. eh := st.errHandler st.errHandler = nil + st.errHandlerResults = nil st.errHandled = true return []*State{st.Next(eh, scope)}, nil } @@ -764,9 +768,17 @@ func (s *Session) exec(ctx context.Context, log *zap.Logger, st *State) (nxt []* case *errHandler: st.action = "error handler initialized" // this step sets error handling step on current state - // and continues on the current path + // and continues on the current path. + // + // The error-var name mapping (error / errorMessage / + // errorStepID -> author-chosen variable names) is stored + // in its own field so it survives across later normal + // steps — previously it was merged into st.results and + // got clobbered the moment any downstream step returned + // its own outputs, making errorMessage etc. disappear + // from scope on any subsequent error. st.errHandler = result.handler - st.results = st.results.MustMerge(result.results) + st.errHandlerResults = result.results // find step that's not error handler and // use it for the next step diff --git a/server/pkg/wfexec/state.go b/server/pkg/wfexec/state.go index 4ad6b06135..923fc81f05 100644 --- a/server/pkg/wfexec/state.go +++ b/server/pkg/wfexec/state.go @@ -48,6 +48,22 @@ type ( // error handling step errHandler Step + // error handler result variable name mapping. + // + // Populated when an error-handler step runs and holds the + // "error" / "errorMessage" / "errorStepID" -> variable-name + // mapping declared on the handler. Read by the error-injection + // path when a downstream step fails, so that the actual error + // values end up under the author-configured variable names. + // + // Must not be conflated with `results` (previous step outputs): + // previously both lived in `results`, so the first non-error + // step after an error handler clobbered the mapping and made + // `errorMessage` etc. disappear from scope on any later error. + // Carried alongside `errHandler` in State.Next() with the same + // lifecycle — set together, replaced together, cleared together. + errHandlerResults *expr.Vars + // error handled flag, this gets restarted on every new state! errHandled bool @@ -83,13 +99,14 @@ func FinalState(ses *Session, scope *expr.Vars) *State { func (s State) Next(current Step, scope *expr.Vars) *State { return &State{ - stateId: nextID(), - owner: s.owner, - sessionId: s.sessionId, - parent: s.step, - errHandler: s.errHandler, - results: s.results, - loops: s.loops, + stateId: nextID(), + owner: s.owner, + sessionId: s.sessionId, + parent: s.step, + errHandler: s.errHandler, + errHandlerResults: s.errHandlerResults, + results: s.results, + loops: s.loops, step: current, scope: scope, From d6f214b0fb0ce4d22bd128d069860f9fe4ccf7a9 Mon Sep 17 00:00:00 2001 From: Toocky Date: Sat, 11 Apr 2026 13:59:23 -0600 Subject: [PATCH 10/19] Collapse error step body argument into message The original design split the error step body from the legacy message argument so authors could theoretically write a terse flat fallback and a longer rich toast body. In practice nobody would ever write two different strings and the extra field just added confusing UI surface for zero real benefit. message is now the single source of truth: required, used as both the flat error string and the rich toast content. title and severity remain as optional rich-toast decorations. Backwards compatible with legacy workflows that only configured message, since the flat string is still what it always was. - Backend convErrorStep drops body/metaBody entirely; flat message is now just the evaluated message arg. - Backend verifyStep drops body from the allowed args. - Frontend configurator drops the Body form-group and bodyArg. - toast.js mixin no longer reads workflow.error.body; the flat err.message is the body. - Locale keys for error-step.body removed and message/title labels updated to reflect the simplified semantics. --- .../src/components/Configurator/Error.vue | 28 ++-------- lib/vue/src/mixins/toast.js | 14 +++-- .../en/corteza-webapp-workflow/general.yaml | 7 +-- .../automation/service/workflow_converter.go | 51 ++++++++----------- 4 files changed, 33 insertions(+), 67 deletions(-) diff --git a/client/web/workflow/src/components/Configurator/Error.vue b/client/web/workflow/src/components/Configurator/Error.vue index 1507e5f456..9ed6b2419c 100644 --- a/client/web/workflow/src/components/Configurator/Error.vue +++ b/client/web/workflow/src/components/Configurator/Error.vue @@ -30,21 +30,6 @@ /> - - - - - - + { @@ -239,7 +221,7 @@ export default { }, onFieldInput () { - const pick = this.titleArg.expr || this.bodyArg.expr || this.messageArg.expr || '' + const pick = this.titleArg.expr || this.messageArg.expr || '' const preview = this.stripLiteralQuotes(pick) this.$emit('update-default-value', { value: preview ? `Stop workflow with error: ${preview}` : 'Stop workflow with error', diff --git a/lib/vue/src/mixins/toast.js b/lib/vue/src/mixins/toast.js index e41702935f..220126a955 100644 --- a/lib/vue/src/mixins/toast.js +++ b/lib/vue/src/mixins/toast.js @@ -54,12 +54,10 @@ export default { for (const meta of candidates) { if (!meta || typeof meta !== 'object') continue const title = meta['workflow.error.title'] - const body = meta['workflow.error.body'] const severity = meta['workflow.error.severity'] - if (title || body || severity) { + if (title || severity) { return { title: title || '', - body: body || '', severity: severity || 'error', } } @@ -88,16 +86,16 @@ export default { ? 'info' : 'danger' - // Prefer the rich body; fall back to the flat error message - // (which the backend also sets to body when available). + // The toast body is always the flat error message (which + // is equal to the author-configured `message` argument). // - // NOTE: body/title originate from workflow-author-controlled + // NOTE: message/title originate from workflow-author-controlled // meta fields. BootstrapVue's $bvToast.toast(content, opts) - // renders both body and title as plain text (no v-html), + // renders both content and title as plain text (no v-html), // which is what we want here — do not switch to an HTML // variant without sanitising, as that would expose the // workflow editor as a stored-XSS surface. - const body = wfErr.body || err.message || '' + const body = err.message || '' const toastTitle = wfErr.title || title || '' this.toast(body, { title: toastTitle, variant, solid: true }) diff --git a/locale/en/corteza-webapp-workflow/general.yaml b/locale/en/corteza-webapp-workflow/general.yaml index d6996bee42..83adf6e63c 100644 --- a/locale/en/corteza-webapp-workflow/general.yaml +++ b/locale/en/corteza-webapp-workflow/general.yaml @@ -7,14 +7,11 @@ error: Error error-expression: Error message (expression) error-step: message: - label: Message (legacy, required) - description: Plain fallback message shown by older clients and as the flat error string. + label: Message + description: The error text shown to the end user. Required. title: label: Title description: Short headline shown in rich toast notifications. Optional. - body: - label: Body - description: Longer body text shown to the end user. Overrides message for display when set. Optional. severity: label: Severity description: Controls the toast variant and icon in the end-user UI. diff --git a/server/automation/service/workflow_converter.go b/server/automation/service/workflow_converter.go index 78b2f7cd12..ab376d3322 100644 --- a/server/automation/service/workflow_converter.go +++ b/server/automation/service/workflow_converter.go @@ -425,25 +425,24 @@ func (svc workflowConverter) convFunctionStep(g *wfexec.Graph, s *types.Workflow // Expects ZERO outgoing paths. // // Supported arguments: -// - message (required, string) backwards-compatible flat error message +// - message (required, string) the error text. Rendered as the flat +// error string and also as the body of the rich toast when title or +// severity are set. // - title (optional, string) short headline for rich toast rendering -// - body (optional, string) long-form body, overrides message for display // - severity (optional, string) one of "error" (default), "warning", "info" // -// title/body/severity are attached to the resulting error's meta under -// the "workflow.error.*" namespace and surfaced to the frontend via the -// errors package's JSON marshaller. The error's flat message stays -// equal to body (or message as fallback) so older clients still render -// a sensible string. +// title/severity are attached to the resulting error's meta under the +// "workflow.error.*" namespace and surfaced to the frontend via the +// errors package's JSON marshaller. The error's flat message is always +// equal to the evaluated message argument, so older clients that do +// not understand the meta fields still render a sensible string. func (svc workflowConverter) convErrorStep(s *types.WorkflowStep) (wfexec.Step, error) { const ( argMessage = "message" argTitle = "title" - argBody = "body" argSeverity = "severity" metaTitle = "workflow.error.title" - metaBody = "workflow.error.body" metaSeverity = "workflow.error.severity" severityError = "error" @@ -480,7 +479,6 @@ func (svc workflowConverter) convErrorStep(s *types.WorkflowStep) (wfexec.Step, var ( message = resolveStr(result, argMessage) title = resolveStr(result, argTitle) - body = resolveStr(result, argBody) severity = resolveStr(result, argSeverity) ) @@ -492,14 +490,9 @@ func (svc workflowConverter) convErrorStep(s *types.WorkflowStep) (wfexec.Step, severity = severityError } - // pick the flat message: prefer body, fall back to message, // fall back to a literal "ERROR" so we never emit an empty error - flat := body - if flat == "" { - flat = message - } - if flat == "" { - flat = "ERROR" + if message == "" { + message = "ERROR" } // Tag the error as a user-authored workflow error step so that @@ -507,15 +500,12 @@ func (svc workflowConverter) convErrorStep(s *types.WorkflowStep) (wfexec.Step, // meta) across the wire even under production masking. This is // a narrow, opt-in bypass — generic KindAutomation errors from // elsewhere are not affected. - e := errors.Automation("%s", flat).Apply(errors.Meta(errors.MetaWorkflowErrorSafe, true)) + e := errors.Automation("%s", message).Apply(errors.Meta(errors.MetaWorkflowErrorSafe, true)) if title != "" { e = e.Apply(errors.Meta(metaTitle, title)) } - if body != "" { - e = e.Apply(errors.Meta(metaBody, body)) - } - if title != "" || body != "" || severity != severityError { + if title != "" || severity != severityError { e = e.Apply(errors.Meta(metaSeverity, severity)) } @@ -934,22 +924,21 @@ func verifyStep(s *types.WorkflowStep, in, out types.WorkflowPathSet) types.Work checks = append(checks, gatewayCheck(zero(arguments), zero(results))...) case types.WorkflowStepKindError: - // Error step supports four optional string arguments: - // message (legacy, still required for backwards compatibility) - // title (optional, rich toast headline) - // body (optional, rich toast body — overrides message for display) - // severity (optional, "error"|"warning"|"info") + // Error step supports three string arguments: + // message (required) the error text. Doubles as the rich + // toast body and the flat error string. + // title (optional) rich toast headline + // severity (optional) "error"|"warning"|"info" checks = append(checks, requiredArg("message", expr.String{}), checkArg("title", expr.String{}), - checkArg("body", expr.String{}), checkArg("severity", expr.String{}), - count(1, 4, arguments), + count(1, 3, arguments), func() error { - allowed := map[string]bool{"message": true, "title": true, "body": true, "severity": true} + allowed := map[string]bool{"message": true, "title": true, "severity": true} for _, a := range s.Arguments { if !allowed[a.Target] { - return errors.Internal("%s step does not accept argument %q (allowed: message, title, body, severity)", s.Kind, a.Target) + return errors.Internal("%s step does not accept argument %q (allowed: message, title, severity)", s.Kind, a.Target) } } return nil From 14e3e595ef3a920c7b7246d70044b681c68c2ad7 Mon Sep 17 00:00:00 2001 From: Toocky Date: Sat, 11 Apr 2026 14:05:47 -0600 Subject: [PATCH 11/19] Fix iterator inside parallel gateway circular dependency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In makeGraph(), the iterator step requires both its next and exit child steps to already exist in the graph, and the join gateway requires all its parent steps to already exist. When an iterator's exit path leads to a join gateway with multiple parents, neither can resolve first — the resolution loop stalls and reports a generic 'failed to resolve workflow step dependencies' error. Fix: - Add an exported PathAdder interface and AddPath method on joinGateway so parents can be registered after construction (mutex-guarded, deduped). - Change convGateway 'join' case to create the gateway eagerly with whatever parents are already resolved, instead of returning nil, nil. This breaks the cycle: the join exists, the iterator finds its exit step, the iterator resolves, the iterator branch resolves, and the missing parents become available. - Add a backfill pass after the main resolution loop that walks every join gateway and registers any parents that resolved later via PathAdder.AddPath. The runtime check at joinGateway.Exec (paths.Contains(r.Parent)) still guards against unknown parents — the backfill pass runs synchronously to completion before any session executes so the graph is always fully wired before runtime. --- .../automation/service/workflow_converter.go | 33 +++++++++++++++++-- server/pkg/wfexec/gateways.go | 18 ++++++++++ 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/server/automation/service/workflow_converter.go b/server/automation/service/workflow_converter.go index ab376d3322..7c45b69a9e 100644 --- a/server/automation/service/workflow_converter.go +++ b/server/automation/service/workflow_converter.go @@ -150,6 +150,32 @@ func (svc workflowConverter) makeGraph(def *types.Workflow) (*wfexec.Graph, type } } + // Backfill join gateway parents that weren't resolved at the time the + // join gateway itself was created. This is necessary because join gateways + // are now created eagerly (with whatever parents are available) so that + // circular dependencies (e.g. iterator -> join -> iterator) can be broken. + for _, step := range ss { + if step.Kind != types.WorkflowStepKindGateway || step.Ref != "join" { + continue + } + resolved := g.StepByID(step.ID) + if resolved == nil { + continue + } + pa, ok := resolved.(wfexec.PathAdder) + if !ok { + continue + } + for _, path := range def.Paths { + if path.ChildID != step.ID { + continue + } + if parent := g.StepByID(path.ParentID); parent != nil { + pa.AddPath(parent) + } + } + } + for pos, path := range def.Paths { if g.StepByID(path.ChildID) == nil { wfii = wfii.Append(fmt.Errorf("failed to resolve step with ID %d", path.ChildID), map[string]int{"path": pos}) @@ -259,12 +285,13 @@ func (svc workflowConverter) convGateway(g *wfexec.Graph, s *types.WorkflowStep, var ( ss []wfexec.Step ) + // Collect whatever parents are already resolved. Missing parents will + // be backfilled after the main resolution loop via AddPath. This avoids + // circular dependencies where a parent (e.g. an iterator) needs this + // join gateway to exist in the graph before it itself can resolve. for _, p := range in { if parent := g.StepByID(p.ParentID); parent != nil { ss = append(ss, parent) - } else { - // unresolved parent, come back later. - return nil, nil } } diff --git a/server/pkg/wfexec/gateways.go b/server/pkg/wfexec/gateways.go index 3097640ef7..eabfe5c933 100644 --- a/server/pkg/wfexec/gateways.go +++ b/server/pkg/wfexec/gateways.go @@ -27,6 +27,13 @@ func NewGatewayPath(s Step, t pathTester) (gwp *GatewayPath, err error) { return &GatewayPath{to: s, test: t}, nil } +// PathAdder is implemented by gateways that support lazy parent registration. +// This allows resolving circular dependencies during graph construction where +// parent steps aren't yet in the graph when the gateway is created. +type PathAdder interface { + AddPath(Step) +} + // joinGateway handles merging/joining of multiple paths into // a single path forward type ( @@ -39,6 +46,17 @@ type ( } ) +// AddPath adds a parent path to the join gateway after initial creation. +// This allows resolving circular dependencies where parent steps aren't yet +// in the graph at the time the join gateway is constructed. +func (gw *joinGateway) AddPath(s Step) { + gw.l.Lock() + defer gw.l.Unlock() + if !gw.paths.Contains(s) { + gw.paths = append(gw.paths, s) + } +} + // JoinGateway fn initializes join gateway with all paths that are expected to be partial func JoinGateway(ss ...Step) *joinGateway { return &joinGateway{ From a95202e29b2375759d58ee25168e60be08103028 Mon Sep 17 00:00:00 2001 From: Toocky Date: Sat, 11 Apr 2026 16:22:46 -0600 Subject: [PATCH 12/19] Merge all parallel branch scopes at join gateway joinGateway.Exec previously started its merge from paths[0]'s scope only and merged in every parent's immediate step results on top. Any variable set mid-branch in paths[1..N] lived in that branch's scope but was never in the final step's results, so it was silently dropped at the join. Whichever branch happened to be paths[0] worked; the others lost their intermediate variables. This matched the reported "variables don't always pass through the joined flow" symptom and explained why authors worked around it by moving Base64-style steps out of parallel forks entirely. Fix: merge every parent's scope (not just paths[0]'s), then merge every parent's results on top, and do both in reverse path order so that paths[0] wins conflicts deterministically. This preserves the intra-branch rule that step results overwrite earlier scope values and gives a clear precedence hierarchy: paths[0].results > ... > paths[N].results > paths[0].scope > ... > paths[N].scope No behaviour change for workflows that previously relied on paths[0] winning (that's now the explicit rule). Variables that used to be silently dropped from non-paths[0] branches are now preserved. --- server/pkg/wfexec/gateways.go | 48 +++++++++++++++++++++++------------ 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/server/pkg/wfexec/gateways.go b/server/pkg/wfexec/gateways.go index eabfe5c933..e63cc9ae90 100644 --- a/server/pkg/wfexec/gateways.go +++ b/server/pkg/wfexec/gateways.go @@ -74,12 +74,30 @@ func JoinGateway(ss ...Step) *joinGateway { } } -// Exec fn on join gateway can be called multiple times, even multiple times parent the same parent +// Exec fn on join gateway can be called multiple times, even multiple times for the same parent. // -// Func will collect results from each parent path. +// It collects scope and results from every parent path and merges them when +// all configured paths have reported in. // -// Join gateway is ready to continue when all configured paths have been collected -// Results are merged to preserve changes from all parallel paths +// Merge rules: +// - Every parent's full scope is merged in (previously only paths[0]'s +// scope was kept, so variables set mid-branch in any other path were +// silently dropped — this is the "variables lost at parallel join" +// class of bug). +// - Every parent's immediate step results are merged on top of the +// combined scope, mirroring the intra-branch rule that a step's +// results overwrite earlier scope values on conflict. +// - Within each tier (scopes, then results), lower path index wins on +// key conflicts. paths[0] is the preferred branch. This is +// implemented by iterating paths in REVERSE order and relying on +// MustMerge's last-writer-wins semantics: the last iterator passed +// to MustMerge wins, so the earliest path ends up applied last and +// takes precedence. +// +// Final precedence, high to low: +// +// paths[0].results > paths[1].results > ... > paths[N].results > +// paths[0].scope > paths[1].scope > ... > paths[N].scope func (gw *joinGateway) Exec(_ context.Context, r *ExecRequest) (ExecResponse, error) { gw.l.Lock() defer gw.l.Unlock() @@ -100,21 +118,19 @@ func (gw *joinGateway) Exec(_ context.Context, r *ExecRequest) (ExecResponse, er return &partial{}, nil } - // All collected, merge results from all paths into base scope - var merged *expr.Vars - if len(gw.paths) > 0 && gw.scopes[r.SessionID][gw.paths[0]] != nil { - merged = gw.scopes[r.SessionID][gw.paths[0]].MustMerge() - } - - var allResults []expr.Iterator - for _, p := range gw.paths { - if gw.results[r.SessionID][p] != nil { - allResults = append(allResults, gw.results[r.SessionID][p]) + // Merge scopes first, in reverse path order so paths[0] wins on conflict. + merged := &expr.Vars{} + for i := len(gw.paths) - 1; i >= 0; i-- { + if s := gw.scopes[r.SessionID][gw.paths[i]]; s != nil { + merged = merged.MustMerge(s) } } - if merged != nil && len(allResults) > 0 { - merged = merged.MustMerge(allResults...) + // Then merge results on top, also in reverse path order. + for i := len(gw.paths) - 1; i >= 0; i-- { + if res := gw.results[r.SessionID][gw.paths[i]]; res != nil { + merged = merged.MustMerge(res) + } } // all inbound paths visited, cleanup scopes and results for the session From cd8e323698365d7a42d1359780f093956105b8e9 Mon Sep 17 00:00:00 2001 From: Toocky Date: Sat, 11 Apr 2026 16:26:39 -0600 Subject: [PATCH 13/19] Surface join gateway variable conflicts as runtime warnings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When two parallel branches produce conflicting values for the same variable, the merge still proceeds with paths[0] winning but the colliding branches are now reported as non-fatal warnings. - Frame gains a Warnings []string field alongside the existing Action/Error so the editor test panel and admin session log can render them in their existing per-step display. - State gains a matching warnings slice that MakeFrame copies into the emitted frame. - A new responseWithWarnings wrapper response lets a step return its normal *expr.Vars payload together with a list of warnings. The session unwraps it on dispatch, attaches warnings to the current State (for frame emission) and also mirrors each warning through zap.Warn so ops can grep the server log independently of the editor. - joinGateway.Exec runs a detectConflicts pass before merging. For every key seen across multiple branches it compares stringified values; mismatches produce a warning naming the key, the contributing branches/origins (scope vs results), and the clipped values. The merge then proceeds with paths[0] winning as before. Workflows with no conflicts get zero behaviour change — the warning path only kicks in when detectConflicts returns a non-empty slice. --- server/pkg/wfexec/gateways.go | 94 +++++++++++++++++++++++++++++++++++ server/pkg/wfexec/response.go | 19 +++++++ server/pkg/wfexec/session.go | 23 +++++++++ server/pkg/wfexec/state.go | 8 +++ 4 files changed, 144 insertions(+) diff --git a/server/pkg/wfexec/gateways.go b/server/pkg/wfexec/gateways.go index e63cc9ae90..a0698a736d 100644 --- a/server/pkg/wfexec/gateways.go +++ b/server/pkg/wfexec/gateways.go @@ -3,6 +3,7 @@ package wfexec import ( "context" "fmt" + "strings" "sync" "github.com/cortezaproject/corteza/server/pkg/expr" @@ -118,6 +119,14 @@ func (gw *joinGateway) Exec(_ context.Context, r *ExecRequest) (ExecResponse, er return &partial{}, nil } + // Detect cross-branch variable conflicts before merging. For each + // key, collect the set of (pathIndex -> serialised value) entries + // across both scopes and results. If a key has more than one + // distinct value across the branches it is a conflict and we emit + // a warning. The merge still proceeds (paths[0] wins) — the + // warnings are advisory, not fatal. + warnings := gw.detectConflicts(r.SessionID) + // Merge scopes first, in reverse path order so paths[0] wins on conflict. merged := &expr.Vars{} for i := len(gw.paths) - 1; i >= 0; i-- { @@ -137,9 +146,94 @@ func (gw *joinGateway) Exec(_ context.Context, r *ExecRequest) (ExecResponse, er delete(gw.scopes, r.SessionID) delete(gw.results, r.SessionID) + if len(warnings) > 0 { + return ResponseWithWarnings(merged, warnings), nil + } return merged, nil } +// detectConflicts walks every (scope, results) bag from every parent +// branch and returns a human-readable warning for every key that has +// more than one distinct value across branches. +// +// The per-value dedup uses the expr iterator's string rendering as a +// stable fingerprint. Not perfect for complex types but good enough to +// catch the common case of "branch A set x to 1, branch B set x to 2". +// False positives (same logical value, different formatting) are +// acceptable — this is an advisory warning, not a hard error. +func (gw *joinGateway) detectConflicts(sessionID uint64) []string { + type contributor struct { + pathIdx int + origin string // "scope" or "results" + value string + } + + perKey := make(map[string][]contributor) + + collect := func(origin string, bag map[Step]*expr.Vars) { + for i, p := range gw.paths { + v := bag[p] + if v == nil { + continue + } + _ = v.Each(func(k string, tv expr.TypedValue) error { + val := "" + if tv != nil { + val = fmt.Sprintf("%v", tv.Get()) + } + perKey[k] = append(perKey[k], contributor{pathIdx: i, origin: origin, value: val}) + return nil + }) + } + } + + collect("scope", gw.scopes[sessionID]) + collect("results", gw.results[sessionID]) + + var warnings []string + for key, cc := range perKey { + if len(cc) < 2 { + continue + } + // detect whether all contributors share the same value + same := true + for i := 1; i < len(cc); i++ { + if cc[i].value != cc[0].value { + same = false + break + } + } + if same { + continue + } + + // build a list of contributing branches for the warning + branchDescs := make([]string, 0, len(cc)) + for _, c := range cc { + branchDescs = append(branchDescs, + fmt.Sprintf("paths[%d].%s=%s", c.pathIdx, c.origin, truncateForWarning(c.value)), + ) + } + warnings = append(warnings, fmt.Sprintf( + "variable %q has conflicting values at parallel join (%s); paths[0] wins", + key, + strings.Join(branchDescs, ", "), + )) + } + + return warnings +} + +// truncateForWarning clips long values so a warning listing 8 branches +// doesn't explode into an unreadable wall of text. +func truncateForWarning(s string) string { + const max = 40 + if len(s) <= max { + return s + } + return s[:max] + "…" +} + // forkGateway handles forking to multiple paths type forkGateway struct { StepIdentifier diff --git a/server/pkg/wfexec/response.go b/server/pkg/wfexec/response.go index c5a16b34a6..058116dc60 100644 --- a/server/pkg/wfexec/response.go +++ b/server/pkg/wfexec/response.go @@ -52,3 +52,22 @@ type ( func LoopBreak() *loopBreak { return &loopBreak{} } func LoopContinue() *loopContinue { return &loopContinue{} } + +// responseWithWarnings wraps a *expr.Vars scope together with a set of +// non-fatal warnings produced while computing it. The session unwraps +// this and attaches the warnings to the current State so they land in +// the emitted Frame's Warnings slice alongside the scope in Frame.Scope. +// +// Used by the join gateway to surface variable conflicts across +// parallel branches without changing the normal *expr.Vars contract. +type responseWithWarnings struct { + scope *expr.Vars + warnings []string +} + +// ResponseWithWarnings constructs a wrapper response. Callers pass the +// scope they would have returned normally plus a list of human-readable +// warning messages that the session should propagate into the frame. +func ResponseWithWarnings(scope *expr.Vars, warnings []string) *responseWithWarnings { + return &responseWithWarnings{scope: scope, warnings: warnings} +} diff --git a/server/pkg/wfexec/session.go b/server/pkg/wfexec/session.go index cbc656ac62..53e83f3d51 100644 --- a/server/pkg/wfexec/session.go +++ b/server/pkg/wfexec/session.go @@ -88,6 +88,13 @@ type ( Action string `json:"action,omitempty"` Error string `json:"error,omitempty"` + + // Warnings are non-fatal messages produced during step execution. + // Populated from State.warnings by MakeFrame. Used today by the + // join gateway to surface cross-branch variable conflicts but + // intended as a general channel for any step that wants to report + // a soft problem without failing the workflow. + Warnings []string `json:"warnings,omitempty"` } // ExecRequest is passed to Exec() functions and contains all information @@ -758,6 +765,22 @@ func (s *Session) exec(ctx context.Context, log *zap.Logger, st *State) (nxt []* } log.Debug("step executed", zap.String("resultType", fmt.Sprintf("%T", result))) + + // Unwrap a responseWithWarnings wrapper. The inner scope is + // treated exactly like a plain *expr.Vars return by the switch + // below; the warnings are attached to state so MakeFrame picks + // them up on the next frame emission, and mirrored into the + // server log so ops can grep for them without the editor. + if rw, isRW := result.(*responseWithWarnings); isRW { + if len(rw.warnings) > 0 { + st.warnings = append(st.warnings, rw.warnings...) + for _, w := range rw.warnings { + log.Warn("workflow step warning", zap.String("warning", w)) + } + } + result = rw.scope + } + switch result := result.(type) { case *expr.Vars: // most common (successful) result diff --git a/server/pkg/wfexec/state.go b/server/pkg/wfexec/state.go index 923fc81f05..d433b13225 100644 --- a/server/pkg/wfexec/state.go +++ b/server/pkg/wfexec/state.go @@ -70,6 +70,13 @@ type ( loops []Iterator action string + + // warnings accumulated during the current step's execution. + // Set by the session when a step returns a responseWithWarnings + // wrapper and surfaced via MakeFrame into Frame.Warnings so the + // editor test panel can render them alongside the existing + // action/error info. + warnings []string } ) @@ -170,6 +177,7 @@ func (s State) MakeFrame() *Frame { StateID: s.stateId, NextSteps: s.next.IDs(), Action: s.action, + Warnings: append([]string(nil), s.warnings...), } var wg sync.WaitGroup From 8e592d393822f26d644d2d1e1a64bf2f05392a8b Mon Sep 17 00:00:00 2001 From: Toocky Date: Sat, 11 Apr 2026 16:32:39 -0600 Subject: [PATCH 14/19] Add static analyzer for parallel-join variable conflicts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduces a design-time analyzer that walks the workflow graph and warns when two or more parallel branches joining at the same gateway can write the same variable name. Runs during validateWorkflow alongside the existing graph conversion so warnings are populated on every save without requiring a test run. Output lives in a new Workflow.Warnings field (parallel to Issues, same WorkflowIssueSet shape) so the frontend can render warnings with the existing per-step culprit mapping and badge logic. What the analyzer considers a "branch write": - Function/Iterator/ExecWorkflow step: its declared Results targets. - Expressions step: its Arguments targets (the assignment LHS). - ErrorHandler step: its Results targets (author-chosen error var names). - Everything else (Error, Termination, Break, Continue, Debug, Delay, Prompt, Visual, plain Gateway): no scope writes. Subworkflow walking is intentionally NOT recursive. A subworkflow's effect on the calling scope is exactly the ExecWorkflow step's own declared results (see convExecWorkflowStep which Evals s.Results against the subworkflow return value). The static declaration is the full contract, so there's nothing to gain from walking inside. Dynamic writes (vars[computed] = ...) are not detected. Documented as a known limitation in the analyzer source. The analyzer is non-fatal by construction — it never adds to Issues and never blocks save/run. Pair with the runtime detectConflicts warning in joinGateway.Exec to catch what static analysis missed. --- server/automation/service/workflow.go | 5 + .../automation/service/workflow_analyzer.go | 215 ++++++++++++++++++ server/automation/types/workflow.go | 6 + 3 files changed, 226 insertions(+) create mode 100644 server/automation/service/workflow_analyzer.go diff --git a/server/automation/service/workflow.go b/server/automation/service/workflow.go index dd89d42996..00f692f2d0 100644 --- a/server/automation/service/workflow.go +++ b/server/automation/service/workflow.go @@ -676,6 +676,11 @@ func (svc *workflow) validateWorkflow(ctx context.Context, wf *types.Workflow) ( g, wf.Issues = Convert(svc, wf) + // Non-fatal static analysis. Produces wf.Warnings so the editor + // can surface design-time problems (e.g. variable conflicts at + // parallel joins) without blocking save or run. + wf.Warnings = analyzeWorkflow(wf) + tt, _, err = store.SearchAutomationTriggers(ctx, svc.store, types.TriggerFilter{ WorkflowID: id.Strings(types.WorkflowSet{wf}.IDs()...), Deleted: filter.StateExcluded, diff --git a/server/automation/service/workflow_analyzer.go b/server/automation/service/workflow_analyzer.go new file mode 100644 index 0000000000..66f0c7db40 --- /dev/null +++ b/server/automation/service/workflow_analyzer.go @@ -0,0 +1,215 @@ +package service + +import ( + "fmt" + "sort" + + "github.com/cortezaproject/corteza/server/automation/types" +) + +// analyzeWorkflow performs non-fatal static analysis on a workflow +// definition and returns a WorkflowIssueSet suitable for assignment to +// wf.Warnings. Analysis is intentionally conservative: it only reports +// on patterns that can be decided purely from the workflow graph and +// the declared step metadata, without evaluating expressions. +// +// Current checks: +// +// - Parallel-join variable conflicts: for every join gateway, walk +// every upstream branch back to the matching fork (or the workflow +// entry) and collect the set of variable names each branch can +// write. Any variable name written in more than one branch is +// reported as a potential conflict at the join step. Matches the +// runtime detectConflicts check in wfexec.joinGateway.Exec but +// fires at save time so authors see the warning before they run. +// +// Assumptions (documented so future readers know the limits): +// +// - Only statically declared writes are considered. An Expressions +// step's writes are its argument target names; a Function or +// ExecWorkflow step's writes are its result target names. Dynamic +// writes inside expression bodies (e.g. vars[computed] = ...) are +// not detected. +// - Subworkflow contents are NOT walked. A subworkflow invocation's +// writes into the calling scope are exactly the ExecWorkflow step's +// own declared results, so the step's static declaration already +// captures the full contract. This is how the runtime wires +// subworkflow results back too (see convExecWorkflowStep). +// - The analyzer never fails the workflow. If an unsupported topology +// is encountered (e.g. a broken path with no reachable fork) it +// silently skips that join and moves on. +func analyzeWorkflow(wf *types.Workflow) types.WorkflowIssueSet { + if wf == nil || len(wf.Steps) == 0 || len(wf.Paths) == 0 { + return nil + } + + // step index lookup: stepID -> position in wf.Steps (used as + // culprit index for per-step badge placement in the editor) + stepIdx := make(map[uint64]int, len(wf.Steps)) + stepByID := make(map[uint64]*types.WorkflowStep, len(wf.Steps)) + for i, s := range wf.Steps { + stepIdx[s.ID] = i + stepByID[s.ID] = s + } + + // parents[stepID] = list of parent step IDs feeding into stepID + parents := make(map[uint64][]uint64, len(wf.Steps)) + for _, p := range wf.Paths { + parents[p.ChildID] = append(parents[p.ChildID], p.ParentID) + } + + var warnings types.WorkflowIssueSet + + for _, s := range wf.Steps { + if s.Kind != types.WorkflowStepKindGateway || s.Ref != "join" { + continue + } + + // For each incoming branch, collect the set of variables that + // branch can write. A "branch" is defined as the set of steps + // strictly between a fork (or entry) and the join, following + // parent edges backwards from each direct parent of the join. + branchWrites := make([]map[string]bool, 0, len(parents[s.ID])) + for _, parentID := range parents[s.ID] { + writes := collectBranchWrites(parentID, stepByID, parents) + if len(writes) > 0 { + branchWrites = append(branchWrites, writes) + } + } + + if len(branchWrites) < 2 { + continue + } + + // For each variable name, count how many branches wrote it. + counts := make(map[string]int) + for _, bw := range branchWrites { + for name := range bw { + counts[name]++ + } + } + + var conflicts []string + for name, c := range counts { + if c >= 2 { + conflicts = append(conflicts, name) + } + } + if len(conflicts) == 0 { + continue + } + + sort.Strings(conflicts) + for _, name := range conflicts { + warnings = warnings.Append( + fmt.Errorf( + "variable %q is written by more than one parallel branch joining here; paths[0] wins at runtime, other branches' values are discarded. Rename the conflicting outputs (e.g. %s_a, %s_b) if you need them all.", + name, name, name, + ), + map[string]int{"step": stepIdx[s.ID]}, + ) + } + } + + return warnings +} + +// collectBranchWrites walks parent edges backwards from the given +// starting step, collecting the set of variable names that any step +// on the branch can write. Walking stops at a fork gateway (that's +// where the branch began) or when no more parents exist (workflow +// entry). The starting step itself is included. +// +// Cycles are possible if the workflow has iterators (loop-back edges). +// A visited set prevents infinite walks; iterator bodies still get +// their writes collected on the first visit. +func collectBranchWrites( + startID uint64, + stepByID map[uint64]*types.WorkflowStep, + parents map[uint64][]uint64, +) map[string]bool { + writes := make(map[string]bool) + visited := make(map[uint64]bool) + + var walk func(id uint64) + walk = func(id uint64) { + if visited[id] { + return + } + visited[id] = true + + s := stepByID[id] + if s == nil { + return + } + + // Stop at a fork — that's the upstream boundary of the branch. + // Do not collect the fork's writes; it doesn't produce any. + if s.Kind == types.WorkflowStepKindGateway && s.Ref == "fork" { + return + } + + for name := range stepWrites(s) { + writes[name] = true + } + + for _, pid := range parents[id] { + walk(pid) + } + } + + walk(startID) + return writes +} + +// stepWrites returns the set of variable names a step can write into +// the shared scope based on its declared metadata. Returns an empty +// map for step kinds that do not produce scope writes. +func stepWrites(s *types.WorkflowStep) map[string]bool { + out := make(map[string]bool) + if s == nil { + return out + } + + switch s.Kind { + case types.WorkflowStepKindExpressions: + // Expressions step declares its assignments via Arguments, + // each with an explicit Target name that becomes the scope + // key. This is how convExpressionStep / ExpressionsStep wire + // outputs at runtime. + for _, a := range s.Arguments { + if a != nil && a.Target != "" { + out[a.Target] = true + } + } + + case types.WorkflowStepKindFunction, + types.WorkflowStepKindIterator, + types.WorkflowStepKindExecWorkflow: + // Function / iterator / subworkflow call: step results are + // the declared outputs. For ExecWorkflow specifically, the + // runtime evaluates s.Results against the subworkflow's + // return value, so the declared targets ARE the contract. + for _, r := range s.Results { + if r != nil && r.Target != "" { + out[r.Target] = true + } + } + + case types.WorkflowStepKindErrHandler: + // Error handler declares author-chosen variable names for + // error / errorMessage / errorStepID via Results targets. + // These become scope variables when a downstream step errors. + for _, r := range s.Results { + if r != nil && r.Target != "" { + out[r.Target] = true + } + } + + default: + // Error, Termination, Break, Continue, Debug, Delay, Prompt, + // Visual and plain Gateway steps do not write to scope. + } + + return out +} diff --git a/server/automation/types/workflow.go b/server/automation/types/workflow.go index dc1b6fc84c..aee8ea44a0 100644 --- a/server/automation/types/workflow.go +++ b/server/automation/types/workflow.go @@ -36,6 +36,12 @@ type ( // Collection of issues from the last parse Issues WorkflowIssueSet `json:"issues,omitempty"` + // Collection of non-fatal warnings from the last parse. + // Uses the same shape as Issues (description + culprit) so + // the frontend can render them with the same per-step mapping. + // Warnings do not prevent the workflow from running. + Warnings WorkflowIssueSet `json:"warnings,omitempty"` + RunAs uint64 `json:"runAs,string"` OwnedBy uint64 `json:"ownedBy,string"` From ef8a991fa58e2c3c4d2665422d380ad222d64152 Mon Sep 17 00:00:00 2001 From: Toocky Date: Sat, 11 Apr 2026 16:37:15 -0600 Subject: [PATCH 15/19] Render workflow warnings in editor alongside issues Mirrors the existing issues pattern for the new workflow.warnings payload (populated by the backend analyzer) and for the runtime Frame.Warnings emitted by joinGateway.Exec during a test run. - New this.warnings map populated from workflow.warnings, keyed by stepID, identical shape and assembly to this.issues. - hasWarnings computed + yellow header badge next to the existing red issues badge when wf.warnings is non-empty. - Yellow warning triangle icon (new public/icons/warning.svg, same silhouette as issue.svg with a warning-yellow fill) rendered on the canvas node when a step has warnings but no issues. Issues take precedence on the badge position since they are more severe. - Clicking the warning icon opens the shared issues modal which now has two sections (Issues in red, Warnings in yellow). The modal only renders a section if that list is non-empty. - Locale keys editor:detected-warnings and editor:warnings added. --- client/web/workflow/public/icons/warning.svg | 4 + .../src/components/WorkflowEditor.vue | 82 +++++++++++++++++-- locale/en/corteza-webapp-workflow/editor.yaml | 2 + 3 files changed, 82 insertions(+), 6 deletions(-) create mode 100644 client/web/workflow/public/icons/warning.svg diff --git a/client/web/workflow/public/icons/warning.svg b/client/web/workflow/public/icons/warning.svg new file mode 100644 index 0000000000..69c44acc5d --- /dev/null +++ b/client/web/workflow/public/icons/warning.svg @@ -0,0 +1,4 @@ + + + + diff --git a/client/web/workflow/src/components/WorkflowEditor.vue b/client/web/workflow/src/components/WorkflowEditor.vue index 74d15b1f62..dc0291ca14 100644 --- a/client/web/workflow/src/components/WorkflowEditor.vue +++ b/client/web/workflow/src/components/WorkflowEditor.vue @@ -147,6 +147,17 @@ +
+ + {{ $t('editor:detected-warnings') }} + +
+
-

- {{ issue[0].toUpperCase() + issue.slice(1) }} -

+
+ {{ $t('editor:issues') }} +
+
+

+ {{ issue[0].toUpperCase() + issue.slice(1) }} +

+
+
+ +
+
+ {{ $t('editor:warnings') }} +
+
+

+ {{ warning[0].toUpperCase() + warning.slice(1) }} +

+
@@ -527,6 +562,7 @@ export default { vertices: {}, edges: {}, issues: {}, + warnings: {}, highlights: [], @@ -547,6 +583,7 @@ export default { issuesModal: { show: false, issues: [], + warnings: [], }, dryRun: { @@ -691,6 +728,10 @@ export default { return (this.workflow.issues || []).length }, + hasWarnings () { + return (this.workflow.warnings || []).length + }, + getRunAs () { if (this.runAsUser) { const { userID, name, username, email } = this.runAsUser @@ -972,15 +1013,19 @@ export default { const isSelected = this.selection.includes(cell.mxObjectId) const shadow = isSelected ? 'shadow' : 'shadow-sm' const issue = this.getIcon('issue') + const warningIcon = this.getIcon('warning') const playIcon = this.getIcon('play') const stopIcon = this.getIcon('stop') const opacity = kind === 'trigger' && !vertex.triggers.enabled ? 'opacity: 0.7;' : '' let test = '' let issues = '' + let warnings = '' let id = '' if (this.issues[cell.id]) { issues = `` + } else if (this.warnings[cell.id]) { + warnings = `` } else { id = `${cell.id}` } @@ -1130,6 +1175,7 @@ export default { test + id + issues + + warnings + '' + '' + `
` + @@ -1800,7 +1846,8 @@ export default { const itemType = cell.edge ? 'edge' : item.config.kind if (event.target.id === 'openIssues') { - this.issuesModal.issues = this.issues[cell.id] + this.issuesModal.issues = this.issues[cell.id] || [] + this.issuesModal.warnings = this.warnings[cell.id] || [] this.issuesModal.show = true } else if (event.target.id === 'testWorkflow') { this.dryRun.cellID = cell.id @@ -2459,6 +2506,29 @@ export default { }) } + // Assemble warnings (same shape as issues, non-fatal). Rendered + // with a yellow badge on affected steps so authors can see them + // without the editor refusing to save or run the workflow. + this.warnings = {} + if (this.workflow.warnings) { + this.workflow.warnings.forEach(({ culprit, description }) => { + if (culprit) { + const { step = -1, trigger = -1 } = culprit + let stepID = '' + + if (step >= 0) { + stepID = (this.workflow.steps[step] || {}).stepID + } else if (trigger >= 0) { + stepID = (this.triggers[trigger] || {}).meta?.visual?.id || '' + } + + if (stepID) { + this.warnings[stepID] ? this.warnings[stepID].push(description) : this.warnings[stepID] = [description] + } + } + }) + } + this.deferred = false this.triggersPathsChanged = false diff --git a/locale/en/corteza-webapp-workflow/editor.yaml b/locale/en/corteza-webapp-workflow/editor.yaml index 1d9b8c20a9..ced9d88c9a 100644 --- a/locale/en/corteza-webapp-workflow/editor.yaml +++ b/locale/en/corteza-webapp-workflow/editor.yaml @@ -7,6 +7,7 @@ delete: Delete deleted: Deleted 'detected-changes': "Changes detected " 'detected-issues': Issues detected +'detected-warnings': Warnings detected disabled: Disabled help: Help handle: (Handle) @@ -15,6 +16,7 @@ id: "ID:" 'initial-scope': Initial scope 'input-ids-or-handles': The initial scope gets injected into the workflow at execution. To load available variables, input the related IDs/Handles below issues: Issues +warnings: Warnings 'load-and-configure': Load and Configure 'modify-initial-scope-if-no-variables-are-loaded': If you do not wish to load any variables, click "Configure" to modify the initial scope before running workflow 'open-webapp-on-prompt-use' : "WARNING: If prompts are used in the workflow, make sure that the related webapp is also opened. Otherwise workflow will timeout" From 14090a71b510796410e7b14fff666158ef28cde4 Mon Sep 17 00:00:00 2001 From: Toocky Date: Sat, 11 Apr 2026 17:23:37 -0600 Subject: [PATCH 16/19] Suppress misleading no-dependency errors from stall reporter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the step resolution loop stalls, a step may have no unresolved child or parent neighbours. In that case it is not actually waiting on anything — its own conversion is failing (bad ref, missing required argument, unknown function, etc.) and verifyStep or the converter error path has already recorded the specific reason in wfii. Appending a "waiting for [] and/or []" entry on top adds noise that actively misleads the user into looking for a dependency problem that does not exist. The stall reporter now skips any unresolved step whose unresolved neighbour lists are both empty; the pre-existing issues on that step are left to speak for themselves. --- server/automation/service/workflow_converter.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/server/automation/service/workflow_converter.go b/server/automation/service/workflow_converter.go index 7c45b69a9e..0f5af14023 100644 --- a/server/automation/service/workflow_converter.go +++ b/server/automation/service/workflow_converter.go @@ -116,7 +116,13 @@ func (svc workflowConverter) makeGraph(def *types.Workflow) (*wfexec.Graph, type if !progress { // nothing resolved for 1 cycle — report every unresolved step - // individually with details about what it is waiting for. + // that is genuinely waiting on a dependency. If a step has no + // unresolved neighbours in either direction, it is failing to + // convert for its own reasons (bad ref, missing required + // arg, unknown function, ...) — verifyStep and the converter + // error path have already added those issues to wfii, so + // adding a "waiting for [] and/or []" message on top would + // just be misleading noise. for _, step := range def.Steps { if g.StepByID(step.ID) != nil { continue @@ -136,6 +142,12 @@ func (svc workflowConverter) makeGraph(def *types.Workflow) (*wfexec.Graph, type } } + if len(unresolvedChildren) == 0 && len(unresolvedParents) == 0 { + // not a dependency problem — let the other issues on + // this step speak for themselves. + continue + } + culprit := map[string]int{"step": IDs[step.ID]} wfii = wfii.Append(fmt.Errorf( "failed to resolve workflow step dependencies for %s step (ref: %q, ID: %d): waiting for unresolved child step(s) [IDs: %v] and/or unresolved parent step(s) [IDs: %v]", From 0bb69641fe617a548ef34bc74ff2c463af62d64a Mon Sep 17 00:00:00 2001 From: Toocky Date: Sat, 11 Apr 2026 19:13:20 -0600 Subject: [PATCH 17/19] Address review: correctness fixes for warnings + verifier Review C2: replace fmt.Sprintf("%v", tv.Get()) as the detectConflicts fingerprint with encoding/json marshalling. %v prints maps in random iteration order and pointers as addresses, producing both false positives (same map compared unequal because keys swapped) and false negatives (different pointers compared equal because both printed their address only). encoding/json sorts object keys on output so structurally equal values serialise identically. Failed marshals fall back to %v, still consistent within a single run. Review C3: collectBranchWrites now stops at any join gateway other than the home join being analyzed. In nested parallels the outer branch's upstream chain passes through the inner join -> the inner fork -> the outer branch; walking through the inner join used to bleed outer-branch writes into every inner branch and flagged spurious conflicts. The inner join's own analysis handles its incoming branches independently so we must not cross it. Review C4: drop iterator from stepWrites. Iterator loop vars (item, counter, isFirst, ...) are injected into the body scope per iteration and never survive the iterator's exit path, so two parallel iterators sharing the idiomatic "item" target are not actually in conflict at the downstream join. Steps inside the iterator body still get walked normally via parent edges. Review M1: warning messages no longer refer to the paths[i] abstraction that authors cannot correlate with the canvas. They now name the winning branch by its parent step ID and list each contributing branch by its parent step ID, origin (scope/results) and value fingerprint. Review m5: truncateForWarning is now rune-aware so non-ASCII values cannot be split mid-codepoint. Review m11: isNetworkError recognises EAI_AGAIN (DNS transient) and axios >= 1.x ERR_NETWORK generic network codes. Review m13: drop the redundant .Wrap(err) on errors.Internal calls in workflowStepDefConv. parseExpressionsAs already wraps its inner error through fmt.Errorf("%w") so the outer message already contains the full chain; the extra .Wrap doubled the inner error in both the formatted text and the wrap chain. Review m14: verifyStep now validates severity as a save-time error when the expression is a simple quoted literal ("error", "warning", "info"). Non-literal expressions (e.g. vars.level) fall through and are normalised at runtime as before. Catches authors typing e.g. "danger" without running the workflow first. --- .../workflow/src/views/Workflow/Editor.vue | 7 +- .../automation/service/workflow_analyzer.go | 50 ++++++++++--- .../automation/service/workflow_converter.go | 55 +++++++++++++- server/pkg/wfexec/gateways.go | 75 ++++++++++++++----- 4 files changed, 152 insertions(+), 35 deletions(-) diff --git a/client/web/workflow/src/views/Workflow/Editor.vue b/client/web/workflow/src/views/Workflow/Editor.vue index ab61a70989..81d07333ec 100644 --- a/client/web/workflow/src/views/Workflow/Editor.vue +++ b/client/web/workflow/src/views/Workflow/Editor.vue @@ -133,8 +133,9 @@ export default { // failure. A plain JS error thrown from inside a .then() handler // must not be misreported as "server unreachable", so we only // match on known axios/fetch signals: - // - axios error code indicates network/timeout (ECONNREFUSED, - // ETIMEDOUT, ENETUNREACH, ENOTFOUND) + // - axios error code indicates network/timeout/DNS (ECONNREFUSED, + // ETIMEDOUT, ENETUNREACH, ENOTFOUND, EAI_AGAIN) or the + // axios ≥ 1.x generic ERR_NETWORK // - axios marks the request as sent but with no response // (err.request set, err.response absent) which is the // canonical "server did not reply" case @@ -142,7 +143,7 @@ export default { // Anything else falls through as a generic error. const isNetworkError = e => { if (!e || typeof e === 'string') return false - if (e.code && /^E(CONN|TIMED|NETUN|NOTFO)/i.test(e.code)) return true + if (e.code && /^(E(CONN|TIMED|NETUN|NOTFO|AI_AGAIN)|ERR_NETWORK)/i.test(e.code)) return true if (e.request && !e.response) return true if (typeof e.message === 'string' && /Network Error|Failed to fetch/i.test(e.message)) return true return false diff --git a/server/automation/service/workflow_analyzer.go b/server/automation/service/workflow_analyzer.go index 66f0c7db40..660746090f 100644 --- a/server/automation/service/workflow_analyzer.go +++ b/server/automation/service/workflow_analyzer.go @@ -69,9 +69,13 @@ func analyzeWorkflow(wf *types.Workflow) types.WorkflowIssueSet { // branch can write. A "branch" is defined as the set of steps // strictly between a fork (or entry) and the join, following // parent edges backwards from each direct parent of the join. + // The home join ID is passed in so the walker can stop at any + // *other* join it encounters — otherwise, in nested parallels, + // the outer branch's writes get unioned into every inner + // branch and produce spurious inner-join conflicts. branchWrites := make([]map[string]bool, 0, len(parents[s.ID])) for _, parentID := range parents[s.ID] { - writes := collectBranchWrites(parentID, stepByID, parents) + writes := collectBranchWrites(parentID, s.ID, stepByID, parents) if len(writes) > 0 { branchWrites = append(branchWrites, writes) } @@ -103,7 +107,7 @@ func analyzeWorkflow(wf *types.Workflow) types.WorkflowIssueSet { for _, name := range conflicts { warnings = warnings.Append( fmt.Errorf( - "variable %q is written by more than one parallel branch joining here; paths[0] wins at runtime, other branches' values are discarded. Rename the conflicting outputs (e.g. %s_a, %s_b) if you need them all.", + "variable %q is written by more than one parallel branch joining here; the value from the first-configured parent wins at runtime and the others are discarded. Rename the conflicting outputs (e.g. %s_a, %s_b) if you need them all.", name, name, name, ), map[string]int{"step": stepIdx[s.ID]}, @@ -116,15 +120,25 @@ func analyzeWorkflow(wf *types.Workflow) types.WorkflowIssueSet { // collectBranchWrites walks parent edges backwards from the given // starting step, collecting the set of variable names that any step -// on the branch can write. Walking stops at a fork gateway (that's -// where the branch began) or when no more parents exist (workflow -// entry). The starting step itself is included. +// on the branch can write. Walking stops when it hits: +// +// - A fork gateway (upstream boundary of the branch). +// - Any join gateway other than `homeJoinID`. In nested parallels, +// an inner branch's upstream chain passes through the inner join, +// then the inner fork, then the outer branch — walking through +// the inner join would pull the outer branch's writes into every +// inner branch and produce spurious inner-join conflicts. Stopping +// at any *other* join cuts that bleed. +// - No more parent edges (workflow entry). +// +// The starting step itself is included. // // Cycles are possible if the workflow has iterators (loop-back edges). // A visited set prevents infinite walks; iterator bodies still get // their writes collected on the first visit. func collectBranchWrites( startID uint64, + homeJoinID uint64, stepByID map[uint64]*types.WorkflowStep, parents map[uint64][]uint64, ) map[string]bool { @@ -149,6 +163,13 @@ func collectBranchWrites( return } + // Stop at any join other than the one we're analyzing. The + // inner join's own analysis will handle its incoming branches + // independently; we must not cross it. + if s.Kind == types.WorkflowStepKindGateway && s.Ref == "join" && id != homeJoinID { + return + } + for name := range stepWrites(s) { writes[name] = true } @@ -184,18 +205,27 @@ func stepWrites(s *types.WorkflowStep) map[string]bool { } case types.WorkflowStepKindFunction, - types.WorkflowStepKindIterator, types.WorkflowStepKindExecWorkflow: - // Function / iterator / subworkflow call: step results are - // the declared outputs. For ExecWorkflow specifically, the - // runtime evaluates s.Results against the subworkflow's - // return value, so the declared targets ARE the contract. + // Function / subworkflow call: step results are the declared + // outputs. For ExecWorkflow specifically, the runtime evaluates + // s.Results against the subworkflow's return value, so the + // declared targets ARE the contract. for _, r := range s.Results { if r != nil && r.Target != "" { out[r.Target] = true } } + case types.WorkflowStepKindIterator: + // Iterator step results (item, counter, isFirst, ...) are + // loop-local: they're injected into the body scope on each + // iteration and do NOT persist past the iterator's exit path. + // Treating them as branch writes produces false positives + // (two parallel iterators both called "item" are not actually + // in conflict at the downstream join). Steps inside the + // iterator body still get their writes collected through the + // normal parent-edge walk. + case types.WorkflowStepKindErrHandler: // Error handler declares author-chosen variable names for // error / errorMessage / errorStepID via Results targets. diff --git a/server/automation/service/workflow_converter.go b/server/automation/service/workflow_converter.go index 0f5af14023..2013ae5b84 100644 --- a/server/automation/service/workflow_converter.go +++ b/server/automation/service/workflow_converter.go @@ -222,11 +222,16 @@ func (svc workflowConverter) makeGraph(def *types.Workflow) (*wfexec.Graph, type // if this func returns nil for step and error, assume unresolved dependencies func (svc workflowConverter) workflowStepDefConv(g *wfexec.Graph, def *types.Workflow, s *types.WorkflowStep, in, out []*types.WorkflowPath) (bool, error) { if err := svc.parseExpressionsAs("argument", s.Arguments...); err != nil { - return false, errors.Internal("invalid %s step (ref: %q): %s", s.Kind, s.Ref, err).Wrap(err) + // parseExpressionsAs already wraps its inner error with a + // descriptive message via fmt.Errorf("%w"), so the returned + // error's .Error() text already includes the full chain. + // Don't add another .Wrap(err) on top — that would double + // the inner error in both the message AND the wrap chain. + return false, errors.Internal("invalid %s step (ref: %q): %s", s.Kind, s.Ref, err) } if err := svc.parseExpressionsAs("result", s.Results...); err != nil { - return false, errors.Internal("invalid %s step (ref: %q): %s", s.Kind, s.Ref, err).Wrap(err) + return false, errors.Internal("invalid %s step (ref: %q): %s", s.Kind, s.Ref, err) } conv, err := func() (wfexec.Step, error) { @@ -982,6 +987,27 @@ func verifyStep(s *types.WorkflowStep, in, out types.WorkflowPathSet) types.Work } return nil }, + // If severity is provided as a literal string expression, + // validate it at save time. Non-literal expressions (e.g. + // vars.level) fall through and are normalised at runtime. + func() error { + sev := types.ExprSet(s.Arguments).GetByTarget("severity") + if sev == nil { + return nil + } + lit, ok := severityLiteralValue(sev.Expr) + if !ok { + return nil + } + switch lit { + case "error", "warning", "info": + return nil + } + return errors.Internal( + "%s step severity must be one of \"error\", \"warning\" or \"info\" (got %q)", + s.Kind, lit, + ) + }, zero(results), last, ) @@ -1072,3 +1098,28 @@ func verifyStep(s *types.WorkflowStep, in, out types.WorkflowPathSet) types.Work return ii } + +// severityLiteralValue returns the unquoted content of an expression +// string when it is a simple single- or double-quoted literal (e.g. +// "error" or 'warning'), along with true. For any other expression — +// a variable reference, a function call, a concatenation — it returns +// "", false so the caller can skip save-time validation and leave the +// runtime normalisation to do its job. +func severityLiteralValue(expr string) (string, bool) { + s := strings.TrimSpace(expr) + if len(s) < 2 { + return "", false + } + q := s[0] + if (q != '"' && q != '\'') || s[len(s)-1] != q { + return "", false + } + inner := s[1 : len(s)-1] + // Reject anything that still contains a quote of the same kind — + // that would mean the literal is not the whole expression (e.g. + // "foo" + "bar" — each side is a literal but the whole isn't). + if strings.ContainsRune(inner, rune(q)) { + return "", false + } + return inner, true +} diff --git a/server/pkg/wfexec/gateways.go b/server/pkg/wfexec/gateways.go index a0698a736d..1ee702c6da 100644 --- a/server/pkg/wfexec/gateways.go +++ b/server/pkg/wfexec/gateways.go @@ -2,6 +2,7 @@ package wfexec import ( "context" + "encoding/json" "fmt" "strings" "sync" @@ -156,16 +157,33 @@ func (gw *joinGateway) Exec(_ context.Context, r *ExecRequest) (ExecResponse, er // branch and returns a human-readable warning for every key that has // more than one distinct value across branches. // -// The per-value dedup uses the expr iterator's string rendering as a -// stable fingerprint. Not perfect for complex types but good enough to -// catch the common case of "branch A set x to 1, branch B set x to 2". -// False positives (same logical value, different formatting) are -// acceptable — this is an advisory warning, not a hard error. +// The per-value fingerprint is produced by JSON-marshaling the underlying +// value with map keys sorted. This gives a stable, deterministic string +// even for map-valued contributors — fmt.Sprintf("%v", ...) prints maps +// in random iteration order and pointers as addresses, which produces +// both false positives and false negatives. encoding/json sorts object +// keys on output (see encoding/json package docs), so two structurally +// equal maps serialise identically. +// +// Values that fail to marshal (cyclic references, unsupported types) +// fall back to "%v" formatting; they're still compared consistently +// within a single run, just not robustly across types. func (gw *joinGateway) detectConflicts(sessionID uint64) []string { type contributor struct { - pathIdx int - origin string // "scope" or "results" - value string + parentID uint64 + pathIdx int + origin string // "scope" or "results" + value string + } + + fingerprint := func(tv expr.TypedValue) string { + if tv == nil { + return "null" + } + if b, err := json.Marshal(tv.Get()); err == nil { + return string(b) + } + return fmt.Sprintf("%v", tv.Get()) } perKey := make(map[string][]contributor) @@ -176,12 +194,17 @@ func (gw *joinGateway) detectConflicts(sessionID uint64) []string { if v == nil { continue } + var parentID uint64 + if p != nil { + parentID = p.ID() + } _ = v.Each(func(k string, tv expr.TypedValue) error { - val := "" - if tv != nil { - val = fmt.Sprintf("%v", tv.Get()) - } - perKey[k] = append(perKey[k], contributor{pathIdx: i, origin: origin, value: val}) + perKey[k] = append(perKey[k], contributor{ + parentID: parentID, + pathIdx: i, + origin: origin, + value: fingerprint(tv), + }) return nil }) } @@ -207,17 +230,27 @@ func (gw *joinGateway) detectConflicts(sessionID uint64) []string { continue } - // build a list of contributing branches for the warning + // build a list of contributing branches for the warning, + // naming branches by their parent step ID so authors can + // correlate with the canvas instead of an abstract paths[i]. branchDescs := make([]string, 0, len(cc)) for _, c := range cc { branchDescs = append(branchDescs, - fmt.Sprintf("paths[%d].%s=%s", c.pathIdx, c.origin, truncateForWarning(c.value)), + fmt.Sprintf("step %d.%s=%s", c.parentID, c.origin, truncateForWarning(c.value)), ) } + + // name the winner — paths[0] — by its actual parent step ID + var winner uint64 + if len(gw.paths) > 0 && gw.paths[0] != nil { + winner = gw.paths[0].ID() + } + warnings = append(warnings, fmt.Sprintf( - "variable %q has conflicting values at parallel join (%s); paths[0] wins", + "variable %q has conflicting values at parallel join (%s); the value from step %d wins", key, strings.Join(branchDescs, ", "), + winner, )) } @@ -225,13 +258,15 @@ func (gw *joinGateway) detectConflicts(sessionID uint64) []string { } // truncateForWarning clips long values so a warning listing 8 branches -// doesn't explode into an unreadable wall of text. +// doesn't explode into an unreadable wall of text. Rune-aware so that +// non-ASCII values can never be split mid-codepoint. func truncateForWarning(s string) string { - const max = 40 - if len(s) <= max { + const maxRunes = 40 + runes := []rune(s) + if len(runes) <= maxRunes { return s } - return s[:max] + "…" + return string(runes[:maxRunes]) + "…" } // forkGateway handles forking to multiple paths From 1694079115295721ed8d7bc99692d75104a0cb8e Mon Sep 17 00:00:00 2001 From: Toocky Date: Sat, 11 Apr 2026 19:16:23 -0600 Subject: [PATCH 18/19] Address review: latent bug hardening MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review M3: Error.vue computed getters (messageArg/titleArg/severityArg) no longer mutate item.config.arguments. They were previously calling findOrCreateArg, which pushed a new arg onto the array and invoked this.\$set — a side effect inside a computed getter that happened to work today only because created() pre-populated every target and the create branch was never hit at runtime. Any future filter on the arguments array would re-enter the mutating getter during reactivity graph evaluation. The getters are now pure findArg + makeArg fallback reads; mutation is hoisted into a new ensureArgs() method called from created() and implicitly by findOrCreateArg for write-path callers. Review M8: when workflowCreate/workflowUpdate returns an unexpected shape without a workflowID, the save handler now console.warns the raw payload before toasting. The toast still shows a user-facing message but the malformed payload is no longer silently discarded, so the path is actually diagnosable from the browser console. Review m2: document the concurrency model of State.warnings instead of guarding it. Each *State is owned by a single step-execution goroutine for its entire lifecycle — Session.exec and the corresponding eventHandler run on the same goroutine — and sibling parallel branches use distinct *State values. No cross-state sharing means no mutex is required, just the documentation of the invariant so future code doesn't accidentally cross-post. Review m9 (no change, verified): the title-only severity default allegedly mismatches between Go default and frontend variant. Verified this is not a real mismatch. Backend attaches workflow.error.severity meta whenever title or severity is non-default; when only title is set, severity falls through as "error", which the frontend maps to variant "danger". When neither is set, no meta attaches and the legacy plain-toast path runs. All three cases are consistent. --- .../src/components/Configurator/Error.vue | 65 ++++++++++++------- .../workflow/src/views/Workflow/Editor.vue | 6 +- server/pkg/wfexec/state.go | 8 +++ 3 files changed, 55 insertions(+), 24 deletions(-) diff --git a/client/web/workflow/src/components/Configurator/Error.vue b/client/web/workflow/src/components/Configurator/Error.vue index 9ed6b2419c..1c418c0203 100644 --- a/client/web/workflow/src/components/Configurator/Error.vue +++ b/client/web/workflow/src/components/Configurator/Error.vue @@ -136,14 +136,19 @@ export default { }, computed: { + // Computed getters are pure reads. All four allowed-target args + // are pre-created in `created()` and also re-created by + // `ensureArgs()` before any method that needs write access. This + // keeps the computeds side-effect-free so Vue's reactivity graph + // can't re-enter the getter while it is mutating arguments. messageArg () { - return this.findOrCreateArg('message') + return this.findArg('message') || makeArg('message') }, titleArg () { - return this.findOrCreateArg('title') + return this.findArg('title') || makeArg('title') }, severityArg () { - return this.findOrCreateArg('severity') + return this.findArg('severity') || makeArg('severity') }, // severityIsLiteral: true when the severity expression is either @@ -181,31 +186,45 @@ export default { }, created () { - // Normalise config.arguments: keep only allowed targets, ensure all of - // them exist (even if empty) so v-model bindings are stable. - const existing = Array.isArray(this.item.config.arguments) ? this.item.config.arguments : [] - const byTarget = {} - existing.forEach(({ target, type, value, expr }) => { - if (!ALLOWED_TARGETS.includes(target)) return - byTarget[target] = { - target, - type: type || 'String', - expr: expr || (value ? `"${value}"` : ''), - } - }) - - const args = ALLOWED_TARGETS.map(t => byTarget[t] || makeArg(t)) - this.$set(this.item.config, 'arguments', args) + this.ensureArgs() }, methods: { - findOrCreateArg (target) { + // ensureArgs normalises item.config.arguments: keeps only allowed + // targets and guarantees one entry per target, even if empty. Safe + // to call from any write path. Does NOT run from computed getters. + ensureArgs () { + const existing = Array.isArray(this.item.config.arguments) ? this.item.config.arguments : [] + const byTarget = {} + existing.forEach(({ target, type, value, expr }) => { + if (!ALLOWED_TARGETS.includes(target)) return + byTarget[target] = { + target, + type: type || 'String', + expr: expr || (value ? `"${value}"` : ''), + } + }) + + const args = ALLOWED_TARGETS.map(t => byTarget[t] || makeArg(t)) + this.$set(this.item.config, 'arguments', args) + }, + + // findArg is a pure read. Returns the matching arg object or + // undefined when absent. Safe to call from computeds. + findArg (target) { const args = this.item.config.arguments || [] - let arg = args.find(a => a.target === target) + return args.find(a => a.target === target) + }, + + // findOrCreateArg is a mutating read used by write paths only. + // It guarantees the arg exists on item.config.arguments before + // returning it. Never call from inside a computed getter — use + // findArg with a fallback instead. + findOrCreateArg (target) { + let arg = this.findArg(target) if (!arg) { - arg = makeArg(target) - args.push(arg) - this.$set(this.item.config, 'arguments', args) + this.ensureArgs() + arg = this.findArg(target) || makeArg(target) } return arg }, diff --git a/client/web/workflow/src/views/Workflow/Editor.vue b/client/web/workflow/src/views/Workflow/Editor.vue index 81d07333ec..fbfbc2c3f9 100644 --- a/client/web/workflow/src/views/Workflow/Editor.vue +++ b/client/web/workflow/src/views/Workflow/Editor.vue @@ -213,7 +213,11 @@ export default { if (!saved || !saved.workflowID) { // Defensive: API returned an unexpected shape. Don't crash on - // saved.workflowID later down the chain. + // saved.workflowID later down the chain. Emit the raw payload + // to the browser console so the malformed response is still + // diagnosable — the toast can't show it safely and silently + // dropping it would make this path impossible to debug. + console.warn('workflow save: unexpected response payload', saved) this.toastDanger( this.$t('notification:failed-save-unexpected-response'), this.$t('notification:failed-save'), diff --git a/server/pkg/wfexec/state.go b/server/pkg/wfexec/state.go index d433b13225..86f9e6030a 100644 --- a/server/pkg/wfexec/state.go +++ b/server/pkg/wfexec/state.go @@ -76,6 +76,14 @@ type ( // wrapper and surfaced via MakeFrame into Frame.Warnings so the // editor test panel can render them alongside the existing // action/error info. + // + // Concurrency: each *State is owned by a single step-execution + // goroutine for its entire lifecycle (see Session.exec and the + // goroutine launched around it). Appends to warnings happen in + // exec(); MakeFrame is called synchronously on the same + // goroutine from the subsequent eventHandler invocation. + // Sibling parallel branches run on distinct *State values so + // there is no cross-state sharing. No mutex needed. warnings []string } ) From 1e36f68a689efa65341c989a8e1ab044b0df3e7a Mon Sep 17 00:00:00 2001 From: Toocky Date: Sat, 11 Apr 2026 19:26:49 -0600 Subject: [PATCH 19/19] Address review: lock masking matrix + document intent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review M6: add Test_writeHttpJSON_workflowErrorSafeBypass to lock the four-cell matrix for KindAutomation error masking so the opt-in MetaWorkflowErrorSafe bypass semantics cannot be silently changed by future refactors. The test exercises: 1. automation + safe flag + masked → meta preserved 2. automation + no flag + masked → meta stripped 3. non-automation + safe flag + masked → meta stripped (flag is a no-op on non-automation kinds — defensive regression check against a future author bypassing the kind gate) 4. automation + safe flag + unmasked → meta preserved All four pass. The flat message is always visible in all cases, matching the pre-existing contract that Error.Error() is always sent to the client even under mask. Review M2: add a prominent doc block on State.errHandlerResults explaining the intentional non-propagation to the running scope. An Expressions step downstream of an error handler that references a to-be-populated error variable name will not see it until the error actually fires and the injection path runs. This is by design — silently seeding author-chosen variable names into scope would pollute the namespace and make debugging harder (a nil from a reference cannot be distinguished from a typo). Documenting the contract so future readers do not "fix" it into scope pollution. --- server/pkg/errors/http_test.go | 52 ++++++++++++++++++++++++++++++++++ server/pkg/wfexec/state.go | 13 +++++++++ 2 files changed, 65 insertions(+) diff --git a/server/pkg/errors/http_test.go b/server/pkg/errors/http_test.go index ced4e1797d..13e2122769 100644 --- a/server/pkg/errors/http_test.go +++ b/server/pkg/errors/http_test.go @@ -88,3 +88,55 @@ func Test_writeHttpJSON(t *testing.T) { req.NotContains(buf.String(), "meta") req.NotContains(buf.String(), "stack") } + +// Locks the masking matrix for KindAutomation errors so the +// MetaWorkflowErrorSafe opt-in bypass semantics cannot be +// silently changed by future refactors. +// +// Cells: +// 1. KindAutomation + MetaWorkflowErrorSafe=true + masked → meta preserved +// 2. KindAutomation + no flag + masked → meta stripped +// 3. KindInternal + MetaWorkflowErrorSafe=true + masked → meta stripped +// (flag is a no-op on non-automation kinds) +// 4. KindAutomation + MetaWorkflowErrorSafe=true + unmasked → meta preserved +func Test_writeHttpJSON_workflowErrorSafeBypass(t *testing.T) { + req := require.New(t) + + // 1. automation + safe flag + masked → meta visible + safe := Automation("author message").Apply( + Meta(MetaWorkflowErrorSafe, true), + Meta("workflow.error.title", "Upstream failure"), + ) + buf := bytes.NewBuffer(nil) + writeHttpJSON(context.Background(), buf, safe, true) + req.Contains(buf.String(), "author message", "message must always be visible") + req.Contains(buf.String(), "workflow.error.title", "safe flag must preserve meta under mask") + req.Contains(buf.String(), "Upstream failure") + + // 2. automation + no flag + masked → meta stripped + plain := Automation("runtime error").Apply(Meta("internal", "secret")) + buf.Truncate(0) + writeHttpJSON(context.Background(), buf, plain, true) + req.Contains(buf.String(), "runtime error", "flat message must still be visible") + req.NotContains(buf.String(), "internal", "unflagged automation meta must be stripped under mask") + req.NotContains(buf.String(), "secret") + + // 3. non-automation kind + safe flag + masked → meta stripped + // (the flag is only honoured for KindAutomation — it must not + // leak meta from other error kinds even if the flag is set) + nonAuto := Internal("internal boom").Apply( + Meta(MetaWorkflowErrorSafe, true), + Meta("leak", "pls no"), + ) + buf.Truncate(0) + writeHttpJSON(context.Background(), buf, nonAuto, true) + req.Contains(buf.String(), "internal boom") + req.NotContains(buf.String(), "leak", "safe flag on non-automation must be ignored") + req.NotContains(buf.String(), "pls no") + + // 4. automation + safe flag + unmasked → meta visible (debug path) + buf.Truncate(0) + writeHttpJSON(context.Background(), buf, safe, false) + req.Contains(buf.String(), "author message") + req.Contains(buf.String(), "Upstream failure") +} diff --git a/server/pkg/wfexec/state.go b/server/pkg/wfexec/state.go index 86f9e6030a..8045d1d0a4 100644 --- a/server/pkg/wfexec/state.go +++ b/server/pkg/wfexec/state.go @@ -62,6 +62,19 @@ type ( // `errorMessage` etc. disappear from scope on any later error. // Carried alongside `errHandler` in State.Next() with the same // lifecycle — set together, replaced together, cleared together. + // + // Intentional non-propagation to the running scope: these + // variables are NOT merged into the happy-path scope. An + // Expressions step downstream of the error handler that + // references, say, `myErrorMessage` will not see it until an + // error actually fires and the error-injection path writes + // the real value under that name. This is by design — silently + // seeding author-chosen variable names into scope as empty/nil + // would pollute the scope namespace and make debugging harder + // (expression referencing `myErrorMessage` returning nil would + // not be distinguishable from a typo). Authors who want a + // default value should initialise it explicitly in an + // Expressions step before the error handler. errHandlerResults *expr.Vars // error handled flag, this gets restarted on every new state!