fix: pair microflow splits with nearest merge#327
Conversation
AI Code ReviewCritical IssuesNone found. Moderate IssuesNone found. Minor Issues
What Looks Good
RecommendationApprove. The PR correctly fixes the split/merge pairing issue with thoughtful algorithmic improvements, includes relevant regression tests, and maintains code quality. Minor cleanups (unused parameters) are trivial and do not block merging. Note: No MDL syntax changes were introduced, so syntax design and full-stack consistency checks were not applicable. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
AI Code ReviewCritical IssuesNone found. Moderate IssuesNone found. Minor Issues
What Looks Good
RecommendationApprove. The PR correctly fixes issue #326 with appropriate tests and minimal, focused changes. No blockers identified. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
The ctx and activityMap parameters were carried over from a sibling helper but never used by the BFS traversal. Removes them and the matching `_ = ctx; _ = activityMap` discard lines, addressing AI review feedback on PR mendixlabs#327. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
AI Code ReviewWhat Looks GoodThe PR correctly addresses issue #326 by fixing the split/merge pairing logic in the microflow describer:
RecommendationApprove the PR. The fix correctly implements the nearest-common-merge pairing logic while ignoring error-handler flows, includes appropriate test coverage, and maintains consistency with the project's architecture and coding standards. No changes to MDL syntax or full-stack feature wiring were needed since this is a behavioral fix to existing DESCRIBE functionality. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
|
Follow-up pushed in This extends guard-branch detection from direct EndEvent checks to recursive terminality before the split merge boundary. It preserves the guard form when a terminating branch contains actions or nested splits, instead of wrapping the continuation in a synthetic/empty Validation rerun on the branch:
|
AI Code ReviewCritical IssuesNone found. Moderate IssuesNone found. Minor Issues
What Looks Good
RecommendationApprove. The PR correctly fixes the microflow split/merge pairing bug with appropriate test coverage and maintains full consistency with the project's architecture and coding standards. The minor notes above do not block merging. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
ako
left a comment
ako
left a comment
There was a problem hiding this comment.
Review: fix: pair microflow splits with nearest merge
Summary: Correct fix for a real bug. The two root causes (wrong merge pairing and shallow guard detection) are addressed cleanly with the right algorithmic tools. Good test coverage across the key structural cases.
— nearest-merge selection
Replacing the "any common merge" set intersection with BFS shortest-path distances is the right approach. sorts by max-distance first (nearest merge relative to the farthest branch), then sum-distance, then ID for determinism. Map iteration over is non-deterministic but the sort at the end makes the final selection deterministic. ✓
The BFS in uses a distance-comparison guard instead of a simple visited set:
if previous, ok := distances[item.id]; ok && previous <= item.distance {
continue
}With uniform edge weights this guard is never triggered (BFS naturally visits nodes in shortest-path order), so a plain check would be equivalent. No behavior difference — just worth noting for future readers.
Guard detection —
The recursive DFS correctly handles multi-activity terminating branches that the old single-step check missed. The on every recursive branch call avoids cross-branch contamination but allocates O(visited) memory per branch per depth level. For typical microflows (< 100 activities) this is negligible; worth a comment for anyone who hits a pathological deeply-nested case.
with nil
The nested-split case:
nestedMergeID := splitMergeMap[currentID] // nil map read → returns ""Reading from a nil map in Go is safe (returns zero value), so passing for doesn't panic — it results in every nested split being traversed with , which means the termination check continues past any merge. The test relies on this implicitly: it passes for and the test graph happens to work because both inner branches are EndEvents. This is fragile — a test graph with an actual merge after the inner split would silently produce wrong results. A nil guard and a comment would make the contract explicit.
Inconsistent test setup pattern
pre-builds using . passes . The inconsistency is confusing — it looks like an oversight rather than a deliberate choice. The nested guard test should build its own to match production behavior.
Minor: called in but old is still present
is no longer called by but remains in the file. If it has no other callers it should be removed to avoid dead code.
grep -r 'collectReachableNodes' mdl/executor/Bug test script
covers the exact symptom from the issue and documents the before/after clearly. ✓
Verdict
Approve. The fix is correct and addresses both root causes. The test inconsistency and the possible dead code are the two cleanup items worth addressing before or after merge.
AI Code ReviewCritical IssuesNone found. Moderate IssuesNone found. Minor IssuesNone found. What Looks Good
RecommendationApprove the PR. The fix correctly resolves the reported bug with appropriate testing, follows project conventions, and maintains full-stack consistency for the existing DESCRIBE command functionality. No changes are needed. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
The describer no longer references collectReachableNodes; selectNearestCommonMerge uses collectReachableDistances exclusively. Drop the unused helper to keep the file focused. Addresses ako's cleanup note on PR mendixlabs#327. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
AI Code ReviewCritical IssuesNone found. Moderate IssuesNone found. Minor Issues
What Looks Good
RecommendationApprove the PR. The fix is correct, well-tested, and maintains project conventions. The bug test provides executable validation, and the unit tests ensure robustness. No blocking issues remain. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
Symptom: sequential if-without-else structures could be described with the continuation of the first split nested inside that split. Root cause: split/merge pairing selected an arbitrary common reachable merge from map iteration, so a later downstream merge could be chosen before the immediate branch convergence. Fix: rank common merge candidates by nearest branch distance, then total distance and ID, while ignoring non-normal flows for structural pairing. Tests: add unit coverage for nearest-merge selection and for keeping the continuation after a sequential split at top level.
Adds an MDL script under mdl-examples/bug-tests/ exercising sequential if-without-else blocks. The describe → exec → describe fixpoint confirms the first split is paired with its nearest merge, keeping the ifs as siblings rather than nesting the continuation inside the first if's body. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The ctx and activityMap parameters were carried over from a sibling helper but never used by the BFS traversal. Removes them and the matching `_ = ctx; _ = activityMap` discard lines, addressing AI review feedback on PR mendixlabs#327. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Symptom: describer could wrap a fall-through continuation in an empty or synthetic ELSE when the true branch terminated through nested or multi-activity flow instead of a direct EndEvent. Root cause: guard detection only checked whether the true branch destination was an EndEvent. It missed branches that reached a return through actions, nested IFs, enum splits, or inheritance splits. Fix: detect branch terminality recursively before the split merge boundary and use the guard form whenever the true branch terminates while the false branch starts the continuation path. Direct return-vs-return splits still keep the normal IF/ELSE shape. Tests: added traversal regressions for multi-activity terminal guards and nested terminal guards, while keeping existing sequential IF and IF/ELSE traversal coverage green.
Symptom: describe could indent a shared tail under one branch when two split branches converged directly on an activity or downstream split instead of an ExclusiveMerge. Root cause: split pairing only considered ExclusiveMerge objects, so traversal had no stop point for branches that rejoined at a regular object. Fix: choose the nearest common executable join, and continue from that object after emitting the enclosing split when the join is not an ExclusiveMerge. Tests: added a synthetic common-tail traversal regression and ran make test.
Symptom: a branch containing a nested inheritance split could be classified as a terminal guard even when one split case fell through to the parent merge. Describe then closed the parent if without emitting the shared continuation, producing a microflow with a missing return value. Root cause: terminal-branch analysis recursed into nested splits using only the nested split merge. When that nested split had no own merge, the analysis ignored the parent merge and followed the fallthrough branch all the way to a terminal end event. Fix: reuse the parent merge as the nested stop point when a nested split has no merge of its own. This keeps fallthrough branches non-terminal for guard detection. Tests: added a synthetic inheritance-split fallthrough regression for branchFlowTerminatesBeforeMerge.
The describer no longer references collectReachableNodes; selectNearestCommonMerge uses collectReachableDistances exclusively. Drop the unused helper to keep the file focused. Addresses ako's cleanup note on PR mendixlabs#327. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
hjotha
force-pushed
the
submit/describer-nearest-split-merge
branch
from
cd308e5 to
7ccab5d
Compare
AI Code ReviewWhat Looks Good
RecommendationApprove the PR. The changes correctly fix the split/merge pairing bug with appropriate test coverage, follow the project's architectural boundaries, and maintain scope discipline. The solution properly addresses the root cause (finding nearest merge while ignoring error-handler flows) and includes verification that the describe→exec→describe cycle becomes a fixpoint. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
|
Review follow-up status on the current head:
Local validation after the update was green: make build
make test
make lint-go |
3 participants
Part of #332.
Summary
Closes #326
Validation
make buildmake lint-gomake test