fix(automation): close the default-routable footgun on refuse-to-execute guards (#3863) (#3889)

#3881 stopped a `fault` edge swallowing a guard refusal, keyed on `errorClass`.
That field defaults to 'runtime' — right for compatibility, since every executor
written before the split keeps its routing — but it leaves the footgun pointing
the other way: a NEW guard is routable unless its author remembers to classify
it, and forgetting is silent.

`refuseNode(reason)` returns a guard-class failure, so writing a guard and
marking it un-routable become one act. Its doc states the test for using it
(re-running unchanged can never succeed AND the fix is to edit metadata) and the
inverse, because over-marking turns a recoverable integration into a dead run.

Five guards that were never marked are now un-routable — all missing required
config or a defective graph, none able to succeed on a retry:

  - `http` with no url
  - `subflow` with no flowName, and subflow past max nesting depth (a recursive
    graph nests exactly as deep next run)
  - `map` with no flowName
  - `connector_action` with no connectorId/actionId

The seven crud-nodes guards from #3881 move to the helper — same behaviour, one
spelling.

A behavioural inventory test drives every known guard through the engine with a
fault edge attached and asserts it stays fatal, matching the refusal text so a
guard failing for a different reason cannot pass vacuously. Verified to have
teeth: un-marking one fails its row immediately. The negative half is pinned
too — a plain failure and a thrown error must still route.

Deliberately not marked: a degraded connector (#3017 recovers automatically), a
collection that did not resolve to an array, a collection over the iteration
cap, a subflow that failed on its own. The world caused those.

The flows.mdx and skill callouts now state the guard/runtime TEST rather than a
closed list, which this PR had already made stale within its own branch.

Considered and rejected: making `errorClass` required on the result type —
compile-time enforcement, but it breaks 281 call sites plus third-party
executors for a type-only gain over the helper.

Author: os-zhuang

.changeset/guard-refusal-chokepoint.md

@@ -0,0 +1,51 @@
+---
+"@objectstack/service-automation": patch
+---
+
+fix(automation): close the default-routable footgun on refuse-to-execute guards (#3863)
+
+#3881 stopped a `fault` edge from swallowing a guard refusal, keyed on
+`NodeExecutionResult.errorClass`. That field defaults to `'runtime'`, which was
+right for compatibility — every executor written before the split keeps its
+routing — but it leaves the footgun pointing the other way: **a new guard is
+routable unless its author remembers to classify it**, and forgetting is silent.
+Nothing in the type system catches it.
+
+Three changes close that for the guards that exist and make the next one hard to
+get wrong.
+
+**`refuseNode(reason)`** — one call that returns a guard-class failure, so
+"write a guard" and "mark it un-routable" become the same act. Its doc states
+the test for using it: re-running unchanged can never succeed AND the fix is to
+edit metadata. It also states the inverse, because over-marking is not the safe
+direction — classifying a handleable condition as `guard` turns a recoverable
+integration into a dead run.
+
+**Five guards that were never marked** are now un-routable. All are missing
+required config or a defective graph, none can succeed on a retry:
+
+- `http` with no `url`
+- `subflow` with no `config.flowName`, and `subflow` exceeding max nesting depth
+  (a recursive graph nests exactly as deep next run)
+- `map` with no `config.flowName`
+- `connector_action` with no `connectorId` / `actionId`
+
+The seven `crud-nodes` guards from #3881 move to the helper — same behaviour,
+one spelling.
+
+**A behavioural inventory test** drives every known guard through the engine
+with a fault edge attached and asserts it is still fatal, matching on the
+refusal text so a guard failing for a different reason cannot pass vacuously.
+Verified to have teeth: un-marking one guard fails its row immediately. The
+negative half is pinned too — a plain node failure and a thrown error must still
+route, since that is what fault edges are for.
+
+Deliberately **not** marked, and why: a degraded connector (#3017 says recovery
+is automatic), a collection that did not resolve to an array, a collection over
+the iteration cap, and a subflow that failed on its own. Those are conditions
+the world caused, and an author must be able to handle them.
+
+Considered and rejected: making `errorClass` required on the result type. It
+would enforce classification at compile time, but it breaks every node executor
+returning a failure — 281 call sites across the repo plus third-party
+executors — for a type-only gain over the helper.

content/docs/automation/flows.mdx

@@ -593,15 +593,22 @@ recovery never erases the record of what failed.
 **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
+- **Runtime** — the world did not cooperate, and a later run could succeed: an
+  `http` node got a 404, a connector rate-limited or is degraded, the data engine
+  rejected a write, a collection arrived the wrong shape, a subflow failed on its
+  own. Routed to the fault edge.
+- **Guard** — a refuse-to-execute check found the *metadata* wrong. **Never
+  routed:** these stay fatal whether or not a `fault` edge exists, and the run
   fails with the guard's own message.
 
+A failure is a guard when both hold: re-running the flow unchanged could never
+succeed, **and** the fix is to edit metadata. In practice that is a missing
+required config key (a data node with no `objectName`, an `http` node with no
+`url`, a `subflow` or `map` with no `flowName`, a `connector_action` with no
+`connectorId`/`actionId`), a filter token that resolved to nothing so the
+condition was dropped from the query, a graph that recurses past the nesting
+ceiling, or a run that would execute unscoped.
+
 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

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

@@ -3,6 +3,7 @@
 import type { PluginContext } from '@objectstack/core';
 import { defineActionDescriptor } from '@objectstack/spec/automation';
 import type { AutomationEngine, ConnectorActionContext } from '../engine.js';
+import { refuseNode } from '../guard-refusal.js';
 
 /**
  * Connector built-in node — `connector_action` (generic integration dispatch).
@@ -49,10 +50,9 @@ export function registerConnectorNodes(engine: AutomationEngine, ctx: PluginCont
         async execute(node, variables, context) {
             const cfg = node.connectorConfig;
             if (!cfg?.connectorId || !cfg?.actionId) {
-                return {
-                    success: false,
-                    error: `connector_action '${node.id}': connectorConfig.connectorId and .actionId are required`,
-                };
+                return refuseNode(
+                    `connector_action '${node.id}': connectorConfig.connectorId and .actionId are required`,
+                );
             }
 
             const handler = engine.resolveConnectorAction(cfg.connectorId, cfg.actionId);

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

@@ -7,6 +7,7 @@ import type { DroppedFieldsEvent } from '@objectstack/spec/data';
 import type { AutomationEngine } from '../engine.js';
 import { interpolate, interpolateFilter, type VariableMap } from './template.js';
 import { readAliasedConfig } from './config-aliases.js';
+import { refuseNode } from '../guard-refusal.js';
 import { resolveRunDataContext } from '../runtime-identity.js';
 
 /**
@@ -161,7 +162,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', errorClass: 'guard' };
+                if (!objectName) return refuseNode('get_record: objectName required');
 
                 // `filters` → `filter` is now handled at load by the ADR-0087 D2
                 // conversion layer ('flow-node-crud-filter-alias'), so the executor
@@ -170,7 +171,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, errorClass: 'guard' };
+                if ('error' in filterResult) return refuseNode(filterResult.error);
                 const filter = filterResult.filter;
                 const fields = cfg.fields as string[] | undefined;
                 const limit = typeof cfg.limit === 'number' ? cfg.limit : undefined;
@@ -222,7 +223,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', errorClass: 'guard' };
+                if (!objectName) return refuseNode('create_record: objectName required');
 
                 const fields = interpolate(cfg.fields ?? {}, variables, context) as Record<string, unknown>;
                 const outputVariable = cfg.outputVariable as string | undefined;
@@ -303,14 +304,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', errorClass: 'guard' };
+                if (!objectName) return refuseNode('update_record: objectName required');
 
                 // `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, errorClass: 'guard' };
+                if ('error' in filterResult) return refuseNode(filterResult.error);
                 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 +377,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', errorClass: 'guard' };
+                if (!objectName) return refuseNode('delete_record: objectName required');
 
                 // `filters` → `filter` converted at load (ADR-0087 D2); read canonical.
                 // The highest-stakes of the three: an erased condition here is the
@@ -385,7 +386,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, errorClass: 'guard' };
+                if ('error' in filterResult) return refuseNode(filterResult.error);
                 const filter = filterResult.filter;
 
                 const data = getData();

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

@@ -4,6 +4,7 @@ import type { PluginContext } from '@objectstack/core';
 import { defineActionDescriptor } from '@objectstack/spec/automation';
 import { randomUUID } from 'node:crypto';
 import type { AutomationEngine } from '../engine.js';
+import { refuseNode } from '../guard-refusal.js';
 import { interpolate } from './template.js';
 
 /**
@@ -95,7 +96,7 @@ export function registerHttpNodes(engine: AutomationEngine, ctx: PluginContext):
             const cfg = interpolate(raw, variables, context) as Record<string, unknown>;
 
             const url = cfg.url as string | undefined;
-            if (!url) return { success: false, error: 'http: url is required' };
+            if (!url) return refuseNode('http: url is required');
 
             const durable = cfg.durable === true;
             const headers = cfg.headers as Record<string, string> | undefined;

packages/services/service-automation/src/builtin/map-node.ts

@@ -4,6 +4,7 @@ import type { PluginContext } from '@objectstack/core';
 import { defineActionDescriptor } from '@objectstack/spec/automation';
 import type { AutomationContext } from '@objectstack/spec/contracts';
 import type { AutomationEngine } from '../engine.js';
+import { refuseNode } from '../guard-refusal.js';
 import { interpolate } from './template.js';
 
 /** Hard cap on map fan-out — turns a runaway collection into a clean error. */
@@ -74,7 +75,7 @@ export function registerMapNode(engine: AutomationEngine, ctx: PluginContext): v
       const flowName =
         typeof cfg.flowName === 'string' ? cfg.flowName : typeof cfg.flow === 'string' ? cfg.flow : undefined;
       if (!flowName) {
-        return { success: false, error: `map '${node.id}': config.flowName (the per-item subflow) is required` };
+        return refuseNode(`map '${node.id}': config.flowName (the per-item subflow) is required`);
       }
 
       const iteratorVariable =

packages/services/service-automation/src/builtin/subflow-node.ts

@@ -5,6 +5,7 @@ import { defineActionDescriptor } from '@objectstack/spec/automation';
 import type { AutomationContext } from '@objectstack/spec/contracts';
 import type { AutomationEngine } from '../engine.js';
 import { interpolate } from './template.js';
+import { refuseNode } from '../guard-refusal.js';
 
 /** Hard cap on subflow nesting — turns an accidental cycle into a clean error. */
 const MAX_SUBFLOW_DEPTH = 16;
@@ -55,16 +56,17 @@ export function registerSubflowNode(engine: AutomationEngine, ctx: PluginContext
       const flowName =
         typeof cfg.flowName === 'string' ? cfg.flowName : typeof cfg.flow === 'string' ? cfg.flow : undefined;
       if (!flowName) {
-        return { success: false, error: `subflow '${node.id}': config.flowName is required` };
+        return refuseNode(`subflow '${node.id}': config.flowName is required`);
       }
 
       // Cycle guard: depth rides on the context so it accumulates across nesting.
       const depth = Number((context as { $subflowDepth?: number } | undefined)?.$subflowDepth ?? 0);
       if (depth >= MAX_SUBFLOW_DEPTH) {
-        return {
-          success: false,
-          error: `subflow '${flowName}': max nesting depth (${MAX_SUBFLOW_DEPTH}) exceeded — recursive subflow?`,
-        };
+        // A graph that recurses past the ceiling is a metadata defect, not a
+        // transient condition — the next run nests exactly as deep (#3863).
+        return refuseNode(
+          `subflow '${flowName}': max nesting depth (${MAX_SUBFLOW_DEPTH}) exceeded — recursive subflow?`,
+        );
       }
 
       // Map inputs (resolve `{var}` against the parent's variables/context).