fix(automation): close the default-routable footgun on refuse-to-execute guards (#3863) - #3889
Merged
Merged
Conversation
…ute guards (#3863) #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. 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. 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:
|
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
marked this pull request as ready for review
July 28, 2026 14:46
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.
Why
#3881 stopped a
faultedge from swallowing a guard refusal, keyed onNodeExecutionResult.errorClass. That field defaults to'runtime'— the right call for compatibility, since every executor written before the split keeps its routing — but it leaves the footgun pointing the other way:Nothing in the type system catches that. The property #3881 secured can be un-secured by omission, which is the weakest possible way to hold a safety guarantee.
Auditing for it also turned up five guards that were already unmarked.
What changed
1.
refuseNode(reason)— one act, not twoWriting a guard and marking it un-routable become the same call:
Its doc states the test for using it — re-running unchanged can never succeed AND the fix is to edit metadata — and states the inverse just as plainly, because over-marking is not the safe direction: classifying a handleable condition as
guardturns a recoverable integration into a dead run.2. Five guards that were never marked
All are a missing required config key or a defective graph; none can succeed on a retry:
httpwith nourlsubflowwith noconfig.flowNamesubflowpast max nesting depthmapwith noconfig.flowNameconnector_actionwith noconnectorId/actionIdThe seven
crud-nodesguards from #3881 move to the helper — same behaviour, one spelling.3. A behavioural inventory test
Every known guard is driven through the engine with a fault edge attached and asserted still fatal — matching on the refusal text, so a guard that starts failing for a different reason cannot pass vacuously.
Verified to have teeth: temporarily un-marking the
httpguard fails its row immediately (1 failed | 13 passed), then passes again once restored. A test that pins a safety property is worth little unless you have watched it fail.The negative half is pinned in the same file — a plain node failure and a thrown error must still route, since that is what fault edges exist for.
Deliberately not marked
Judgment calls, recorded so they are not silently revisited:
Each is a condition the world caused, and an author must be able to handle it.
Considered and rejected
Making
errorClassrequired onNodeExecutionResultwould enforce classification at compile time — the genuinely airtight fix. Measured the blast radius: 281 non-testsuccess: falsesites across the repo (58 in automation) plus 82 in tests, and it would break third-party executors at compile time. That is a large, noisy change for a type-only gain over the helper, so it is out of scope here; if the executor surface is ever revised for other reasons, it is worth folding in then.Verification
packages/services/service-automation: 421 passed, no regressions (was 407 before this branch's 14 new tests).check-role-wordgate OK.tsc --noEmiton the package: 2 errors, both pre-existing inengine.test.tsaboutresumeAuthority— unchanged from the base.Generated by Claude Code