diff --git a/.changeset/fault-edge-label-lint.md b/.changeset/fault-edge-label-lint.md new file mode 100644 index 0000000000..7a76a1990f --- /dev/null +++ b/.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. diff --git a/content/docs/automation/flows.mdx b/content/docs/automation/flows.mdx index 930705ed85..d782100a7e 100644 --- a/content/docs/automation/flows.mdx +++ b/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: [ ] ``` + +**`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`. + + 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. +#### 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: diff --git a/packages/cli/src/utils/lint-flow-patterns.test.ts b/packages/cli/src/utils/lint-flow-patterns.test.ts index 683844928a..f7ac8c10fa 100644 --- a/packages/cli/src/utils/lint-flow-patterns.test.ts +++ b/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, 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); + }, + ); +}); diff --git a/packages/cli/src/utils/lint-flow-patterns.ts b/packages/cli/src/utils/lint-flow-patterns.ts index dffb470afd..e3925fc2ce 100644 --- a/packages/cli/src/utils/lint-flow-patterns.ts +++ b/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(); + 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; } diff --git a/packages/services/service-automation/src/fault-edge-guard-containment.test.ts b/packages/services/service-automation/src/fault-edge-guard-containment.test.ts index a77b334004..45155ac882 100644 --- a/packages/services/service-automation/src/fault-edge-guard-containment.test.ts +++ b/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); + }); +}); diff --git a/skills/objectstack-automation/SKILL.md b/skills/objectstack-automation/SKILL.md index b68cd8b8de..0acb693057 100644 --- a/skills/objectstack-automation/SKILL.md +++ b/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 -> `{.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 `{.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*