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

Two of the three items left open on #3863. Both make 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 an edge written

  { 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 the source, alongside
the real success path, and never on a failure. The run still aborts when the
node fails.

Silent both ways: the author believes failures are handled, and never notices
the handler running when nothing went wrong. Especially natural for an AI
author, since the label is exactly what the intent sounds like.

Narrow by construction, because a label IS load-bearing on a branching node — a
decision/approval executor returns a branchLabel and traversal prefers the edge
carrying it. Excluded: edges out of those node types, conditional edges, and
edges already typed 'fault'. Matches the obvious synonyms case-insensitively.
No findings against the shipped showcase.

An alias (accepting label:'error' as type:'fault') was considered and rejected:
two spellings for one concept read worse than one spelling plus a diagnostic
that names the fix.

Also pinned: a handled failure does not consume a flow-level retry. A fault edge
handles one node; errorHandling.retry replays the flow FROM THE START, re-running
everything that already succeeded. The two must not compound. That held by
construction — a routed failure never propagates out of executeNode — and 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 that `label` does
not select a path except on a branching node.

Author: os-zhuang

.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*