fix(automation): a fault edge must not switch off a guardrail (#3863) - #3881
Merged
Conversation
A `fault` edge routes a failed node to a handler instead of aborting the run — the right primitive for the world not cooperating (a 404, a rate-limit, a rejected write). It was also 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. Surfacing as ordinary node failures, one declared edge silently disabled them. Reproduced in a test before fixing: a `fault` edge on a `delete_record` whose filter has a typo (`{record.ownr}`) made #3810's protection against emptying the object vanish — guard fired, handler swallowed it, run reported success: true. The exact fail-open direction #3810 was opened to close, reachable from one edge, and the first thing an AI loop reaches for to make a diagnostic go away. Failures now carry a class. `NodeExecutionResult.errorClass` is 'runtime' (default, so every existing executor keeps its routing) or 'guard'. Guard-class failures are never routed: fatal with or without a fault edge, failing with the guard's own message. Thrown guards are covered too — UnscopedRunDataAccessError is branded through a shared `guard-refusal` module, so the catch path cannot become the bypass the return path no longer is. Marked guard-class: three resolveNodeFilter refusals, four `objectName required` refusals, UnscopedRunDataAccessError. Genuine engine failures stay runtime-class and keep routing. Also here: - `{<nodeId>.error}` carries a failed node's message alongside run-wide `{$error}`, which names only the most recent failure — a handler shared by two fault edges could not tell which node it was handling. Additive. - Fault edges are documented for the first time (flows.mdx + automation skill), including the routable/not-routable split and an explicit "do not add one to silence a guard error". A run taking a fault branch still reports success, and the failed step keeps status 'failure' and its message in the trace (#3356/#3407). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EdCvGtER9SzJopS4Yh3Pa1
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 1 package(s): 6 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
os-zhuang
marked this pull request as ready for review
July 28, 2026 14:15
os-zhuang
pushed a commit
that referenced
this pull request
Jul 28, 2026
The #3881 callout in flows.mdx and the automation skill enumerated the guards that existed at the time. This PR adds five more, which makes an enumeration the wrong shape — it was already stale by the end of its own branch. Both now state the TEST instead: a failure is a guard when re-running unchanged could never succeed AND the fix is to edit metadata. The examples stay, as examples rather than as the definition, and the runtime side gains the cases this PR deliberately left routable (a degraded connector, a subflow that failed on its own) so the two lists read as one contract. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EdCvGtER9SzJopS4Yh3Pa1
os-zhuang
added a commit
that referenced
this pull request
Jul 28, 2026
…ute 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this turned out to be
#3863 was filed to build node-level error branches. On opening the engine, they already exist and work:
type: 'fault'is in the edge schema,executeNoderoutes both the thrown and the returned failure paths to the handler,$erroris populated, and there are tests. So the feature was not the gap.The gap was that it routed everything — including guard refusals. That is the fail-open hole flagged as the hard design constraint when the issue was promoted, and it is live in
maintoday.The bug, reproduced before fixing
Attach a
faultedge to adelete_recordwhose filter has a typo:{record.ownr}does not resolve, so the only condition is erased and the filter would collapse to{}— every row. #3810's guard correctly refuses the node. The fault edge then swallowed the refusal, ran the handler, and the run reportedsuccess: true.One declared edge silently disabled the platform's protection against emptying an object. That is precisely the fail-open direction #3810 was opened to close, and it is the first thing an AI authoring loop reaches for when trying to make a diagnostic go away — the same suppression dynamic that got the
$optionalproposal (#3862) closed as not planned.The test asserting this lands in the tree failing, then passes with the fix.
The fix: failures carry a class
NodeExecutionResult.errorClass—'runtime'(default) or'guard'.faultedgeruntimehttp404, connector rate-limit, data engine rejected a writeguardDefaulting to
runtimemeans every existing executor keeps its current routing; this is not a behaviour change for anything except the guards.Thrown guards are covered too.
resolveNodeFilterreturns its refusal;UnscopedRunDataAccessErrorthrows one. Both paths are contained, via a sharedguard-refusalmodule (its own file becauseengine.tsalready importsruntime-identity.ts, so the guard cannot import the engine back). Otherwise the catch path would simply become the bypass the return path no longer is.Marked guard-class: the three
resolveNodeFilterrefusals, the fourobjectName requiredrefusals, andUnscopedRunDataAccessError. Genuine engine failures (get_record(x) failed: …) stay runtime-class and keep routing.Also in scope (the rest of the issue's v1)
{<nodeId>.error}now carries a failed node's message alongside run-wide{$error}.$errornames 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. Purely additive —$erroris unchanged.status: 'failure'with its message — verified with a test, since the service-automation: runAs:'user' runs data ops with a credential-less user — trigger context never carries the actor's permission sets/positions (follow-up to #1888) #3356/service-automation: update_record reportssuccesswhen written fields are silently stripped — no observability for dropped writes (split from #3356) #3407 lesson is that recovery must not erase the record.content/docs/automation/flows.mdxand the automation skill, including the routable/not-routable table. The skill says plainly not to add a fault edge to silence a guard error — that is the misuse the class split now makes impossible, and the skill is what an AI author reads.Verification
fault-edge-guard-containment.test.ts— 6 tests: guard refusal contained with a fault edge present; runtime failure on the same shape still routes; thrown ADR-0049 guard also contained; guard fatal without a fault edge;{<nodeId>.error}published; failed step still traced after recovery.packages/services/service-automationfull suite: 407 passed, no regressions — the pre-existing fault-edge tests still pass, which is the point of defaulting toruntime.check-role-wordgate OK.tsc --noEmiton the package reports 2 errors, both inengine.test.tsaboutresumeAuthority— confirmed pre-existing by stashing this branch's changes and re-running on the base.Deferred (still tracked on #3863)
Interaction with flow-level
errorHandling.retry; error edges that loop back (needs the ADR-0044type: 'back'cycle treatment); whetherlabel: 'error'should be an accepted alias.Generated by Claude Code