diff --git a/.changeset/flow-loop-per-iteration-containment-lint.md b/.changeset/flow-loop-per-iteration-containment-lint.md new file mode 100644 index 0000000000..52decf86da --- /dev/null +++ b/.changeset/flow-loop-per-iteration-containment-lint.md @@ -0,0 +1,35 @@ +--- +"@objectstack/lint": patch +--- + +flows: warn on a `loop` body with a fallible node and no containment, and on a `try_catch` with no `catch` (#14394) + +Two authoring-time rules in the flow anti-pattern family, both `warning`: + +- **`flow-loop-body-uncontained`** — a `loop` whose `body` region runs a node + that can end the run (a record read/write, `http`, `notify`, + `connector_action`, `script`, `subflow`, `map`, `approval`) with no + `try_catch` between the loop and that node. The `loop` executor iterates with + a bare `await` and has no `try`/`catch` at all, so the first failing item ends + the whole run: later items are never processed, and the work already done is + not even reported. The finding names the loop, the node, and the prescribed + spelling. +- **`flow-try-catch-without-catch`** — the near-miss, and the first target + rather than an extra: `catch` is optional in the schema, and omitting it makes + the container fail through, so an author who wrapped the node and stopped + there gets **zero** containment and previously got no diagnostic either. + Measured, the no-`catch` run and the unwrapped control produce identical + output; a `retry` policy only delays that. + +Both stay warnings under the family's severity bar: a loop deliberately allowed +to stop at the first failure, and a retry-then-fail `try_catch`, are legitimate +readings the rule cannot disprove. + +`content/docs/automation/flows.mdx` documents `loop { try_catch { … } }` as the +per-iteration containment spelling, with the measured minimal handler — one bare +`assignment` node, `edges` and `errorVariable` omitted — and the three `catch` +spellings the schema refuses (`catch` omitted gives no containment; `catch: {}` +and `catch: { nodes: [] }` are rejected, the region's `nodes` being `.min(1)`). + +No spec, engine or runtime change: the containment capability already exists and +was measured working (5 of 5 iterations, items 4-5 processed, run completes). diff --git a/content/docs/automation/flows.mdx b/content/docs/automation/flows.mdx index b429a2f307..795312a925 100644 --- a/content/docs/automation/flows.mdx +++ b/content/docs/automation/flows.mdx @@ -411,7 +411,15 @@ Runs its `body` region once per item of a collection, binding the current item maxIterations: 500, // hard cap (clamped to the engine ceiling) body: { // single-entry/single-exit region nodes: [ - { id: 'send', type: 'script', label: 'Notify', config: { /* … */ } }, + // Per-iteration containment — see the subsection below. A body node that + // can fail, wrapped in nothing, ends the WHOLE run at the first failure. + { + id: 'guard', type: 'try_catch', label: 'Guarded iteration', + config: { + try: { nodes: [{ id: 'send', type: 'script', label: 'Notify', config: { /* … */ } }], edges: [] }, + catch: { nodes: [{ id: 'handled', type: 'assignment', label: 'Handled' }] }, + }, + }, ], edges: [], }, @@ -422,6 +430,77 @@ Runs its `body` region once per item of a collection, binding the current item A `loop` node with **no `body`** keeps the legacy flat-graph behavior — the container is additive. +#### Per-iteration containment: `loop { try_catch { … } }` + +A `loop` body has **no error handling of its own**. The container iterates with a +bare `await`, so a body node that returns `success: false` (or throws) propagates +straight out of the loop and ends the run: every later item is never processed, +and the work already done is not even reported. Measured on the engine — a 5-item +sweep whose 3rd item fails touched 3 items, reported `acted: 0`, and finished +`status: failed`. + +The containment spelling is a `try_catch` **inside the body**, one per iteration. +Measured with the same 5-item sweep: all 5 iterations run, items 4 and 5 are +processed, and the run completes. + +```typescript +{ + id: 'each_case', + type: 'loop', + label: 'For each breached case', + config: { + collection: '{cases}', + iteratorVariable: 'currentCase', + body: { + nodes: [ + { + id: 'guard', + type: 'try_catch', + label: 'Guarded iteration', + config: { + try: { + nodes: [ + { + id: 'notify_owner', type: 'notify', label: 'Notify owner', + config: { title: 'SLA breach', recipients: ['{currentCase.owner}'] }, + }, + ], + edges: [], + }, + // The shortest handler that works: ONE bare `assignment` node with no + // `config` at all. `edges` omitted; `errorVariable` omitted (it + // defaults to `$error`). + catch: { nodes: [{ id: 'handled', type: 'assignment', label: 'Handled' }] }, + }, + }, + ], + edges: [], + }, + }, +} +``` + +**A `catch` region cannot be empty.** `FlowRegionSchema.nodes` is `.min(1)`, so +only the last row below is usable: + +| `catch` spelling | result | +|:---|:---| +| omitted entirely | parses — and contains **nothing**: the container fails through exactly like an unwrapped node | +| `catch: {}` | rejected — `catch.nodes`: expected array, received undefined | +| `catch: { nodes: [] }` | rejected — `catch.nodes`: too small, expected at least 1 item | +| `catch: { nodes: [ …one node… ] }` | parses, and contains | + +Two authoring-time lint rules cover this pair (both warnings, so neither fails a +build): `flow-loop-body-uncontained` names a loop body running a node that can +fail with no `try_catch` between the loop and it, and +`flow-try-catch-without-catch` names the near-miss — a `try_catch` whose `catch` +is absent, which gives **zero** containment while looking like containment. A +`retry` policy does not substitute: it re-runs the `try` region and then fails +anyway. + +Deliberately letting the sweep stop at the first failure is a legitimate choice — +that is why both rules warn rather than gate. + ### Parallel block Declares N branch regions that run **concurrently** and **join implicitly** when @@ -465,6 +544,14 @@ events). } ``` +`catch` is optional in the schema, and omitting it is the trap: with no `catch` +the container **fails** when the `try` region fails, so the failure propagates +exactly as if nothing had been wrapped (measured — the no-`catch` run and the +unwrapped control produce identical output). `retry` only delays that. The +`flow-try-catch-without-catch` lint rule names the shape at authoring time; the +minimal handler is one bare `assignment` node, as shown under "Per-iteration +containment" above. + > BPMN `parallel_gateway` / `join_gateway` / `boundary_event` remain in the > protocol as the **interop** representation and map onto these constructs on > import/export — they are not the native authoring model. diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index 9f89bdcc55..559328467c 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -734,6 +734,8 @@ export { FLOW_MULTIPLE_DEFAULT_EDGES, FLOW_INERT_NODE_CONDITION, FLOW_MULTI_WRITE_UNFILTERED, + FLOW_LOOP_BODY_UNCONTAINED, + FLOW_TRY_CATCH_WITHOUT_CATCH, } from './lint-flow-patterns.js'; export { lintLivenessProperties } from './lint-liveness-properties.js'; diff --git a/packages/lint/src/lint-flow-patterns.test.ts b/packages/lint/src/lint-flow-patterns.test.ts index 6070c1f9e3..3960a6a398 100644 --- a/packages/lint/src/lint-flow-patterns.test.ts +++ b/packages/lint/src/lint-flow-patterns.test.ts @@ -1,7 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect } from 'vitest'; -import { TimeRelativeTriggerSchema, LoopConfigSchema, ParallelConfigSchema, FlowSchema } from '@objectstack/spec/automation'; +import { TimeRelativeTriggerSchema, LoopConfigSchema, ParallelConfigSchema, TryCatchConfigSchema, FlowSchema } from '@objectstack/spec/automation'; // [#5659] The shared identity reduction, asserted beside the rule that consumes // it — the rule's verdict and the drivers' verdict are one object now. import { reduceFilterVerdict } from '@objectstack/spec/data'; @@ -25,6 +25,8 @@ import { FLOW_MULTIPLE_DEFAULT_EDGES, FLOW_INERT_NODE_CONDITION, FLOW_MULTI_WRITE_UNFILTERED, + FLOW_LOOP_BODY_UNCONTAINED, + FLOW_TRY_CATCH_WITHOUT_CATCH, } from './lint-flow-patterns.js'; const CEL = (source: string) => ({ dialect: 'cel', source }); @@ -572,7 +574,11 @@ describe('lintFlowPatterns — user-less runAs unscoped (#1888 / ADR-0049 / ADR- }); it('flags a loop-body write — the shape that passed the build and is refused at run time', () => { - const fnds = lintFlowPatterns(sweepFlow()); + // Scoped to this rule (#14394): the same fixture now also trips the + // containment warning — an `update_record` inside a `loop` body with no + // `try_catch` — a different verdict about a different node. The twin + // below pins that co-occurrence explicitly rather than filtering it away. + const fnds = lintFlowPatterns(sweepFlow()).filter((f) => f.rule === FLOW_RUNAS_UNSCOPED); expect(fnds).toHaveLength(1); expect(fnds[0].rule).toBe(FLOW_RUNAS_UNSCOPED); expect(fnds[0].severity).toBe('error'); @@ -586,7 +592,11 @@ describe('lintFlowPatterns — user-less runAs unscoped (#1888 / ADR-0049 / ADR- it('flags a loop-body delete_record the same way', () => { const fnds = lintFlowPatterns(sweepFlow({ bodyNodeType: 'delete_record' })); - expect(fnds.map((f) => f.rule)).toEqual([FLOW_RUNAS_UNSCOPED]); + // Both verdicts, in the order the pass emits them: the flow-level `runAs` + // error, then the #14394 containment warning for the same nested write — + // an unscoped loop-body write is ALSO an uncontained one, and each rule + // says its own thing about it exactly once. + expect(fnds.map((f) => f.rule)).toEqual([FLOW_RUNAS_UNSCOPED, FLOW_LOOP_BODY_UNCONTAINED]); expect(fnds[0].message).toContain("its data node 'touch' (delete_record), in loop 'loop_rows' body,"); }); @@ -1240,9 +1250,11 @@ describe('#5383 — flow-inert-node-condition descends into a loop body', () => }); it('flags it, scoped to the region so the message still names exactly one node', () => { - const fnds = lintFlowPatterns(nested()); - // Exactly one finding overall: no collateral from the descent, and no second - // copy reported against the enclosing `loop`. + // Scoped to this rule: the fixture's `create_record` also trips the #14394 + // containment warning, which is a different rule about a different node. + const fnds = lintFlowPatterns(nested()).filter((f) => f.rule === FLOW_INERT_NODE_CONDITION); + // Exactly one finding for this rule: no collateral from the descent, and no + // second copy reported against the enclosing `loop`. expect(fnds).toHaveLength(1); expect(fnds[0].rule).toBe(FLOW_INERT_NODE_CONDITION); expect(fnds[0].where).toBe( @@ -1352,7 +1364,9 @@ describe('#5383 — the branch-routing family reads the region’s own edges', ( { id: 'b1', source: 'gate', target: 'nudge', condition: 'lead.score > 50' }, { id: 'b2', source: 'gate', target: 'skip', isDefault: true, condition: 'lead.score <= 50' }, ], - })); + // Scoped to this rule: the body's `notify` also trips the #14394 + // containment warning. + })).filter((f) => f.rule === FLOW_DEFAULT_EDGE_WITH_CONDITION); expect(fnds).toHaveLength(1); expect(fnds[0].rule).toBe(FLOW_DEFAULT_EDGE_WITH_CONDITION); // The severity asymmetry the issue called out: a build-stopping rule that @@ -1445,7 +1459,9 @@ describe('#5383 — a recursive config scan does not double-report the container const fnds = lintFlowPatterns(loopBodyFlow({ nodes: [{ id: 'send_reminder', type: 'notify', config: { title: 'Reminder: {{lead.name}}' } }], edges: [], - })); + // Scoped to this rule: the body's `notify` also trips the #14394 + // containment warning. + })).filter((f) => f.rule === FLOW_DOUBLE_BRACE_INTERP); // Exactly one. The `loop`'s own config physically CONTAINS `body`, and // `collectTemplateStrings` is recursive, so descending without stripping the // region slots would report this a SECOND time against 'loop_leads'. @@ -1720,12 +1736,14 @@ describe('lintFlowPatterns — unbounded bulk write (#5482)', () => { */ describe('inside a nested region (#5383 / #5635)', () => { it('flags a loop-body sweep, scoped to the region, exactly once', () => { + // Scoped to this rule: a `delete_record` in a loop body also trips the + // #14394 containment warning. const fnds = lintFlowPatterns(loopBodyFlow({ nodes: [ { id: 'sweep', type: 'delete_record', config: { objectName: 'campaign_member', multi: true } }, ], edges: [], - })); + })).filter((f) => f.rule === FLOW_MULTI_WRITE_UNFILTERED); expect(fnds).toHaveLength(1); expect(fnds[0].rule).toBe(FLOW_MULTI_WRITE_UNFILTERED); expect(fnds[0].where).toBe( @@ -1765,7 +1783,7 @@ describe('lintFlowPatterns — unbounded bulk write (#5482)', () => { const fnds = lintFlowPatterns(loopBodyFlow({ nodes: [{ id: 'loop_touchpoints', type: 'loop', label: 'Loop Touchpoints', config: nestedTouchpointsResetLoop }], edges: [], - })); + })).filter((f) => f.rule === FLOW_MULTI_WRITE_UNFILTERED); expect(fnds).toHaveLength(1); expect(fnds[0].rule).toBe(FLOW_MULTI_WRITE_UNFILTERED); expect(fnds[0].where).toBe( @@ -1781,7 +1799,316 @@ describe('lintFlowPatterns — unbounded bulk write (#5482)', () => { config: { objectName: 'campaign_member', filter: { lead_id: '{lead.id}' }, multi: true }, }], edges: [], - }))).toHaveLength(0); + // Scoped to this rule: a bounded write is still an uncontained one, so + // the #14394 warning is present and is not this rule's business. + })).filter((f) => f.rule === FLOW_MULTI_WRITE_UNFILTERED)).toHaveLength(0); + }); + }); +}); + +/** + * #13681 / #14394 — per-iteration containment, the rule PAIR. + * + * The measured facts these cases pin (measurement on the real `AutomationEngine`, + * recorded on #13681): a 5-item sweep whose 3rd item fails processes 3 items and + * reports `acted: 0` when the body is bare; the same sweep processes all 5 and + * completes when the body is `try_catch { try, catch }`; and a `try_catch` with + * NO `catch` produces output byte-identical to the bare control. Hence one rule + * for the missing wrapper and one for the wrapper that contains nothing. + */ +describe('per-iteration containment (#13681 / #14394)', () => { + /** A scheduled sweep whose per-item work is whatever the case puts in the body. */ + const sweep = (bodyNodes: unknown[], bodyEdges: unknown[] = [], loopExtra: Record = {}) => ({ + flows: [{ + name: 'case_sla_monitor', + label: 'Case SLA monitor', + type: 'schedule', + // `system`, so the fixture answers about containment only and does not + // also trip the user-less `runAs` gate (FLOW_RUNAS_UNSCOPED). + runAs: 'system', + nodes: [ + { id: 'start', type: 'start', label: 'Start', config: { triggerType: 'schedule', schedule: 'cron:0 9 * * *' } }, + { + id: 'each', type: 'loop', label: 'For each breached case', + config: { + collection: '{cases}', iteratorVariable: 'currentCase', + body: { nodes: bodyNodes, edges: bodyEdges }, + ...loopExtra, + }, + }, + ], + edges: [{ id: 'e1', source: 'start', target: 'each' }], + }], + }); + + const notifyOwner = { + id: 'notify_owner', type: 'notify', label: 'Notify owner', + config: { title: 'SLA breach', recipients: ['{currentCase.owner}'] }, + }; + + /** + * The measured minimal `catch`: one bare `assignment` node with NO `config`. + * `edges` omitted (defaults to `[]`), `errorVariable` omitted (defaults to + * `$error`). Quoted verbatim from #13681's measurement — not re-derived. + */ + const MINIMAL_CATCH = { nodes: [{ id: 'handled', type: 'assignment', label: 'Handled' }] }; + + describe('rule A — a loop body with a fallible node and no containment', () => { + it('flags the fallible node, naming the loop AND the node', () => { + const fnds = lintFlowPatterns(sweep([notifyOwner])); + expect(fnds).toHaveLength(1); + expect(fnds[0].rule).toBe(FLOW_LOOP_BODY_UNCONTAINED); + expect(fnds[0].where).toBe( + "flow 'case_sla_monitor' · loop 'each' body · node 'notify_owner' (notify)", + ); + // The loop is named in the message too — `where` locates the node, the + // message says which iteration boundary is unprotected. + expect(fnds[0].message).toContain("inside loop 'each'"); + expect(fnds[0].message).toContain('no `try_catch` between them'); + // Warning, not a build gate: fail-fast is a legitimate reading. + expect(fnds[0].severity).toBeUndefined(); + // The prescribed spelling, with the measured minimal `catch`. + expect(fnds[0].hint).toContain("type: 'try_catch'"); + expect(fnds[0].hint).toContain("{ id: 'handled', type: 'assignment', label: 'Handled' }"); + expect(fnds[0].hint).toContain('`catch: {}` and `catch: { nodes: [] }` are both refused'); + }); + + it('flags every fallible builtin in the body, once each', () => { + const fallible = [ + { id: 'n1', type: 'get_record', label: 'Get', config: { objectName: 'case', filter: { id: '{currentCase.id}' } } }, + { id: 'n2', type: 'create_record', label: 'Create', config: { objectName: 'task' } }, + { id: 'n3', type: 'update_record', label: 'Update', config: { objectName: 'case', fields: { seen: true }, filter: { id: '{currentCase.id}' } } }, + { id: 'n4', type: 'delete_record', label: 'Delete', config: { objectName: 'task', filter: { case_id: '{currentCase.id}' } } }, + { id: 'n5', type: 'http', label: 'Call', config: { url: 'https://example.test' } }, + { id: 'n6', type: 'notify', label: 'Notify', config: { title: 'x' } }, + { id: 'n7', type: 'connector_action', label: 'Connector', config: { connectorId: 'c', action: 'a' } }, + { id: 'n8', type: 'script', label: 'Script', config: { function: 'f' } }, + { id: 'n9', type: 'subflow', label: 'Subflow', config: { flowName: 'child' } }, + { id: 'n10', type: 'map', label: 'Map', config: { collection: '{items}' } }, + { id: 'n11', type: 'approval', label: 'Approval', config: { approvers: ['{$User.Id}'] } }, + ]; + const fnds = lintFlowPatterns(sweep(fallible)).filter((f) => f.rule === FLOW_LOOP_BODY_UNCONTAINED); + expect(fnds.map((f) => f.where)).toEqual( + fallible.map((n) => `flow 'case_sla_monitor' · loop 'each' body · node '${n.id}' (${n.type})`), + ); + }); + + it('leaves the provably non-fallible builtins alone', () => { + // Read off their executors: `assignment` / `decision` return success on + // every path; `wait` and `screen` suspend and have no failure return. + const fnds = lintFlowPatterns(sweep([ + { id: 'a', type: 'assignment', label: 'Set', config: { assignments: { seen: true } } }, + { id: 'd', type: 'decision', label: 'Gate' }, + { id: 'w', type: 'wait', label: 'Wait', config: { durationMs: 1000 } }, + ])); + expect(fnds).toHaveLength(0); + }); + + it('leaves an UNKNOWN (plugin-registered) node type alone — the declared decision', () => { + // Counting unread executors fallible would flag every third-party node in + // every loop; this family's list is a record of readings, not a guess. + // The cost is a false negative here, stated in the rule's docblock. + expect(lintFlowPatterns(sweep([ + { id: 'x', type: 'acme_charge_card', label: 'Charge', config: { amount: 10 } }, + ]))).toHaveLength(0); + }); + + it('leaves a legacy flat-graph loop (no `body`) alone', () => { + // `LoopConfigSchema.body` is optional — a body-less loop is the legacy + // flat-graph form and has no region to judge. + const legacy = { + flows: [{ + name: 'case_sla_monitor', label: 'Case SLA monitor', type: 'schedule', runAs: 'system', + nodes: [ + { id: 'start', type: 'start', label: 'Start', config: { triggerType: 'schedule', schedule: 'cron:0 9 * * *' } }, + { id: 'each', type: 'loop', label: 'Loop', config: { collection: '{cases}', iteratorVariable: 'currentCase' } }, + { id: 'notify_owner', type: 'notify', label: 'Notify owner', config: { title: 'SLA breach' } }, + ], + edges: [{ id: 'e1', source: 'start', target: 'each' }, { id: 'e2', source: 'each', target: 'notify_owner' }], + }], + }; + expect(lintFlowPatterns(legacy).filter((f) => f.rule === FLOW_LOOP_BODY_UNCONTAINED)).toHaveLength(0); + }); + + it('reads ancestry, not depth — a `try_catch` inside a `parallel` branch still contains', () => { + const guarded = { + id: 'guard', type: 'try_catch', label: 'Guarded', + config: { try: { nodes: [notifyOwner], edges: [] }, catch: MINIMAL_CATCH }, + }; + expect(lintFlowPatterns(sweep([{ + id: 'fan', type: 'parallel', label: 'Fan out', + config: { + branches: [ + { name: 'notify', nodes: [guarded], edges: [] }, + { name: 'log', nodes: [{ id: 'note', type: 'assignment', label: 'Note' }], edges: [] }, + ], + }, + }]))).toHaveLength(0); + }); + + it('flags a fallible node inside a `parallel` branch, with the branch in `where`', () => { + const fnds = lintFlowPatterns(sweep([{ + id: 'fan', type: 'parallel', label: 'Fan out', + config: { + branches: [ + { name: 'email', nodes: [{ id: 'send', type: 'notify', label: 'Send', config: { title: 'x' } }], edges: [] }, + { name: 'log', nodes: [{ id: 'note', type: 'assignment', label: 'Note' }], edges: [] }, + ], + }, + }])); + expect(fnds).toHaveLength(1); + expect(fnds[0].rule).toBe(FLOW_LOOP_BODY_UNCONTAINED); + expect(fnds[0].where).toBe( + "flow 'case_sla_monitor' · loop 'each' body → parallel 'fan' branch 0 · node 'send' (notify)", + ); + }); + + it('reports a nested loop ONCE, against the inner loop where the wrap belongs', () => { + const fnds = lintFlowPatterns(sweep([{ + id: 'each_line', type: 'loop', label: 'For each line', + config: { + collection: '{currentCase.lines}', iteratorVariable: 'line', + body: { nodes: [notifyOwner], edges: [] }, + }, + }])); + expect(fnds).toHaveLength(1); + // Scoped to the INNER loop's own graph, not reported a second time through + // the outer one: per-iteration containment is a per-loop question, and the + // `try_catch` belongs in the innermost body. + expect(fnds[0].where).toBe( + "flow 'case_sla_monitor' · loop 'each' body · loop 'each_line' body · node 'notify_owner' (notify)", + ); + }); + }); + + describe('rule B — a `try_catch` with no `catch` region', () => { + it('flags the near-miss inside a loop body, and says it dies like an unwrapped node', () => { + const fnds = lintFlowPatterns(sweep([{ + id: 'guard', type: 'try_catch', label: 'Guarded', + config: { try: { nodes: [notifyOwner], edges: [] } }, + }])); + // Exactly one finding: rule A treats ANY enclosing `try_catch` as + // containment precisely so this shape is named once, by the rule that can + // name the missing key. + expect(fnds).toHaveLength(1); + expect(fnds[0].rule).toBe(FLOW_TRY_CATCH_WITHOUT_CATCH); + expect(fnds[0].where).toBe( + "flow 'case_sla_monitor' · loop 'each' body · node 'guard' (try_catch)", + ); + expect(fnds[0].severity).toBeUndefined(); + expect(fnds[0].message).toContain('declares no `catch` region'); + expect(fnds[0].message).toContain('identical output'); + expect(fnds[0].hint).toContain("{ id: 'handled', type: 'assignment', label: 'Handled' }"); + }); + + it('flags a TOP-LEVEL catch-less `try_catch` too — it fails through wherever it is written', () => { + const fnds = lintFlowPatterns({ + flows: [{ + name: 'charge', label: 'Charge', type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start', config: { triggerType: 'api' } }, + { id: 'guard', type: 'try_catch', label: 'Guarded', config: { try: { nodes: [{ id: 'pay', type: 'http', label: 'Pay', config: { url: 'https://example.test' } }], edges: [] } } }, + ], + edges: [{ id: 'e1', source: 'start', target: 'guard' }], + }], + }); + expect(fnds).toHaveLength(1); + expect(fnds[0].rule).toBe(FLOW_TRY_CATCH_WITHOUT_CATCH); + expect(fnds[0].where).toBe("flow 'charge' · node 'guard' (try_catch)"); + }); + + it('says what `retry` does and does not buy, when one is declared', () => { + const fnds = lintFlowPatterns(sweep([{ + id: 'guard', type: 'try_catch', label: 'Guarded', + config: { + try: { nodes: [notifyOwner], edges: [] }, + retry: { maxRetries: 3, backoffMs: 1000, backoffMultiplier: 2 }, + }, + }])); + expect(fnds).toHaveLength(1); + expect(fnds[0].rule).toBe(FLOW_TRY_CATCH_WITHOUT_CATCH); + expect(fnds[0].message).toContain('delays that outcome rather than changing it'); + }); + + it('says NOTHING about a `catch` that is present but malformed — the schema refuses those', () => { + // `catch: {}` and `catch: { nodes: [] }` are rejected by the parse itself + // (`FlowRegionSchema.nodes` is `.min(1)`), loudly and with the schema's own + // message. Lint speaks for the shape the schema ACCEPTS and the runtime + // then makes useless. + for (const malformed of [{}, { nodes: [] }]) { + expect(TryCatchConfigSchema.safeParse({ + try: { nodes: [{ id: 'x', type: 'assignment', label: 'X' }] }, catch: malformed, + }).success).toBe(false); + expect(lintFlowPatterns(sweep([{ + id: 'guard', type: 'try_catch', label: 'Guarded', + config: { try: { nodes: [notifyOwner], edges: [] }, catch: malformed }, + }])).filter((f) => f.rule === FLOW_TRY_CATCH_WITHOUT_CATCH)).toHaveLength(0); + } + // …and the minimal spelling the docs prescribe IS accepted. + expect(TryCatchConfigSchema.safeParse({ + try: { nodes: [{ id: 'x', type: 'assignment', label: 'X' }] }, catch: MINIMAL_CATCH, + }).success).toBe(true); + }); + }); + + /** + * The NEGATIVE CONTROL the ruling asks for, made executable: the exact + * spelling `content/docs/automation/flows.mdx` §"Per-iteration containment" + * teaches — same ids, same labels — must both PARSE under `FlowSchema` and + * produce no finding from either rule. + */ + describe('the documented spelling — `loop { try_catch { try, catch } }`', () => { + const DOCUMENTED_FLOW = { + name: 'case_sla_monitor', + label: 'Case SLA monitor', + type: 'schedule' as const, + runAs: 'system' as const, + nodes: [ + { id: 'start', type: 'start', label: 'Start', config: { triggerType: 'schedule', schedule: 'cron:0 9 * * *' } }, + { id: 'find_cases', type: 'get_record', label: 'Find breached cases', config: { objectName: 'case', filter: { sla_breached: true }, outputVariable: 'cases' } }, + { + id: 'each_case', type: 'loop', label: 'For each breached case', + config: { + collection: '{cases}', + iteratorVariable: 'currentCase', + body: { + nodes: [{ + id: 'guard', type: 'try_catch', label: 'Guarded iteration', + config: { + try: { + nodes: [{ id: 'notify_owner', type: 'notify', label: 'Notify owner', config: { title: 'SLA breach', recipients: ['{currentCase.owner}'] } }], + edges: [], + }, + // The measured minimal handler: one bare `assignment`, no + // `config`; `edges` and `errorVariable` both omitted. + catch: { nodes: [{ id: 'handled', type: 'assignment', label: 'Handled' }] }, + }, + }], + edges: [], + }, + }, + }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'find_cases' }, + { id: 'e2', source: 'find_cases', target: 'each_case' }, + ], + }; + + it('parses under FlowSchema — the docs example is authorable', () => { + const parsed = FlowSchema.safeParse(DOCUMENTED_FLOW); + expect(parsed.success ? [] : parsed.error.issues.map((i) => `${i.path.join('.')}: ${i.message}`)).toEqual([]); + expect(parsed.success).toBe(true); + }); + + it('raises NO finding from either containment rule', () => { + const fnds = lintFlowPatterns({ flows: [DOCUMENTED_FLOW] }); + expect(fnds.filter((f) => f.rule === FLOW_LOOP_BODY_UNCONTAINED)).toHaveLength(0); + expect(fnds.filter((f) => f.rule === FLOW_TRY_CATCH_WITHOUT_CATCH)).toHaveLength(0); + }); + + it('raises no finding from ANY flow rule — the page teaches a clean shape', () => { + expect(lintFlowPatterns({ flows: [DOCUMENTED_FLOW] })).toHaveLength(0); }); }); }); diff --git a/packages/lint/src/lint-flow-patterns.ts b/packages/lint/src/lint-flow-patterns.ts index 86972b321e..0ffd416a26 100644 --- a/packages/lint/src/lint-flow-patterns.ts +++ b/packages/lint/src/lint-flow-patterns.ts @@ -53,6 +53,21 @@ * specifically (range operators `>=`/`<=` are not flagged — they're the building * block of the correct pattern), keeping false positives near zero. * + * #13681 / #14394 — per-iteration containment, as a PAIR of rules. A `loop` + * body has no error handling of its own: `loop-node.ts` iterates with a bare + * `await`, so the first item whose node fails ends the entire run and every + * later item goes unprocessed, silently. The containment spelling exists and was + * measured to work — `loop { body: [ try_catch { try, catch } ] }` processed all + * 5 rows of a sweep whose 3rd row fails — but it is undiscoverable, and its + * near-miss form fails *identically to no wrapper at all*: with `catch` omitted + * the container fails through (`try-catch-node.ts:190`). So + * {@link FLOW_LOOP_BODY_UNCONTAINED} names the uncontained body and + * {@link FLOW_TRY_CATCH_WITHOUT_CATCH} names the wrapper that contains nothing, + * and they divide one node between them: any enclosing `try_catch` counts as + * containment for the first rule, so a catch-less one is reported once, by the + * second, which is the rule that can name the missing key. Both stay warnings — + * fail-fast on the first bad row, and retry-then-fail, are legitimate readings. + * * ## Every graph in the flow, not just the top-level one (#5383) * * These rules used to read `flow.nodes` / `flow.edges` flat, so every one of @@ -146,7 +161,7 @@ import type { FlowNodeParsed, FlowEdgeParsed } from '@objectstack/spec/automatio // driver-sql, driver-mongodb and driver-memory execute. This linter asks it // rather than hand-writing a fourth copy; see {@link filterCarriesNoCondition}. import { reduceFilterVerdict } from '@objectstack/spec/data'; -import { stripRegions } from './flow-walk.js'; +import { stripRegions, REGION_SLOTS, MAX_REGION_DEPTH } from './flow-walk.js'; export interface FlowLintFinding { where: string; @@ -230,6 +245,40 @@ export const FLOW_INERT_NODE_CONDITION = 'flow-inert-node-condition'; * how it divides labour with the #3810 run-time guard. */ export const FLOW_MULTI_WRITE_UNFILTERED = 'flow-multi-write-unfiltered'; +/** + * #13681 / #14394 — a `loop` body that runs a node which can fail, with no + * `try_catch` between the loop and that node. The first failing item kills the + * whole sweep: `loop-node.ts:123-135` iterates with a bare `await` and no + * `try`/`catch` anywhere in the file, so the body's failure propagates out of + * the container and the remaining items are never processed. + * + * A **warning**, per the severity policy at the top of this file: a loop whose + * body is deliberately allowed to die (fail fast on the first bad row) is a + * legitimate reading, and the rule cannot prove the author did not mean it. What + * it can do is make the choice visible at authoring time — today it is silent, + * and the measured consequence is a 5-row sweep that processes 3 rows and + * reports a run that "completed" nothing unusual. + * + * See {@link scanUncontainedLoopBodies} for the containment judgement, and + * {@link FALLIBLE_NODE_TYPES} for what counts as fallible and why. + */ +export const FLOW_LOOP_BODY_UNCONTAINED = 'flow-loop-body-uncontained'; +/** + * #13681 / #14394 — the near-miss shape: a `try_catch` that declares no `catch` + * region. `catch` is optional in the schema (`control-flow.zod.ts:315`) and + * omitting it makes the container **fail** (`try-catch-node.ts:190`), so the + * wrapped region dies exactly like an unwrapped one — measured side by side, the + * no-`catch` run and the no-wrapper control produce identical output. + * + * This is the family's first target for the containment pair, not an extra: an + * author who reaches for `try_catch` has recognised the hazard and stopped one + * key short, and before this rule got **zero containment and zero diagnostics**. + * A warning rather than an error under the same policy — a `retry`-only + * `try_catch` (retry the region, then fail loudly) is a legitimate reading. + * + * See {@link scanTryCatchWithoutCatch}. + */ +export const FLOW_TRY_CATCH_WITHOUT_CATCH = 'flow-try-catch-without-catch'; /** * Node types that ship in the box. `config.condition` is only ever READ on the @@ -257,6 +306,79 @@ const INERT_CONDITION_NODE_TYPES = new Set([ /** Node types that perform a data operation — the ones `flow.runAs` governs (#1888). */ const DATA_NODE_TYPES = new Set(['get_record', 'create_record', 'update_record', 'delete_record']); +/** + * #14394 — node types whose executor can end a run: it returns `success: false` + * (or throws) for a reason that is **not knowable at authoring time** — an + * absent row, a refused write, a 500, a connector outage, a child flow that + * died. These are the nodes {@link FLOW_LOOP_BODY_UNCONTAINED} judges a loop + * body by. + * + * Membership means "we have READ this executor and found such a path", the same + * stronger claim {@link INERT_CONDITION_NODE_TYPES} makes, and for the same + * reason: `node.type` is an open namespace (ADR-0018), so a literal list is the + * only honest one. Per-type, with the failure the reading found: + * + * - `get_record` / `create_record` / `update_record` / `delete_record` — each + * wraps its ObjectQL call in `try`/`catch` and returns + * `success: false, error: '() failed: …'` + * (`crud-nodes.ts:262, :359, :452, :515`). + * - `http` — a non-2xx (when `failOnError`), a timeout/abort, or a failed + * durable enqueue (`http-nodes.ts:208, :249-253`). + * - `notify` — no title, an empty resolved recipient set, or a delivery throw + * (`notify-node.ts:293, :300, :446`). The measured #13681 case exactly: one + * row with a null owner killed the sweep. + * - `connector_action` — a degraded connector, an unresolvable action, or a + * throwing call (`connector-nodes.ts:69, :76, :124`). + * - `script` — the named function is not registered on this host, or it threw + * (`screen-nodes.ts:250, :294`). + * - `subflow` — the child flow failed, or paused without a run id + * (`subflow-node.ts:118, :147`). + * - `map` — the collection did not resolve to an array, exceeded the item cap, + * or an item's child run failed (`map-node.ts:115, :118, :189`). + * - `approval` — invalid config, a missing `$runId` / object / record id in + * context, or a throwing request (`plugin-approvals/src/approval-node.ts:136, + * :145-147, :212`). Plugin-registered but in-box, and this file already + * imports its type constant for the ADR-0044 rules. + * + * Deliberately **absent**, each also by reading the executor: + * + * - `assignment`, `decision` — every path returns `success: true` + * (`logic-nodes.ts:76-78, :137-141`). `assignment` is also the documented + * minimal `catch` handler, so flagging it would warn about the fix. + * - `wait`, `screen` — they suspend (`success: true, suspend: true`) and have + * no failure return (`wait-node.ts:262, :284, :290`; `screen-nodes.ts:44-96`). + * - `start` / `end` — sentinels, and a region may not contain them at all. + * - `loop` / `parallel` / `try_catch` — containers. They do fail, but only + * *because* something inside them failed, so the walk descends to the leaf + * that carries the real failure instead of reporting the wrapper (which would + * also double-report). + * - **Every node type not in this list**, including plugin-registered ones. An + * unknown executor can certainly return `success: false`, so counting them + * fallible would be defensible for a warning — but it would flag nodes nobody + * here has read, and this family's precedent is that the list is a record of + * readings, not a guess. The cost is a false negative on a third-party node; + * the alternative is a false positive on every one of them. + */ +const FALLIBLE_NODE_TYPES: ReadonlySet = new Set([ + 'get_record', 'create_record', 'update_record', 'delete_record', + 'http', 'notify', 'connector_action', 'script', 'subflow', 'map', + APPROVAL_NODE_TYPE, +]); + +/** + * The container that provides containment inside a loop body, and the config key + * whose absence makes it provide none (#14394). + * + * Rule A treats **any** enclosing `try_catch` as containment — including one + * with no `catch`, which contains nothing. That is deliberate division of + * labour, not an oversight: the catch-less container is reported once, by + * {@link FLOW_TRY_CATCH_WITHOUT_CATCH}, naming the one key that fixes it. + * Reporting it from both rules would tell an author who wrapped their node that + * they must wrap it, which is the one thing they did do. + */ +const TRY_CATCH_NODE_TYPE = 'try_catch'; +const LOOP_NODE_TYPE = 'loop'; + /** * How {@link FLOW_RUNAS_UNSCOPED} names the identity the run would use — ONE * wording, true whether the author wrote `runAs:'user'` or wrote nothing (#5693). @@ -1071,6 +1193,185 @@ function scanApprovalReviseLoops( } } +/** + * The minimal `catch` region, measured end to end on the real `AutomationEngine` + * (#13681) and quoted verbatim by both containment rules and by + * `content/docs/automation/flows.mdx`. + * + * `catch` cannot be empty: `FlowRegionSchema.nodes` is `.min(1)` + * (`control-flow.zod.ts:142`), so `catch: {}` and `catch: { nodes: [] }` are + * both refused by the parse. The shortest handler that works is one bare + * `assignment` node with no `config` at all — its descriptor declares no + * `required` keys and its executor with no assignments sets nothing and returns + * success. `edges` may be omitted (it defaults to `[]`), and `errorVariable` may + * be omitted (it defaults to `$error`). + */ +const MINIMAL_CATCH_SPELLING = + "`catch: { nodes: [ { id: 'handled', type: 'assignment', label: 'Handled' } ] }` — one bare " + + '`assignment` node with no `config` is the shortest handler that works; `edges` and ' + + '`errorVariable` may both be omitted'; + +/** `catch` cannot be empty — the two spellings the schema refuses. */ +const EMPTY_CATCH_REFUSALS = + 'A `catch` region cannot be empty: its `nodes` is `.min(1)`, so `catch: {}` and ' + + '`catch: { nodes: [] }` are both refused by the parse.'; + +/** + * Is this value a region dictionary — `{ nodes: [...] }` — as authored? + * + * Read raw rather than parsed, like the rest of this module: `FlowNodeSchema.config` + * is an open `z.record`, so a region arrives as an ordinary record even in a + * parsed stack, and this file promises never to throw on a malformed one. + */ +function regionNodesOf(value: unknown): AnyRec[] | undefined { + if (!value || typeof value !== 'object' || Array.isArray(value)) return undefined; + const nodes = (value as AnyRec).nodes; + return Array.isArray(nodes) ? (nodes as AnyRec[]) : undefined; +} + +/** + * #14394 rule A — a `loop` body that runs a fallible node with no `try_catch` + * between the loop and that node. + * + * The judgement is **ancestry, not depth**: a `try_catch` two levels down inside + * a `parallel` branch still contains what it wraps, and a fallible node three + * levels down with no `try_catch` on the path to the loop still kills the sweep. + * So the descent from a loop body reports a fallible node and otherwise walks on + * through any container, with two stops: + * + * - **`try_catch` — stop.** Everything below it is contained (see + * {@link TRY_CATCH_NODE_TYPE} for why a catch-less one counts here and is + * reported by rule B instead). One finding per fallible node, never two. + * - **a nested `loop` — stop.** That loop is its own subject: it appears as a + * node of its own graph in {@link collectFlowGraphs}, this scan runs per + * graph, and it is judged there. Without the stop every fallible node in a + * nested body would be reported once per enclosing loop, and the finding that + * matters — the innermost loop, where the wrap belongs — would be the one + * buried in duplicates. + * + * Loops with **no `body`** are skipped entirely: `LoopConfigSchema.body` is + * optional (`control-flow.zod.ts:216`) and a body-less `loop` is the legacy + * flat-graph form, which has no region to judge. + */ +function scanUncontainedLoopBodies( + at: string, + nodes: AnyRec[], + findings: FlowLintFinding[], +): void { + for (const node of nodes) { + if (node.type !== LOOP_NODE_TYPE) continue; + const bodyNodes = regionNodesOf(((node.config ?? {}) as AnyRec).body); + if (!bodyNodes) continue; + const loopId = typeof node.id === 'string' && node.id ? node.id : '(unnamed loop)'; + + const visit = (region: AnyRec[], trail: string, depth: number): void => { + if (depth > MAX_REGION_DEPTH) return; + for (const child of region) { + if (!child || typeof child !== 'object') continue; + const type = typeof child.type === 'string' ? child.type : ''; + if (type === TRY_CATCH_NODE_TYPE || type === LOOP_NODE_TYPE) continue; + + const childId = typeof child.id === 'string' && child.id ? child.id : '(unnamed node)'; + if (FALLIBLE_NODE_TYPES.has(type)) { + findings.push({ + where: `${at} · ${trail} · node '${childId}' (${type})`, + message: + `runs inside loop '${loopId}' with no \`try_catch\` between them — a '${type}' node can return ` + + `\`success: false\` or throw, and the loop body's failure propagates straight out of the ` + + `container (\`loop-node.ts\` iterates with a bare \`await\` and has no \`try\`/\`catch\` at all). ` + + `The first failing item therefore ends the whole run: every later item is never processed, and ` + + `the work already done is not even reported (measured on the real engine — a 5-item sweep ` + + `failing at item 3 touched 3 items and reported \`acted: 0\`).`, + hint: + `Contain the failure per iteration: put a \`try_catch\` INSIDE the body and move this node into ` + + `its \`try\` region — \`loop { body: { nodes: [ { type: 'try_catch', config: { try, catch } } ] } }\`. ` + + `${EMPTY_CATCH_REFUSALS} The minimal handler is ${MINIMAL_CATCH_SPELLING}. Measured: with it, all ` + + `5 items are processed and the run completes. If this loop is MEANT to stop at the first failure, ` + + `that is a legitimate reading and this stays a warning. ` + + // The tracker ids stay OUT of the runtime string (`check:doc-authoring`): + // an author reading this hint cannot resolve `#NNNN`. The measurement + // and the ruling behind this rule are #13681 / #14394; the docblock on + // {@link FLOW_LOOP_BODY_UNCONTAINED} carries them for the reader who can. + `See content/docs/automation/flows.mdx §"Per-iteration containment".`, + // Warning, not `error`: see the severity policy at the top of this + // file. Fail-fast on the first bad row is a real intent this rule + // cannot distinguish from an oversight. + rule: FLOW_LOOP_BODY_UNCONTAINED, + }); + continue; + } + + // Any other container (`parallel` today; whatever the protocol adds + // next) is walked through: its region slots come from the one shared + // table, so a new construct is descended without editing this rule. + const slots = REGION_SLOTS.get(type); + if (!slots) continue; + const cfg = (child.config ?? {}) as AnyRec; + for (const slot of slots) { + const value = cfg[slot]; + if (Array.isArray(value)) { + // A `many` slot (`parallel.config.branches`) — an array of regions. + value.forEach((branch, index) => { + const branchNodes = regionNodesOf(branch); + if (branchNodes) visit(branchNodes, `${trail} → ${type} '${childId}' branch ${index}`, depth + 1); + }); + continue; + } + const slotNodes = regionNodesOf(value); + if (slotNodes) visit(slotNodes, `${trail} → ${type} '${childId}' ${slot}`, depth + 1); + } + } + }; + + visit(bodyNodes, `loop '${loopId}' body`, 0); + } +} + +/** + * #14394 rule B — a `try_catch` with no `catch` region, anywhere in the flow. + * + * Measured (#13681): the container fails through, and the run is byte-identical + * to the one with no `try_catch` at all. `retry`, when present, only delays it. + * + * A `catch` that is PRESENT but malformed is deliberately not this rule's + * business: `catch: {}` and `catch: { nodes: [] }` are refused by the parse + * itself, loudly, with the schema's own message. Lint speaks for the shape the + * schema accepts and the runtime then makes useless. + */ +function scanTryCatchWithoutCatch( + at: string, + nodes: AnyRec[], + findings: FlowLintFinding[], +): void { + for (const node of nodes) { + if (node.type !== TRY_CATCH_NODE_TYPE) continue; + const cfg = (node.config ?? {}) as AnyRec; + if (cfg.catch !== undefined) continue; + const nodeId = typeof node.id === 'string' && node.id ? node.id : '(unnamed node)'; + const hasRetry = (cfg as AnyRec).retry !== undefined; + findings.push({ + where: `${at} · node '${nodeId}' (${TRY_CATCH_NODE_TYPE})`, + message: + `declares no \`catch\` region, so it contains NOTHING — when the \`try\` region fails the container ` + + `fails with it and the failure propagates exactly as if the nodes had never been wrapped (measured: ` + + `the no-\`catch\` run and the unwrapped control produce identical output).` + + (hasRetry + ? ' Its `retry` policy re-runs the `try` region first, which delays that outcome rather than changing it.' + : '') + + ` Inside a \`loop\` body this is the silent one: the author has recognised the hazard, wrapped the ` + + `node, and still loses every item after the first failure.`, + hint: + `Add the handler: ${MINIMAL_CATCH_SPELLING}. ${EMPTY_CATCH_REFUSALS} If failing loudly really is the ` + + `intent, a \`try_catch\` with only \`try\` (and optionally \`retry\`) is a legitimate retry-then-fail ` + + `shape — this is a warning, not a gate. ` + + // Tracker ids stay out of the runtime string — see the sibling rule above. + `See content/docs/automation/flows.mdx §"Per-iteration containment".`, + // Warning, not `error`: retry-then-fail is a real reading of this shape. + rule: FLOW_TRY_CATCH_WITHOUT_CATCH, + }); + } +} + /** * Lint every flow for known authoring anti-patterns — its own graph AND every * nested ADR-0031 region (#5383). Returns a (possibly empty) list of findings; @@ -1256,6 +1557,21 @@ export function lintFlowPatterns(stack: AnyRec): FlowLintFinding[] { // what puts the loop-body sweep — the standard shape for a scheduled // purge, and this rule's main habitat — in range (#5383/#5635). scanUnboundedBulkWrites(at, graphNodes, findings); + + // (g) #13681/#14394 — a `loop` body running a fallible node with no + // `try_catch` between the loop and it. Per graph like the rest, and + // that is what keeps the count right: every `loop` node belongs to + // exactly one graph, so its body is descended exactly once, and a + // nested loop is judged in its own graph rather than through its + // parent (see {@link scanUncontainedLoopBodies}). + scanUncontainedLoopBodies(at, graphNodes, findings); + + // (h) #13681/#14394 — the near-miss: a `try_catch` with no `catch`. Scanned + // everywhere, not only inside a loop: the container fails through + // wherever it is written. Inside a loop body it is the shape (g) + // deliberately treats as contained, so exactly one of the two rules + // ever speaks about a given node. + scanTryCatchWithoutCatch(at, graphNodes, findings); } } return findings;