feat(cli,automation): catch label: 'error' written where type: 'fault' was meant (#3863) - #3893
Merged
Merged
Conversation
…ult'` was meant (#3863) Two of the three items left open on #3863. Both make 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 an edge written { 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 the source, alongside the real success path, and never on a failure. The run still aborts when the node fails. Silent both ways: the author believes failures are handled, and never notices the handler running when nothing went wrong. Especially natural for an AI author, since the label is exactly what the intent sounds like. Narrow by construction, because a label IS load-bearing on a branching node — a decision/approval executor returns a branchLabel and traversal prefers the edge carrying it. Excluded: edges out of those node types, conditional edges, and edges already typed 'fault'. Matches the obvious synonyms case-insensitively. No findings against the shipped showcase. An alias (accepting label:'error' as type:'fault') was considered and rejected: two spellings for one concept read worse than one spelling plus a diagnostic that names the fix. Also pinned: a handled failure does not consume a flow-level retry. A fault edge handles one node; errorHandling.retry replays the flow FROM THE START, re-running everything that already succeeded. The two must not compound. That held by construction — a routed failure never propagates out of executeNode — and 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 that `label` does not select a path except on a branching node. 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 2 package(s): 22 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 15:09
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.
Two of the three items left open on #3863. Both make the fault-edge contract legible; neither changes routing behaviour.
1. New lint —
flow-error-label-not-faulttype: 'fault'is what routes a failure.labelis cosmetic on an ordinary edge. So this, which reads exactly like error handling:is an ordinary out-edge — and
traverseNextruns every unconditional out-edge in parallel (Promise.all). The consequence is the opposite of the intent:Silent in both directions — the author believes failures are handled and never notices the handler firing when nothing went wrong. The misreading is especially natural for an AI author, since
label: 'error'is precisely what the intent sounds like. That is what makes it worth a build-time diagnostic instead of a puzzled look at a run trace weeks later.Deliberately narrow, because a label is load-bearing on a branching node: a
decision/approvalexecutor returns abranchLabeland traversal then prefers the edge carrying it. Excluded from the rule:decision/approval/screen/try_catch(the label is a real branch selector there)faultMatches the obvious synonyms case-insensitively (
error,failure,failed,catch,on_error,fault). Verified against the shipped showcase: no findings.An alias was considered and rejected. Accepting
label: 'error'as if it weretype: 'fault'would fix the immediate confusion but leave two spellings for one concept, which reads worse than one spelling plus a diagnostic that names the fix.2. Pinned: a handled failure does not consume a flow-level retry
The two recovery mechanisms have different scopes and must not compound:
faultedgeerrorHandling: { strategy: 'retry' }Flow-level retry replays every node that already succeeded — a second notification, a second created record. So a failure a fault edge handled must not also consume a retry, or declaring a handler would silently multiply upstream side effects.
That already held by construction (a routed failure never propagates out of
executeNode, so the flow-level catch never fires). It was never asserted. Now a test runs a flow witherrorHandling.retryconfigured plus a fault edge, and checks the upstream node ran exactly once.Docs
content/docs/automation/flows.mdxand the automation skill gain both points, plus a correction on the edge-property table:labeldoes not select a path except on a branching node. That row previously read as though a label were generally meaningful, which is the belief this lint exists to correct.Verification
packages/clilint suite: 50 passed (7 new, covering the flag, the synonyms, and all four exclusion cases).packages/services/service-automation: 427 passed, no regressions.objectstack validateonapp-showcase: 0 findings from the new rule.tsc --noEmitreports nothing inlint-flow-patterns.ts;check-role-wordgate OK.Still open on #3863
The third item — a
faultedge that loops back.registerFlow's cycle check skips onlytype === 'back', andtypeis a single-valued enum, so an edge cannot be bothfaultandback. That makes "fail → clean up → return to the node and retry" unexpressible today. It needs either the cycle check to treat fault edges as declared back-edges (with amaxRevisions-style bound, since an unbounded fault loop is a new footgun) ortypeto become composable. That is a behaviour/schema decision and belongs in its own PR.Generated by Claude Code