feat(cli,automation): catch label: 'error' written where type: 'fault' was meant (#3863)

.changeset/fault-edge-label-lint.md

@@ -0,0 +1,52 @@
+---
+"@objectstack/cli": minor
+"@objectstack/service-automation": patch
+---
+
+feat(cli,automation): catch `label: 'error'` written where `type: 'fault'` was meant (#3863)
+
+Two of the three items left open on #3863. Both are about making the fault-edge
+contract legible; neither changes routing behaviour.
+
+**New lint — `flow-error-label-not-fault`.** `type: 'fault'` is what routes a
+failure; `label` is cosmetic on an ordinary edge. So this, which reads exactly
+like error handling:
+
+```ts
+{ source: 'charge_card', target: 'flag_for_review', label: 'error' }
+```
+
+is an ordinary out-edge — and `traverseNext` runs every unconditional out-edge
+in parallel. The handler fires on every **successful** run of `charge_card`,
+concurrently with the real success path, and never on a failure. The run still
+aborts when the node fails.
+
+Silent in both directions: the author believes failures are handled, and never
+notices the handler running when nothing went wrong. The reading is especially
+natural for an AI author, since the label is precisely what the intent sounds
+like — which is why this is worth a build-time diagnostic rather than leaving it
+to a puzzled look at a run trace.
+
+Deliberately narrow, because a label IS load-bearing on a branching node: a
+`decision` / `approval` executor returns a `branchLabel` and traversal then
+prefers the edge carrying it. Edges out of those node types are excluded, as are
+conditional edges (a guarded path is not the unconditional footgun) and edges
+already typed `fault`. Matches the obvious synonyms (`error`, `failure`,
+`catch`, `on_error`, …) case-insensitively. Verified against the shipped
+showcase: no findings.
+
+An alias — accepting `label: 'error'` as if it were `type: 'fault'` — was
+considered and rejected: two spellings for one concept is harder to read than
+one spelling plus a diagnostic that names the fix.
+
+**Pinned: a handled failure does not consume a flow-level retry.** The two
+recovery mechanisms have different scopes and must not compound — a `fault` edge
+handles one node, while `errorHandling.retry` replays the flow **from the
+start**, re-running every node that already succeeded (a second notification, a
+second created record). A failure a fault edge handled is not a flow failure, so
+it does not consume a retry. That already held by construction (a routed failure
+never propagates out of `executeNode`); it is now a test, so a refactor of the
+catch path cannot quietly change it.
+
+Docs and the automation skill gain both points, plus a note on the edge-property
+table that `label` does not select a path except on a branching node.

content/docs/automation/flows.mdx

@@ -564,7 +564,7 @@ Edges connect nodes and define the execution path:
 | `target` | `string` | ✅ | Target node ID |
 | `type` | `enum` | optional | `'default'` (success), `'fault'` (error), `'conditional'` (expression-guarded), or `'back'` (declared back-edge, ADR-0044); defaults to `'default'` |
 | `condition` | `string` | optional | Boolean expression for branching |
-| `label` | `string` | optional | Label displayed on the connector |
+| `label` | `string` | optional | Label displayed on the connector — cosmetic only. It does **not** select a path except on a branching node (`decision` / `approval`), which picks its out-edge by label. |
 
 ### Fault edges — handling a failed node
 
@@ -578,6 +578,16 @@ edges: [
 ]
 ```
 
+<Callout type="warn">
+**`type: 'fault'` is what routes — a label is not.** Writing
+`{ source: 'charge_card', target: 'flag_for_review', label: 'error' }` without
+the type leaves an ordinary edge, and every unconditional out-edge is traversed
+(in parallel) on **success**. The handler then runs whenever the node
+*succeeds*, never when it fails, and the run still aborts on failure. Both
+halves are silent, so `objectstack validate` reports this as
+`flow-error-label-not-fault`.
+</Callout>
+
 The handler reads what went wrong from two variables:
 
 | Variable | Scope | Use |
@@ -616,6 +626,21 @@ reported success. Re-running changes nothing either: the fix is to correct the
 metadata, which `objectstack validate` will point at.
 </Callout>
 
+#### Fault edges vs. `errorHandling.retry`
+
+The two recovery mechanisms operate at different scopes and do not compound:
+
+| | Scope | On failure |
+| :--- | :--- | :--- |
+| `fault` edge | one node | traversal continues from the handler; the run completes |
+| `errorHandling: { strategy: 'retry' }` | the whole flow | the flow re-runs **from the start** |
+
+A failure a fault edge handled is not a flow failure, so it does **not** consume
+a retry. That matters because flow-level retry replays every node that already
+succeeded — a second notification, a second created record. Prefer a fault edge
+where the failure is local; reach for `retry` only when replaying the whole flow
+is genuinely safe.
+
 ## Variables
 
 Flows use variables to pass data between nodes and to/from callers:

packages/cli/src/utils/lint-flow-patterns.test.ts

@@ -12,6 +12,7 @@ import {
   FLOW_APPROVAL_REVISE_UNMARKED_BACKEDGE,
   FLOW_APPROVAL_REVISE_DISABLED,
   FLOW_RUNAS_UNSCOPED,
+  FLOW_ERROR_LABEL_NOT_FAULT,
 } from './lint-flow-patterns.js';
 
 const CEL = (source: string) => ({ dialect: 'cel', source });
@@ -377,3 +378,73 @@ describe('lintFlowPatterns — user-less runAs unscoped (#1888 / ADR-0049 / ADR-
     });
   });
 });
+
+/**
+ * #3863 — `label: 'error'` without `type: 'fault'`.
+ *
+ * The label is cosmetic on an ordinary edge, so the handler runs on every
+ * SUCCESS (unconditional out-edges all traverse, in parallel) and never on a
+ * failure. Both halves are silent, which is why this is worth a build-time
+ * diagnostic rather than a runtime one.
+ */
+function edgeFlow(edge: Record<string, unknown>, sourceType = 'http') {
+  return {
+    flows: [{
+      name: 'edge_flow',
+      type: 'autolaunched',
+      nodes: [
+        { id: 'start', type: 'start', config: {} },
+        { id: 'op', type: sourceType, config: { url: 'https://x.test' } },
+        { id: 'handler', type: 'notify', config: { topic: 'x' } },
+        { id: 'end', type: 'end' },
+      ],
+      edges: [
+        { id: 'e1', source: 'start', target: 'op' },
+        { id: 'e2', source: 'op', target: 'end' },
+        { id: 'e_h', source: 'op', target: 'handler', ...edge },
+      ],
+    }],
+  };
+}
+
+describe('flow-error-label-not-fault (#3863)', () => {
+  it("flags label:'error' left at the default type", () => {
+    const fnds = lintFlowPatterns(edgeFlow({ label: 'error' }));
+    expect(fnds).toHaveLength(1);
+    expect(fnds[0].rule).toBe(FLOW_ERROR_LABEL_NOT_FAULT);
+    // The message must name the actual consequence, not just the mismatch.
+    expect(fnds[0].message).toContain('SUCCESSFUL');
+    expect(fnds[0].hint).toContain("type: 'fault'");
+  });
+
+  it.each(['Error', 'FAILURE', 'catch', 'on_error', 'fault'])(
+    "flags the equivalent label %s",
+    (label) => {
+      const fnds = lintFlowPatterns(edgeFlow({ label }));
+      expect(fnds.map((f) => f.rule)).toContain(FLOW_ERROR_LABEL_NOT_FAULT);
+    },
+  );
+
+  it("does NOT flag the correct shape (type: 'fault')", () => {
+    expect(lintFlowPatterns(edgeFlow({ label: 'error', type: 'fault' }))).toHaveLength(0);
+  });
+
+  it('does NOT flag an unlabelled fault edge', () => {
+    expect(lintFlowPatterns(edgeFlow({ type: 'fault' }))).toHaveLength(0);
+  });
+
+  it('does NOT flag an ordinary labelled edge', () => {
+    expect(lintFlowPatterns(edgeFlow({ label: 'approved' }))).toHaveLength(0);
+  });
+
+  it('does NOT flag a CONDITIONAL edge — a guarded path is not the footgun', () => {
+    expect(lintFlowPatterns(edgeFlow({ label: 'error', condition: "status == 'bad'" }))).toHaveLength(0);
+  });
+
+  it.each(['decision', 'approval'])(
+    "does NOT flag label:'error' out of a %s node — the label IS the branch selector",
+    (sourceType) => {
+      expect(lintFlowPatterns(edgeFlow({ label: 'error' }, sourceType))).toHaveLength(0);
+    },
+  );
+});

packages/cli/src/utils/lint-flow-patterns.ts

@@ -77,10 +77,36 @@ export const FLOW_APPROVAL_REVISE_DISABLED = 'flow-approval-revise-disabled';
  * resolves NO USER, and a schedule is only the most obvious such trigger.
  */
 export const FLOW_RUNAS_UNSCOPED = 'flow-runas-unscoped';
+export const FLOW_ERROR_LABEL_NOT_FAULT = 'flow-error-label-not-fault';
 
 /** 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']);
 
+/**
+ * #3863 — an edge LABELLED like an error path but not TYPED as one.
+ *
+ * Error routing is `type: 'fault'`. `label` is cosmetic on an ordinary edge, so
+ * `{ source, target, label: 'error' }` without the type does not mean "go here
+ * on failure" — it is an unconditional out-edge, and `traverseNext` runs every
+ * unconditional out-edge in parallel. The handler therefore fires on every
+ * SUCCESSFUL run of the source node, concurrently with the real success path,
+ * and never on a failure.
+ *
+ * Silent in both directions: the author believes errors are handled (they are
+ * not — the run still aborts) and never notices the handler running when
+ * nothing went wrong. The reading is especially natural for an AI author, since
+ * `label: 'error'` is exactly what the intent sounds like.
+ *
+ * Deliberately narrow, because a label IS meaningful on a branching node: a
+ * `decision`/`approval` executor returns a `branchLabel` and traversal then
+ * prefers the edge with that label, so `label: 'error'` there is a real branch
+ * selector. Conditional edges are likewise legitimate. Both are excluded.
+ */
+const ERROR_LABELS = new Set(['error', 'fault', 'failure', 'failed', 'catch', 'on_error', 'onerror', 'on error']);
+
+/** Node types whose executor selects an out-edge BY LABEL (`branchLabel`). */
+const BRANCH_LABEL_NODE_TYPES = new Set(['decision', 'approval', 'screen', 'try_catch']);
+
 /**
  * Does this flow auto-launch on a SCHEDULE (so a run carries no trigger user)?
  * Accepts the three author-time signals: `flow.type === 'schedule'`, a start-node
@@ -225,6 +251,46 @@ function edgeLabelOf(e: AnyRec): string {
  *    `type: 'back'`, so `registerFlow` rejects it as an un-declared cycle. The
  *    lint fires at compile time with the specific fix (mark the resubmit edge).
  */
+/**
+ * #3863 — flag edges labelled like an error path but left at the default type.
+ * See {@link ERROR_LABELS} for why this is a footgun and what is excluded.
+ */
+function scanErrorLabelledEdges(
+  flowName: string,
+  nodes: AnyRec[],
+  edges: AnyRec[],
+  findings: FlowLintFinding[],
+): void {
+  const typeById = new Map<string, string>();
+  for (const n of nodes) {
+    if (typeof n.id === 'string') typeById.set(n.id, typeof n.type === 'string' ? n.type : '');
+  }
+
+  for (const e of edges) {
+    const label = typeof e.label === 'string' ? e.label.trim().toLowerCase() : '';
+    if (!ERROR_LABELS.has(label)) continue;
+    if (e.type === 'fault') continue; // already an error path — nothing to say
+    if (e.condition) continue; // a guarded edge is not the unconditional footgun
+    const src = typeof e.source === 'string' ? e.source : '';
+    // A branching node picks its out-edge BY label, so the label is load-bearing.
+    if (BRANCH_LABEL_NODE_TYPES.has(typeById.get(src) ?? '')) continue;
+
+    findings.push({
+      where: `flow '${flowName}' · edge '${src}' → '${String(e.target)}'`,
+      message:
+        `edge is labelled '${String(e.label)}' but its type is '${String(e.type ?? 'default')}', not 'fault' — ` +
+        `so it is an ORDINARY out-edge. Unconditional out-edges all run in parallel, so '${String(e.target)}' ` +
+        `executes on every SUCCESSFUL run of '${src}' and never on a failure. The error path the label ` +
+        `describes does not exist, and the run still aborts when '${src}' fails.`,
+      hint:
+        `Add \`type: 'fault'\` to this edge. Only runtime failures route — a guard refusal (a filter token ` +
+        `that resolved to nothing, a missing required config key, an unscoped run) stays fatal by design and ` +
+        `must be fixed in the metadata, not handled. (#3863)`,
+      rule: FLOW_ERROR_LABEL_NOT_FAULT,
+    });
+  }
+}
+
 function scanApprovalReviseLoops(
   flowName: string,
   nodes: AnyRec[],
@@ -432,6 +498,11 @@ export function lintFlowPatterns(stack: AnyRec): FlowLintFinding[] {
 
     // (c) ADR-0044 — approval send-back-for-revision loop footguns.
     scanApprovalReviseLoops(flowName, nodes, edges, findings);
+
+    // (d) #3863 — an edge labelled like an error path but typed 'default' is an
+    //     unconditional out-edge: the handler runs on every SUCCESS, in parallel
+    //     with the real path, and never on a failure.
+    scanErrorLabelledEdges(flowName, nodes, edges, findings);
   }
   return findings;
 }

packages/services/service-automation/src/fault-edge-guard-containment.test.ts

@@ -287,3 +287,64 @@ describe('#3863 — a fault edge must not swallow a guard refusal', () => {
         expect(data.seen.deleted).toBe(false);
     });
 });
+
+/**
+ * #3863 — the boundary between the two recovery mechanisms.
+ *
+ * Node-level: a `fault` edge, precise — it handles one node and traversal
+ * continues from the handler.
+ * Flow-level: `errorHandling.retry`, blunt — it re-runs the flow FROM THE START,
+ * so every node that already succeeded runs a second time, side effects and all.
+ *
+ * They must not compound. A node whose fault edge handled it is not a flow
+ * failure, so it must not also consume a retry — otherwise declaring a handler
+ * would silently multiply the side effects of everything upstream of it. That
+ * holds today by construction (a routed failure never propagates out of
+ * `executeNode`), and this pins it so a refactor of the catch path cannot
+ * quietly change it.
+ */
+describe('#3863 — a handled failure does not trigger flow-level retry', () => {
+    it('runs the upstream node exactly once when a fault edge handles the failure', async () => {
+        const engine = new AutomationEngine(createTestLogger());
+        let upstreamRuns = 0;
+        let handlerRuns = 0;
+
+        engine.registerNodeExecutor({
+            type: 'script',
+            async execute(node) {
+                if (node.id === 'upstream') { upstreamRuns++; return { success: true }; }
+                if (node.id === 'risky') return { success: false, error: 'upstream 503' };
+                handlerRuns++;
+                return { success: true };
+            },
+        });
+        engine.registerFlow('handled_no_retry', {
+            name: 'handled_no_retry',
+            label: 'Handled No Retry',
+            type: 'autolaunched',
+            errorHandling: { strategy: 'retry', maxRetries: 3, retryDelayMs: 1 },
+            nodes: [
+                { id: 'start', type: 'start', label: 'Start' },
+                { id: 'upstream', type: 'script' as any, label: 'Upstream' },
+                { id: 'risky', type: 'script' as any, label: 'Risky' },
+                { id: 'handler', type: 'script' as any, label: 'Handler' },
+                { id: 'end', type: 'end', label: 'End' },
+            ],
+            edges: [
+                { id: 'e0', source: 'start', target: 'upstream' },
+                { id: 'e1', source: 'upstream', target: 'risky' },
+                { id: 'e2', source: 'risky', target: 'end' },
+                { id: 'e_fault', source: 'risky', target: 'handler', type: 'fault' },
+                { id: 'e3', source: 'handler', target: 'end' },
+            ],
+        } as any);
+
+        const result = await engine.execute('handled_no_retry');
+
+        expect(result.success).toBe(true);
+        expect(handlerRuns).toBe(1);
+        // The point: retry is configured and was NOT consumed. If a handled
+        // failure counted as a flow failure, `upstream` would have run again.
+        expect(upstreamRuns).toBe(1);
+    });
+});

skills/objectstack-automation/SKILL.md

@@ -139,9 +139,15 @@ variables: [
 > `label` is **required** on the flow and on every node.
 
 > **Handling a failed node: a `fault` edge.** `{ source, target, type: 'fault' }`
-> routes a failed node to a handler instead of ending the run; the handler reads
-> `{<nodeId>.error}` (or run-wide `{$error}`). The run then reports success, and
-> the failed step stays in the trace.
+> routes a failed node to a handler instead of ending the run. **`type: 'fault'`
+> is what routes — a `label: 'error'` alone does nothing:** the edge stays
+> ordinary, and every unconditional out-edge traverses on SUCCESS, so the
+> handler would run when the node succeeds and never when it fails
+> (`objectstack validate` reports `flow-error-label-not-fault`).
+> A handled failure does NOT consume a flow-level `errorHandling.retry`, which
+> replays the flow from the start — prefer a fault edge when the failure is
+> local. The handler reads `{<nodeId>.error}` (or run-wide `{$error}`). The run
+> then reports success, and the failed step stays in the trace.
 >
 > **It is not a way past a guardrail.** Only *runtime* failures route — a 404, a
 > rate-limit, a rejected write, a subflow that failed on its own. A *guard*