From fcbd2cb94ef738930cb97ffa1245c316938861e7 Mon Sep 17 00:00:00 2001
From: taly <taly@radware.com>
Date: Wed, 1 Jul 2026 11:43:10 +0300
Subject: [PATCH 1/2] fix: skip unknown step types / transform ops with a
 warning instead of crashing

Cross-app parity with the flowrunner-cli fix. The JS engine had the same
two forward-compat failures:
- flowRunner.js threw on an unknown step type -> the whole run halted.
- transformOps.js silently downgraded an unknown transform op to
  base64_decode -> the wrong operation executed.

Both now degrade gracefully (skip-with-machine-readable-warning, continue):
- Unknown step type -> {status:'skipped', unsupported:true} + a user warning.
- Unknown transform op -> recorded in executeTransformOps' warnings[]
  (TRANSFORM_OP_UNSUPPORTED marker), surfaced by the transform step; the op's
  output var is left unset. normalizeTransformOp is left tolerant because the
  editor (flowStepComponents.js) also calls it.

Also fixes the Jest harness: setup.js require()d the vendored UMD
drawflow.min.js, unloadable under the app's "type":"module", so npm test was
fully broken (CI never runs it). Added a CommonJS marker package.json in the
vendored dir. This unblocked the suite (122 passing) and revealed 4
pre-existing flowVisualizer.test.js failures (tracked separately).

Adds 2 unit tests (RED->GREEN). Bible updated (changelog + gotchas).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 __tests__/flowRunner.test.js        | 15 +++++++++++++++
 __tests__/transformOps.test.js      | 17 +++++++++++++++++
 assets/vendor/drawflow/package.json |  4 ++++
 changelog.md                        |  2 ++
 flowRunner.js                       | 20 +++++++++++++++++++-
 gotchas.md                          |  6 ++++++
 transformOps.js                     | 17 +++++++++++++++++
 7 files changed, 80 insertions(+), 1 deletion(-)
 create mode 100644 assets/vendor/drawflow/package.json

diff --git a/__tests__/flowRunner.test.js b/__tests__/flowRunner.test.js
index fe016aa..5f03ce8 100644
--- a/__tests__/flowRunner.test.js
+++ b/__tests__/flowRunner.test.js
@@ -156,6 +156,21 @@ describe('FlowRunner', () => {
         global.clearTimeout = originalGlobalClearTimeout;
     });
 
+    describe('graceful degradation (unknown step types)', () => {
+        test('unknown step type is skipped with a warning, not thrown or stopped', async () => {
+            const step = { id: 'u1', name: 'Future Step', type: 'future_unknown_type' };
+            await runner._executeSingleStepLogic(step, {});
+            const last = runner.state.results[runner.state.results.length - 1];
+            expect(last.status).toBe('skipped');
+            expect(last.unsupported).toBe(true);
+            // the flow is NOT halted
+            expect(runner.state.stopRequested).toBe(false);
+            expect(mockOnError).not.toHaveBeenCalled();
+            // a warning was surfaced to the user
+            expect(mockOnMessage).toHaveBeenCalled();
+        });
+    });
+
     describe('state management', () => {
         it('should initialize with isRunning and isStepping false', () => {
             expect(runner.isRunning()).toBe(false);
diff --git a/__tests__/transformOps.test.js b/__tests__/transformOps.test.js
index d42cc45..6e62d71 100644
--- a/__tests__/transformOps.test.js
+++ b/__tests__/transformOps.test.js
@@ -36,4 +36,21 @@ describe('transformOps', () => {
         expect(context.decoded.payload.exp).toBe(123);
         expect(context.decoded.header.alg).toBe('none');
     });
+
+    test('unknown transform op is skipped with a warning (not downgraded to base64_decode)', async () => {
+        const context = {};
+        const ops = [
+            // "SGVsbG8" base64url-decodes to "Hello"; the old bug set decoded = "Hello".
+            { op: 'totally_unknown_future_op', set: 'decoded', args: ['SGVsbG8'], options: {} },
+            { op: 'base64_encode', set: 'enc', args: ['hi'], options: { base64: 'url' } },
+        ];
+        const output = await executeTransformOps(ops, context, { evaluatePath: simpleEvaluatePath });
+        expect(context.decoded).toBeUndefined();       // unknown op skipped, NOT base64-decoded
+        expect(context.enc).toBeDefined();             // subsequent known op still ran
+        expect(output.updatedVars).toEqual(['enc']);
+        expect(output.warnings).toHaveLength(1);
+        expect(output.warnings[0].op).toBe('totally_unknown_future_op');
+        expect(output.warnings[0].set).toBe('decoded');
+        expect(output.warnings[0].status).toBe('skipped');
+    });
 });
diff --git a/assets/vendor/drawflow/package.json b/assets/vendor/drawflow/package.json
new file mode 100644
index 0000000..c409e42
--- /dev/null
+++ b/assets/vendor/drawflow/package.json
@@ -0,0 +1,4 @@
+{
+  "type": "commonjs",
+  "//": "Vendored UMD build of Drawflow. The app's root package.json is \"type\": \"module\", which would make Node treat this .js as ESM and break the Jest setup's require() of it. This marker keeps only this vendored dir CommonJS so it stays require-able in Node/Jest. The browser loads drawflow.min.js via a plain <script> tag, so this has no effect at runtime."
+}
diff --git a/changelog.md b/changelog.md
index 577d306..14e3f11 100644
--- a/changelog.md
+++ b/changelog.md
@@ -16,6 +16,8 @@
 - **CI:** added `schemas/**` to `paths-ignore` so schema-doc changes don't trigger a rebuild.
 - **Fix (schema drift):** `schemas/flow-v1.schema.json` declared the internal model names `thenSteps`/`elseSteps`/`loopSteps`; corrected to the real wire keys `then`/`else`/`steps` (verified against sample flows and `flowCore.js:111-116`; the CLI reads only these).
 - **Research / proposal:** produced the multi-agent councilled [FlowMap & UX evolution proposal](docs/flowmap-evolution.md) and documented the **cross-app FlowMap contract** (three apps share `.flow.json`: FlowRunner UI, flowrunner-cli, ShowRunner portal) in `architecture.md`/`gotchas.md`/`masterplan.md` + the schema `$comment`. **Direction:** sequence over big-bang; graceful-degradation (unknown step/op ⇒ skip-with-warning, never crash) first; adopt React Flow for the node view only behind packaged-build evidence; `schemaVersion` + subflows gated on *deployed* CLI support.
+- **Fix (graceful degradation — JS engine, parity with the CLI):** the runner no longer **crashes** on an unknown step type (`flowRunner.js` threw → halted the whole run) and `transformOps.js` no longer **silently downgrades** an unknown transform op to `base64_decode` (the exact bug just fixed in flowrunner-cli). Both now **skip-with-a-machine-readable-warning** and continue: an unknown step type yields a `status:'skipped', unsupported:true` result + a user-visible warning; an unknown transform op is recorded in `executeTransformOps`' `warnings[]` (`TRANSFORM_OP_UNSUPPORTED` marker) and surfaced by the transform step. `normalizeTransformOp` is intentionally left tolerant because the *editor* (`flowStepComponents.js`) calls it — the editor's own silent rewrite of unknown ops on open is a separate follow-up. This is schema-evolution P1 from the proposal.
+- **Test harness fix:** `npm test` was fully broken — `__tests__/setup.js` `require()`s the vendored UMD `drawflow.min.js`, which Node treats as ESM under the app's root `"type": "module"`, so **every** suite failed to load (CI never caught it — `build.yml` runs only `npm run dist`). Added `assets/vendor/drawflow/package.json` (`"type": "commonjs"`) scoping just that vendored dir to CJS (browser `<script>` load unaffected). This unblocked the suite (**122 passing**) and revealed **4 pre-existing failures in `flowVisualizer.test.js`** (Drawflow drag/double-click/add-step under jsdom) — pre-dates this change, tracked as a follow-up.
 
 ---
 
diff --git a/flowRunner.js b/flowRunner.js
index 033637c..761314f 100644
--- a/flowRunner.js
+++ b/flowRunner.js
@@ -469,7 +469,19 @@ export class FlowRunner {
                     result = await this._executeTransformStep(processedStep, stepContext);
                     break;
                 default:
-                    throw new Error(`Unknown step type: ${processedStep.type}`);
+                    // Graceful degradation: an unknown/newer step type is SKIPPED with a
+                    // machine-readable warning instead of throwing (which would halt the
+                    // whole run). status 'skipped' is not an error, so the flow continues.
+                    result = {
+                        status: 'skipped',
+                        output: `Skipped: unsupported step type "${processedStep.type}".`,
+                        error: null,
+                        extractionFailures: [],
+                        unsupported: true,
+                    };
+                    logger.warn(`[FlowRunner] STEP_TYPE_UNSUPPORTED id=${step.id} type=${JSON.stringify(processedStep.type)} - skipped, not executed.`);
+                    this.onMessage(`Skipped step "${step.name || step.id}": unsupported step type "${processedStep.type}".`, 'warning');
+                    break;
             }
 
             // Never overwrite the freshly-created context of a loop level when
@@ -520,6 +532,12 @@ export class FlowRunner {
         try {
             const ops = Array.isArray(step.ops) ? step.ops : [];
             const output = await executeTransformOps(ops, context, { evaluatePath: this.evaluatePathFn });
+            if (Array.isArray(output.warnings) && output.warnings.length > 0) {
+                for (const warning of output.warnings) {
+                    logger.warn(`[FlowRunner] Transform step "${step.name}" (ID: ${step.id}): ${warning.message}`);
+                    this.onMessage(`Transform step "${step.name || step.id}": ${warning.message}`, 'warning');
+                }
+            }
             return { status: 'success', output: output, error: null };
         } catch (error) {
             return { status: 'error', output: null, error: error.message || 'Transform step failed' };
diff --git a/gotchas.md b/gotchas.md
index cce0393..09027e8 100644
--- a/gotchas.md
+++ b/gotchas.md
@@ -38,6 +38,12 @@ The `.flow.json` format is shared by **three independently-versioned apps**: Flo
 - The `release` job is gated `if: github.event_name == 'push' && ref is main/master`, so **only a real push/merge to main publishes** — PR and manual runs never touch the live release (verified: dispatch run → `release` skipped). Build a branch without publishing: `gh workflow run "FlowRunner Build" --ref <branch>`, then `gh run download <run-id>`.
 - Trap: `workflow_dispatch`/`pull_request` only work once the workflow file defining them is on the **default branch** — you can't dispatch a trigger that exists only on a feature branch.
 
+### 2c. `npm test` was broken: vendored UMD `require()` under `"type": "module"`
+- **Symptom:** every Jest suite fails to load with `Must use import to load ES Module: .../drawflow.min.js` (0 tests run). Went unnoticed because **CI never runs `npm test`** (`build.yml` runs only `npm run dist`).
+- **Cause:** `__tests__/setup.js` `require()`s the vendored **UMD** `assets/vendor/drawflow/drawflow.min.js`; the root `package.json` `"type": "module"` makes Node treat that `.js` as ESM, so `require()` throws.
+- **Fix:** `assets/vendor/drawflow/package.json` = `{"type": "commonjs"}` scopes only that vendored dir to CJS (the browser loads Drawflow via a `<script>` tag, so runtime is unaffected; the file isn't imported by the app).
+- **Note:** fixing this unmasked **4 pre-existing failures in `flowVisualizer.test.js`** (Drawflow drag/double-click/add-step under jsdom). They pre-date the fix — treat as a known follow-up, not a regression.
+
 ## Execution engine
 
 ### 3. `substituteVariablesInStep` depends on being called as a method
diff --git a/transformOps.js b/transformOps.js
index e98a172..11408b6 100644
--- a/transformOps.js
+++ b/transformOps.js
@@ -239,6 +239,23 @@ export async function executeTransformOps(ops, context, options = {}) {
     const output = { updatedVars: [], warnings: [] };
     const list = Array.isArray(ops) ? ops : [];
     for (let i = 0; i < list.length; i++) {
+        // Graceful degradation: an unknown/newer transform op is SKIPPED with a
+        // machine-readable warning rather than silently downgraded to base64_decode
+        // (which would run the wrong operation) or thrown (which would fail the step).
+        const rawOp = list[i] && typeof list[i] === 'object' ? list[i] : {};
+        if (!isTransformOpName(rawOp.op)) {
+            const setHint = typeof rawOp.set === 'string' ? rawOp.set : null;
+            output.warnings.push({
+                type: 'unsupported_transform_op',
+                op: rawOp.op ?? null,
+                set: setHint,
+                index: i,
+                status: 'skipped',
+                message: `Unsupported transform op "${rawOp.op}" at position ${i + 1}; skipped (${setHint ? `output variable "${setHint}" left unset` : 'no output variable set'}).`,
+            });
+            console.warn(`[transformOps] TRANSFORM_OP_UNSUPPORTED op=${JSON.stringify(rawOp.op)} set=${JSON.stringify(setHint)} index=${i} - skipped, not executed (refusing to silently substitute base64_decode).`);
+            continue;
+        }
         const normalized = normalizeTransformOp(list[i]);
         if (!normalized.set || typeof normalized.set !== 'string') {
             throw new Error(`Transform op ${i + 1} is missing a valid output variable.`);

From 12f9f4236ccfb86fde62ed17c3f2d2286dfa30e1 Mon Sep 17 00:00:00 2001
From: taly <taly@radware.com>
Date: Wed, 1 Jul 2026 12:14:57 +0300
Subject: [PATCH 2/2] test: green flowVisualizer suite (jsdom Drawflow
 interactions)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

All 4 failures were test-setup/isolation issues, not bugs in
flowVisualizer.js. flowVisualizer.js is unchanged.

- "node moved event triggers layout update callback": the nodeMoved
  handler treats the node element's style.left/top as authoritative
  (a real Drawflow drag updates both the data model's pos_x/pos_y AND
  the DOM element's style.left/top in lock-step). The test only mutated
  the data model, so the handler kept reading the stale rendered
  position (100,100). Fixed the test to also set the element's
  style.left/top, matching a real drag. Assertion unchanged.

- "dragging a node updates flow model layout": same DOM-vs-data-model
  issue as above. Additionally, this test created a second
  FlowVisualizer without destroying the beforeEach one, leaking that
  visualizer's .node-editor-modal into document.body and breaking the
  two tests below. Fixed by destroying the old visualizer first and by
  moving the DOM element alongside the data model. Assertions unchanged.

- "double-click opens node editor modal": passed document.querySelector
  ('.node-editor-modal'), which returned a leaked stale modal (display
  "none") from the drag test instead of the current visualizer's modal
  (display "flex"). Fixed by querying visualizer.nodeEditorModal
  directly. Assertions unchanged.

- "add step button triggers add-step callback": same leaked-modal
  problem via document.querySelector('.node-editor-add') — it grabbed a
  stale, unwired button. Fixed by querying the button within
  visualizer.nodeEditorModal. Assertions unchanged.

Full suite: 126 passed, 0 skipped.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 __tests__/flowVisualizer.test.js | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/__tests__/flowVisualizer.test.js b/__tests__/flowVisualizer.test.js
index 49a93c7..3a831d6 100644
--- a/__tests__/flowVisualizer.test.js
+++ b/__tests__/flowVisualizer.test.js
@@ -174,9 +174,17 @@ describe('FlowVisualizer (Drawflow)', () => {
         await waitForVisualizerRender();
 
         const drawflowId = visualizer.nodeIdByStepId.get('step1');
+        // A real Drawflow drag updates BOTH the data model (pos_x/pos_y) AND the
+        // node element's style.left/top in lock-step (see drawflow position()).
+        // The nodeMoved handler treats style.left/top as authoritative, so the
+        // test must move the DOM element too — mutating the data model alone is
+        // not how the interaction reaches the handler.
         const nodeData = visualizer.editor.drawflow.drawflow.Home.data[drawflowId];
         nodeData.pos_x = 300;
         nodeData.pos_y = 400;
+        const nodeEl = container.querySelector(`#node-${drawflowId}`);
+        nodeEl.style.left = '300px';
+        nodeEl.style.top = '400px';
 
         visualizer.editor.dispatch('nodeMoved', drawflowId);
         expect(mockCallbacks.onNodeLayoutUpdate).toHaveBeenCalledWith('step1', 300, 400);
@@ -231,15 +239,23 @@ describe('FlowVisualizer (Drawflow)', () => {
         mockCallbacks.onNodeLayoutUpdate = (id, x, y) => {
             handleVisualizerNodeLayoutUpdate(id, x, y);
         };
+        // Replace the beforeEach visualizer: destroy the old one first so its
+        // modal (appended to document.body) does not leak into later tests.
+        visualizer.destroy();
         visualizer = new FlowVisualizer(container, mockCallbacks);
 
         visualizer.render(mockFlow, null);
         await waitForVisualizerRender();
 
         const drawflowId = visualizer.nodeIdByStepId.get('step1');
+        // Mirror a real Drawflow drag: move both the data model and the DOM
+        // element, which the nodeMoved handler reads as authoritative.
         const nodeData = visualizer.editor.drawflow.drawflow.Home.data[drawflowId];
         nodeData.pos_x = 500;
         nodeData.pos_y = 600;
+        const nodeEl = container.querySelector(`#node-${drawflowId}`);
+        nodeEl.style.left = '500px';
+        nodeEl.style.top = '600px';
 
         visualizer.editor.dispatch('nodeMoved', drawflowId);
 
@@ -255,7 +271,10 @@ describe('FlowVisualizer (Drawflow)', () => {
         const dblClickEvent = new MouseEvent('dblclick', { bubbles: true });
         node.dispatchEvent(dblClickEvent);
 
-        const modal = document.querySelector('.node-editor-modal');
+        // Query THIS visualizer's modal. Each FlowVisualizer appends its own
+        // .node-editor-modal to document.body, so a bare document.querySelector
+        // can return a different visualizer's modal.
+        const modal = visualizer.nodeEditorModal;
         const modalTitle = modal.querySelector('.node-editor-title');
         expect(modal.style.display).toBe('flex');
         expect(modalTitle.textContent).toContain('Step 1');
@@ -268,7 +287,9 @@ describe('FlowVisualizer (Drawflow)', () => {
         const node = container.querySelector('.drawflow-node.flow-node[data-step-id="step1"]');
         node.dispatchEvent(new MouseEvent('dblclick', { bubbles: true }));
 
-        const addButton = document.querySelector('.node-editor-add');
+        // Use THIS visualizer's add button; a bare document.querySelector can
+        // return the button of another visualizer's modal (see modal note above).
+        const addButton = visualizer.nodeEditorModal.querySelector('.node-editor-add');
         addButton.click();
 
         expect(mockCallbacks.onRequestAddStepAfter).toHaveBeenCalledWith(