|
|
@@ -0,0 +1,420 @@
|
|
|
+# Uniform Node Output Envelope - Design
|
|
|
+
|
|
|
+Date: 2026-08-12
|
|
|
+
|
|
|
+## Goal and framing
|
|
|
+
|
|
|
+Every node type in `nodes/` shapes its output differently. A field path like
|
|
|
+`data.result.x` means "look inside the `result` key" only if the upstream node
|
|
|
+happens to call its payload `result`. Swap that node for one that doesn't, and
|
|
|
+the path silently resolves to nothing rather than failing - which is exactly
|
|
|
+what happened in production: a `code` node (`{result, executionTime}`) was
|
|
|
+replaced by a `set-fields` node, `data.result.generate` stopped resolving, the
|
|
|
+condition reading it took the false branch, and an image-generation pipeline
|
|
|
+stopped producing images while still reporting success.
|
|
|
+
|
|
|
+Two mitigations are already shipped: an unresolvable path now fails the node
|
|
|
+instead of returning `undefined`, and the editor is being changed to offer
|
|
|
+real paths instead of a free-text box. This document asks the deeper question:
|
|
|
+should SmartBotic also adopt n8n's fix, a uniform `{json, binary}` envelope
|
|
|
+that every node emits, so a path means the same thing regardless of which node
|
|
|
+produced it?
|
|
|
+
|
|
|
+The answer below is **not yet**, for reasons grounded in the numbers in
|
|
|
+section 4 and the discoveries in section 1. Section 6 says what would change
|
|
|
+that.
|
|
|
+
|
|
|
+## 1. What every node emits today
|
|
|
+
|
|
|
+85 node files were read directly (`Object.keys` isn't enough; every
|
|
|
+`outputSchema` was pulled from the loaded module). This is the full set, not a
|
|
|
+sample.
|
|
|
+
|
|
|
+### Method
|
|
|
+
|
|
|
+```
|
|
|
+find nodes -name '*.js' | wc -l # 85
|
|
|
+node -e "console.log(JSON.stringify(require('./nodes/.../x.js').outputSchema))"
|
|
|
+```
|
|
|
+run once per file. All 85 loaded without error. The table below groups them by
|
|
|
+shape family rather than listing 85 rows of near-duplicates (the 11 chat
|
|
|
+nodes - `openai-chat`, `groq-chat`, `mistral-chat`, and so on - are one
|
|
|
+provider-parameterized template and share one output shape).
|
|
|
+
|
|
|
+### Shape families
|
|
|
+
|
|
|
+| Family | Node types (count) | Top-level output keys |
|
|
|
+|---|---|---|
|
|
|
+| `{result, executionTime}` | `code` (1) | `result`, `executionTime` |
|
|
|
+| `{data}` (see finding below) | `set-fields` (1) | `data` per schema; **flat object per actual code** |
|
|
|
+| `{success, content, json, model, ...}` | `openai-chat`, `groq-chat`, `mistral-chat`, `deepseek-chat`, `fireworks-chat`, `together-chat`, `xai-chat`, `openrouter-chat`, `perplexity-chat`, `deepinfra-chat`, `ollama-chat` (11) | `success`, `error`, `content`, `json`, `model`, `finishReason`, `usage`, `hadImage`, `imageBase64`, `mimeType`, `sourceUrl`, `models` (ollama omits `finishReason`/`usage`/`models`) |
|
|
|
+| `{url, statusCode, headers, body, ok, file?, storage?}` | `http-request` (1) | `url`, `checksum`, `statusCode`, `headers`, `body`, `ok`, `file`, `storage` |
|
|
|
+| `{result, workflowId, executionId, status}` | `call-workflow` (1) | `result`, `workflowId`, `workflowName`, `executionId`, `status` |
|
|
|
+| Loop bookkeeping, two branches | `loop` (1) | `_isLoop`, `_items`, `_outputField`, `_itemVariable`, `_indexVariable`, `_continueOnError`, `_activeBranch`, `totalItems`, `isFirst`, `isLast` - the per-iteration payload isn't in this object at all (see below) |
|
|
|
+| `{merged, count}` | `merge` (1) | `merged`, `count` |
|
|
|
+| Feed/list shapes | `rss-reader` (1) | `items[]` plus nine feed-level fields (`feedTitle`, `itemCount`, `newItemCount`, ...) |
|
|
|
+| Storage CRUD | `storage-get`, `storage-insert`, `storage-update`, `storage-delete`, `storage-query`, `storage-list-collections` (6) | each different: `document`/`found`, `id`/`success`, `documents[]`/`totalCount`/`page`, `collections[]`/`count` |
|
|
|
+| SQL | `mysql-query`, `postgresql-query` (2) | `rows[]`, `metadata{affectedRows,insertId,rowCount}`, `columns[]`, `query` |
|
|
|
+| SD.cpp job-queue | `sdcpp-txt2img`, `sdcpp-img2img`, `sdcpp-edit`, `sdcpp-upscale`, `sdcpp-txt2vid` (5, submit) + `sdcpp-job-status`, `sdcpp-job-wait` (2, poll) + `sdcpp-fetch-output`, `sdcpp-model`, `sdcpp-model-load`, `sdcpp-unload`, `sdcpp-upscaler-load`, `sdcpp-health`, `sdcpp-architecture` (7, other) = 14 | submit: `jobId`, `status`, `position`, `request`; poll: `jobId`, `status`, `done`, `succeeded`, `outputs[]`, `urls[]`, `error`; the rest are each their own shape |
|
|
|
+| OCR job lifecycle | `ocr-upload`, `ocr-job-status`, `ocr-retry-job`, `ocr-list-jobs`, `ocr-delete-job`, `ocr-job-stats`, `ocr-limitations` (7) | `{job:{id,status,...}}`, `{job, result:{text}, usage, timings}`, `{jobs[], total}`, `{jobId, deleted}`, `{stats:{...}}`, `{limitations:{...}}` |
|
|
|
+| IMAP | `imap-fetch`, `imap-search`, `imap-modify`, `imap-extract-attachments`, `imap-trigger` (5) | `{uid,subject,from,...,body,raw}`, `{uids[],count,emails[]}`, `{success,action,processed,results[]}`, `{attachments[],count,totalSize}`, `{emails[],uids[],count,mailbox,timestamp}` |
|
|
|
+| Messaging integrations | `smtp-send`, `telegram-send`, `bluesky`, `nextcloud-talk` (4) | each its own: `{success,messageId,accepted,to,subject}`, `{success,messageId,chatId,date}`, `{uri,cid,url,handle,did,imageCount}`, `{error,success,messageId,token,messages[],count}` |
|
|
|
+| Triggers - HTTP | `get-trigger`, `post-trigger`, `put-trigger` (3) | `{method,path,query,headers,timestamp,clientIp}` (+`body` for POST/PUT) |
|
|
|
+| Triggers - other | `click-trigger`, `schedule-trigger`, `form-trigger`, `workflow-input`, `error-trigger`, `database-change`, `file-watch`, `imap-trigger` (8) | each its own: click is `{timestamp,triggeredBy}`; schedule adds nine date/time fields; form is `{form:{...},submittedAt,clientIp}`; workflow-input's schema promises only `{calledBy}` but its real output is the caller's dynamic fields plus `calledBy` (schema-vs-code gap, see below) |
|
|
|
+| Flow control | `if-condition`, `switch`, `filter`, `sort-limit-dedupe`, `split-out`, `aggregate`, `wait`, `wait-for-approval`, `stop-and-error`, `configurator`, `respond-to-webhook`, `workflow-output` (12) | each its own bookkeeping shape; none carry a data payload named consistently |
|
|
|
+| Data utility | `json`, `datetime`, `template`, `crypto`, `image` (5) | `{value}`, `{value,timestamp}`, `{text}`, `{result,algorithm,...}`, ~20 image-metadata/EXIF fields plus optional `file` |
|
|
|
+| Misc | `comfyui-prompt`, `utils-test` (2) | comfyui is its own AI-image shape; utils-test declares an empty schema |
|
|
|
+
|
|
|
+That totals 85 (11 chat + 1 code + 1 set-fields + 1 http-request + 1
|
|
|
+call-workflow + 1 loop + 1 merge + 1 rss-reader + 6 storage + 2 sql + 14 sdcpp
|
|
|
++ 7 ocr + 5 imap + 4 messaging + 3 http-trigger + 8 other-trigger + 12
|
|
|
+flow-control + 5 data-utility + 2 misc = 85).
|
|
|
+
|
|
|
+**Every one of these is a different top-level key set.** There is no field
|
|
|
+name that means the same thing across even two unrelated families - `count`
|
|
|
+alone means "documents matched" (storage-query), "rows returned" (sql),
|
|
|
+"items produced" (split-out/aggregate), "attachments extracted"
|
|
|
+(imap-extract-attachments), and "duplicates removed" doesn't use `count` at
|
|
|
+all (`removedDuplicates`).
|
|
|
+
|
|
|
+### Two discoveries made while reading the code, not just the schema
|
|
|
+
|
|
|
+**`set-fields`'s `outputSchema` is wrong.** It declares `{data: {type: 'any',
|
|
|
+description: 'The rebuilt object'}}` (`nodes/core/set-fields.js:8`-ish), but
|
|
|
+`execute()` (`nodes/core/set-fields.js:132`) does `return output;` where
|
|
|
+`output` **is** the rebuilt object - not `{data: output}`. The node's own
|
|
|
+documentation of its shape doesn't match what it produces. This is very
|
|
|
+likely *why* the original incident was silent rather than loud: a `code` node
|
|
|
+really does return `{result, executionTime}` (verified against
|
|
|
+`nodes/core/code.js:897`), so `data.result.x` worked. A `set-fields` node
|
|
|
+returns the fields flat, so `data.result.x` silently found nothing. The
|
|
|
+editor's planned "offer real paths" mitigation depends on `outputSchema`
|
|
|
+being accurate; for this node today it is not, and the plan already assumes
|
|
|
+otherwise.
|
|
|
+
|
|
|
+**The engine already runs three different addressing conventions
|
|
|
+simultaneously**, none of them the ones in `outputSchema`:
|
|
|
+1. `$node['Name'].prop` - resolved in `workflow_engine.cpp:3383` by dumping
|
|
|
+ each node's raw `NodeExecutionResult.output` verbatim into a JS object
|
|
|
+ keyed by node name, then evaluating the expression against it directly.
|
|
|
+ Whatever the node's `execute()` literally returned is what this sees -
|
|
|
+ including the `set-fields` flat-object bug above.
|
|
|
+2. Bare `data.x` - resolved by unwrapping `input.data` once
|
|
|
+ (`workflow_engine.cpp:3407`-3416), with a special case that further
|
|
|
+ unwraps a spurious `input.data.data` double-nesting if it detects one.
|
|
|
+3. `data.loop.item` / `data.loop.index` / etc. - not a real object path at
|
|
|
+ all. `simplifyLoopVariablePaths()` (`workflow_engine.cpp:3312`-3346) does
|
|
|
+ a **textual find-and-replace** on the expression string before it is
|
|
|
+ evaluated, turning `data.loop.item` into bare `item` so it matches a
|
|
|
+ loop-body convenience variable the engine injects separately
|
|
|
+ (`workflow_engine.cpp:2688`-2696). There is no `loop` key in any node's
|
|
|
+ actual output; `data.loop.item` works purely because of string surgery
|
|
|
+ on the *expression text*, not because any object has that shape.
|
|
|
+
|
|
|
+So today there are already three incompatible addressing conventions live in
|
|
|
+production workflows, on top of the 20-plus incompatible output shapes in
|
|
|
+section 1's table. A uniform envelope would collapse the shape problem but
|
|
|
+would still need to pick one winner among these three access conventions -
|
|
|
+and retire two macro-expansion hacks (`data.data` unwrapping,
|
|
|
+`data.loop.var` text substitution) that workflows currently, silently, rely
|
|
|
+on.
|
|
|
+
|
|
|
+## 2. The proposed canonical shape
|
|
|
+
|
|
|
+n8n's envelope is `{json: {...}, binary: {...}}` per item, with an array of
|
|
|
+items flowing on each connection (nodes are meant to be n-in-n-out over item
|
|
|
+lists). SmartBotic's engine is single-object-per-connection, not
|
|
|
+list-of-items-per-connection (only `loop` and `split-out` explicitly turn one
|
|
|
+value into many, and they do it by branching/iterating rather than by
|
|
|
+carrying an array down every wire). Copying n8n's array-of-items model wholesale
|
|
|
+would be a bigger change than the field-naming problem being solved, so the
|
|
|
+proposal narrows to n8n's *envelope*, not its item-list execution model:
|
|
|
+
|
|
|
+```
|
|
|
+{
|
|
|
+ json: <object>, // the node's semantic payload - what a downstream
|
|
|
+ // node's expression should address as $json.x
|
|
|
+ binary: <object|null>,// { data, mimeType, filename, size, checksum } when
|
|
|
+ // the node produced a file; null otherwise
|
|
|
+ meta: <object> // everything else the node reports today that
|
|
|
+ // isn't the payload: statusCode, executionTime,
|
|
|
+ // success/error, jobId, count, ...
|
|
|
+}
|
|
|
+```
|
|
|
+
|
|
|
+`$json` becomes the one path prefix that means the same thing after any
|
|
|
+node. `$binary` is only ever present when there is a file. `meta` is where
|
|
|
+today's "everything else" keys go, still addressable but explicitly marked
|
|
|
+as not-the-payload.
|
|
|
+
|
|
|
+### The mapping, node family by node family - and the awkward cases named
|
|
|
+
|
|
|
+| Node family | `json` | `binary` | `meta` |
|
|
|
+|---|---|---|---|
|
|
|
+| `code` | `result` (as-is, whatever shape the user's code returned) | none | `executionTime` |
|
|
|
+| `set-fields` | the rebuilt object itself | none | none (there's nothing else to put there - this node's *entire* purpose is `json`) |
|
|
|
+| chat nodes (11) | `json` field when Response Format is JSON, else `{content: <text>}` - **awkward**: today `content` and `json` are siblings, both populated; wrapping means picking which one is "the" payload, and a workflow that reads `content` when JSON mode is on would need to move to `meta.content` | `imageBase64`/`mimeType` when `hadImage` | `success`, `error`, `model`, `finishReason`, `usage`, `attempts`, `hadImage`, `sourceUrl` |
|
|
|
+| `http-request` | `body` - **awkward**: `body` is only sometimes JSON; for `responseMode: binary` there is no meaningful `json` at all, and for a plain-text response `json` would hold a string, which breaks the "json is always an object" assumption everything else in this design relies on | `file` when `responseMode: binary` | `url`, `checksum`, `statusCode`, `headers`, `ok`, `storage` |
|
|
|
+| `call-workflow` | `result` (whatever the sub-workflow's `workflow-output` node produced) - **awkward**: this is a second layer of envelope-inside-envelope once `workflow-output` itself is wrapped; a badly-written sub-workflow can put anything in `result`, so `call-workflow`'s `json` can't itself be trusted to satisfy the envelope's own contract one level down | none directly (a file inside `result` stays inside `result`, doesn't get hoisted) | `workflowId`, `workflowName`, `executionId`, `status` |
|
|
|
+| `loop` | **the hard case.** The node has two branches with two different meanings: the `loop` branch's per-iteration payload is *not present in the loop node's own output at all* - it's injected separately into the body's `input` (`workflow_engine.cpp:2688`). The `done` branch's payload is `{[outputField]: [...]}`, an array of everything collected. Putting the loop node itself into the envelope model doesn't fix the deeper issue: the per-iteration item was never one of this node's outputs to begin with, so envelope-wrapping `loop` doesn't touch the actual footgun (see below) | file items pass through inside the array on `done`; per-iteration passes through whatever the item's own binary was | `_isLoop`, `totalItems`, `isFirst`, `isLast`, etc. stay internal/engine, not user-facing `meta` |
|
|
|
+| `merge` | `merged` | none | `count` |
|
|
|
+| triggers - HTTP (`get/post/put-trigger`) | `body` for POST/PUT, `{}` for GET | none (a multipart file upload is a separate design already in flight per `2026-08-09-form-trigger-design.md`) | `method`, `path`, `query`, `headers`, `timestamp`, `clientIp` |
|
|
|
+| triggers - other (schedule, click, form, workflow-input, error, database-change, file-watch) | **awkward, one per trigger.** `schedule-trigger`'s payload *is* its meta (there is no separate business payload, the whole point is the fired time); `workflow-input`'s payload is the caller's dynamic, config-driven fields, which by definition aren't known at design time, so `json` here can't be schema-checked the way it can for `code` or `set-fields`; `form-trigger`'s payload is `form` (may itself contain a `binary`-shaped file value per field) | only `form-trigger`, when a field is a file | trigger-specific bookkeeping (`triggerName`, `executionCount`, `clientIp`, `isFirstRun`, `cursor`, ...) |
|
|
|
+| storage nodes (6) | `document` (get), the input row (insert/update - there is no "result row" to speak of, just an id) - **awkward**: `storage-insert`/`storage-update`'s useful payload *is* `id`, which today lives where `meta` would live; forcing it into `json` as `{id: ...}` is defensible but arbitrary | none | `success`, `collection`, `error`, and for `storage-query`: `totalCount`, `page`, `hasMore` |
|
|
|
+| SQL nodes (2) | `rows` | none | `metadata`, `columns`, `query` |
|
|
|
+| SD.cpp job-queue (14) | job-submit: `{jobId}`; poll: `{outputs, urls}` on success - **awkward**: these are async job handles, not payloads; `json` on a submit response is nearly empty by definition, all the interesting content (`status`, `position`, `percent`, `done`) is legitimately meta | `sdcpp-fetch-output`'s `files[]` array - the one SD.cpp node that actually produces bytes | everything else: `status`, `position`, `percent`, `error`, `params`, ... |
|
|
|
+| OCR nodes (7) | `result.text` (job-status only; the rest are job metadata with no payload) | none | `job`, `usage`, `timings`, `jobs`, `stats`, `limitations` |
|
|
|
+| IMAP (5) | `body`/`emails`/`attachments` depending on node | `imap-extract-attachments`'s `attachments[]` (already binary-shaped) | headers, uid, mailbox, counts |
|
|
|
+| messaging (4) | none really - these are fire-and-confirm nodes; forcing a `json` key onto `smtp-send` means inventing a payload where none exists | none | `success`, `messageId`, and node-specific confirmation fields |
|
|
|
+| flow control (12) | most have no payload at all - `if-condition`'s output *is* `{result: bool, matchedConditions}`, entirely meta by this scheme; `filter`/`sort-limit-dedupe`/`aggregate` report counts about a list they don't themselves carry forward (the list flows via the connection's actual data, which today usually just passes the *input* through unchanged) | none | everything |
|
|
|
+| data utility (5) | `value` (json/datetime), `text` (template), `result` (crypto) | `image`'s `file` when manipulating | the rest |
|
|
|
+
|
|
|
+**The two structural problems this table exposes, which a mechanical
|
|
|
+per-node mapping can't paper over:**
|
|
|
+
|
|
|
+1. **Not every node has a payload.** `if-condition`, `smtp-send`,
|
|
|
+ `schedule-trigger`, the SD.cpp submit nodes - roughly a third of the 85 -
|
|
|
+ exist to report a decision or a confirmation, not to carry data forward.
|
|
|
+ Forcing `json: {}` on these either adds a meaningless empty object to
|
|
|
+ every downstream expression's mental model, or the envelope needs an
|
|
|
+ explicit "this node has no payload" state, which is one more thing every
|
|
|
+ node author and every path resolver has to handle.
|
|
|
+2. **The loop per-iteration item was never a node output to begin with.**
|
|
|
+ It's engine-injected into the body's `input`, bypassing the output-schema
|
|
|
+ layer entirely (`workflow_engine.cpp:2688`-2696, plus the
|
|
|
+ `data.loop.item` → `item` text-substitution hack at
|
|
|
+ `workflow_engine.cpp:3312`). Wrapping `loop`'s own `outputSchema` in
|
|
|
+ `{json, binary, meta}` does not touch this, because the thing users
|
|
|
+ actually reference (`data.loop.item.x`) isn't reachable through the loop
|
|
|
+ node's output at all. Fixing *this* specific inconsistency requires
|
|
|
+ changing how the engine injects loop-body input, which is an orthogonal
|
|
|
+ change to the envelope regardless of whether the envelope ships.
|
|
|
+
|
|
|
+## 3. Compatibility strategy
|
|
|
+
|
|
|
+A flag day is ruled out by the brief: there are live scheduled pipelines (see
|
|
|
+section 4 - 9 of 14 workflows are active). Three options considered:
|
|
|
+
|
|
|
+**A. Flag day.** Rejected outright per the constraint.
|
|
|
+
|
|
|
+**B. Additive - canonical keys appear alongside today's keys.** Every node
|
|
|
+keeps producing its current output *and* a `json`/`binary`/`meta` view
|
|
|
+alongside it (either the node itself emits both, or the engine wraps every
|
|
|
+node's raw output into `{...currentOutput, json: <derived>, binary:
|
|
|
+<derived>, meta: <derived>}` at the point it stores `NodeExecutionResult`).
|
|
|
+Existing `data.x` and `$node['Name'].x` expressions keep resolving exactly as
|
|
|
+today, unchanged. New expressions can use `$json.x`.
|
|
|
+
|
|
|
+Cost: every node's output now has two names for the same value
|
|
|
+(`data.result.x` and `data.json.x` for a `code` node, forever, unless a
|
|
|
+second migration later removes the old keys - which is the same flag-day
|
|
|
+problem deferred, not avoided). The editor's node-picker (the second shipped
|
|
|
+mitigation) would need to decide which of two equally-valid paths to offer,
|
|
|
+which reintroduces a version of the original ambiguity one layer up:
|
|
|
+now there are two "real" paths instead of one wrong one silently failing.
|
|
|
+Every node file in `nodes/` needs a second return value or a wrapping layer
|
|
|
+in the engine; 85 files is not enormous, but it is not zero, and every
|
|
|
+future new node needs to remember to populate `json`/`binary`/`meta`
|
|
|
+correctly or repeat the exact inconsistency this design exists to fix.
|
|
|
+
|
|
|
+**C. Resolver fallback - try canonical, then fall back to legacy shape.**
|
|
|
+The path resolver tries `$json.x` first; if that node hasn't been migrated
|
|
|
+(or the canonical projection is empty), it falls back to trying the node's
|
|
|
+raw output directly. This requires no changes to `nodes/` at all - the
|
|
|
+resolver alone decides.
|
|
|
+
|
|
|
+Cost: this is worse than option B, not better. It reintroduces exactly the
|
|
|
+silent-fallback behavior that the "unresolvable path now fails the node"
|
|
|
+mitigation was built to eliminate. A fallback chain of "try the new way,
|
|
|
+then quietly try the old way" is indistinguishable, from the workflow
|
|
|
+author's perspective, from "the path might mean one of two different
|
|
|
+things depending on version, and you can't tell which without checking."
|
|
|
+That is the bug, reintroduced as a feature. **Rejected.**
|
|
|
+
|
|
|
+**Recommendation: if this is done at all, option B (additive), with a hard
|
|
|
+sunset.** Ship canonical keys alongside legacy ones, but attach a real
|
|
|
+deadline and a real removal, not an indefinite dual-write. Given the count in
|
|
|
+section 4 (157 total expressions across all workflows, all trivially
|
|
|
+enumerable), a realistic sunset is: ship additive, run the (small, fully
|
|
|
+enumerable) migration script from section 5 immediately after shipping the
|
|
|
+additive layer rather than waiting, and remove the legacy keys within one
|
|
|
+release once the migration script confirms zero remaining legacy references.
|
|
|
+The "dual meaning for months" cost that makes option B expensive at n8n's
|
|
|
+scale (thousands of customer workflows nobody can touch) does not apply here
|
|
|
+at 14 workflows - which is itself part of the argument in section 6 for
|
|
|
+"not yet, but if we ever have ten times this many workflows, B is the
|
|
|
+right shape."
|
|
|
+
|
|
|
+## 4. Blast radius, counted
|
|
|
+
|
|
|
+Fetched every workflow from the running instance:
|
|
|
+
|
|
|
+```
|
|
|
+GET /api/v1/workflows?pageSize=200 (admin token)
|
|
|
+-> {"total": 14, "workflows": [...]} # all 14 returned in one page
|
|
|
+```
|
|
|
+
|
|
|
+| Metric | Count |
|
|
|
+|---|---|
|
|
|
+| Workflows total | 14 |
|
|
|
+| Workflows active (would run on their trigger, incl. scheduled) | 9 |
|
|
|
+| Workflows with at least one `{{ }}` expression | 12 |
|
|
|
+| Total `{{ }}` expressions across all workflows | 157 |
|
|
|
+| Of those, `$node['Name'].prop`-style references | 150 |
|
|
|
+| Of those, bare `data.prop`-style references | 22 |
|
|
|
+| (some expressions contain more than one reference, so 150+22 > 157) | |
|
|
|
+
|
|
|
+Per-workflow breakdown:
|
|
|
+
|
|
|
+| Workflow | Active | Expressions | `$node` refs | `data.` refs |
|
|
|
+|---|---|---|---|---|
|
|
|
+| verify-sort-limit-dedupe | no | 0 | 0 | 0 |
|
|
|
+| verify-if-condition-optional-field-opts-out | no | 0 | 0 | 0 |
|
|
|
+| Tier 1 Showcase | no | 3 | 0 | 3 |
|
|
|
+| Backfill image descriptions | no | 10 | 8 | 2 |
|
|
|
+| tmp condition probe (data.result.generate) | no | 1 | 0 | 0 |
|
|
|
+| 35photo2anime - sdcpp | **yes** | 7 | 6 | 1 |
|
|
|
+| anime from upload | **yes** | 1 | 1 | 0 |
|
|
|
+| [SWF] anime: process one image | **yes** | 33 | 38 | 0 |
|
|
|
+| Email OCR - Attachment to Text Reply | **yes** | 9 | 7 | 1 |
|
|
|
+| reddit2image | no | 29 | 22 | 7 |
|
|
|
+| [SWF] zsebhoki - publish anime post | **yes** | 42 | 48 | 6 |
|
|
|
+| sub: double a number | **yes** | 2 | 0 | 2 |
|
|
|
+| [SWF] SDCCP v2 generate image | **yes** | 18 | 18 | 0 |
|
|
|
+| ERROR | **yes** | 2 | 2 | 0 |
|
|
|
+
|
|
|
+Every active workflow that references upstream data at all uses
|
|
|
+`$node['Name'].prop`, not bare `data.prop` (the two exceptions with `data.`
|
|
|
+refs among active workflows - `35photo2anime`, `Email OCR`, `sub: double a
|
|
|
+number` - use it for exactly one or two simple pass-through values each, not
|
|
|
+for anything that reaches into a specific node's shape). This matters for
|
|
|
+section 3: whichever compatibility approach is chosen, the majority of the
|
|
|
+real exposure is through `$node[...]`, which is a single resolver code path
|
|
|
+(`workflow_engine.cpp:3383`) - not scattered across many mechanisms - making
|
|
|
+a mechanical rewrite (section 5) more tractable than the raw expression
|
|
|
+count alone suggests.
|
|
|
+
|
|
|
+**Named findings from reading each active workflow's expressions against its
|
|
|
+upstream node types** (not merely the syntax count above):
|
|
|
+
|
|
|
+- `[SWF] zsebhoki - publish anime post` and `[SWF] SDCCP v2 generate image`
|
|
|
+ account for 86 of the 150 `$node` references, the great majority reaching
|
|
|
+ into `workflow-input`'s dynamic per-workflow fields (`title_hu`, `baseUrl`,
|
|
|
+ `slug`, ...) or a `set-fields` node's flat output. Both of those access
|
|
|
+ patterns are *already* consistent under the current model (workflow-input
|
|
|
+ and set-fields both put their payload at the node's own top level, not
|
|
|
+ behind a shared key) - which is exactly the case that a canonical `$json`
|
|
|
+ wrapper would *change the meaning of*, since both would move from
|
|
|
+ top-level to `.json.` on migration. These two workflows alone are ~55% of
|
|
|
+ all references in the system.
|
|
|
+- No workflow in this instance currently uses the exact repro shape from the
|
|
|
+ incident (`code` swapped for `set-fields`, `data.result.x` reads). The
|
|
|
+ closest is `tmp condition probe (data.result.generate)`, whose name says
|
|
|
+ it exists specifically to probe this bug, and its only expression is the
|
|
|
+ literal `{{true}}` - it looks like a scratch workflow left over from
|
|
|
+ diagnosing the original incident, not a live path exercising it.
|
|
|
+
|
|
|
+## 5. Migration path
|
|
|
+
|
|
|
+**Can stored workflows be rewritten mechanically?** Mostly yes, for the
|
|
|
+`$node['Name'].prop` majority, *if* the mapping table in section 2 is
|
|
|
+finalized first: rewriting `$node['X'].body` to `$node['X'].json` for an
|
|
|
+`http-request` upstream, or `$node['X'].result` to `$node['X'].json` for a
|
|
|
+`call-workflow` upstream, is a pattern substitution keyed on
|
|
|
+`(upstream node's `type`, referenced property)` - both of which are fully
|
|
|
+known statically from each workflow's `nodes[]` and `connections[]` arrays,
|
|
|
+without executing anything. A script that walks all 14 workflows, resolves
|
|
|
+each `$node['X'].prop` reference's upstream node type via the `connections`
|
|
|
+graph, and looks up `prop` in that type's section-2 mapping row could rewrite
|
|
|
+the unambiguous cases automatically.
|
|
|
+
|
|
|
+**Where a human has to decide:**
|
|
|
+- Any reference into a family flagged "awkward" in section 2 - `http-request`
|
|
|
+ bodies that aren't JSON, `call-workflow` results (envelope-inside-envelope),
|
|
|
+ chat nodes read for `content` while also in JSON mode, anything touching
|
|
|
+ `loop`'s per-iteration item (which, per section 2, isn't fixable by this
|
|
|
+ change at all without also changing the engine's loop-body input
|
|
|
+ injection).
|
|
|
+- `workflow-input`'s dynamic fields: since the accessible keys are
|
|
|
+ config-defined per workflow rather than fixed by `outputSchema`, no
|
|
|
+ mechanical script can know in advance whether `title_hu` should map to
|
|
|
+ `json.title_hu` or stay where it is; a human has to decide per workflow
|
|
|
+ whether the caller-supplied fields *are* the payload (move to `json`) or
|
|
|
+ are configuration-like (stay in `meta`).
|
|
|
+- The 22 bare `data.` references need per-site classification: some are
|
|
|
+ genuine data-payload reads that would move under `$json`, and at least one
|
|
|
+ (`data.loop.item.x` in `reddit2image` and `zsebhoki`) is the
|
|
|
+ text-substitution loop-variable hack from section 1, which this migration
|
|
|
+ can't touch without an engine change to how loop-body input is built.
|
|
|
+
|
|
|
+**Rollback if a rewrite is wrong:** every workflow document already carries
|
|
|
+`_version` and an update history via the standard `storage-update` versioning
|
|
|
+(`documentData`/`version` in the `storage-update` output, per section 1's
|
|
|
+table) - the database is versioned per document, so a bad mechanical rewrite
|
|
|
+is a straightforward revert to the prior `_version` of that one workflow, not
|
|
|
+a fleet-wide rollback. Given only 14 workflows total, the safer path is
|
|
|
+simpler still: dry-run the rewrite script and diff its output against every
|
|
|
+current workflow by hand before writing anything back, since the total
|
|
|
+diff to review across all 14 workflows is on the order of 150 lines.
|
|
|
+
|
|
|
+## 6. Recommendation
|
|
|
+
|
|
|
+**Not yet.** Reasons, weighed against each other rather than restated:
|
|
|
+
|
|
|
+1. **The blast radius is genuinely small.** 14 workflows, 157 expressions,
|
|
|
+ 9 active. This is a fleet size where a human can review every affected
|
|
|
+ expression in an afternoon (section 5's rollback plan says as much) - the
|
|
|
+ scenario a uniform envelope is *for* (an organization with hundreds of
|
|
|
+ workflows across many authors, where "just read them all" stopped being
|
|
|
+ an option) does not describe this instance yet.
|
|
|
+2. **The two mitigations already shipped address the actual failure mode
|
|
|
+ more directly than an envelope would.** The incident wasn't caused by
|
|
|
+ the *existence* of inconsistent shapes - it was caused by an
|
|
|
+ inconsistent shape failing *silently*. "Unresolvable path fails the
|
|
|
+ node" converts every one of the awkward cases in section 2 into a loud
|
|
|
+ failure at the point the workflow is edited or first run, rather than a
|
|
|
+ wrong answer discovered downstream. The editor offering real paths
|
|
|
+ removes the main way an author would type a wrong path in the first
|
|
|
+ place. Both of these are cheaper than an envelope and address the
|
|
|
+ observed harm (silent wrong answers) more precisely than the envelope
|
|
|
+ does (the envelope makes paths *consistent*, not *correct* - a
|
|
|
+ migrated-but-wrong `$json` reference into `call-workflow`'s
|
|
|
+ envelope-inside-envelope case, section 2, would still fail exactly the
|
|
|
+ same way, just under a new key name).
|
|
|
+3. **A third of the 85 node types have no real payload at all**
|
|
|
+ (section 2, finding 1). Forcing them into `{json, binary, meta}` doesn't
|
|
|
+ simplify anything for those nodes - it adds a key that's usually `{}`
|
|
|
+ for every flow-control, confirmation, and job-submission node in the
|
|
|
+ system, which is most of the count.
|
|
|
+4. **The one structural bug that genuinely reproduces the incident - `loop`'s
|
|
|
+ per-iteration item bypassing the output-schema layer entirely
|
|
|
+ (section 1, section 2 finding 2) - is not fixed by adopting an envelope.**
|
|
|
+ It's fixed by changing how the engine injects loop-body input, which is a
|
|
|
+ smaller, independent, more surgical change than a 85-node migration, and
|
|
|
+ would do more to prevent a second version of this exact incident than the
|
|
|
+ envelope would.
|
|
|
+5. **The `set-fields` schema/code mismatch found in section 1 should be
|
|
|
+ fixed regardless of this decision** - it's a two-line change
|
|
|
+ (`nodes/core/set-fields.js`'s `outputSchema` should say the object is
|
|
|
+ flat, matching what `execute()` actually returns) that directly
|
|
|
+ undermines the editor's "offer real paths" mitigation for exactly the
|
|
|
+ node type involved in the original incident.
|
|
|
+
|
|
|
+**What would change this recommendation:**
|
|
|
+- The workflow count growing by an order of magnitude, especially across
|
|
|
+ multiple authors who don't all know each other's conventions - at that
|
|
|
+ scale "read every expression by hand" (section 5's actual rollback plan
|
|
|
+ today) stops being viable, and a canonical addressing scheme earns its
|
|
|
+ cost.
|
|
|
+- A second incident of the same shape occurring *after* both shipped
|
|
|
+ mitigations are live - that would mean the mitigations aren't sufficient
|
|
|
+ and the deeper fix is warranted after all.
|
|
|
+- `nodes/` being opened to third-party or community-contributed node
|
|
|
+ authors, where there's no longer a small team that can hold 85 shapes in
|
|
|
+ their head well enough to write correct paths by hand; consistency stops
|
|
|
+ being a nice-to-have and becomes the only way an unfamiliar author can
|
|
|
+ guess a path correctly on the first try.
|
|
|
+
|
|
|
+If any of those happen, this document's section 2 table and section 3's
|
|
|
+option B (additive, with a real sunset, not an indefinite dual-write) are
|
|
|
+the starting point - not a redo from scratch.
|