Skip to content

fix(workflow): a save accepted workflows the engine cannot run - #70

Merged
agnt-gg merged 2 commits into
agnt-gg:mainfrom
rimusz:fix/workflow-save-validates-engine-shape
Aug 23, 2026
Merged

fix(workflow): a save accepted workflows the engine cannot run#70
agnt-gg merged 2 commits into
agnt-gg:mainfrom
rimusz:fix/workflow-save-validates-engine-shape

Conversation

@rimusz

@rimusz rimusz commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

What

POST /workflows/save accepted workflows that WorkflowEngine cannot execute, stored them, and answered 201. The mismatch surfaced later, once, at activation — as a TypeError inside the workflow process that reached the caller as an unqualified error.

This adds a shape check derived from what the engine actually dereferences, and wires it into both the save path and the engine constructor.

Why

The engine reads exactly one node/edge shape. WorkflowDesigner.vue produces it. Every other writer invents its own, and nothing checked:

TypeError: Cannot read properties of undefined (reading 'toLowerCase')
    at WorkflowEngine._initializeNodeNameMapping (WorkflowEngine.js:518)

node.text was undefined. Two things made this hard to see:

  1. The save reported success. saveWorkflow validated only that the body was an object, then JSON.stringify'd it into the row. The workflow appeared in the list looking fine.
  2. The error never left the workflow process. WorkflowProcessBridge.fetchWorkflowState returns { status: 'error' } for any caught IPC exception, so /workflows/:id/status reported error with no indication of which node or which field.

On one developer machine 22 of 22 stored workflows were in a shape the engine could not execute, across five distinct variants — none of them authored in the UI:

shape source
fields, id, nodeType, position, type marketplace / plugin-style installs
data, id, position, type react-flow style, agent tooling
data, id, type react-flow style
code, id, name, type script-style
config, id, name, type config-style

Edges in those rows use source/target; the engine reads edge.start.id / edge.end.id.

How

backend/src/workflow/validateWorkflowShape.js — a pure, dependency-free check. Every rule corresponds to a property the engine dereferences, and the module comment cites the line:

rule engine site
node.text non-empty WorkflowEngine.js:518node.text.toLowerCase()
node.id non-empty, unique :197new Map(nodes.map(n => [n.id, n]))
node.type non-empty :114import(.../triggers/${node.type}.js)
edge.start.id resolves :201, :204
edge.end.id resolves :405, :408, :527

Nothing here encodes taste. If the engine stops reading a field, the rule for it should go too.

Wired in twice, because save-time validation alone would only protect rows written from now on:

  • saveWorkflow answers 400 with a per-problem list naming the node and the field, and stores nothing.
  • The WorkflowEngine constructor asserts the same shape. Rows that predate this — plus anything arriving via /workflows/import or a direct DB write, neither of which passes through save — now fail with a diagnosis instead of a TypeError.

Two deliberate choices:

  • Absent nodes/edges are normalised to [], not rejected. A blank draft must stay savable, and both collections are iterated unconditionally (:516, :527). WorkflowImportService.js:39-40 already coerces them the same way.
  • The engine normalises into a copy, leaving the caller's object alone — the worker parses it straight out of the DB row. Covered by a test.

Testing

50 new tests across 3 files. 11 fail without this change, including a direct reproduction of the original failure (expected TypeError: Cannot read properties of unde… to not be an instance of TypeError).

The fixtures under "shapes found in a real database" are the verbatim key sets of the five malformed variants above, not invented examples.

Mutation tested — 8 mutants, 8 killed, 0 survived:

mutant result
stop requiring node.text killed (14)
stop reporting duplicate node ids killed (1)
stop checking edges resolve killed (4)
stop requiring node.type killed (3)
reject a blank draft (over-strict) killed (3)
normalise by mutating the caller's object killed (1)
drop the engine-side assert killed (3)
drop save-time validation killed (6)

Full backend suite: 292 passed / 293 files. The one failure, UpdateScheduler.status.test.js > replaces the previous pass rather than accumulating, is pre-existing and unrelated — I verified it fails identically on untouched origin/main (1 failed | 289 passed), it imports nothing this PR touches, and it passes in isolation. Looks order-dependent.

Notes for the reviewer

  • This is a behaviour change at the API boundary: callers currently POSTing a malformed workflow get 201 today and will get 400 after this. That is the point — those workflows could never run — but it is worth a deliberate nod, particularly for anything installing workflows from a remote marketplace (frontend/src/store/features/marketplace.js:442,532 passes assetData through untransformed).
  • Not addressed here: WorkflowProcessBridge.fetchWorkflowState collapsing every IPC exception to { status: 'error' } is why this took so long to diagnose. Worth surfacing the underlying message, but it is a separate change.
  • Also not addressed: /workflows/import still does not validate. The engine-side assert catches it at activation, but a 400 at import time would be friendlier.

WorkflowEngine reads exactly one node/edge shape. The workflow designer
produces it; every other writer invents its own. Nothing checked.

saveWorkflow validated only that the body was an object, then stringified
whatever arrived into the row. The mismatch surfaced later, once, at
activation:

    TypeError: Cannot read properties of undefined (reading 'toLowerCase')
        at WorkflowEngine._initializeNodeNameMapping (WorkflowEngine.js:518)

and reached the caller as `{ status: 'error' }`, because
WorkflowProcessBridge.fetchWorkflowState returns that for any caught IPC
exception. So the save answered 201, the workflow sat in the list looking
fine, and activation failed with a string naming neither the node nor the
field. On one developer machine 22 of 22 stored workflows were in a shape
the engine could not execute, across five different variants.

Adds validateWorkflowShape: a pure check of only those properties the
engine dereferences — node.text (:518), node.id (:197), node.type (:114),
edge.start.id (:201) and edge.end.id (:405,:527) — plus unique node ids and
edge endpoints that resolve. Nothing here encodes taste; if the engine
stops reading a field the rule for it should go too.

Wired in twice, because save-time validation alone would only protect rows
written from now on:

  - saveWorkflow answers 400 with a per-problem list naming the node and
    the field, and stores nothing.
  - The WorkflowEngine constructor asserts the same shape, so rows that
    predate this — and those arriving via /workflows/import or a direct DB
    write, neither of which passes through save — fail with a diagnosis
    instead of a TypeError.

Absent nodes/edges are normalised to [] rather than rejected: a blank draft
must stay savable, and both collections are iterated unconditionally.
WorkflowImportService already coerces them the same way. The engine
normalises into a copy, leaving the caller's object alone.

50 tests. 11 fail without this change, including the original TypeError.
Fixtures under "shapes found in a real database" are the verbatim key sets
of the five malformed variants observed in the wild.
Copilot AI lite review requested due to automatic review settings August 22, 2026 09:24

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new validator currently allows edges to “resolve” even when there are zero declared nodes, which can let invalid workflows pass validation despite being non-executable.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR prevents storing workflows that the backend WorkflowEngine cannot execute by introducing a shared, engine-derived shape validation and applying it both at the /workflows/save API boundary and inside the engine constructor to avoid late, unqualified TypeError failures at activation.

Changes:

  • Added validateWorkflowShape / assertWorkflowShape to validate the exact node/edge fields the engine dereferences.
  • Wired shape validation into WorkflowService.saveWorkflow (400 + per-problem details; stores nothing on failure) and into WorkflowEngine construction (asserts + normalizes missing nodes/edges to []).
  • Added focused Vitest coverage for save-path behavior and engine-constructor backstop behavior.
File summaries
File Description
backend/src/workflow/validateWorkflowShape.js New workflow shape validator + assertion used by both API and engine.
backend/src/workflow/validateWorkflowShape.test.js Unit tests covering accepted designer shape + rejected real-world malformed shapes.
backend/src/services/WorkflowService.js Adds save-time shape validation returning 400 with detailed diagnostics.
backend/src/services/WorkflowService.saveWorkflow.test.js Tests that malformed workflows are rejected and never persisted/updated.
backend/src/workflow/WorkflowEngine.js Adds constructor assertion and normalization for missing nodes/edges.
backend/src/workflow/WorkflowEngine.shapeGuard.test.js Tests engine constructor fails with diagnosis (not TypeError) and preserves input.
Review details
  • Files reviewed: 6/6 changed files
  • Comments generated: 4
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread backend/src/workflow/validateWorkflowShape.js Outdated
Comment thread backend/src/workflow/validateWorkflowShape.js Outdated
Comment thread backend/src/workflow/WorkflowEngine.js Outdated
Comment thread backend/src/workflow/validateWorkflowShape.js Outdated
…ilot review)

Three things Copilot was right about.

An edge list with an empty node list skipped the unknown-node-id check
entirely, because the guard was `nodeList.length && !declaredIds.has(...)`.
That is not a blank draft — a blank draft has no edges either — and letting
it through left _findStartNodes with no start node and a fallback that
indexes nodes[0] on an empty array. Now checked unconditionally.

describe() rendered an unexpected object as "a object", which reached the
caller verbatim in the 400 `details`.

The comments cited WorkflowEngine.js line numbers that this change itself
shifted (:516 -> 530, :527 -> 541). Replaced with method names in all four
files, since the numbers drift on the first edit above them.

53 tests, +3 for the endpoint rule and the article.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants