fix(automation): a fault edge must not switch off a guardrail (#3863)

.changeset/fault-edge-guard-containment.md

@@ -0,0 +1,51 @@
+---
+"@objectstack/service-automation": minor
+---
+
+fix(automation): a `fault` edge must not switch off a guardrail (#3863)
+
+A `fault` edge routes a failed node to a handler instead of aborting the run.
+That is the right primitive for the world not cooperating — an `http` node that
+404s, a connector that rate-limited, a rejected write.
+
+It was also, until now, routing the **refuse-to-execute** family. Those guards
+report that the METADATA is wrong, not that an operation failed: #3810
+(interpolation erased a filter condition), ADR-0049/#1888 (the run would execute
+unscoped), a data node naming no object. Because they surfaced as ordinary node
+failures, one declared edge silently disabled them.
+
+**The live consequence, reproduced in a test before the fix:** attach a `fault`
+edge to a `delete_record` whose filter has a typo (`{record.ownr}`), and #3810's
+protection against emptying the object was gone — the guard fired, the handler
+swallowed it, and the run reported `success: true`. That is the exact fail-open
+direction #3810 was opened to close, reachable from a single edge, and it is the
+kind of suppression an AI authoring loop reaches for first when trying to make a
+diagnostic go away.
+
+**Failures now carry a class.** `NodeExecutionResult.errorClass` is `'runtime'`
+(default — every existing executor keeps its current routing) or `'guard'`.
+Guard-class failures are never routed: they stay fatal with or without a `fault`
+edge, and the run fails with the guard's own message. Thrown guards are covered
+too — `UnscopedRunDataAccessError` is branded via a shared `guard-refusal`
+module, so the engine's catch path cannot become the bypass the return path no
+longer is.
+
+Marked as guard-class: the three `resolveNodeFilter` refusals (#3810), the four
+`objectName required` refusals, and `UnscopedRunDataAccessError` (ADR-0049).
+Genuine engine failures (`get_record(x) failed: …`) stay runtime-class and keep
+routing.
+
+**Also in this change**
+
+- `{<nodeId>.error}` now carries a failed node's message alongside the run-wide
+  `{$error}`. `$error` names only the most recent failure, so a handler shared by
+  two fault edges could not tell which node it was handling; `{charge_card.error}`
+  is addressable from any downstream template. Additive — `$error` is unchanged.
+- Fault edges are **documented** for the first time (`content/docs/automation/flows.mdx`
+  and the automation skill), including the routable/not-routable split. The skill
+  entry says plainly not to add a fault edge to silence a guard error, since that
+  is the misuse the class split now makes impossible.
+
+A run that takes a fault branch still reports success, and the failed step still
+carries `status: 'failure'` and its message in the trace — recovery does not
+erase the record of what failed (#3356/#3407).

content/docs/automation/flows.mdx

@@ -560,6 +560,49 @@ Edges connect nodes and define the execution path:
 | `condition` | `string` | optional | Boolean expression for branching |
 | `label` | `string` | optional | Label displayed on the connector |
 
+### Fault edges — handling a failed node
+
+A `fault` edge routes a **failed** node to a handler instead of aborting the
+run. Without one, any node failure ends the run.
+
+```typescript
+edges: [
+  { id: 'e_ok',    source: 'charge_card', target: 'mark_paid' },
+  { id: 'e_fault', source: 'charge_card', target: 'flag_for_review', type: 'fault' },
+]
+```
+
+The handler reads what went wrong from two variables:
+
+| Variable | Scope | Use |
+| :--- | :--- | :--- |
+| `{<nodeId>.error}` | the failing node | `{charge_card.error}` — addressable by name, so one handler shared by several fault edges can tell which node it is handling |
+| `{$error}` | run-wide | `{$error.nodeId}` / `{$error.message}` — the most recent failure only |
+
+A run that takes a fault branch and completes reports **success**, but the
+failed step stays in the run trace with `status: 'failure'` and its message —
+recovery never erases the record of what failed.
+
+<Callout type="warn">
+**A fault edge does not disable a guardrail.** Failures come in two classes, and
+only one is routable.
+
+- **Runtime** — the world did not cooperate: an `http` node got a 404, a
+  connector rate-limited, the data engine rejected a write. Routed to the fault
+  edge.
+- **Guard** — the *metadata* is wrong, and a refuse-to-execute check said so: a
+  filter token resolved to nothing so the condition was dropped from the query,
+  a data node names no object, or the run would execute unscoped. **Never
+  routed.** These stay fatal whether or not a `fault` edge exists, and the run
+  fails with the guard's own message.
+
+The split is deliberate. A dropped filter condition does not narrow a query, it
+widens it — so if guards were routable, one `fault` edge on a `delete_record`
+would turn off the protection against emptying the object while the run still
+reported success. Re-running changes nothing either: the fix is to correct the
+metadata, which `objectstack validate` will point at.
+</Callout>
+
 ## Variables
 
 Flows use variables to pass data between nodes and to/from callers:

packages/services/service-automation/src/builtin/crud-nodes.ts

@@ -161,7 +161,7 @@ export function registerCrudNodes(engine: AutomationEngine, ctx: PluginContext):
             async execute(node, variables, context) {
                 const cfg = (node.config ?? {}) as Record<string, unknown>;
                 const objectName = String(readAliasedConfig(cfg, 'get_record', 'objectName', ['object'], ctx.logger) ?? '');
-                if (!objectName) return { success: false, error: 'get_record: objectName required' };
+                if (!objectName) return { success: false, error: 'get_record: objectName required', errorClass: 'guard' };
 
                 // `filters` → `filter` is now handled at load by the ADR-0087 D2
                 // conversion layer ('flow-node-crud-filter-alias'), so the executor
@@ -170,7 +170,7 @@ export function registerCrudNodes(engine: AutomationEngine, ctx: PluginContext):
                     cfg.filter, variables, context, 'get_record',
                     'would have read rows the filter was written to exclude',
                 );
-                if ('error' in filterResult) return { success: false, error: filterResult.error };
+                if ('error' in filterResult) return { success: false, error: filterResult.error, errorClass: 'guard' };
                 const filter = filterResult.filter;
                 const fields = cfg.fields as string[] | undefined;
                 const limit = typeof cfg.limit === 'number' ? cfg.limit : undefined;
@@ -222,7 +222,7 @@ export function registerCrudNodes(engine: AutomationEngine, ctx: PluginContext):
             async execute(node, variables, context) {
                 const cfg = (node.config ?? {}) as Record<string, unknown>;
                 const objectName = String(readAliasedConfig(cfg, 'create_record', 'objectName', ['object'], ctx.logger) ?? '');
-                if (!objectName) return { success: false, error: 'create_record: objectName required' };
+                if (!objectName) return { success: false, error: 'create_record: objectName required', errorClass: 'guard' };
 
                 const fields = interpolate(cfg.fields ?? {}, variables, context) as Record<string, unknown>;
                 const outputVariable = cfg.outputVariable as string | undefined;
@@ -303,14 +303,14 @@ export function registerCrudNodes(engine: AutomationEngine, ctx: PluginContext):
             async execute(node, variables, context) {
                 const cfg = (node.config ?? {}) as Record<string, unknown>;
                 const objectName = String(readAliasedConfig(cfg, 'update_record', 'objectName', ['object'], ctx.logger) ?? '');
-                if (!objectName) return { success: false, error: 'update_record: objectName required' };
+                if (!objectName) return { success: false, error: 'update_record: objectName required', errorClass: 'guard' };
 
                 // `filters` → `filter` converted at load (ADR-0087 D2); read canonical.
                 const filterResult = resolveNodeFilter(
                     cfg.filter, variables, context, 'update_record',
                     'would have matched — and overwritten — rows the filter was written to exclude',
                 );
-                if ('error' in filterResult) return { success: false, error: filterResult.error };
+                if ('error' in filterResult) return { success: false, error: filterResult.error, errorClass: 'guard' };
                 const filter = filterResult.filter;
                 // `fields` is the single canonical write-map key — no alias (the wrong key
                 // `fieldValues` is corrected at the authoring source + rejected by graph-lint).
@@ -376,7 +376,7 @@ export function registerCrudNodes(engine: AutomationEngine, ctx: PluginContext):
             async execute(node, variables, context) {
                 const cfg = (node.config ?? {}) as Record<string, unknown>;
                 const objectName = String(readAliasedConfig(cfg, 'delete_record', 'objectName', ['object'], ctx.logger) ?? '');
-                if (!objectName) return { success: false, error: 'delete_record: objectName required' };
+                if (!objectName) return { success: false, error: 'delete_record: objectName required', errorClass: 'guard' };
 
                 // `filters` → `filter` converted at load (ADR-0087 D2); read canonical.
                 // The highest-stakes of the three: an erased condition here is the
@@ -385,7 +385,7 @@ export function registerCrudNodes(engine: AutomationEngine, ctx: PluginContext):
                     cfg.filter, variables, context, 'delete_record',
                     'would have matched every remaining row and deleted it',
                 );
-                if ('error' in filterResult) return { success: false, error: filterResult.error };
+                if ('error' in filterResult) return { success: false, error: filterResult.error, errorClass: 'guard' };
                 const filter = filterResult.filter;
 
                 const data = getData();

packages/services/service-automation/src/engine.ts

@@ -18,6 +18,7 @@ import { ConnectorSchema } from '@objectstack/spec/integration';
 // engine at module load in both ESM and CJS builds.
 import { ExpressionEngine, validateExpression } from '@objectstack/formula';
 import { runIsUnscopedUserMode, flowTouchesData } from './runtime-identity.js';
+import { isGuardRefusal } from './guard-refusal.js';
 
 // ─── Node Executor Interface (Plugin Extension Point) ───────────────
 
@@ -52,10 +53,37 @@ export interface NodeExecutor {
     ): Promise<NodeExecutionResult>;
 }
 
+/**
+ * Why a node failed — the question a `fault` edge's routing decision turns on
+ * (#3863).
+ *
+ *  - `runtime` — the world did not cooperate: an `http` node got a 404, a
+ *    connector rate-limited, the data engine rejected a write. The metadata is
+ *    fine and a later run could succeed. A declared `fault` edge routes it.
+ *  - `guard` — the METADATA is wrong, and a refuse-to-execute guard said so:
+ *    interpolation erased a filter condition (#3810), a data node names no
+ *    object, a run would execute unscoped (ADR-0049/#1888). Re-running changes
+ *    nothing, and the refusal IS the safety property. Not routable — it stays
+ *    fatal whether or not a `fault` edge exists.
+ *
+ * The split exists because without it a `fault` edge is a one-edge switch that
+ * turns off the platform's data-safety guarantees: attach one to a
+ * `delete_record` and #3810's protection against emptying the object is gone,
+ * while the run still reports success. Absent, this defaults to `runtime`, so
+ * every executor written before the field keeps its current routing behaviour.
+ */
+export type NodeFailureClass = 'runtime' | 'guard';
+
 export interface NodeExecutionResult {
     success: boolean;
     output?: Record<string, unknown>;
     error?: string;
+    /**
+     * #3863 — why this failed, which decides whether a `fault` edge may route it.
+     * Only meaningful when `success` is false; defaults to `runtime`.
+     * See {@link NodeFailureClass}.
+     */
+    errorClass?: NodeFailureClass;
     /**
      * #3407: advisory warnings surfaced on the step's log entry. The step still
      * SUCCEEDS — a warning flags a legal-but-surprising outcome (e.g. an
@@ -2777,6 +2805,24 @@ export class AutomationEngine implements IAutomationService {
         }
     }
 
+    /**
+     * #3863 — publish a failed node's message under `<nodeId>.error` alongside
+     * the run-wide `$error`.
+     *
+     * `$error` names only the most recent failure, so a flow with more than one
+     * `fault` edge converging on a shared handler cannot tell which node it is
+     * handling. Keying by node id makes `{cleanup.error}` addressable from any
+     * downstream template, which is what a handler needs to branch or report.
+     * Merges into an existing entry so a node's earlier `output` survives.
+     */
+    private setNodeError(variables: Map<string, unknown>, nodeId: string, message: string): void {
+        const prior = variables.get(nodeId);
+        const base = prior && typeof prior === 'object' && !Array.isArray(prior)
+            ? (prior as Record<string, unknown>)
+            : {};
+        variables.set(nodeId, { ...base, error: message });
+    }
+
     /**
      * Execute a node with timeout support, fault edge handling, and step logging.
      */
@@ -2859,10 +2905,16 @@ export class AutomationEngine implements IAutomationService {
                     error: { code: 'EXECUTION_ERROR', message: errMsg },
                 });
 
-                // Check for fault edges
-                const faultEdge = flow.edges.find(e => e.source === node.id && e.type === 'fault');
+                // #3863 — a guard that THROWS is as un-routable as one that
+                // returns: `UnscopedRunDataAccessError` (ADR-0049/#1888) reports
+                // that the metadata would run unscoped, and rerouting it would
+                // let a `fault` edge disable the elevation check.
+                const faultEdge = isGuardRefusal(execErr)
+                    ? undefined
+                    : flow.edges.find(e => e.source === node.id && e.type === 'fault');
                 if (faultEdge) {
                     variables.set('$error', { nodeId: node.id, message: errMsg });
+                    this.setNodeError(variables, node.id, errMsg);
                     const faultTarget = flow.nodes.find(n => n.id === faultEdge.target);
                     if (faultTarget) {
                         await this.executeNode(faultTarget, flow, variables, context, steps);
@@ -2887,9 +2939,16 @@ export class AutomationEngine implements IAutomationService {
 
                 // Write error output to variable context for downstream nodes
                 variables.set('$error', { nodeId: node.id, message: errMsg, output: result.output });
-
-                // Check for fault edges
-                const faultEdge = flow.edges.find(e => e.source === node.id && e.type === 'fault');
+                this.setNodeError(variables, node.id, errMsg);
+
+                // #3863 — only a `runtime` failure may be routed. A `guard`
+                // refusal says the METADATA is wrong (#3810 erased a filter
+                // condition, a data node names no object); rerouting it would
+                // make a single `fault` edge a switch that turns the guard off
+                // while the run still reports success.
+                const faultEdge = result.errorClass === 'guard'
+                    ? undefined
+                    : flow.edges.find(e => e.source === node.id && e.type === 'fault');
                 if (faultEdge) {
                     const faultTarget = flow.nodes.find(n => n.id === faultEdge.target);
                     if (faultTarget) {