fix: preserve no-merge branch continuations#355
fix: preserve no-merge branch continuations#355hjotha wants to merge 6 commits intomendixlabs:mainfrom
Conversation
AI Code ReviewCritical IssuesNone found. Moderate IssuesNone found. Minor IssuesNone found. What Looks Good
RecommendationApprove the PR. The fix resolves the reported issue where parent flow incorrectly continued from the original split instead of the branch tail in no-merge if/else statements. The solution is well-tested and follows existing code patterns. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
AI Code ReviewWhat Looks Good
RecommendationApprove the PR. The changes are minimal, well-tested, and directly address the reported bug without introducing syntax changes or violating architectural boundaries. The fix ensures correct flow graph construction for no-merge Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
hjotha
force-pushed
the
submit/builder-preserve-no-merge-branch-continuation
branch
from
6bb1846 to
353dde1
Compare
hjotha
force-pushed
the
submit/builder-preserve-no-merge-branch-continuation
branch
from
353dde1 to
b29a5a2
Compare
AI Code ReviewCritical Issues
Moderate Issues
Minor Issues
What Looks Good
Recommendation Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
Symptom: when an if/else did not need an ExclusiveMerge because one branch was terminal, the builder could expose the original split as the continuation point instead of the tail of the non-terminal branch. Root cause: addIfStatement only carried a deferred split case to the parent flow. It did not remember the actual tail activity, pending branch case, or anchor from the branch that continued without a merge. Fix: track the no-merge continuation endpoint while building each branch, including pending case and anchor state from nested compound statements, and hand that endpoint to the parent flow instead of the stale split. Tests: add a synthetic builder regression where the else branch continues after the then branch returns, proving the following statement is wired from the branch tail rather than from the split.
Adds an MDL script under mdl-examples/bug-tests/ exercising an IF with one terminal branch and one continuing branch (no merge). After exec, `mx check` reports 0 errors, confirming the parent flow attaches to the continuing branch tail rather than the split. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Symptom: a nested IF whose continuing branch skips a local merge could lose its @anchor metadata when the parent IF connected that tail to its merge or next statement. In affected models this changed branch layout and could make Studio Pro report variables as out of scope after round-trip. Root cause: addIfStatement only consumed pending branch metadata when nextFlowCase was non-empty. No-merge continuations can carry only nextFlowAnchor, so those anchors were ignored. Fix: treat pending anchors as pending flow metadata even without a case value, and route their origin/destination sides through shared helper functions. Tests: added a synthetic nested no-merge IF regression that asserts the branch tail preserves a bottom-to-top anchor when it connects to the parent merge, and ran make build plus make test.
Symptom: describe could print the false-path continuation of an if-without-else inside an else block when the true branch terminated through multiple statements. Root cause: guard detection only recognized a true branch that jumped directly to an EndEvent. Multi-statement terminal true branches fell back to normal IF/ELSE rendering. Fix: detect terminal true branches recursively and use layout to identify the false flow as the horizontal continuation rather than an explicit else branch. Tests: add a traversal regression with a multi-statement terminal true branch and a false-path tail that must remain after end if.
hjotha
force-pushed
the
submit/builder-preserve-no-merge-branch-continuation
branch
from
96b05c6 to
c49894a
Compare
The no-merge branch test helper used the generic name objectByID. When combined with the enum-split tests, the executor package had two helpers with the same name and failed to compile. Rename the helper to noMergeObjectByID so both PRs can be merged together without changing test behavior. Tests: - go test ./mdl/executor -run 'TestBuildFlowGraph_NoMerge|TestBuildFlowGraph_NestedNoMerge' -count=1 - make build - make test
ako
left a comment
ako
left a comment
There was a problem hiding this comment.
Review — fix: preserve no-merge branch continuations
Overview
Correct fix for a real wiring bug. The core change — tracking noMergeExitID from the non-terminal branch's tail instead of always wiring the parent to the split — is sound, and the TestBuildFlowGraph_NoMergeIfElseContinuesFromBranchTail test directly demonstrates the before/after behaviour. The bug test MDL is well-documented.
Moderate: branchDestinationAnchor reversal has no test and no explanatory comment
- if stmtAnchor != nil && stmtAnchor.To != ast.AnchorSideUnset {
- return stmtAnchor
+ if branchAnchor != nil && branchAnchor.To != ast.AnchorSideUnset {
+ return branchAnchor
}
- return branchAnchor
+ return stmtAnchorThis swaps which anchor takes precedence for the first activity in a branch — a 4-line change that quietly reverses existing behaviour with no comment explaining why the previous order was wrong, and no regression test covering it.
The new order (branch anchor's To beats statement anchor's To) is plausible — the branch-level annotation from the split should govern how the flow arrives at the first branch statement, not the statement's own anchor — but without a test or comment this is invisible to reviewers and future maintainers. The TestBuildFlowGraph_NestedNoMergeTailCarriesAnchorToParentMerge test touches anchor propagation but doesn't exercise this specific precedence swap.
Moderate: flowLooksLikeGuardContinuation uses Y-position equality as a heuristic
return dest.GetPosition().Y == split.GetPosition().YThis works for builder-generated layouts where the guard's false path is always placed horizontally at split-Y. For MPRs with manually repositioned activities (Studio Pro drag-and-drop), or for builder layouts that drift due to future spacing changes, the equality check could misidentify or silently miss the guard pattern.
The previous !branchFlowStartsAtTerminal check was wrong because it triggered when the false branch led to a real merge, but the Y-position substitute is equally brittle in the other direction. Worth a comment documenting the layout contract this relies on, and ideally a test with an MPR fixture that verifies the heuristic doesn't regress when the guard is at a non-standard Y.
Minor: else { fb.nextConnectionPoint = splitID } drops nextFlowCase/nextFlowAnchor
The old code always set nextFlowCase and nextFlowAnchor in the deferred path. The new code only sets them when noMergeExitID != "":
if noMergeExitID != "" {
fb.nextConnectionPoint = noMergeExitID
fb.nextFlowCase = noMergeExitCase
fb.nextFlowAnchor = noMergeExitAnchor
} else {
fb.nextConnectionPoint = splitID
// nextFlowCase and nextFlowAnchor NOT set here
}Analysis shows noMergeExitID is always populated when this path is reached (both IF-with-ELSE and IF-without-ELSE set it before this block), so the else branch is a dead code path in practice. But if it were ever reached, the missing nextFlowCase/nextFlowAnchor would silently produce an uncased flow from the split — the same bug this PR is fixing. Worth a // unreachable: noMergeExitID is always set above comment, or a panic/return to make it fail loudly.
Minor: pending case/anchor pattern duplicated three times
pendingThenCase / pendingThenAnchor / pendingElseCase / pendingElseAnchor appears in three separate loops (IF+ELSE then-body, IF+ELSE else-body, IF-without-ELSE then-body), each with the same 10-line check block. Not a blocker, but the repetition will make future anchor-handling changes error-prone.
Positive
noMergeExitIDcorrectly handles all four combinations: then-returns/else-continues, else-returns/then-continues, empty then-body, empty else-body.- Builder test is precise — it asserts the specific
elseID → afterIDflow exists and thatafterIDhas no other origin. !needMergeguard structure is clean: only enters the no-merge branch when the invariant holds.
Symptom: review noted that no-merge branch continuation routing relied on anchor precedence and guard-layout heuristics that were not visible in code or tests. Root cause: the implementation preserved the right flow metadata but left the split-to-branch destination precedence and guard continuation layout contract implicit. Fix: add comments for branch destination precedence, guard continuation Y matching, and the defensive no-merge fallback; add a regression test pinning that branch-level `to` anchors win on the split-to-branch flow. Tests: run the focused no-merge/anchor tests, then make build and make test.
3 participants
Closes part of #351.
Part of #332.
Summary
This extracts one focused split/join builder fix from the audit staging branch.
When an
if/elsehas one terminal branch and one continuing branch, the builder does not need to insert anExclusiveMerge. In that no-merge shape, the parent flow must continue from the tail of the non-terminal branch. Previously it could continue from the original split instead, leaving the branch body disconnected or causing the following statement to be wired to the wrong origin.Changes
ifbranches.Validation
make buildmake lint-gomake test