From 2a217a243144cad7baf85a03e2fd5199f41571f4 Mon Sep 17 00:00:00 2001 From: Ako Date: Wed, 21 Oct 2026 00:51:50 +0000 Subject: [PATCH 01/12] fix: a note's line leaves the side that faces its element Every AnnotationFlow was written 2 -> 0 (note bottom -> element top), so a note below its activity drew its line out of its own bottom edge and round both boxes. The builder now picks the facing sides once every object has its final position, the writer writes them, and the reader carries stored sides so a Studio Pro-drawn line survives a rewrite. The rule is Studio Pro's, measured on 523 distinct Studio Pro-drawn note lines: the element side follows vertical-then-horizontal separation in 98%, and notes use the activity numbering (a note left of its activity stores 1 -> 3), not the separate numbering the issue proposed. Refs ako/mxcli#1508 (case 2), mendixlabs/mxcli#1598 Co-Authored-By: Claude Opus 5.5 --- ...1-note-line-always-leaves-note-bottom.json | 1 + .../flow-geometry-1508-note-line-sides.mdl | 37 ++++++++ mdl/backend/modelsdk/microflow.go | 6 ++ .../modelsdk/microflow_annotation_test.go | 25 ++++++ mdl/backend/modelsdk/microflow_write.go | 16 ++-- .../cmd_microflows_annotation_sides_test.go | 90 +++++++++++++++++++ .../cmd_microflows_builder_annotations.go | 65 ++++++++++++++ mdl/executor/cmd_microflows_builder_graph.go | 4 + sdk/microflows/microflows.go | 12 +++ 9 files changed, 251 insertions(+), 5 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-10-21-note-line-always-leaves-note-bottom.json create mode 100644 mdl-examples/bug-tests/flow-geometry-1508-note-line-sides.mdl create mode 100644 mdl/executor/cmd_microflows_annotation_sides_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-21-note-line-always-leaves-note-bottom.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-21-note-line-always-leaves-note-bottom.json new file mode 100644 index 0000000000..5738963ba8 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-21-note-line-always-leaves-note-bottom.json @@ -0,0 +1 @@ +{"area":"mdl/executor","date":"2026-10-21","symptom":"Every Microflows$AnnotationFlow was written OriginConnectionIndex 2 -> DestinationConnectionIndex 0 (note bottom -> element top) wherever the note sat, so a note placed BELOW its activity drew its line out of its own bottom edge and round both boxes. The reader dropped the stored sides too, so a rewrite of a Studio Pro-drawn note below its activity flipped it to 2 -> 0.","cause":"annotationFlowToGen hardcoded SetOriginConnectionIndex(2)/SetDestinationConnectionIndex(0), and microflows.AnnotationFlow had no field for the sides, so neither the builder nor the reader could say otherwise","file":"mdl/executor/cmd_microflows_builder_annotations.go (orientAnnotationFlows, facingSides), mdl/backend/modelsdk/microflow_write.go (annotationFlowToGen), mdl/backend/modelsdk/microflow.go (annotationFlowsFromGen), sdk/microflows/microflows.go (AnnotationFlow.Sides)","insight":"The issue proposed a separate note numbering (top 0, bottom 1, left 2, right 3); measuring Studio Pro's own documents refuted it in one pass: 891 AnnotationFlows from ako/TestApp, PedApp and the mx-test-projects .mpk apps (523 distinct). A note level with its activity on the left stores 1 -> 3 and on the right 3 -> 1, which only the activity numbering (0 top, 1 right, 2 bottom, 3 left) explains; a note below stores 0 -> 2 (58 of 67). The element side follows 'vertically clear -> top/bottom, else left/right' in 98%; the NOTE side is not predictable from geometry (Studio Pro often stores 1 -> 0 for a note above, presumably from where the author dragged), so do not chase the origin past 'the facing side'. Sides are chosen after separateRows, because notes are placed before rows and containers settle, and loop-body positions are relative to the loop's top-left, so the comparison is done in absolute coordinates.","refs":["ako/mxcli#1508","mendixlabs/mxcli#1598"]} diff --git a/mdl-examples/bug-tests/flow-geometry-1508-note-line-sides.mdl b/mdl-examples/bug-tests/flow-geometry-1508-note-line-sides.mdl new file mode 100644 index 0000000000..f7490af0a3 --- /dev/null +++ b/mdl-examples/bug-tests/flow-geometry-1508-note-line-sides.mdl @@ -0,0 +1,37 @@ +mdl 1; +-- ============================================================================ +-- Bug: a note's line always left the note's bottom edge (ako/mxcli#1508, case 2; +-- mendixlabs/mxcli#1598) +-- ============================================================================ +-- +-- Every Microflows$AnnotationFlow was written OriginConnectionIndex 2 -> +-- DestinationConnectionIndex 0 (note bottom -> element top), wherever the note +-- sat. A note BELOW its activity then drew its line out of its own bottom edge +-- and round the outside of both boxes. +-- +-- Fixed: the line leaves the side of the note that faces its element and enters +-- the element's facing side. Studio Pro numbers a note's sides like an +-- activity's (0 top, 1 right, 2 bottom, 3 left), measured on 523 distinct +-- Studio Pro-drawn note lines. +-- +-- Verify: +-- mxcli exec mdl-examples/bug-tests/flow-geometry-1508-note-line-sides.mdl -p app.mpr +-- mxcli bson dump -p app.mpr --type microflow --object MyFirstModule.ACT_NoteSides \ +-- | grep -B2 -A2 AnnotationFlow +-- Expected: 'Note above' 2 -> 0, 'Note below' 0 -> 2, 'Note left' 1 -> 3. +-- ============================================================================ + +create or modify microflow MyFirstModule.ACT_NoteSides () returns Boolean as $r +begin + @position(400, 200) + @annotation(text: 'Note above', position: (400, 60), size: (200, 60)) + log info node 'A' 'hello'; + @position(800, 200) + @annotation(text: 'Note below', position: (800, 400), size: (200, 60)) + log info node 'B' 'world'; + @position(1300, 200) + @annotation(text: 'Note left', position: (1050, 200), size: (180, 60)) + log info node 'C' 'again'; + @position(1500, 200) + return true; +end; diff --git a/mdl/backend/modelsdk/microflow.go b/mdl/backend/modelsdk/microflow.go index 7bb63efa17..cc9784410e 100644 --- a/mdl/backend/modelsdk/microflow.go +++ b/mdl/backend/modelsdk/microflow.go @@ -358,9 +358,15 @@ func annotationFlowsFromGen(items []element.Element) []*microflows.AnnotationFlo if !ok { continue } + // The sides are carried, so a rewrite of a Studio Pro-drawn note keeps + // the line where its author put it. f := µflows.AnnotationFlow{ OriginID: model.ID(g.OriginRefID()), DestinationID: model.ID(g.DestinationRefID()), + Sides: µflows.ConnectionSides{ + Origin: int(g.OriginConnectionIndex()), + Destination: int(g.DestinationConnectionIndex()), + }, } f.ID = model.ID(g.ID()) flows = append(flows, f) diff --git a/mdl/backend/modelsdk/microflow_annotation_test.go b/mdl/backend/modelsdk/microflow_annotation_test.go index 297041f587..1941bd4691 100644 --- a/mdl/backend/modelsdk/microflow_annotation_test.go +++ b/mdl/backend/modelsdk/microflow_annotation_test.go @@ -66,3 +66,28 @@ func TestAnnotationFlowToGen(t *testing.T) { t.Errorf("Line (BezierCurve) missing on major>=10: %v", err) } } + +// TestAnnotationFlowToGen_WritesTheChosenSides guards that the sides the builder +// picked reach the document, and that a flow nobody oriented keeps the +// note-above drawing (2 -> 0) every earlier write produced +// (mendixlabs/mxcli#1598, case 2). +func TestAnnotationFlowToGen_WritesTheChosenSides(t *testing.T) { + sides := func(af *microflows.AnnotationFlow) (int32, int32) { + t.Helper() + raw, err := (&codec.Encoder{}).Encode(annotationFlowToGen(af, newestFlowVersion)) + if err != nil { + t.Fatalf("encode: %v", err) + } + r := bson.Raw(raw) + return r.Lookup("OriginConnectionIndex").Int32(), r.Lookup("DestinationConnectionIndex").Int32() + } + below := µflows.AnnotationFlow{OriginID: "ann-1", DestinationID: "act-1", + Sides: µflows.ConnectionSides{Origin: 0, Destination: 2}} + if o, d := sides(below); o != 0 || d != 2 { + t.Errorf("note below its activity: written %d -> %d, want 0 -> 2", o, d) + } + unoriented := µflows.AnnotationFlow{OriginID: "ann-1", DestinationID: "act-1"} + if o, d := sides(unoriented); o != 2 || d != 0 { + t.Errorf("unoriented flow: written %d -> %d, want the note-above default 2 -> 0", o, d) + } +} diff --git a/mdl/backend/modelsdk/microflow_write.go b/mdl/backend/modelsdk/microflow_write.go index cda3698a76..41c1a81bdf 100644 --- a/mdl/backend/modelsdk/microflow_write.go +++ b/mdl/backend/modelsdk/microflow_write.go @@ -494,11 +494,17 @@ func annotationFlowToGen(af *microflows.AnnotationFlow, v flowVersion) element.E g.SetID(element.ID(af.ID)) g.SetOriginID(element.ID(af.OriginID)) g.SetDestinationID(element.ID(af.DestinationID)) - // A note sits ABOVE the element it documents, so its line leaves the note's - // bottom edge and enters the element's top. Both indexes were 0 (top), which - // drew the line out of the top of the note and back down around it. - g.SetOriginConnectionIndex(2) - g.SetDestinationConnectionIndex(0) + // The builder picks the sides that face each other (orientAnnotationFlows); + // a flow nothing oriented is drawn as for a note ABOVE its element — out of + // the note's bottom edge, into the element's top. Both indexes were once 0 + // (top), which drew the line out of the top of the note and back down + // around it. + origin, destination := int32(2), int32(0) + if af.Sides != nil { + origin, destination = int32(af.Sides.Origin), int32(af.Sides.Destination) + } + g.SetOriginConnectionIndex(origin) + g.SetDestinationConnectionIndex(destination) // An annotation flow is a Flow like a sequence flow, with the same floors: // the Line was written from 10.0 on, six minors before it existed. if v.hasLine() { diff --git a/mdl/executor/cmd_microflows_annotation_sides_test.go b/mdl/executor/cmd_microflows_annotation_sides_test.go new file mode 100644 index 0000000000..d202941281 --- /dev/null +++ b/mdl/executor/cmd_microflows_annotation_sides_test.go @@ -0,0 +1,90 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// A note's line leaves the side of the note that faces its element and enters +// the side of the element that faces the note (mendixlabs/mxcli#1598, case 2). +// The writer stored 2 -> 0 (note bottom -> element top) for every note, so a +// note placed BELOW its activity drew its line out of its own bottom edge and +// round the outside of both boxes. +// +// The expected sides are Studio Pro's, measured rather than assumed: of 523 +// distinct Studio Pro-authored AnnotationFlows (ako/TestApp, PedApp and nine +// customer apps), the element's side follows "vertically separated -> top or +// bottom, else left or right" in 513 (98%), and the numbering is the activity +// one (0 top, 1 right, 2 bottom, 3 left): a note level with its activity on the +// left stores 1 -> 3, on the right 3 -> 1. The issue's guess that a note is +// numbered "top 0, bottom 1, left 2, right 3" does not survive those two. +func TestAnnotationFlowLeavesTheFacingSide(t *testing.T) { + cases := []struct { + name string + note string + origin, dst int + }{ + {"above", "(400, 80)", AnchorBottom, AnchorTop}, + {"below", "(400, 360)", AnchorTop, AnchorBottom}, + {"left", "(120, 200)", AnchorRight, AnchorLeft}, + {"right", "(680, 200)", AnchorLeft, AnchorRight}, + // Diagonal: vertically clear of the activity, so the vertical sides win, + // as they do in Studio Pro's own drawings of a note above and to one side. + {"above-left", "(150, 60)", AnchorBottom, AnchorTop}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + fb := buildFlowFromMDL(t, ` + @position(400, 200) + @annotation(text: 'note', position: `+tc.note+`, size: (200, 60)) + log info node 'A' 'a';`) + if len(fb.annotationFlows) != 1 { + t.Fatalf("got %d annotation flows, want 1", len(fb.annotationFlows)) + } + got := fb.annotationFlows[0].Sides + if got == nil { + t.Fatalf("note %s: no sides chosen; the writer falls back to %d -> %d", + tc.note, AnchorBottom, AnchorTop) + } + if got.Origin != tc.origin || got.Destination != tc.dst { + t.Errorf("note %s of an activity at (400, 200): line %d -> %d, want %d -> %d", + tc.note, got.Origin, got.Destination, tc.origin, tc.dst) + } + }) + } +} + +// A note on an activity INSIDE a loop is wired from the parent collection, and +// the two positions are in different coordinate planes: the body's are +// relative to the loop box. Compared raw, a note above the loop would read as +// below an activity whose body-relative y is small. +func TestAnnotationFlowSidesAcrossALoopBoundary(t *testing.T) { + note := µflows.Annotation{BaseMicroflowObject: microflows.BaseMicroflowObject{ + BaseElement: model.BaseElement{ID: "note"}, + Position: model.Point{X: 500, Y: 400}, + Size: model.Size{Width: 200, Height: 60}, + }} + inner := µflows.ActionActivity{BaseActivity: microflows.BaseActivity{BaseMicroflowObject: microflows.BaseMicroflowObject{ + BaseElement: model.BaseElement{ID: "inner"}, + Position: model.Point{X: 200, Y: 80}, // relative to the loop's top-left + Size: model.Size{Width: ActivityWidth, Height: ActivityHeight}, + }}} + loop := µflows.LoopedActivity{ + BaseMicroflowObject: microflows.BaseMicroflowObject{ + BaseElement: model.BaseElement{ID: "loop"}, + Position: model.Point{X: 500, Y: 200}, + Size: model.Size{Width: 400, Height: 160}, // top-left (300, 120) + }, + ObjectCollection: µflows.MicroflowObjectCollection{Objects: []microflows.MicroflowObject{inner}}, + } + af := µflows.AnnotationFlow{OriginID: "note", DestinationID: "inner"} + orientAnnotationFlows([]microflows.MicroflowObject{note, loop}, []*microflows.AnnotationFlow{af}) + // The inner activity is at absolute (500, 200); the note, at y=400, is below it. + if af.Sides == nil || af.Sides.Origin != AnchorTop || af.Sides.Destination != AnchorBottom { + t.Fatalf("sides %+v, want %d -> %d (note below the loop-body activity)", af.Sides, AnchorTop, AnchorBottom) + } +} diff --git a/mdl/executor/cmd_microflows_builder_annotations.go b/mdl/executor/cmd_microflows_builder_annotations.go index 045c81d09c..9ba9813d5f 100644 --- a/mdl/executor/cmd_microflows_builder_annotations.go +++ b/mdl/executor/cmd_microflows_builder_annotations.go @@ -390,6 +390,71 @@ func (fb *flowBuilder) newAnnotation(note ast.MicroflowAnnotation, pos model.Poi return annotation } +// orientAnnotationFlows picks, for every note line that has no sides yet, the +// side of the note that faces its element and the side of the element that +// faces the note (mendixlabs/mxcli#1598, case 2). It runs once every object has +// its final position, since a note is placed before rows and containers settle. +// +// Studio Pro's own drawings are the rule: of 523 distinct note lines in +// Studio Pro-authored apps, the element's side is top or bottom whenever the +// two boxes are vertically clear of each other, and left or right only when +// they overlap vertically — 98% agreement. Writing 2 -> 0 for every note drew +// a note below its activity with a line that leaves its own bottom edge. +func orientAnnotationFlows(objects []microflows.MicroflowObject, flows []*microflows.AnnotationFlow) { + boxes := map[model.ID]canvasBox{} + collectCanvasBoxes(objects, model.Point{}, boxes) + for _, af := range flows { + if af.Sides != nil { + continue + } + note, ok1 := boxes[af.OriginID] + target, ok2 := boxes[af.DestinationID] + if !ok1 || !ok2 { + continue + } + af.Sides = facingSides(note, target) + } +} + +// canvasBox is an object's box in absolute canvas coordinates. +type canvasBox struct{ left, top, right, bottom int } + +// collectCanvasBoxes records the absolute box of every object, descending into +// loop bodies. A loop's children are positioned relative to the loop box's +// top-left corner, and a note outside a loop may be wired to an activity +// inside it, so comparing raw positions would mix two planes. +func collectCanvasBoxes(objects []microflows.MicroflowObject, origin model.Point, out map[model.ID]canvasBox) { + for _, o := range objects { + if o == nil { + continue + } + p, sz := o.GetPosition(), objectSize(o) + x, y := origin.X+p.X, origin.Y+p.Y + out[o.GetID()] = canvasBox{x - sz.Width/2, y - sz.Height/2, x + sz.Width/2, y + sz.Height/2} + if loop, ok := o.(*microflows.LoopedActivity); ok && loop.ObjectCollection != nil { + collectCanvasBoxes(loop.ObjectCollection.Objects, + model.Point{X: x - sz.Width/2, Y: y - sz.Height/2}, out) + } + } +} + +// facingSides returns the sides at which a line from note to target attaches. +// Vertical separation wins over horizontal; boxes that overlap fall back to the +// note-above drawing the writer has always used. +func facingSides(note, target canvasBox) *microflows.ConnectionSides { + switch { + case note.bottom < target.top: + return µflows.ConnectionSides{Origin: AnchorBottom, Destination: AnchorTop} + case note.top > target.bottom: + return µflows.ConnectionSides{Origin: AnchorTop, Destination: AnchorBottom} + case note.right < target.left: + return µflows.ConnectionSides{Origin: AnchorRight, Destination: AnchorLeft} + case note.left > target.right: + return µflows.ConnectionSides{Origin: AnchorLeft, Destination: AnchorRight} + } + return µflows.ConnectionSides{Origin: AnchorBottom, Destination: AnchorTop} +} + func (fb *flowBuilder) linkAnnotation(annotationID, activityID model.ID) { fb.annotationFlows = append(fb.annotationFlows, µflows.AnnotationFlow{ BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, diff --git a/mdl/executor/cmd_microflows_builder_graph.go b/mdl/executor/cmd_microflows_builder_graph.go index 90dbaf9bcc..e0a8d181e5 100644 --- a/mdl/executor/cmd_microflows_builder_graph.go +++ b/mdl/executor/cmd_microflows_builder_graph.go @@ -266,6 +266,10 @@ func (fb *flowBuilder) buildFlowGraph(stmts []ast.MicroflowStatement, returns *a // this is the first point where a row's real depth is known. fb.separateRows() + // Note lines after everything has moved: the sides that face each other + // depend on where the note and its element finally sit. + orientAnnotationFlows(fb.objects, fb.annotationFlows) + return µflows.MicroflowObjectCollection{ BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, Objects: fb.objects, diff --git a/sdk/microflows/microflows.go b/sdk/microflows/microflows.go index c10f451a3d..90b122f75a 100644 --- a/sdk/microflows/microflows.go +++ b/sdk/microflows/microflows.go @@ -364,6 +364,18 @@ type AnnotationFlow struct { model.BaseElement OriginID model.ID `json:"originId"` DestinationID model.ID `json:"destinationId"` + // Sides is where the line attaches at each end: the note's side and the + // object's. nil means nothing chose them, and the writer draws the line as + // for a note sitting above its object. + Sides *ConnectionSides `json:"sides,omitempty"` +} + +// ConnectionSides is the pair of connection indexes a flow attaches at, in the +// numbering a SequenceFlow uses for an activity — 0 top, 1 right, 2 bottom, +// 3 left. Studio Pro numbers a note's sides the same way. +type ConnectionSides struct { + Origin int `json:"origin"` + Destination int `json:"destination"` } // Events From 1e401a3357b2e1e56d9f54232add3426ec5df71c Mon Sep 17 00:00:00 2001 From: Ako Date: Wed, 21 Oct 2026 00:57:19 +0000 Subject: [PATCH 02/12] fix: the merge after a loop in a branch lands right of the loop box measureLoopStatement left out the 100px iterator column addLoopStatement reserves on a foreach loop's left, so an IF holding a loop measured its branch 100px short and put its merge inside the loop box (loop 790..1110, merge 1050..1090), with the loop's outgoing line running right to left. Both now use one LoopIteratorSpace constant. Refs ako/mxcli#1508 (case 4), mendixlabs/mxcli#1598 Co-Authored-By: Claude Opus 5.5 --- ...-after-loop-in-branch-inside-loop-box.json | 1 + ...ometry-1508-merge-after-loop-in-branch.mdl | 30 ++++++++++ .../cmd_microflows_builder_control.go | 12 ++-- mdl/executor/layout.go | 11 +++- mdl/executor/layout_loop_in_branch_test.go | 60 +++++++++++++++++++ 5 files changed, 106 insertions(+), 8 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-10-21-merge-after-loop-in-branch-inside-loop-box.json create mode 100644 mdl-examples/bug-tests/flow-geometry-1508-merge-after-loop-in-branch.mdl create mode 100644 mdl/executor/layout_loop_in_branch_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-21-merge-after-loop-in-branch-inside-loop-box.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-21-merge-after-loop-in-branch-inside-loop-box.json new file mode 100644 index 0000000000..b7c4b5aae6 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-21-merge-after-loop-in-branch-inside-loop-box.json @@ -0,0 +1 @@ +{"area":"mdl/executor","date":"2026-10-21","symptom":"The merge after an IF whose branch holds a foreach LOOP landed inside the loop's box: LoopedActivity 950;200 320x160 spans x 790..1110, ExclusiveMerge 1070;200 spans 1050..1090, so the loop's outgoing line ran right to left into it. mx check: 0 errors.","cause":"measureLoopStatement sized a loop as body + 2*LoopPadding while addLoopStatement builds body + 2*LoopPadding + iteratorSpace (a local 100); the IF's mergeX comes from the measured branch width, so it fell 100px short for every foreach loop at every nesting level. WHILE has no iterator column and no mismatch.","file":"mdl/executor/layout.go (LoopIteratorSpace, measureLoopStatement), mdl/executor/cmd_microflows_builder_control.go (addLoopStatement)","insight":"The issue's cause was right in effect but not in mechanism: the builder's `+iteratorSpace` width is overwritten by fitContainerSize after the body is built, so the column reaches the final width through innerStartX instead. The measurer still has to include it, because the IF reads the measured width before the loop exists. A centre-to-centre layout invariant (merge.X >= loop.X) passed throughout, so the test compares box EDGES (merge.X - MergeSize/2 >= loop.X + Width/2); TestLayoutInvariants_NoBackwardMergeAndEndRightmost treats a LoopedActivity as ActivityWidth wide and could not see it.","refs":["ako/mxcli#1508","mendixlabs/mxcli#1598"]} diff --git a/mdl-examples/bug-tests/flow-geometry-1508-merge-after-loop-in-branch.mdl b/mdl-examples/bug-tests/flow-geometry-1508-merge-after-loop-in-branch.mdl new file mode 100644 index 0000000000..dd5d9e9edf --- /dev/null +++ b/mdl-examples/bug-tests/flow-geometry-1508-merge-after-loop-in-branch.mdl @@ -0,0 +1,30 @@ +mdl 1; +-- ============================================================================ +-- Bug: the merge after an IF whose branch holds a LOOP landed inside the loop +-- box (ako/mxcli#1508, case 4; mendixlabs/mxcli#1598) +-- ============================================================================ +-- +-- Stored before the fix: LoopedActivity 950;200 320x160 (x 790..1110) and the +-- ExclusiveMerge at 1070;200 (x 1050..1090), so the loop's outgoing line ran +-- right to left. The layout measurer sized a foreach loop without the 100px +-- iterator column the builder reserves on its left. +-- +-- Verify: exec, then +-- mxcli bson dump -p app.mpr --type microflow --object MyFirstModule.ACT_MergeAfterLoop +-- Expected: the merge's left edge (x - 20) is at or right of the loop's right +-- edge (x + width/2). +-- ============================================================================ + +create persistent entity MyFirstModule.LoopItem (Code: string(50), Amount: integer); + +create or modify microflow MyFirstModule.ACT_MergeAfterLoop ($Flag: boolean) returns boolean as $r +begin + declare $r boolean = false; + retrieve $L from MyFirstModule.LoopItem; + if $Flag then + loop $i in $L begin set $r = true; end loop; + else + set $r = false; + end if; + return $r; +end; diff --git a/mdl/executor/cmd_microflows_builder_control.go b/mdl/executor/cmd_microflows_builder_control.go index 616b932216..c5a2bb1092 100644 --- a/mdl/executor/cmd_microflows_builder_control.go +++ b/mdl/executor/cmd_microflows_builder_control.go @@ -606,10 +606,10 @@ func (fb *flowBuilder) addLoopStatement(s *ast.LoopStmt) model.ID { // First, measure the loop body to determine size bodyBounds := fb.measurer.measureStatementsSpan(s.Body) - // Calculate loop box size with padding - // Extra width for iterator icon and its label (100 pixels) - iteratorSpace := 100 - loopWidth := max(bodyBounds.Width+2*LoopPadding+iteratorSpace, MinLoopWidth) + // Calculate loop box size with padding, plus the iterator column. The + // measurer uses the same constant, so an IF holding this loop measures it at + // the width it is built at. + loopWidth := max(bodyBounds.Width+2*LoopPadding+LoopIteratorSpace, MinLoopWidth) loopHeight := max(bodyBounds.Height+2*LoopPadding, MinLoopHeight) // Inner positioning: activities start after the iterator icon on the left, @@ -617,10 +617,10 @@ func (fb *flowBuilder) addLoopStatement(s *ast.LoopStmt) model.ID { // (IF/CASE) centred on innerStartY stays within [0, loopHeight]. // // A position is a CENTRE, so the first activity has to start half its own - // width past the iterator column; at LoopPadding+iteratorSpace its left edge + // width past the iterator column; at LoopPadding+LoopIteratorSpace its left edge // landed 60px inside that column, on top of the iterator's icon and the list // variable's label. - innerStartX := LoopPadding + iteratorSpace + ActivityWidth/2 + innerStartX := LoopPadding + LoopIteratorSpace + ActivityWidth/2 innerStartY := loopHeight / 2 // posX is where the builder would CENTRE the next element. A loop box placed diff --git a/mdl/executor/layout.go b/mdl/executor/layout.go index 1ec4907c74..ebf91c49c8 100644 --- a/mdl/executor/layout.go +++ b/mdl/executor/layout.go @@ -34,6 +34,10 @@ const ( VerticalSpacing = 90 // Space between branches for error-handler flows BranchGap = 40 // Minimum edge-to-edge gap between parallel branches LoopPadding = 50 // Padding inside loop boxes + // LoopIteratorSpace is the column a foreach loop keeps on its left for the + // iterator icon and the list variable's label. A WHILE has no iterator and no + // column. + LoopIteratorSpace = 100 MinLoopWidth = 200 MinLoopHeight = 100 ) @@ -316,8 +320,11 @@ func (m *layoutMeasurer) measureLoopStatement(s *ast.LoopStmt) Bounds { // Measure loop body bodyBounds := m.measureStatements(s.Body) - // Loop box size: body + padding on all sides - width := max(bodyBounds.Width+2*LoopPadding, MinLoopWidth) + // Loop box size: body + padding on all sides, plus the iterator column the + // builder reserves on the left. Leaving the column out measured every foreach + // loop 100px short, so the merge of an IF holding one landed inside the + // loop's box (mendixlabs/mxcli#1598, case 4). + width := max(bodyBounds.Width+2*LoopPadding+LoopIteratorSpace, MinLoopWidth) height := max(bodyBounds.Height+2*LoopPadding, MinLoopHeight) return Bounds{Width: width, Height: height} diff --git a/mdl/executor/layout_loop_in_branch_test.go b/mdl/executor/layout_loop_in_branch_test.go new file mode 100644 index 0000000000..8b72e9be5d --- /dev/null +++ b/mdl/executor/layout_loop_in_branch_test.go @@ -0,0 +1,60 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// The merge after an IF whose branch holds a LOOP must sit right of the loop +// box (mendixlabs/mxcli#1598, case 4). measureLoopStatement left out the +// iterator column the builder reserves on the box's left, so the branch measured +// 100px short and the merge landed inside the box: loop 790..1110, merge +// 1050..1090, and the loop's outgoing line ran right to left into it. +func TestMergeAfterALoopInABranchClearsTheLoopBox(t *testing.T) { + for _, tc := range []struct{ name, body string }{ + {"loop in then", ` + declare $r boolean = false; + retrieve $L from MyFirstModule.Item; + if $Flag then + loop $i in $L begin set $r = true; end loop; + else + set $r = false; + end if; + return $r;`}, + {"two activities in the loop", ` + declare $r boolean = false; + retrieve $L from MyFirstModule.Item; + if $Flag then + loop $i in $L begin set $r = true; log info node 'x' 'y'; end loop; + else + set $r = false; + end if; + return $r;`}, + } { + t.Run(tc.name, func(t *testing.T) { + fb := buildFlowFromMDL(t, tc.body) + var loop *microflows.LoopedActivity + var merge *microflows.ExclusiveMerge + for _, o := range fb.objects { + switch v := o.(type) { + case *microflows.LoopedActivity: + loop = v + case *microflows.ExclusiveMerge: + merge = v + } + } + if loop == nil || merge == nil { + t.Fatalf("want a loop and a merge, got loop=%v merge=%v", loop != nil, merge != nil) + } + loopRight := loop.Position.X + loop.Size.Width/2 + mergeLeft := merge.Position.X - MergeSize/2 + if mergeLeft < loopRight { + t.Errorf("merge spans from x=%d, inside the loop box that ends at x=%d (loop centre %d, width %d)", + mergeLeft, loopRight, loop.Position.X, loop.Size.Width) + } + }) + } +} From 42ec0ed807556d3543792df18d4356e885544d30 Mon Sep 17 00:00:00 2001 From: Ako Date: Wed, 21 Oct 2026 00:57:19 +0000 Subject: [PATCH 03/12] fix: while true with early returns no longer reads as irreducible Each decision's false branch walked round the loop's back edge into the decision itself and on into its true branch, so its return counted as shared: describe printed one 'NOT equivalent ... must not be re-executed' warning per decision and lint raised MDL-FLOW01 as often, for a flow whose describe -> exec writes nothing. A branch walk now stops at its own split. A crossing diamond inside a loop stays Interleaved. Refs ako/mxcli#1277, mendixlabs/mxcli#923 Co-Authored-By: Claude Opus 5.5 --- ...ue-early-returns-flagged-recombinable.json | 1 + ...geometry-1277-while-true-early-returns.mdl | 28 +++++++++ ...md_microflows_describe_irreducible_test.go | 24 ++++++++ mdl/microflowgraph/structure.go | 19 ++++-- mdl/microflowgraph/structure_test.go | 58 +++++++++++++++++++ 5 files changed, 124 insertions(+), 6 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-other/2026-10-21-while-true-early-returns-flagged-recombinable.json create mode 100644 mdl-examples/bug-tests/flow-geometry-1277-while-true-early-returns.mdl diff --git a/.claude/skills/fix-issue/findings/mdl-other/2026-10-21-while-true-early-returns-flagged-recombinable.json b/.claude/skills/fix-issue/findings/mdl-other/2026-10-21-while-true-early-returns-flagged-recombinable.json new file mode 100644 index 0000000000..e392e3db7a --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-other/2026-10-21-while-true-early-returns-flagged-recombinable.json @@ -0,0 +1 @@ +{"area":"mdl/microflowgraph","date":"2026-10-21","symptom":"`while true begin if $C = 'a' then return 'A'; end if; … set $S = 'x'; end while;` was described with one '-- WARNING: … NOT equivalent to the microflow and must not be re-executed over it (mxcli #923, recombinable)' line per decision, and lint raised MDL-FLOW01 as often ('overlap 1 node(s)'), although describe -> exec reported Unchanged and mx check gave 0 errors.","cause":"classify measured each branch body with reachableBefore(branch, join), stopping only at the join and the virtual exit. Each early return is its own end event, so the join is the exit; the false branch walked set -> header merge -> the decision itself -> its true branch's end event, which then counted as overlap.","file":"mdl/microflowgraph/structure.go (classify, reachableBefore)","insight":"The reporter's diagnosis (back edge, structure.go) was exact; the minimal fix is to stop each branch walk at its own split — a branch that re-enters its split has gone round a loop. Stripping back edges via dominators would also work but changes the post-dominator input for every consumer (describe warnings, lint, labelCrossedMerges, normalizeCollection). Keep the crossing-diamond-inside-a-loop control: it must stay Interleaved, and it is the case a cruder 'ignore nodes that reach the split' filter would hide, since inside a loop every body node reaches the split.","refs":["ako/mxcli#1277","mendixlabs/mxcli#923"]} diff --git a/mdl-examples/bug-tests/flow-geometry-1277-while-true-early-returns.mdl b/mdl-examples/bug-tests/flow-geometry-1277-while-true-early-returns.mdl new file mode 100644 index 0000000000..0a0a45d436 --- /dev/null +++ b/mdl-examples/bug-tests/flow-geometry-1277-while-true-early-returns.mdl @@ -0,0 +1,28 @@ +mdl 1; +-- ============================================================================ +-- Bug: `while true` with early returns was described with three "NOT +-- equivalent ... must not be re-executed" warnings and three MDL-FLOW01 lint +-- findings, although describe -> exec wrote nothing (ako/mxcli#1277; +-- mendixlabs/mxcli#923, saingge's re-test) +-- ============================================================================ +-- +-- The graph analysis walked each decision's false branch round the loop's back +-- edge into its own true branch and counted that branch's return as shared. +-- +-- Verify: exec, then +-- mxcli -p app.mpr -c "describe microflow MyFirstModule.ACT_LoopReturns" +-- mxcli lint -p app.mpr +-- Expected: no WARNING lines in the description, no MDL-FLOW01 for this flow. +-- ============================================================================ + +create or modify microflow MyFirstModule.ACT_LoopReturns ($Code: string) returns string as $R +begin + declare $Status string = 'new'; + while true + begin + if $Code = 'a' then return 'A'; end if; + if $Code = 'b' then return 'B'; end if; + if $Code = 'c' then return 'C'; end if; + set $Status = 'x'; + end while; +end; diff --git a/mdl/executor/cmd_microflows_describe_irreducible_test.go b/mdl/executor/cmd_microflows_describe_irreducible_test.go index 1b9529cab4..bc24707ba7 100644 --- a/mdl/executor/cmd_microflows_describe_irreducible_test.go +++ b/mdl/executor/cmd_microflows_describe_irreducible_test.go @@ -209,3 +209,27 @@ end;` } } } + +// `while true` with early returns, from saingge's re-test of mendixlabs/mxcli#923: +// describe -> exec wrote nothing, yet describe printed three "NOT equivalent ... +// must not be re-executed" warnings, one per decision, and lint raised +// MDL-FLOW01 three times. The graph analysis walked each decision's false +// branch round the loop's back edge into its own true branch. +func TestDescribeIrreducible_WhileTrueWithEarlyReturnsHasNoWarning(t *testing.T) { + const src = `create microflow M.ACT_Loop ($Code: string) returns string as $R +begin + declare $Status string = 'new'; + while true + begin + if $Code = 'a' then return 'A'; end if; + if $Code = 'b' then return 'B'; end if; + if $Code = 'c' then return 'C'; end if; + set $Status = 'x'; + end while; +end;` + out, oc := describeBuilt(t, src) + if strings.Contains(out, "WARNING") { + t.Errorf("a while-true with early returns is described with a warning:\n%s", out) + } + assertDescriptionRebuildsGraph(t, out, oc) +} diff --git a/mdl/microflowgraph/structure.go b/mdl/microflowgraph/structure.go index 5797b1ba0c..87c9b12a7d 100644 --- a/mdl/microflowgraph/structure.go +++ b/mdl/microflowgraph/structure.go @@ -141,9 +141,16 @@ func classify( // Each branch's body: what it reaches before the join. The join itself and // everything beyond it is the shared continuation and is not part of a body. + // + // Nor is anything past the split itself. A branch that comes back round to + // its own split has gone round a loop, and what it reaches from there is the + // split's other branches, not overlap. Walking on through it made every + // decision with an early return inside `while true` report its own return + // as shared with its sibling (mendixlabs/mxcli#923): the returns are separate + // end events, so the join is the virtual exit and nothing else stopped the walk. bodies := make([]map[model.ID]bool, 0, len(branches)) for _, b := range branches { - bodies = append(bodies, reachableBefore(b, join, succ)) + bodies = append(bodies, reachableBefore(b, join, splitID, succ)) } // Overlap: nodes some pair of branches can both reach before the join. @@ -297,18 +304,18 @@ func immediatePostDominator(id model.ID, pdom map[model.ID]map[model.ID]bool) mo } // reachableBefore returns the nodes reachable from start without passing -// through stop. stop itself is excluded: it is the shared continuation, not part -// of any branch body. -func reachableBefore(start, stop model.ID, succ map[model.ID][]model.ID) map[model.ID]bool { +// through stop or split. stop itself is excluded: it is the shared continuation, +// not part of any branch body. split is the decision whose branch this is. +func reachableBefore(start, stop, split model.ID, succ map[model.ID][]model.ID) map[model.ID]bool { seen := map[model.ID]bool{} - if start == stop || start == exitID { + if start == stop || start == split || start == exitID { return seen } queue := []model.ID{start} for len(queue) > 0 { n := queue[0] queue = queue[1:] - if n == stop || n == exitID || seen[n] { + if n == stop || n == split || n == exitID || seen[n] { continue } seen[n] = true diff --git a/mdl/microflowgraph/structure_test.go b/mdl/microflowgraph/structure_test.go index 202a0fb45b..b92f063a3a 100644 --- a/mdl/microflowgraph/structure_test.go +++ b/mdl/microflowgraph/structure_test.go @@ -327,3 +327,61 @@ func TestAnalyze_GroupedCaseValuesAreOneBranch(t *testing.T) { t.Errorf("grouped case values reported as overlapping branches: %+v", found) } } + +// `while true` with early returns (mendixlabs/mxcli#923, saingge's re-test): +// +// declare; while true begin +// if a then return 'A'; end if; +// if b then return 'B'; end if; +// if c then return 'C'; end if; +// set $Status = 'x'; +// end while; +// +// is stored as a header merge with a back edge, and every return is its own end +// event, so each decision's join is the virtual exit. The false branch of d1 +// then walked set -> header -> d1 -> e1 and "reached" d1's own true branch: +// three Recombinable findings, three describe warnings saying the description +// must not be re-executed, and three MDL-FLOW01 lint hits — for a flow whose +// describe -> exec writes nothing. A branch that re-enters its own split has +// gone round the loop; what it reaches from there is not its body. +func TestAnalyze_WhileTrueWithEarlyReturnsIsNotFlagged(t *testing.T) { + objects := []microflows.MicroflowObject{ + act("declare"), merge("header"), + split("d1"), split("d2"), split("d3"), act("set"), + µflows.EndEvent{BaseMicroflowObject: obj("e1")}, + µflows.EndEvent{BaseMicroflowObject: obj("e2")}, + µflows.EndEvent{BaseMicroflowObject: obj("e3")}, + } + flows := []*microflows.SequenceFlow{ + flow("declare", "header"), flow("header", "d1"), + flow("d1", "e1"), flow("d1", "d2"), + flow("d2", "e2"), flow("d2", "d3"), + flow("d3", "e3"), flow("d3", "set"), + flow("set", "header"), // back edge + } + if found := Analyze(objects, flows); len(found) != 0 { + t.Errorf("while-true with early returns reported as irreducible: %+v", found) + } +} + +// The fix must not hide a crossing that happens inside a loop: the crossing +// diamond, wrapped in a while-true, is still interleaved. +func TestAnalyze_CrossingDiamondInsideALoopIsStillInterleaved(t *testing.T) { + objects := []microflows.MicroflowObject{ + merge("header"), split("s"), act("a"), act("b"), act("d"), act("e"), + µflows.EndEvent{BaseMicroflowObject: obj("end")}, + split("again"), + } + flows := []*microflows.SequenceFlow{ + flow("header", "s"), + flow("s", "a"), flow("s", "b"), + flow("a", "d"), flow("a", "e"), + flow("b", "d"), flow("b", "e"), + flow("d", "again"), flow("e", "again"), + flow("again", "end"), flow("again", "header"), // back edge + } + f := findingFor(t, Analyze(objects, flows), "s") + if f.Class != Interleaved { + t.Errorf("Class = %q, want %q", f.Class, Interleaved) + } +} From f368a112d88a450bf8bd7ab28369e7c2304cd707 Mon Sep 17 00:00:00 2001 From: Ako Date: Wed, 21 Oct 2026 00:57:40 +0000 Subject: [PATCH 04/12] docs: @anchor's from and to name two different flows The layout syntax topic said @anchor names 'which SIDE each end of the outgoing flow attaches to'. The builder has always applied from: to the flow leaving the statement and to: to the flow arriving at it, so an author following the help got the edge into the next statement entering on the left (an S-curve in Studio Pro) while describe echoed the authored sides. The help, its example, the AST comments and the microflow skill now say what is implemented, and give the two-statement recipe for a straight vertical edge. Refs ako/mxcli#1279, mendixlabs/mxcli#990 Co-Authored-By: Claude Opus 5.5 --- .../write-microflows/reference/control-flow.md | 1 + cmd/mxcli/syntax/features_microflow.go | 14 +++++++++++--- mdl/ast/ast_microflow.go | 15 ++++++++++----- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/.claude/skills/mendix/write-microflows/reference/control-flow.md b/.claude/skills/mendix/write-microflows/reference/control-flow.md index 88be47f331..fb5b85699b 100644 --- a/.claude/skills/mendix/write-microflows/reference/control-flow.md +++ b/.claude/skills/mendix/write-microflows/reference/control-flow.md @@ -330,6 +330,7 @@ commit $Product; - **Leave `@position` out unless you are reproducing a hand-made diagram.** Without it the builder lays the flow out itself: the main line wraps onto rows past two canvas widths, a guard's branch drops into the lane below while the main line carries on above it, and a `case` of four or more branches leaves the decision in three groups so its lines do not cross. A statement with `@position` is never moved and is not measured against what is placed around it, so a few hand-placed statements in an otherwise automatic flow are what produces overlaps (mendixlabs/mxcli#1154) - `@position` always appears in DESCRIBE output; `@caption` only when custom; `@color` only when not Default - DESCRIBE MICROFLOW shows `@` annotations before their activities +- `@anchor(from: X, to: Y)` names two different flows: `from:` is the side the flow **leaving** the statement starts from, `to:` the side the flow **arriving** at it enters by. An annotation never reaches the far end of its own outgoing flow — that end is the next statement's `to:`. A straight vertical edge from A down to B is `@anchor(from: bottom)` on A **and** `@anchor(to: top)` on B; `@anchor(from: bottom, to: top)` on A alone bends both of A's edges and leaves B entered on the left (ako/mxcli#1279). A `return`'s `from:` does nothing (an end event has no outgoing flow). - `@anchor(from: X)` is the side the flow **leaving** a statement starts from. On an `if` that is the flow out of its closing merge (the merge has no statement of its own, just as its position rides on the if as `@merge`); the if's `to:` is its incoming flow and `true:` / `false:` its branches. `@anchor(from: bottom, to: top, …)` on an if is how DESCRIBE writes a decision whose merge drops onto the next row (#767) - `@start(x, y)` positions the **start event** and goes on the first statement, because the start has no statement of its own. Omit it and the start is derived — one spacing unit (160) left of the first activity, on its centre line — and a rewrite re-derives it so the start follows the activities when they move. A start that is not at the derived spot was placed by hand (in Studio Pro or with `@start`): it survives a rewrite that does not mention it, and DESCRIBE emits `@start` for it. An explicit `@start` overrides both (#951) - `@position(x, y)` on a **parameter** goes inside the parameter list, ahead of the parameter it places — a parameter is a stored node with its own coordinates, and this is the only annotation it takes. Omit it and the parameters form a row along the top of the canvas (200;53, 300;53, …). The `@start` rule above applies unchanged: a parameter on that derived row is re-derived on a rewrite, one anywhere else was placed by hand, survives, and is emitted by DESCRIBE (#993). Before this, a hand-aligned parameter block was moved back onto the row by any rewrite — including a describe → exec of mxcli's own output: diff --git a/cmd/mxcli/syntax/features_microflow.go b/cmd/mxcli/syntax/features_microflow.go index cc3153bf53..8fc82c259a 100644 --- a/cmd/mxcli/syntax/features_microflow.go +++ b/cmd/mxcli/syntax/features_microflow.go @@ -820,7 +820,9 @@ func init() { Syntax: "@position(x, y) -- the activity's centre point\n" + "@position(x, y) -- also on a PARAMETER, in the ( … ) list\n" + "@start(x, y) -- the start event, on the FIRST statement\n" + - "@anchor(from: right, to: left) -- which SIDE each end of the outgoing flow attaches to\n" + + "@anchor(from: bottom, to: top) -- from = the side the flow LEAVING this statement\n" + + " -- leaves by; to = the side the flow ARRIVING at\n" + + " -- this statement enters by (not its far end)\n" + "@curve(from: (40, -90), to: (-40, 90)) -- the flow's bezier control vectors\n" + "@merge(x, y) -- the implicit merge that closes a split\n" + "@anchor(from: bottom, to: top, true: (…), false: (…)) -- on an IF: to = its incoming flow,\n" + @@ -830,6 +832,12 @@ func init() { "@applyentityaccess | @applyentityaccess(false) -- DOCUMENT-level, before CREATE MICROFLOW/RULE\n" + "@annotation(id: n1, text: 'a note', position: (x, y), size: (w, h))\n" + "@annotation(id: n1) -- attaches THAT note to another activity\n\n" + + "An @anchor never reaches the far end of its own outgoing flow: that end is\n" + + "the NEXT statement's `to:`. A straight vertical edge from A down to B is\n" + + "@anchor(from: bottom) on A and @anchor(to: top) on B; `to: top` on A alone\n" + + "turns the edge INTO A, and the edge into B keeps entering on the left. A\n" + + "RETURN has no outgoing flow, so its `from:` does nothing — the edge into the\n" + + "end event leaves by the previous statement's `from:`. (#990)\n\n" + "Inside ON ERROR ... BEGIN ... END ERROR the annotations mean what they mean\n" + "outside it: @anchor(to:) on the handler's first statement is the side the\n" + "error edge enters, @anchor(from:) and @curve shape the edge leaving a\n" + @@ -885,9 +893,9 @@ func init() { "what a rewrite re-derives (100px above the activity, stacked 60px per extra\n" + "note, 200x50), so an ordinary note keeps the short form. (#1077)", Example: "create microflow MyModule.ACT_Flow (\n @position(145, 0)\n $In: String\n)\nreturns String as $Out\nbegin\n" + - " @start(145, 100)\n @position(200, 100)\n @anchor(from: bottom, to: top)\n" + + " @start(145, 100)\n @position(200, 100)\n @anchor(from: bottom)\n" + " @curve(from: (40, -90), to: (-40, 90))\n declare $Tmp String = $In;\n" + - " @position(200, 300)\n declare $Out String = $Tmp;\n return $Out;\nend;", + " @position(200, 300)\n @anchor(to: top)\n declare $Out String = $Tmp;\n return $Out;\nend;", SeeAlso: []string{"microflow", "microflow.create"}, }) diff --git a/mdl/ast/ast_microflow.go b/mdl/ast/ast_microflow.go index 2af068099e..defdb22caf 100644 --- a/mdl/ast/ast_microflow.go +++ b/mdl/ast/ast_microflow.go @@ -340,12 +340,17 @@ const ( AnchorSideLeft AnchorSide = 3 ) -// FlowAnchors captures the origin/destination anchors for a single -// SequenceFlow. Each side is independently optional: Unset means the builder -// should derive the anchor from the visual direction. +// FlowAnchors captures the sides a statement's flows attach at. Each side is +// independently optional: Unset means the builder should derive the anchor +// from the visual direction. +// +// The two halves name two DIFFERENT flows when written on a statement: From is +// the side the flow leaving it leaves by, To the side the flow arriving at it +// enters by. Nested in a branch form (`true: (from: …, to: …)`) both describe +// that one branch's edge. type FlowAnchors struct { From AnchorSide // OriginConnectionIndex on the outgoing SequenceFlow - To AnchorSide // DestinationConnectionIndex on the outgoing SequenceFlow + To AnchorSide // DestinationConnectionIndex on the INCOMING SequenceFlow } // HasCaption reports whether the statement states a caption, empty or not. @@ -372,7 +377,7 @@ type ActivityAnnotations struct { // canvas wired to nothing. FreeNotes []MicroflowAnnotation Excluded bool // @excluded - Anchor *FlowAnchors // @anchor(from: X, to: Y) — anchors of the flow leaving this statement + Anchor *FlowAnchors // @anchor(from: X, to: Y) — From for the flow leaving, To for the flow arriving // Split-specific anchors for IF statements. When the statement is not an // IF these remain nil. The grammar accepts them on IfStmt only: From f1a5ff8a35ada62395e775bcdea86ec142a9f666 Mon Sep 17 00:00:00 2001 From: Ako Date: Wed, 21 Oct 2026 04:54:31 +0000 Subject: [PATCH 05/12] fix: describe spells a one-input merge on the main line as join/merge 'join finish; merge finish;' builds a merge only one path enters, and describe walked through it and warned that re-executing the description deleted it. On the main line it is now described as written (join junction1; merge junction1;), so the description rebuilds it and there is nothing to warn about. In a branch or loop body it is still walked through, and the warning now says what is true: create or modify keeps the merge, a rebuild removes it. Verified on PedApp 11.13.0: main binary 1 warning, fixed 0; describe -> exec reports Unchanged as modify and as replace; 3 merges before and after. Refs ako/mxcli#1277, mendixlabs/mxcli#923 Co-Authored-By: Claude Opus 5.5 --- ...und-merge-not-represented-by-describe.json | 1 + ...low-geometry-1277-single-inbound-merge.mdl | 51 ++++++++++ mdl/executor/cmd_microflows_derived_layout.go | 6 +- ...lows_describe_single_inbound_merge_test.go | 92 +++++++++++++++++++ mdl/executor/cmd_microflows_show_helpers.go | 9 ++ mdl/executor/cmd_microflows_show_merge.go | 81 +++++++++++++++- mdl/executor/describe_graph_invariant_test.go | 14 ++- 7 files changed, 250 insertions(+), 4 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-10-21-single-inbound-merge-not-represented-by-describe.json create mode 100644 mdl-examples/bug-tests/flow-geometry-1277-single-inbound-merge.mdl create mode 100644 mdl/executor/cmd_microflows_describe_single_inbound_merge_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-21-single-inbound-merge-not-represented-by-describe.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-21-single-inbound-merge-not-represented-by-describe.json new file mode 100644 index 0000000000..109271cd9d --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-21-single-inbound-merge-not-represented-by-describe.json @@ -0,0 +1 @@ +{"area":"mdl/executor","date":"2026-10-21","symptom":"`join finish; merge finish;` (a merge only one path enters) was written by exec and then described as nothing, with '-- WARNING: the merge at (1330, 350) is not represented in this description ... re-executing this MDL DELETES it' (mendixlabs/mxcli#923 MF_RT_Chain). The warning had also stopped being true: `create or modify` from the description reports Unchanged and keeps the merge.","cause":"traverseFlow walks through any unlabelled merge that no split pairs with, so a one-input merge left no trace in the text; droppedMergeWarnings then flagged it with wording from before modify-in-place compared descriptions instead of rebuilding","file":"mdl/executor/cmd_microflows_show_helpers.go (traverseFlow pass-through), mdl/executor/cmd_microflows_show_merge.go (inlineMerges, droppedMergeWarningsIn), mdl/executor/cmd_microflows_derived_layout.go (memo mergeLabels)","insight":"Only the MAIN LINE (indent 0) can spell it back: there a `merge` after a `join` has no open path, so the builder draws exactly one edge into it. In a branch body the branch loop would add its own fall-through edge (no pathOpen guard), and MDL-FLOW04 bans labels in loop bodies, so those keep the (reworded) warning. Labels are minted lazily in walk order (junction1, …) so unused candidates leave no gaps. Two traps: assertDescriptionRebuildsGraph decided 'is this a whole statement?' with strings.Contains(src, \"create \"), which a warning mentioning `create or modify` defeats (fixed to a line check); and the graph comparison is merge-blind, so a merge-count assertion is needed to see the fix at all. Verified on PedApp 11.13.0: main binary 1 warning, fixed 0; describe -> exec Unchanged as modify AND as replace; 3 merges before and after.","refs":["ako/mxcli#1277","mendixlabs/mxcli#923"]} diff --git a/mdl-examples/bug-tests/flow-geometry-1277-single-inbound-merge.mdl b/mdl-examples/bug-tests/flow-geometry-1277-single-inbound-merge.mdl new file mode 100644 index 0000000000..e752edc74b --- /dev/null +++ b/mdl-examples/bug-tests/flow-geometry-1277-single-inbound-merge.mdl @@ -0,0 +1,51 @@ +mdl 1; +-- ============================================================================ +-- Bug: a merge with one incoming path was not represented by describe +-- (ako/mxcli#1277; mendixlabs/mxcli#923, MF_RT_Chain) +-- ============================================================================ +-- +-- `join finish; merge finish;` builds a merge only one path enters. DESCRIBE +-- walked through it and printed "WARNING: the merge at (1330, 350) is not +-- represented in this description ... re-executing this MDL DELETES it" - +-- for a node exec had just written from ordinary MDL. +-- +-- Fixed: on the main line the merge is described as written +-- (`join junction1; merge junction1;`), so the description rebuilds it and has +-- nothing to warn about. In a branch or loop body it is still walked through, +-- and the warning now says what is true: `create or modify` from the +-- description leaves the merge in place, a rebuild removes it. +-- +-- Verify: exec, then +-- mxcli -p app.mpr -c "describe microflow MyFirstModule.MF_RT_Chain" +-- Expected: no WARNING line; `join junction1;` / `merge junction1;` before the +-- return. Executing the description reports `Unchanged microflow`. +-- ============================================================================ + +create or modify microflow MyFirstModule.MF_RT_Chain () returns string as $Out +begin + declare $Out string = 'a'; + + if 1 = 1 then + if 2 = 2 then + set $Out = 'healthy'; + join emit; + else + set $Out = 'watch'; + join review; + end if; + else + set $Out = 'low'; + join review; + end if; + + merge review; + log info node 'T' 'review'; + join emit; + + merge emit; + log info node 'T' 'emit'; + join finish; + + merge finish; + return $Out; +end; diff --git a/mdl/executor/cmd_microflows_derived_layout.go b/mdl/executor/cmd_microflows_derived_layout.go index aeaf1ced97..e2f2fae8ee 100644 --- a/mdl/executor/cmd_microflows_derived_layout.go +++ b/mdl/executor/cmd_microflows_derived_layout.go @@ -263,7 +263,11 @@ func (m *flowAnalysisMemo) splitMergePoints(oc *microflows.MicroflowObjectCollec // mergeLabels is the labelling of a flow's merges through the memo. func (m *flowAnalysisMemo) mergeLabels(oc *microflows.MicroflowObjectCollection) mergeLabels { - compute := func() mergeLabels { return mergeAllLabels(labelRejoinMerges(oc), labelCrossedMerges(oc)) } + compute := func() mergeLabels { + l := mergeAllLabels(labelRejoinMerges(oc), labelCrossedMerges(oc)) + l.inline = newInlineMerges(oc) + return l + } if m == nil { return compute() } diff --git a/mdl/executor/cmd_microflows_describe_single_inbound_merge_test.go b/mdl/executor/cmd_microflows_describe_single_inbound_merge_test.go new file mode 100644 index 0000000000..556a594761 --- /dev/null +++ b/mdl/executor/cmd_microflows_describe_single_inbound_merge_test.go @@ -0,0 +1,92 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/visitor" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// A merge with ONE incoming path is a node mxcli's own `exec` writes from +// ordinary MDL — `join finish; merge finish;` — and DESCRIBE walked straight +// through it, printing a warning that re-executing the description deletes it +// (mendixlabs/mxcli#923, ako/mxcli#1277). On the main line it is represented +// the way it was written, so the description rebuilds it and has nothing to +// warn about. +func TestDescribe_SingleInboundMergeOnTheMainLineIsRepresented(t *testing.T) { + const src = `create microflow M.MF_RT_Chain () returns string as $Out +begin + declare $Out string = 'a'; + if 1 = 1 then + if 2 = 2 then set $Out = 'healthy'; join emit; + else set $Out = 'watch'; join review; end if; + else set $Out = 'low'; join review; end if; + merge review; log info node 'T' 'review'; join emit; + merge emit; log info node 'T' 'emit'; join finish; + merge finish; return $Out; +end;` + out, oc := describeBuilt(t, src) + if strings.Contains(out, "WARNING") { + t.Errorf("the description still warns:\n%s", out) + } + assertDescriptionRebuildsGraph(t, out, oc) + if got, want := countMerges(rebuildDescription(t, out)), countMerges(oc); got != want { + t.Errorf("the description rebuilds %d merges, the flow has %d:\n%s", got, want, out) + } +} + +// The simplest shape: one activity, a one-input merge, the end. Also the +// fixture of TestDroppedMergeWarnings_PassThroughMergeIsFlagged, built from MDL. +func TestDescribe_SingleInboundMergeAfterAnActivityIsRepresented(t *testing.T) { + const src = `create microflow M.MF_Pass () returns Boolean +begin + log info node 'T' 'one'; + join onward; + merge onward; + log info node 'T' 'two'; + return true; +end;` + out, oc := describeBuilt(t, src) + if strings.Contains(out, "WARNING") { + t.Errorf("the description still warns:\n%s", out) + } + if got, want := countMerges(rebuildDescription(t, out)), countMerges(oc); got != want || want != 1 { + t.Errorf("the description rebuilds %d merges, the flow has %d (want 1):\n%s", got, want, out) + } + // Executing the description again describes identically: a fixed point. + again, _ := describeBuilt(t, "create microflow M.MF_Pass () returns Boolean\nbegin\n"+out+"\nend;") + if again != out { + t.Errorf("not a fixed point:\nfirst:\n%s\nsecond:\n%s", out, again) + } +} + +func countMerges(oc *microflows.MicroflowObjectCollection) int { + n := 0 + for _, o := range oc.Objects { + switch v := o.(type) { + case *microflows.ExclusiveMerge: + n++ + case *microflows.LoopedActivity: + if v.ObjectCollection != nil { + n += countMerges(v.ObjectCollection) + } + } + } + return n +} + +// rebuildDescription builds the flow a description body defines. +func rebuildDescription(t *testing.T, body string) *microflows.MicroflowObjectCollection { + t.Helper() + prog, errs := visitor.Build("create microflow M.Described () returns string\nbegin\n" + body + "\nend;") + if len(errs) > 0 { + t.Fatalf("description does not parse: %v\n%s", errs[0], body) + } + mf := prog.Statements[0].(*ast.CreateMicroflowStmt) + fb := &flowBuilder{posX: 100, posY: 100, spacing: HorizontalSpacing, measurer: &layoutMeasurer{}, varTypes: map[string]string{}} + return fb.buildFlowGraph(mf.Body, mf.ReturnType) +} diff --git a/mdl/executor/cmd_microflows_show_helpers.go b/mdl/executor/cmd_microflows_show_helpers.go index 3d9b34a1f6..b107dd4ca2 100644 --- a/mdl/executor/cmd_microflows_show_helpers.go +++ b/mdl/executor/cmd_microflows_show_helpers.go @@ -1168,6 +1168,15 @@ func traverseFlow( return } visited[currentID] = true + // A merge with one way in, on the main line, is spelled the way exec + // was given it — `join L; merge L;` — so the description rebuilds it + // instead of warning that it is lost (see inlineMerges). + if indent == 0 { + if label, ok := labels.inlineLabel(currentID); ok { + *lines = append(*lines, "join "+label+";") + *lines = append(*lines, mergeDeclarationLines(indent, label, obj, annotationsByTarget.layoutKeep(), flowsByOrigin)...) + } + } for _, flow := range flowsByOrigin[currentID] { traverseFlow(ctx, flow.DestinationID, activityMap, flowsByOrigin, flowsByDest, splitMergeMap, visited, entityNames, microflowNames, lines, indent, sourceMap, headerLineCount, annotationsByTarget, labels) } diff --git a/mdl/executor/cmd_microflows_show_merge.go b/mdl/executor/cmd_microflows_show_merge.go index 07d593eabc..9a2e19b0f2 100644 --- a/mdl/executor/cmd_microflows_show_merge.go +++ b/mdl/executor/cmd_microflows_show_merge.go @@ -47,6 +47,77 @@ import ( type mergeLabels struct { byID map[model.ID]string crossed map[model.ID]bool + // inline names the one-input merges of the main line, which a description + // spells as written: `join