From 01c3a846e40665af9f3e6cd1aad526b8d2f058e5 Mon Sep 17 00:00:00 2001 From: Ako Date: Wed, 21 Oct 2026 10:38:43 +0000 Subject: [PATCH 1/4] fix: a restated if outweighs a new one whose condition differs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `create or modify` pairing a new `if` and a restated one against the same stored decision weighed "the same if but for its condition" exactly like "the same if with a change in a branch", and lcsStatements breaks a tie toward the earlier pair. So in if $Out then …; end if; -- new, no @position @position(-299, 200) if $Flag then …; end if; -- restated, branch edited the new `if $Out` took over the stored decision (its condition set in place) and the restated `if $Flag` was inserted as a new node at its stated position, on top of it: two decisions at (-299, 200), both merges at (186, 200). The weights are doubled and a changed condition now weighs 1, below every other partial match, so the restated if pairs with its stored decision and the new one is an insert. Every other ratio is unchanged. Part of ako/mxcli#1508 case 6 (mendixlabs/mxcli#1598). Co-Authored-By: Claude Opus 5.5 --- mdl/executor/cmd_flow_modify.go | 30 +++++++++++++++-------- mdl/executor/flow_declared_match_test.go | 31 ++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 10 deletions(-) diff --git a/mdl/executor/cmd_flow_modify.go b/mdl/executor/cmd_flow_modify.go index e23c0989b1..3648906d85 100644 --- a/mdl/executor/cmd_flow_modify.go +++ b/mdl/executor/cmd_flow_modify.go @@ -1103,14 +1103,16 @@ func (pd *patchDiff) conditionEdit(declared, stored *ast.IfStmt) error { // insert (#859). A loop is paired this way only when its body can be edited in // place (editableLoopBody, ako/mxcli#1106); any other change inside a loop is // refused either way. -// -// An `if` whose condition changes is paired the same way (ako/mxcli#888): the -// condition is set on the stored decision, and a change around it must not -// merge with it into a replace of the decision. func sameBlockShell(declared, stored ast.MicroflowStatement) bool { - if _, _, ok := sameIfShell(declared, stored); ok { - return true - } + _, _, ok := sameIfShell(declared, stored) + return ok +} + +// conditionChanged reports whether two statements are the same `if` but for +// its condition (ifConditionChanged). Such a pair is matched as a pair too +// (ako/mxcli#888): the condition is set on the stored decision, and a change +// around it must not merge with it into a replace of the decision. +func conditionChanged(declared, stored ast.MicroflowStatement) bool { _, _, ok := ifConditionChanged(declared, stored) return ok } @@ -1450,6 +1452,12 @@ func insertAnchor(loc *storedLocator, stored []ast.MicroflowStatement, gapStart, // it too — it is that node, moved (ako/mxcli#818) — but an exact match weighs // more, so of two alike statements the one the script left in place keeps its // node. +// +// An `if` whose condition changes weighs least: of a new `if` and a restated +// one before the same stored decision, the restated one — the same condition +// — is that decision. Weighed alike, the tie went to the earlier pair: the new +// if took the stored decision over and the restated one was inserted as a new +// node, drawn on top of it (mendixlabs/mxcli#1598, case 6). func lcsStatements(declared, stored []ast.MicroflowStatement) [][2]int { n, m := len(declared), len(stored) w := make([][]int, n) @@ -1458,14 +1466,16 @@ func lcsStatements(declared, stored []ast.MicroflowStatement) [][2]int { for j := range stored { switch { case declaredMatches(declared[i], stored[j]): - w[i][j] = 2 + w[i][j] = 4 case sameExceptPositions(declared[i], stored[j]): - w[i][j] = 1 + w[i][j] = 2 case sameBlockShell(declared[i], stored[j]): - w[i][j] = 1 + w[i][j] = 2 case editableLoopBody(declared[i], stored[j]): // A loop edited in place inside is that loop: a change beside // it must not merge with it into a replace (ako/mxcli#1106). + w[i][j] = 2 + case conditionChanged(declared[i], stored[j]): w[i][j] = 1 } } diff --git a/mdl/executor/flow_declared_match_test.go b/mdl/executor/flow_declared_match_test.go index 4e0882af7d..87b777ed5a 100644 --- a/mdl/executor/flow_declared_match_test.go +++ b/mdl/executor/flow_declared_match_test.go @@ -169,6 +169,37 @@ func TestLCSStatements_ExactMatchWinsOverAMove(t *testing.T) { } } +// mendixlabs/mxcli#1598, case 6: a new `if` inserted before a stored one that +// is restated with a change in its branch. The new if is the stored one but +// for its condition, and the restated if is the stored one but for its +// branch; both counted the same, and the tie went to the earlier pair, so the +// new if took over the stored decision (its condition set in place) and the +// restated if was inserted as a new node — on top of it, at the position the +// script states. The same condition is the closer match. +func TestLCSStatements_TheSameConditionWinsOverAChangedOne(t *testing.T) { + stored := parseFlowBody(t, ` @position(-500, 200) + log info node 'N' 'first'; + @position(-299, 200) + if $In = 'x' then + @position(-124, 100) + log info node 'N' 'a'; + end if;`).Body + declared := parseFlowBody(t, ` @position(-500, 200) + log info node 'N' 'first'; + if $In = 'y' then + log info node 'N' 'new'; + end if; + @position(-299, 200) + if $In = 'x' then + @position(-124, 100) + log info node 'N' 'b'; + end if;`).Body + pairs := lcsStatements(declared, stored) + if len(pairs) != 2 || pairs[0] != [2]int{0, 0} || pairs[1] != [2]int{2, 1} { + t.Fatalf("pairs %v, want [[0 0] [2 1]]: the restated if paired with the stored one, the new if inserted", pairs) + } +} + // mendixlabs/mxcli#1505: Studio Pro stores a multi-line note with CRLF line // breaks and describe prints those bytes as they are. An editor that turns // them into LF changed nothing anyone can see on the canvas, yet the free From a4ed9b61a5180dbe0457adb84c305fc991fc7e5d Mon Sep 17 00:00:00 2001 From: Ako Date: Wed, 21 Oct 2026 10:38:56 +0000 Subject: [PATCH 2/4] fix: create or modify does not move a node the script places (mdl 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `create or modify` that inserts statements without @position into a flow whose later statements state theirs made room by moving everything past the gap along. The stored flow then said something other than the script: in ako/mxcli#1508 case 6 `if $Flag` went -299 -> 40, its merge 186 -> 525 and the return 530 -> 869, while the calls replaced in its branch — placed at their own stated positions, never shifted — stayed at (-124, 100) and (60, 100), left of their decision. Re-running the script was refused (the @merge no longer matched), and shifting "consistently" cannot fix that: the next run must move the nodes back. Under mdl 1 every point the script states a node at — @position, @merge, @start, a note's position:, a parameter's @position; loop bodies skipped, their coordinates are relative — is pinned on the mutator (MicroflowMutator.Pin), and a make-room shift (an insert, or a replace longer than what it replaces) that would move a pinned node is refused, naming the room it needs and the node in the way, with the two ways out: state @position on the new statements, or leave the positions after them unstated so they can move along. Pinning is by point, so a node a placed fragment puts at its stated position is pinned too. mdl 0 is unchanged (ADR-0011): there the refusal would fall back to the whole-flow rebuild, losing the merges, curves and $IDs the shift keeps — TestFlowModify_InsertIsSpliced, ADR-0012's headerless describe+insert acceptance test, caught exactly that. Fixes ako/mxcli#1508 case 6 (mendixlabs/mxcli#1598). Co-Authored-By: Claude Opus 5.5 --- ...e-shift-leaves-placed-fragment-behind.json | 1 + .../skills/mendix/write-microflows/SKILL.md | 2 +- .../write-microflows/reference/pitfalls.md | 16 ++ .../skills/mendix/write-nanoflows/SKILL.md | 35 +-- docs-wiki/bug-patterns/flow-graph-geometry.md | 9 + ...try-1508-splice-keeps-stated-positions.mdl | 110 ++++++++ mdl/backend/mfmutator/splice.go | 60 ++++- mdl/backend/mfmutator/splice_pin_test.go | 115 ++++++++ mdl/backend/microflow_mutation.go | 6 + mdl/executor/cmd_flow_modify.go | 11 +- mdl/executor/cmd_flow_modify_patch.go | 82 +++++- .../flow_modify_stated_positions_1508_test.go | 249 ++++++++++++++++++ 12 files changed, 669 insertions(+), 27 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-backend/2026-10-21-splice-shift-leaves-placed-fragment-behind.json create mode 100644 mdl-examples/bug-tests/flow-geometry-1508-splice-keeps-stated-positions.mdl create mode 100644 mdl/backend/mfmutator/splice_pin_test.go create mode 100644 mdl/executor/flow_modify_stated_positions_1508_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-backend/2026-10-21-splice-shift-leaves-placed-fragment-behind.json b/.claude/skills/fix-issue/findings/mdl-backend/2026-10-21-splice-shift-leaves-placed-fragment-behind.json new file mode 100644 index 0000000000..b3a9cdf987 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-backend/2026-10-21-splice-shift-leaves-placed-fragment-behind.json @@ -0,0 +1 @@ +{"area": "mdl/backend", "date": "2026-10-21", "symptom": "create or modify of a nanoflow drawn with @position on every statement, adding an if/else after the declare without @position and editing the two calls in the next if's branch: `spliced: 1 inserted, 1 replaced, 1 dropped`, with `if $Flag` -299 -> 40, its merge 186 -> 525 and the return 530 -> 869, while the replaced calls stayed at (-124, 100)/(60, 100), left of their own decision. Re-running the same script was refused (`the connectors of the If at (40, 200) are redrawn`). With a plain if (no else) the new if took over the stored decision and the restated `if $Flag` was drawn on top of it at (-299, 200), both merges at (186, 200).", "cause": "Three rules that each held alone. makeRoom shifts every top-level node past the cut by the deficit (339); a Replace whose first statement states @position is Placed and written at its stated coordinates, never shifted; movedNode records a move only when the stated position differs from the stored one, so the shifted nodes were never moved back, and the merge is not a move candidate at all. On re-run @merge is geometry but not a position field, so the pair fell to `redrawn`. No-else: lcsStatements weighed ifConditionChanged the same as sameIfShell and the tie went to the earlier pair, so the new if became a condition edit of the stored decision and the restated if a Placed insert, which never runs checkRoom.", "file": "mdl/backend/mfmutator/splice.go (Pin, pinnedPast, makeRoom, Replace), mdl/backend/microflow_mutation.go (MicroflowMutator.Pin), mdl/executor/cmd_flow_modify_patch.go (statedPoints, patch), mdl/executor/cmd_flow_modify.go (lcsStatements weights, mdl 1 gate in planFlowModify)", "insight": "Shift-consistently cannot satisfy idempotence: once the script states -299 and the store says 40, the re-run must move it back or refuse — measured even for a plain one-activity insert into a described flow (+104, then the re-run refused on @merge). So a node at a stated point is pinned and a shift that would move one is refused, naming the room and the node; the mutator pins by point rather than $ID because the node at a stated point may be one a later Placed fragment puts there. Collect stated points from @position, @merge, @start, note and parameter positions, skipping loop bodies (relative coordinates). The control is the same script with the positions after the insert left out: spliced, everything moved by one delta, re-run Unchanged. Gate the pins on mdl 1 (flowRebuildRefused.Applies): under mdl 0 the refusal falls back to the whole-flow rebuild, which deletes the Studio Pro merges and $IDs the shift kept — TestFlowModify_InsertIsSpliced (ADR-0012's describe+insert acceptance, headerless) caught exactly that, and ADR-0011 forbids the change of meaning. Not touched: the shift still moves parameters (y 53) along with the row.", "refs": ["ako/mxcli#1508", "mendixlabs/mxcli#1598"]} diff --git a/.claude/skills/mendix/write-microflows/SKILL.md b/.claude/skills/mendix/write-microflows/SKILL.md index af58d4d850..4cf24d5edd 100644 --- a/.claude/skills/mendix/write-microflows/SKILL.md +++ b/.claude/skills/mendix/write-microflows/SKILL.md @@ -69,7 +69,7 @@ Choose the mode by who owns the microflow ([choose-edit-mode](../choose-edit-mod (targets from `describe microflow X with handles`). `create or modify` of `describe` output patches too: unchanged writes nothing; a statement (even one whose handler returns), guard clause, `return` value, `if` condition, header clause, parameter (added/retyped; removed only if unused) or stated `@position`/`@start` change is - patched in place (a move keeps the node's flows), as is a loop-body activity with new values. A redrawn + patched in place (a move keeps the node's flows), as is a loop-body activity with new values ([an insert needing room past a stated `@position`](reference/pitfalls.md#13-inserting-where-the-script-states-every-position)). A redrawn `@anchor`/`@curve`, other loop-body change, error handler or other `return` added/taken away rebuilds under mdl 0 (`MDL-V1-REBUILD`: IDs renumbered, merges and curves lost) and is refused under `mdl 1;`. **To add, remove or reorder in a loop body, `alter … replace` the whole loop** ([pitfalls](reference/pitfalls.md#11-changing-something-inside-a-loop-body)). **To rebuild deliberately, `drop` and `create` in ONE script** — the grants carry only within it ([pitfalls](reference/pitfalls.md#drop--create-is-still-a-new-document)). diff --git a/.claude/skills/mendix/write-microflows/reference/pitfalls.md b/.claude/skills/mendix/write-microflows/reference/pitfalls.md index b1df1bbddc..1848349607 100644 --- a/.claude/skills/mendix/write-microflows/reference/pitfalls.md +++ b/.claude/skills/mendix/write-microflows/reference/pitfalls.md @@ -376,6 +376,22 @@ alter microflow MyModule.GetCode { }; ``` +### 13. Inserting Where the Script States Every Position + +`describe` output states `@position` on every statement. Under `mdl 1;`, `create or +modify` never moves a node the script draws at a stated position: a statement inserted +**without** `@position` into a gap too small for it is refused, naming the room it needs +and the node in the way (mendixlabs/mxcli#1598). Moving the rest of the row along would +leave the stored flow saying something other than the script, so the next run would move +it back. Either: + +- give the new statements an `@position` where there is room for them, or +- leave the positions of what comes after them unstated (`@position`, `@merge`, a note's + `position:`), so the splice can move it along — the second run of that script is then + Unchanged. + +A headerless (mdl 0) script still moves the row along, as it always did. + ## Implicit Variable Creation (CE0111 Duplicate Variable) These statements **implicitly create a new variable** with the name on the left side: diff --git a/.claude/skills/mendix/write-nanoflows/SKILL.md b/.claude/skills/mendix/write-nanoflows/SKILL.md index 3aadc22379..6740be6af0 100644 --- a/.claude/skills/mendix/write-nanoflows/SKILL.md +++ b/.claude/skills/mendix/write-nanoflows/SKILL.md @@ -11,21 +11,21 @@ description: "Nanoflow syntax in MDL — shared with microflows, but client-side | lines | section | |---|---| | 35-44 | When to Use This Skill | -| 45-78 | Changing an Existing Nanoflow | -| 79-97 | When to Use a Nanoflow vs a Microflow | -| 98-112 | Key Differences from Microflows | -| 113-136 | Nanoflow Structure | -| 137-145 | Naming Convention | -| 146-214 | Supported Activities | -| 215-481 | Real-World Patterns | -| 482-552 | NanoflowCommons JavaScript Actions Reference | -| 553-591 | Disallowed Activities | -| 592-595 | Return Type Restrictions | -| 596-640 | Error Handling in Nanoflows | -| 641-648 | Security (GRANT/REVOKE) | -| 649-661 | Management Commands | -| 662-672 | Common Mistakes | -| 673-687 | Validation Checklist | +| 45-81 | Changing an Existing Nanoflow | +| 82-100 | When to Use a Nanoflow vs a Microflow | +| 101-115 | Key Differences from Microflows | +| 116-139 | Nanoflow Structure | +| 140-148 | Naming Convention | +| 149-217 | Supported Activities | +| 218-484 | Real-World Patterns | +| 485-555 | NanoflowCommons JavaScript Actions Reference | +| 556-594 | Disallowed Activities | +| 595-598 | Return Type Restrictions | +| 599-643 | Error Handling in Nanoflows | +| 644-651 | Security (GRANT/REVOKE) | +| 652-664 | Management Commands | +| 665-675 | Common Mistakes | +| 676-690 | Validation Checklist | # Mendix Nanoflow Skill @@ -58,7 +58,10 @@ Choose the mode by who owns the nanoflow ([choose-edit-mode](../choose-edit-mode the stored end event or decision, and a body without a trailing `return` means the stored end. The header (documentation, return type, parameters added, retyped or — when unused — removed) is set on the stored document, and a stated `@position` or `@start` moves the - stored node, keeping its flows. A change it cannot patch — anything inside a loop body + stored node, keeping its flows. Under `mdl 1;` a node at a stated position is never moved + to make room: a statement inserted without `@position` where the gap is too small is refused, naming + the node in the way — give the new statements an `@position`, or leave `@position` and + `@merge` off the statements after them. A change it cannot patch — anything inside a loop body or error handler, a redrawn `@anchor`/`@curve`, any other `return` added or taken away — rebuilds the whole nanoflow under mdl 0 (warning `MDL-V1-REBUILD`: element IDs renumbered, merges removed, curves reset) and diff --git a/docs-wiki/bug-patterns/flow-graph-geometry.md b/docs-wiki/bug-patterns/flow-graph-geometry.md index 5303e9f98c..3a1d12a62b 100644 --- a/docs-wiki/bug-patterns/flow-graph-geometry.md +++ b/docs-wiki/bug-patterns/flow-graph-geometry.md @@ -94,6 +94,15 @@ translated into the gap; anything that is not on the line (a note) must not size the gap, and anything the script placed absolutely must not be translated. +**Making room is a move the script did not ask for.** Shifting the row after +a gap is consistent only while nothing past the gap has a stated position: +once the script says -299 and the store says 40, the next run must move it back +or refuse, so "shift everything consistently" can never re-run Unchanged. A +node at a stated point is pinned instead, and a shift that would move one is +refused with the room it needs — under mdl 1 only, because under mdl 0 a +refusal becomes a whole-flow rebuild, which loses more than the shift did. +Check a splice fix by re-running the same script, not by describing once. + ## See also - [fix-issue findings](../../.claude/skills/fix-issue/findings/) — the formulas, diff --git a/mdl-examples/bug-tests/flow-geometry-1508-splice-keeps-stated-positions.mdl b/mdl-examples/bug-tests/flow-geometry-1508-splice-keeps-stated-positions.mdl new file mode 100644 index 0000000000..3d7471644d --- /dev/null +++ b/mdl-examples/bug-tests/flow-geometry-1508-splice-keeps-stated-positions.mdl @@ -0,0 +1,110 @@ +mdl 1; +-- ============================================================================ +-- Bug: an inserted if moved every stated node after it, but not the +-- statements replaced in its neighbour's branch +-- (ako/mxcli#1508, case 6; mendixlabs/mxcli#1598) +-- ============================================================================ +-- +-- NF_Do is drawn with a @position on every statement. The reported +-- restatement (below, commented out) keeps every @position, adds an if/else +-- after the declare without one, changes the JS argument and binds the call's +-- result. Before the fix it was spliced in as +-- +-- Modified nanoflow: MyFirstModule.NF_Do (spliced: 1 inserted, 1 replaced, 1 dropped) +-- +-- with `if $Flag` moved -299 -> 40, its merge 186 -> 525 and the return +-- 530 -> 869 to make room, while the two replaced calls stayed at the +-- positions the script states, (-124, 100) and (60, 100): left of their own +-- decision. Re-running the script was refused ("the connectors of the If at +-- (40, 200) are redrawn"). With a plain `if` (no else) the new if took over the +-- stored decision and the restated `if $Flag` was drawn on top of it. +-- +-- Fixed: under mdl 1 a node the script draws at a stated position is not +-- moved to make room. The reported restatement is refused, naming the room it +-- needs and what is in the way (a headerless script still makes room as +-- before: under mdl 0 the refusal would fall back to a whole-flow rebuild, +-- ADR-0011): +-- +-- the inserted statements need 339px more room than there is between the +-- ActionActivity at (-500, 200) and the ExclusiveSplit '$Flag' at +-- (-299, 200), which the script draws where it is; state @position on the +-- new statements, where there is room for them, or leave unstated the +-- positions of what comes after them (@position, @merge, a note's +-- position:) so that it can move along. +-- +-- The restatement that leaves those positions unstated (the last statement of +-- this file) is spliced: everything after the declare moves along by the same +-- amount, the replaced calls with their decision. +-- +-- Verify: exec, then +-- mxcli -p app.mpr -c "describe nanoflow MyFirstModule.NF_Do" +-- Expected: both calls are drawn right of `if $Flag` and left of its merge, +-- and the last statement, run again on its own, reports +-- Unchanged nanoflow: MyFirstModule.NF_Do +-- Uncomment the reported restatement to see the refusal. +-- ============================================================================ + +create javascript action MyFirstModule.JS_Do(Msg: String) returns Boolean platform web as $$ +return Promise.resolve(true); +$$; + +create or modify microflow MyFirstModule.SUB_Check() returns Boolean as $R +begin + declare $R Boolean = true; + return $R; +end; + +create or modify nanoflow MyFirstModule.NF_Do ($Flag: Boolean) returns Boolean as $Out +begin + @position(-500, 200) + declare $Out Boolean = false; + @position(-299, 200) + @merge(186, 200) + if $Flag then + @position(-124, 100) + call javascript action MyFirstModule.JS_Do(Msg = 'a'); + @position(60, 100) + call microflow MyFirstModule.SUB_Check(); + end if; + @position(530, 200) + return $Out; +end; + +-- The reported restatement: refused, nothing written. +-- create or modify nanoflow MyFirstModule.NF_Do ($Flag: Boolean) returns Boolean as $Out +-- begin +-- @position(-500, 200) +-- declare $Out Boolean = false; +-- if $Out then +-- log info node 'Probe' 'x'; +-- else +-- log info node 'Probe' 'y'; +-- end if; +-- @position(-299, 200) +-- @merge(186, 200) +-- if $Flag then +-- @position(-124, 100) +-- call javascript action MyFirstModule.JS_Do(Msg = 'b'); +-- @position(60, 100) +-- $Ok = call microflow MyFirstModule.SUB_Check(); +-- end if; +-- @position(530, 200) +-- return $Out; +-- end; + +-- The same change with the positions after the insert left unstated: spliced. +create or modify nanoflow MyFirstModule.NF_Do ($Flag: Boolean) returns Boolean as $Out +begin + @position(-500, 200) + declare $Out Boolean = false; + if $Out then + log info node 'Probe' 'x'; + else + log info node 'Probe' 'y'; + end if; + if $Flag then + call javascript action MyFirstModule.JS_Do(Msg = 'b'); + $Ok = call microflow MyFirstModule.SUB_Check(); + end if; + return $Out; +end; diff --git a/mdl/backend/mfmutator/splice.go b/mdl/backend/mfmutator/splice.go index b00376e009..b28ff3e354 100644 --- a/mdl/backend/mfmutator/splice.go +++ b/mdl/backend/mfmutator/splice.go @@ -69,6 +69,8 @@ type Mutator struct { // removed holds the $IDs of every element an operation took out, so Save // can prove that nothing still points at one of them. removed map[string]bool + // pinned are the points the script states a node is drawn at (Pin). + pinned map[point]bool } var _ backend.MicroflowMutator = (*Mutator)(nil) @@ -341,7 +343,16 @@ func (m *Mutator) makeRoom(g *graph, fb *fragmentBox, f flowRef, x, y *node) (en b := y.pos.get(ax) - s*y.size.get(ax)/2 need := fb.length(ax) + 2*minGap if deficit := need - s*(b-a); deficit > 0 { - m.shift(g, ax, s, float64(a+b)/2, s*deficit, "") + cut := float64(a+b) / 2 + if p := m.pinnedPast(g, ax, s, cut, ""); p != nil { + blocked := ", and making it would move " + describeNode(p) + "," + if p == y { + blocked = "," + } + return 0, 0, fmt.Errorf("the inserted statements need %dpx more room than there is between %s and %s%s "+ + "which the script draws where it is; %s", deficit, describeNode(x), describeNode(y), blocked, roomAdvice) + } + m.shift(g, ax, s, cut, s*deficit, "") b += s * deficit } @@ -422,7 +433,12 @@ func (m *Mutator) Replace(target model.ID, frag *backend.MicroflowFragment) erro // past target moves along by the difference. near := x.pos.get(ax) - s*x.size.get(ax)/2 if extra := fb.length(ax) - x.size.get(ax); extra > 0 { - m.shift(g, ax, s, float64(x.pos.get(ax)), s*extra, x.id) + cut := float64(x.pos.get(ax)) + if p := m.pinnedPast(g, ax, s, cut, x.id); p != nil { + return fmt.Errorf("the replacing statements are %dpx longer than %s, and making room for them would move %s, "+ + "which the script draws where it is; %s", extra, describeNode(x), describeNode(p), roomAdvice) + } + m.shift(g, ax, s, cut, s*extra, x.id) } centre := point{} centre = centre.set(ax, near+s*fb.length(ax)/2) @@ -700,6 +716,17 @@ func (m *Mutator) Move(target model.ID, to model.Point) error { return nil } +// Pin records that the script states a node is drawn at at +// (mendixlabs/mxcli#1598, case 6). A point rather than a node: the node there +// may be one a later fragment places, at the position its statement states. +func (m *Mutator) Pin(at model.Point) error { + if m.pinned == nil { + m.pinned = map[point]bool{} + } + m.pinned[point{at.X, at.Y}] = true + return nil +} + // bodyFlows returns the flows that run inside loop's body, at any depth. They // are stored in the unit's Flows list, not in the loop, so taking the loop out // has to take them too; left behind they would point at removed objects. @@ -860,10 +887,7 @@ func (m *Mutator) markRemoved(v any) { // so no curve changes. skip is a node that is about to be removed. func (m *Mutator) shift(g *graph, ax axis, s int, cut float64, delta int, skip string) { for _, n := range g.order { - if n.loop != "" || n.id == skip { - continue - } - if float64(s)*(float64(n.pos.get(ax))-cut) <= 0 { + if !shifted(n, ax, s, cut, skip) { continue } n.pos = n.pos.set(ax, n.pos.get(ax)+delta) @@ -871,6 +895,30 @@ func (m *Mutator) shift(g *graph, ax axis, s int, cut float64, delta int, skip s } } +// shifted reports whether shift moves n: a top-level node past the cut. +func shifted(n *node, ax axis, s int, cut float64, skip string) bool { + return n.loop == "" && n.id != skip && float64(s)*(float64(n.pos.get(ax))-cut) > 0 +} + +// pinnedPast returns the first node shift would move that is drawn at a point +// the script states (Pin), or nil. Moving it would make the stored flow say +// something other than the script — a later fragment placed at its own stated +// position would be left behind on the wrong side of a node that moved, and +// re-running the script would move the node back (mendixlabs/mxcli#1598, +// case 6) — so the caller refuses instead. +func (m *Mutator) pinnedPast(g *graph, ax axis, s int, cut float64, skip string) *node { + for _, n := range g.order { + if m.pinned[n.pos] && shifted(n, ax, s, cut, skip) { + return n + } + } + return nil +} + +// roomAdvice is the way out of a refused shift. +const roomAdvice = "state @position on the new statements, where there is room for them, or leave unstated the positions " + + "of what comes after them (@position, @merge, a note's position:) so that it can move along" + // checkIntegrity is the rule-1 guard: no binary anywhere in the unit may // still name an element that was removed, and no two elements may share an // $ID. Either would make the document unopenable, and neither is caught by diff --git a/mdl/backend/mfmutator/splice_pin_test.go b/mdl/backend/mfmutator/splice_pin_test.go new file mode 100644 index 0000000000..b8728bba8d --- /dev/null +++ b/mdl/backend/mfmutator/splice_pin_test.go @@ -0,0 +1,115 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mfmutator + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/backend" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// A node whose position the script states is not moved to make room +// (mendixlabs/mxcli#1598, case 6). The splice moved every node past the gap +// along by what the fragment lacked, including ones the script draws at a +// stated @position — so the stored flow no longer said what the script said, +// a fragment placed at its own stated position was left behind on the wrong +// side of its decision, and re-running the same script moved the nodes back +// (or was refused). Such a shift is refused now, saying which node is in the +// way; the controls show the same splice still makes room when nothing past +// the gap is pinned. +func TestSplice_MakingRoomRefusesToMoveAPinnedNode(t *testing.T) { + positions := func(m *Mutator) map[string]point { + out := map[string]point{} + for _, n := range m.graph().order { + out[n.id] = n.pos + } + return out + } + // b at (420, 200) sits 50px right of a; a 120px activity needs 200. + t.Run("an insert, the node after the gap pinned", func(t *testing.T) { + m, _ := newMutator(t, unit(line(), lineFlows())) + if err := m.Pin(model.Point{X: 420, Y: 200}); err != nil { + t.Fatal(err) + } + err := m.InsertAfter(model.ID(uid("a")), oneActivity()) + if err == nil || !strings.Contains(err.Error(), "the ActionActivity at (420, 200)") || + !strings.Contains(err.Error(), "150px") { + t.Fatalf("got %v, want a refusal naming the pinned activity at (420, 200) and the 150px lacking", err) + } + if p := positions(m)[uid("b")]; p.X != 420 { + t.Errorf("b moved to x=%d before the refusal", p.X) + } + }) + t.Run("control: nothing pinned", func(t *testing.T) { + m, _ := newMutator(t, unit(line(), lineFlows())) + if err := m.InsertAfter(model.ID(uid("a")), oneActivity()); err != nil { + t.Fatal(err) + } + if p := positions(m)[uid("b")]; p.X != 570 { + t.Errorf("b is at x=%d, want it moved along by 150 to 570", p.X) + } + }) + t.Run("control: only nodes before the gap pinned", func(t *testing.T) { + m, _ := newMutator(t, unit(line(), lineFlows())) + for _, p := range []model.Point{{X: 100, Y: 200}, {X: 250, Y: 200}} { + if err := m.Pin(p); err != nil { + t.Fatal(err) + } + } + if err := m.InsertAfter(model.ID(uid("a")), oneActivity()); err != nil { + t.Fatalf("a pin before the gap refused the insert: %v", err) + } + }) + t.Run("a replace longer than what it replaces, the end pinned", func(t *testing.T) { + m, _ := newMutator(t, unit(line(), lineFlows())) + if err := m.Pin(model.Point{X: 600, Y: 200}); err != nil { + t.Fatal(err) + } + err := m.Replace(model.ID(uid("a")), twoActivities()) + if err == nil || !strings.Contains(err.Error(), "the EndEvent at (600, 200)") { + t.Fatalf("got %v, want a refusal naming the pinned end event", err) + } + }) + t.Run("an earlier fragment drawn at a stated point", func(t *testing.T) { + // The script states where the first fragment goes, so it is pinned + // like a stored node: a later insert cannot slide it along. + m, _ := newMutator(t, unit(line(), lineFlows())) + placed := oneActivity() + placed.Placed = true + placed.Objects[0].SetPosition(model.Point{X: 600, Y: 400}) + if err := m.Pin(model.Point{X: 600, Y: 400}); err != nil { + t.Fatal(err) + } + if err := m.InsertAfter(model.ID(uid("b")), placed); err != nil { + t.Fatal(err) + } + err := m.InsertAfter(model.ID(uid("a")), oneActivity()) + if err == nil || !strings.Contains(err.Error(), "(600, 400)") { + t.Fatalf("got %v, want a refusal naming the placed activity at (600, 400)", err) + } + }) +} + +// twoActivities is a two-activity fragment in builder coordinates, 290px long. +func twoActivities() *backend.MicroflowFragment { + act := func(x int) *microflows.ActionActivity { + return µflows.ActionActivity{BaseActivity: microflows.BaseActivity{BaseMicroflowObject: microflows.BaseMicroflowObject{ + BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, + Position: model.Point{X: x, Y: 200}, + Size: model.Size{Width: 120, Height: 60}, + }}} + } + a, b := act(360), act(530) + return &backend.MicroflowFragment{ + Objects: []microflows.MicroflowObject{a, b}, + Flows: []*microflows.SequenceFlow{{ + BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, + OriginID: a.ID, DestinationID: b.ID, + }}, + Entry: a.ID, Exit: b.ID, + } +} diff --git a/mdl/backend/microflow_mutation.go b/mdl/backend/microflow_mutation.go index 4217ff5d09..d381554916 100644 --- a/mdl/backend/microflow_mutation.go +++ b/mdl/backend/microflow_mutation.go @@ -66,6 +66,12 @@ type MicroflowMutator interface { // Move sets where the top-level node target is drawn, and nothing else: // its flows keep their ends, sides and curves (ako/mxcli#818). Move(target model.ID, to model.Point) error + // Pin says the script states that a node is drawn at the point at — a + // stored node, or one a fragment places there. An insert or replace that + // would move the node drawn there to make room is refused instead, so the + // stored flow keeps saying what the script says (mendixlabs/mxcli#1598, + // case 6). + Pin(at model.Point) error // SetHeader writes the document properties of declared — a // *microflows.Microflow or *microflows.Nanoflow built from the statement, // with every property it does not state carried from the stored one — onto diff --git a/mdl/executor/cmd_flow_modify.go b/mdl/executor/cmd_flow_modify.go index 3648906d85..d3fd5a958b 100644 --- a/mdl/executor/cmd_flow_modify.go +++ b/mdl/executor/cmd_flow_modify.go @@ -326,9 +326,18 @@ func planFlowModify(ctx *ExecContext, d *flowDecl) (*flowPlan, error) { // a member of one was written bare (ako/mxcli#885). a.declaredVarTypes = varTypes + // A node the script draws at a stated position is pinned under mdl 1 only + // (ADR-0011: a new refusal applies under the version that introduces it). + // Under mdl 0 the refused splice would fall back to rebuilding the whole + // flow, losing the merges, curves and $IDs the shift keeps, so a headerless + // script makes room as it did (mendixlabs/mxcli#1598, case 6). + var stated []model.Point + if flowRebuildRefused.Applies(ctx.LanguageVersion) { + stated = statedPoints(d) + } p := &flowPlan{a: a, ops: ops, moves: moves, storedFolder: storedFolderOf(stored)} if len(ops) > 0 || len(moves) > 0 || headerChanged { - if p.mut, err = patch(ctx, a, ops, targets, moves); err != nil { + if p.mut, err = patch(ctx, a, ops, targets, moves, stated); err != nil { return nil, cannotSplice("%v", err) } if headerChanged { diff --git a/mdl/executor/cmd_flow_modify_patch.go b/mdl/executor/cmd_flow_modify_patch.go index dcd6cf6eaf..908016b05a 100644 --- a/mdl/executor/cmd_flow_modify_patch.go +++ b/mdl/executor/cmd_flow_modify_patch.go @@ -5,6 +5,7 @@ package executor import ( "bytes" "fmt" + "reflect" "strings" "go.mongodb.org/mongo-driver/v2/bson" @@ -32,6 +33,10 @@ import ( // moves that node, and only it: its flows keep their ends, sides and // curves (no reconnection), and nothing around it moves. A position the // script leaves out keeps what is stored, as before; +// - a node the script draws at a stated position stays there: a fragment +// that needs more room than the stated positions leave is refused, rather +// than everything after it moved along, which left the stored flow saying +// something other than the script (mendixlabs/mxcli#1598, case 6); // - two nodes nobody placed follow the node they were derived from, because // mxcli's own layout put them there and nothing else says where they go: // a start event where the layout derives it follows the first statement @@ -56,10 +61,13 @@ const endSpacing = HorizontalSpacing + HorizontalSpacing/2 // patch opens the stored flow and applies the moves and the splice // operations. The moves go first, so a fragment the script places next to a // moved node is joined to it where it now is, and again after the operations, -// so a node an insert shifted to make room is still drawn where the script -// says (a move sets a position, so applying it twice is applying it once). +// so a node no one placed that follows another (followingEnd) is drawn where +// the script says even if an insert shifted it to make room (a move sets a +// position, so applying it twice is applying it once). Between the two, every +// point the script states a node at is pinned, so an operation that would +// have to move one to make room is refused (mendixlabs/mxcli#1598, case 6). func patch(ctx *ExecContext, a *alterFlowContext, ops []*ast.AlterFlowOperation, targets []mfmutator.Candidate, - moves []flowMove) (backend.MicroflowMutator, error) { + moves []flowMove, stated []model.Point) (backend.MicroflowMutator, error) { mut, err := ctx.Backend.OpenMicroflowForMutation(a.mf.ID) if err != nil { return nil, err @@ -75,6 +83,11 @@ func patch(ctx *ExecContext, a *alterFlowContext, ops []*ast.AlterFlowOperation, if err := move(); err != nil { return nil, err } + for _, p := range stated { + if err := mut.Pin(p); err != nil { + return nil, err + } + } if err := a.applyTo(ctx, mut, ops, targets); err != nil { return nil, err } @@ -138,6 +151,69 @@ func (pd *patchDiff) movedNode(d, s ast.MicroflowStatement) error { return nil } +// statedPoints are the canvas points the declared flow states a node is drawn +// at: each statement's @position, an if's @merge, the @start, every note's +// position and every parameter's. The splice pins whatever is drawn at one, so +// that making room for a fragment does not move a node away from where the +// script draws it (mendixlabs/mxcli#1598, case 6). A loop body is left out: +// its positions are relative to the loop box, and the splice moves no node +// inside one. +func statedPoints(d *flowDecl) []model.Point { + var out []model.Point + add := func(p *ast.Position) { + if p != nil { + out = append(out, model.Point{X: p.X, Y: p.Y}) + } + } + for _, p := range d.params { + add(p.Position) + } + var walk func(rv reflect.Value) + walk = func(rv reflect.Value) { + switch rv.Kind() { + case reflect.Interface: + if !rv.IsNil() { + walk(rv.Elem()) + } + case reflect.Pointer: + if rv.IsNil() { + return + } + switch v := rv.Interface().(type) { + case *ast.ActivityAnnotations: + add(v.Position) + add(v.Merge) + add(v.Start) + for _, notes := range [][]ast.MicroflowAnnotation{v.Notes, v.FreeNotes} { + for _, n := range notes { + add(n.Position) + } + } + return + case *ast.LoopStmt: + walk(reflect.ValueOf(v.Annotations)) + return + case *ast.WhileStmt: + walk(reflect.ValueOf(v.Annotations)) + return + } + walk(rv.Elem()) + case reflect.Struct: + for i := 0; i < rv.NumField(); i++ { + if rv.Type().Field(i).IsExported() { + walk(rv.Field(i)) + } + } + case reflect.Slice, reflect.Array: + for i := 0; i < rv.Len(); i++ { + walk(rv.Index(i)) + } + } + } + walk(reflect.ValueOf(d.body)) + return out +} + // redrawn is the refusal for a statement whose connectors the script draws // differently: the patch moves nodes, and does not reroute flows. func redrawn(st ast.MicroflowStatement) error { diff --git a/mdl/executor/flow_modify_stated_positions_1508_test.go b/mdl/executor/flow_modify_stated_positions_1508_test.go new file mode 100644 index 0000000000..bb4a3cf40d --- /dev/null +++ b/mdl/executor/flow_modify_stated_positions_1508_test.go @@ -0,0 +1,249 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "bytes" + "strings" + "testing" + + "go.mongodb.org/mongo-driver/v2/bson" +) + +// mendixlabs/mxcli#1598 (ako/mxcli#1508), case 6, on a real project write. +// +// The flow is drawn with a @position on every statement. Restated with a new +// if/else after the declare and no @position on it, the splice made room by +// moving everything after the declare along by 339px — while the branch's two +// calls, replaced at the positions the script states, stayed where they were, +// left of their own decision: +// +// Modified nanoflow: MyFirstModule.NF_Do (spliced: 1 inserted, 1 replaced, 1 dropped) +// @position(40, 200) if $Flag was -299 +// @merge(525, 200) its merge was 186 +// @position(-124, 100) call javascript action … +// @position(869, 200) return was 530 +// +// and re-running the same script was refused ("the connectors of the If at +// (40, 200) are redrawn"). The same script with a plain `if` and no else put +// the new if on top of the stored decision. Either way the stored flow said +// something other than the script, so under mdl 1 the splice now refuses to +// move a node the script states, and the script that leaves those positions +// unstated is the control: it is applied, and its re-run is Unchanged. + +const nf1508Setup = `mdl 1; +create javascript action MyFirstModule.JS_Do(Msg: String) returns Boolean platform web as $$ +return Promise.resolve(true); +$$; +create microflow MyFirstModule.SUB_Check() returns Boolean as $R +begin + declare $R Boolean = true; + return $R; +end; +create nanoflow MyFirstModule.NF_Do ($Flag: Boolean) returns Boolean as $Out +begin + @position(-500, 200) + declare $Out Boolean = false; + @position(-299, 200) + @merge(186, 200) + if $Flag then + @position(-124, 100) + call javascript action MyFirstModule.JS_Do(Msg = 'a'); + @position(60, 100) + call microflow MyFirstModule.SUB_Check(); + end if; + @position(530, 200) + return $Out; +end; +` + +// nf1508Modify is the reported restatement: every @position kept, an if/else +// added after the declare without one, the JS argument changed and the call's +// result bound. +const nf1508Modify = `mdl 1; +create or modify nanoflow MyFirstModule.NF_Do ($Flag: Boolean) returns Boolean as $Out +begin + @position(-500, 200) + declare $Out Boolean = false; + if $Out then + log info node 'Probe' 'x'; + else + log info node 'Probe' 'y'; + end if; + @position(-299, 200) + @merge(186, 200) + if $Flag then + @position(-124, 100) + call javascript action MyFirstModule.JS_Do(Msg = 'b'); + @position(60, 100) + $Ok = call microflow MyFirstModule.SUB_Check(); + end if; + @position(530, 200) + return $Out; +end; +` + +func TestCreateOrModify_InsertDoesNotMoveStatedNodes1508(t *testing.T) { + // Each case starts from the flow as the setup draws it. + setup := func(t *testing.T) (*Executor, *bytes.Buffer, func() string) { + t.Helper() + exec, out := openPedAppFixture(t) + if err := agreeExec(t, exec, nf1508Setup); err != nil { + t.Fatalf("setup: %v\n%s", err, out.String()) + } + describe := func() string { + out.Reset() + if err := afRun(t, exec, "describe nanoflow MyFirstModule.NF_Do;"); err != nil { + return err.Error() + } + return out.String() + } + return exec, out, describe + } + refused := func(t *testing.T, src string) { + exec, out, describe := setup(t) + before := nf1508Unit(t, exec) + out.Reset() + err := agreeExec(t, exec, src) + said := out.String() + if err != nil { + said += err.Error() + } + if err == nil || !strings.Contains(said, "which the script draws where it is") || + !strings.Contains(said, "the ExclusiveSplit '$Flag' at (-299, 200)") { + t.Errorf("want a refusal naming the decision the script draws at (-299, 200), got %v\n%s", err, said) + } + if !bytes.Equal(nf1508Unit(t, exec), before) { + t.Errorf("the refused statement wrote the nanoflow:\n%s", describe()) + } + } + t.Run("the reported if/else", func(t *testing.T) { refused(t, nf1508Modify) }) + // A refusal is new meaning, so it applies under mdl 1 only (ADR-0011). + // Under mdl 0 the refused splice would fall back to rebuilding the whole + // flow — losing the merges, curves and $IDs the shift keeps — so a + // headerless script is spliced as it was. + t.Run("mdl 0 splices as before", func(t *testing.T) { + exec, out, _ := setup(t) + out.Reset() + if err := agreeExec(t, exec, strings.TrimPrefix(nf1508Modify, "mdl 1;\n")); err != nil { + t.Fatalf("%v\n%s", err, out.String()) + } + if said := out.String(); strings.Contains(said, "MDL-V1-REBUILD") || + !strings.Contains(said, "Modified nanoflow: MyFirstModule.NF_Do (spliced:") { + t.Errorf("want the headerless script spliced, not rebuilt, got:\n%s", said) + } + }) + // With no else the new if took over the stored decision (the same if but + // for its condition) and the restated `if $Flag` was inserted on top of it. + t.Run("a plain if", func(t *testing.T) { + refused(t, strings.Replace(nf1508Modify, " else\n log info node 'Probe' 'y';\n", "", 1)) + }) + + // The way out the refusal names: leave the positions after the insert + // unstated, so they can move along. Everything moves by the same amount — + // the replaced calls with their decision — and re-running reports Unchanged. + t.Run("control: no position stated after the insert", func(t *testing.T) { + exec, out, describe := setup(t) + free := nf1508Modify + for _, line := range []string{" @position(-299, 200)\n", " @merge(186, 200)\n", " @position(-124, 100)\n", + " @position(60, 100)\n", " @position(530, 200)\n"} { + free = strings.Replace(free, line, "", 1) + } + out.Reset() + if err := agreeExec(t, exec, free); err != nil { + t.Fatalf("%v\n%s", err, out.String()) + } + if !strings.Contains(out.String(), "Modified nanoflow: MyFirstModule.NF_Do") { + t.Errorf("want the nanoflow modified, got:\n%s", out.String()) + } + v := parseFlowView(t, nf1508Unit(t, exec)) + if v.objectAt(t, -500, 200, "Microflows$ActionActivity") == "" { + t.Errorf("the declare the script draws at (-500, 200) moved:\n%s", describe()) + } + split, merge, branch := nf1508Branch(t, v, "$Flag") + if len(branch) != 2 { + t.Fatalf("`if $Flag` has %d activities in its branch, want 2:\n%s", len(branch), describe()) + } + for _, x := range branch { + if x <= split || x >= merge { + t.Errorf("a branch activity of `if $Flag` is at x=%d, outside its decision (%d) and merge (%d):\n%s", + x, split, merge, describe()) + } + } + out.Reset() + if err := agreeExec(t, exec, free); err != nil { + t.Fatalf("re-run: %v\n%s", err, out.String()) + } + if !strings.Contains(out.String(), "Unchanged nanoflow: MyFirstModule.NF_Do") { + t.Errorf("re-run: want Unchanged, got:\n%s", out.String()) + } + }) +} + +// nf1508Unit returns MyFirstModule.NF_Do's stored bytes. +func nf1508Unit(t *testing.T, exec *Executor) []byte { + t.Helper() + ctx := exec.newExecContext(t.Context()) + h, err := getHierarchy(ctx) + if err != nil { + t.Fatal(err) + } + all, err := ctx.Backend.ListNanoflows() + if err != nil { + t.Fatal(err) + } + for _, n := range all { + if n.Name == "NF_Do" && h.GetModuleName(h.FindModuleID(n.ContainerID)) == "MyFirstModule" { + raw, err := ctx.Backend.GetRawUnitBytes(n.ID) + if err != nil { + t.Fatal(err) + } + return append([]byte(nil), raw...) + } + } + t.Fatal("MyFirstModule.NF_Do not found") + return nil +} + +// nf1508Branch returns the x of the decision on cond, of the merge its paths +// join at, and of every activity on those paths. +func nf1508Branch(t *testing.T, v flowView, cond string) (split, merge int, branch []int) { + t.Helper() + var splitID string + for _, id := range v.order { + o := v.objs[id] + if afGet(o, "$Type") != "Microflows$ExclusiveSplit" { + continue + } + if sc, _ := afGet(o, "SplitCondition").(bson.D); sc != nil && afGet(sc, "Expression") == cond { + splitID = id + split, _ = afPoint(o) + } + } + if splitID == "" { + t.Fatalf("no decision on %s", cond) + } + seen := map[string]bool{splitID: true} + for frontier := []string{splitID}; len(frontier) > 0; frontier = frontier[1:] { + for _, f := range v.flows { + dest := afIDOf(afGet(f, "DestinationPointer")) + if afIDOf(afGet(f, "OriginPointer")) != frontier[0] || seen[dest] { + continue + } + seen[dest] = true + o := v.objs[dest] + x, _ := afPoint(o) + switch afGet(o, "$Type") { + case "Microflows$ExclusiveMerge": + merge = x + case "Microflows$ActionActivity": + branch = append(branch, x) + frontier = append(frontier, dest) + } + } + } + if merge == 0 { + t.Fatalf("the decision on %s reaches no merge", cond) + } + return split, merge, branch +} From c4f43e80dc3904c93c3a1266aef6767bed59a431 Mon Sep 17 00:00:00 2001 From: Ako Date: Wed, 21 Oct 2026 10:40:00 +0000 Subject: [PATCH 3/4] docs: list the stated-position refusal among mdl 1's splice refusals versions.md enumerates what `create or modify` refuses under mdl 1 where mdl 0 rebuilds; add the insert that needs room past a node the script places. The review command gains the pattern that #1508 case 6 hit: a new splice refusal changes mdl 0 into a rebuild unless it is gated on the language version. Co-Authored-By: Claude Opus 5.5 --- .claude/commands/mxcli-dev/review.md | 1 + docs-site/src/language/versions.md | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.claude/commands/mxcli-dev/review.md b/.claude/commands/mxcli-dev/review.md index 3ef98554b9..76fa4079d5 100644 --- a/.claude/commands/mxcli-dev/review.md +++ b/.claude/commands/mxcli-dev/review.md @@ -52,6 +52,7 @@ proactively. Add a row after every review that surfaces something new. | 21 | A registry that reads only one source (an `embed.FS`) has no extension point, so the only way to customise its output is to hand-edit the generated block — which a digest fence then refuses to touch on the next run. The feature ends up hostile to the exact case it was built for | API design | When a guard refuses a user's edit, ask whether the thing being edited should have been authorable. Introduce a source abstraction (`fs.FS` + root) and let a project-local directory shadow the embedded set — the walk functions usually already take a root, so the change is contained | | 22 | A test helper that expands a template cross-products every list against every reference (e.g. every `@each $weight` list against every `url()`), inventing artefacts no asset ever shipped and failing on correct input | Test coverage | Scan positionally: a reference must be expanded against the loop it actually sits under. Prove the helper both fires on a real break (delete one shipped file) and stays quiet on correct input — a helper only checked against green code has not been shown to detect anything | | 23 | A copy-to-scaffold path renames files but not the identifiers built from the name (`@mixin mxcli--`, `@import "mxcli-"`), so two artefacts collide the moment both exist — and the symptom is a rule that silently compiles to nothing | Code correctness | Assert the structural contract on the *generated* artefact, not just the shipped ones: factor the built-in's contract test into a helper and run the scaffold through it. Verify once end to end against the real toolchain and record it | +| 24 | A new splice refusal routed through the shared `cannotSplice` path changes **mdl 0** too: there a refusal falls back to the whole-flow rebuild, so a headerless script that used to splice now loses its merges, curves and `$ID`s — a silent change of meaning ADR-0011 rules out (#1508 case 6, caught by `TestFlowModify_InsertIsSpliced`) | Language versioning | Gate the new refusal on `flowRebuildRefused.Applies(ctx.LanguageVersion)` so mdl 0 keeps its old splice; add an mdl 0 case asserting "spliced, no MDL-V1-REBUILD" beside the mdl 1 refusal test | | 24 | A stacked PR's base merged at an older head: commits pushed to the base branch after its merge never reach `main`, while both PRs read as fine (ako/mxcli#1108 lost its last two commits this way) | Merge hygiene | Before merging any stacked PR, and after pushing to a branch whose PR may already be merged, check each commit with `git merge-base --is-ancestor origin/main`; retarget the next PR to `main` so it carries the stranded commits, and say so in its description | | 24 | A writer leaves out a property Studio Pro always writes (an empty `Documentation`, the empty half of an attribute/association pair) or writes one Mendix does not declare. `mx check`, the build and the runtime stay green, but Mendix's merge/diff engine compares property NAMES and throws, so the document becomes unmergeable once Studio Pro saves it (mendixlabs/mxcli#1373) | BSON writing | The doctype gate's `checkPropertySets` catches it per script: `mx convert -p` re-serializes what is not canonical. Fix the writer, or record Mendix's value with `MXCLI_PROPERTY_SETS_RECORD` + `internal/propertysets/cmd/propdefaults`; never guess a default | | 24 | A skill or command still instructs work into a package the repo has deleted (`sdk/mpr` test locations, a symptom table moved to `findings/*.jsonl` years prior) — the doc reads as authoritative and every instruction in it is a compile error or a no-op | Docs quality | When a package is deleted or a doc is restructured, grep `.claude/` for its name in the same PR. A deletion that leaves the guidance behind is worse than no guidance, because the reader trusts it | diff --git a/docs-site/src/language/versions.md b/docs-site/src/language/versions.md index 0fa153f21a..01e6dd4a62 100644 --- a/docs-site/src/language/versions.md +++ b/docs-site/src/language/versions.md @@ -196,7 +196,7 @@ A construct with no mechanical rewrite is reported with the reason, and `fmt` re `-p app.mpr` also settles a bare `commit $X;` in a `create or modify microflow|nanoflow`, with or without the header. Since mendixlabs/mxcli#895 a bare commit means *with* events, Studio Pro's default; an older mxcli stored the same statement *without* events. Where the stored flow commits the variable without events, `fmt --upgrade -p app.mpr` writes `commit $X without events;`, so re-running the script keeps what is stored instead of turning the event handlers on (or, inside a loop under `mdl 1`, being refused). A new flow, a new commit, or a stored commit with events is left as written; where the stored flow commits the variable both ways, the statement is left and reported. Without `-p`, `fmt` prints a note (`MDL067`) for each flow with a bare commit. -`-p app.mpr` also tells the upgrade which statements `exec` would refuse once the header is there. Under `mdl 1` a `create or modify microflow|nanoflow` of a stored flow is applied as a patch, and a change the patch cannot make (inside a loop body anything but an activity replaced by one of the same kind, an error handler, a redrawn connector, a `return` added) is refused with nothing written; without the header the same statement rebuilds the whole flow (`MDL-V1-REBUILD`). No rewrite keeps that meaning, so `fmt` names each such statement and leaves the header off that file, applying the rest of the upgrade. Change those flows with `alter`, or drop and create them, and upgrade again; `--force-header` adds the header regardless. `mxcli check script.mdl -p app.mpr` reports the same statements (an `MDL-V1-REBUILD` error under `mdl 1`, the warning without the header), computed by the same code as `exec` and `diff`. +`-p app.mpr` also tells the upgrade which statements `exec` would refuse once the header is there. Under `mdl 1` a `create or modify microflow|nanoflow` of a stored flow is applied as a patch, and a change the patch cannot make (inside a loop body anything but an activity replaced by one of the same kind, an error handler, a redrawn connector, a `return` added, a statement inserted without `@position` that needs room past a node the script places) is refused with nothing written; without the header the same statement rebuilds the whole flow (`MDL-V1-REBUILD`). No rewrite keeps that meaning, so `fmt` names each such statement and leaves the header off that file, applying the rest of the upgrade. Change those flows with `alter`, or drop and create them, and upgrade again; `--force-header` adds the header regardless. `mxcli check script.mdl -p app.mpr` reports the same statements (an `MDL-V1-REBUILD` error under `mdl 1`, the warning without the header), computed by the same code as `exec` and `diff`. ## Behaviour changes to know about From 9442043f133f857a012d251f0de24445dc465779 Mon Sep 17 00:00:00 2001 From: Ako Date: Wed, 21 Oct 2026 13:00:34 +0000 Subject: [PATCH 4/4] fix: a splice making room leaves the flow's parameters where they are MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit shift moved every top-level node past the cut, and a parameter is a top-level object of the stored object collection: one if/else inserted into NF_Do moved `$Flag` from its derived (200, 53) to (539, 53), a longer replace then to (699, 53), and from then on describe printed a parameter @position no script stated. mxcli's derived parameter row is absolute and right of the start event, so almost every insert's cut falls left of it. A parameter is not on the line the room is made on. Studio Pro authors draw it above the start event — 63 of 71 in PedApp and JTSBootLogboek sit at the start's x, none within 60px of the flow line — and an insert in Studio Pro moves nothing. shifted() now leaves parameters out, and with it pinnedPast, so a stated parameter position no longer blocks a splice it is not in the way of. Fixes ako/mxcli#1547. Co-Authored-By: Claude Opus 5.5 --- ...6-10-21-splice-shift-moves-parameters.json | 1 + ...geometry-1547-splice-leaves-parameters.mdl | 35 +++++++++++++ mdl/backend/mfmutator/splice.go | 19 ++++--- .../mfmutator/splice_parameters_test.go | 50 +++++++++++++++++++ .../flow_modify_stated_positions_1508_test.go | 9 ++++ 5 files changed, 108 insertions(+), 6 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-backend/2026-10-21-splice-shift-moves-parameters.json create mode 100644 mdl-examples/bug-tests/flow-geometry-1547-splice-leaves-parameters.mdl create mode 100644 mdl/backend/mfmutator/splice_parameters_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-backend/2026-10-21-splice-shift-moves-parameters.json b/.claude/skills/fix-issue/findings/mdl-backend/2026-10-21-splice-shift-moves-parameters.json new file mode 100644 index 0000000000..c7daa32352 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-backend/2026-10-21-splice-shift-moves-parameters.json @@ -0,0 +1 @@ +{"area": "mdl/backend", "date": "2026-10-21", "symptom": "A splice that makes room for an inserted fragment (create or modify / alter … insert) moved the flow's parameters along with the row: one if/else inserted into NF_Do moved $Flag from its derived (200, 53) to (539, 53), a longer replace then to (699, 53), and from then on describe printed a parameter @position no script stated. Inserting before the first statement of a two-parameter microflow moved $Count from (300, 53) to (410, 53).", "cause": "mfmutator shift (splice.go) moves every top-level node past the cut, and a Microflows$MicroflowParameter is a top-level object of the unit's object collection (the decoded model keeps parameters apart, so nothing in the executor sees them as nodes). mxcli's derived parameter row is absolute (200 + idx*pitch, 53), right of the start event, so almost every insert's cut falls left of it.", "file": "mdl/backend/mfmutator/splice.go (shifted)", "insight": "Measure the authored convention before deciding what 'the row' is: 63 of 71 Studio Pro-authored parameters (PedApp 19/20, JTSBootLogboek 44/51) sit exactly at the start event's x, all above every other node, none within 60px of the flow line — a parameter belongs to the flow, not to the stretch of line past an insert, and Studio Pro moves nothing on insert. The decoded model hides this: microflows.Microflow carries Parameters apart from ObjectCollection, so a probe that type-switches the collection for *MicroflowParameter does not compile, while the BSON the mutator walks has them inline. Excluding them in shifted also keeps them out of pinnedPast, so a stated parameter position can no longer block a splice it is not in the way of.", "refs": ["ako/mxcli#1547", "ako/mxcli#1508"]} diff --git a/mdl-examples/bug-tests/flow-geometry-1547-splice-leaves-parameters.mdl b/mdl-examples/bug-tests/flow-geometry-1547-splice-leaves-parameters.mdl new file mode 100644 index 0000000000..5b6f8b2c9b --- /dev/null +++ b/mdl-examples/bug-tests/flow-geometry-1547-splice-leaves-parameters.mdl @@ -0,0 +1,35 @@ +mdl 1; +-- ============================================================================ +-- Bug: a splice making room moved the flow's parameters along with the row +-- (ako/mxcli#1547) +-- ============================================================================ +-- +-- The second statement inserts a log before the first one, so the splice +-- makes room by moving everything after the start event along. Before the fix +-- that included the two parameters, which mxcli draws on a row above the flow +-- (x 200 and 300, y 53): both moved right, and from then on describe printed a +-- @position on each that no script ever stated, the row drifting further with +-- every insert. +-- +-- Fixed: a parameter is not on the line the room is made on, so it stays +-- where it is. Studio Pro authors draw a parameter above the start event, and +-- inserting an activity in Studio Pro moves nothing. +-- +-- Verify: exec, then +-- mxcli -p app.mpr -c "describe microflow MyFirstModule.ACT_ParamRow" +-- Expected: the parameters carry no @position (they are still on the derived +-- row), and the new log is first. +-- ============================================================================ + +create or modify microflow MyFirstModule.ACT_ParamRow ($Name: String, $Count: Integer) +begin + log info node 'Probe' 'first ' + $Name; + log info node 'Probe' 'second ' + toString($Count); +end; + +create or modify microflow MyFirstModule.ACT_ParamRow ($Name: String, $Count: Integer) +begin + log info node 'Probe' 'inserted'; + log info node 'Probe' 'first ' + $Name; + log info node 'Probe' 'second ' + toString($Count); +end; diff --git a/mdl/backend/mfmutator/splice.go b/mdl/backend/mfmutator/splice.go index b28ff3e354..c0ce3e42a1 100644 --- a/mdl/backend/mfmutator/splice.go +++ b/mdl/backend/mfmutator/splice.go @@ -881,10 +881,11 @@ func (m *Mutator) markRemoved(v any) { } // shift moves every top-level node past the cut (along axis ax, in the -// direction of s) by delta. The sweep keeps everything on either side of the -// cut exactly as it was relative to its neighbours, so only the flows that -// cross the cut get longer; a flow's control vectors are relative to its ends, -// so no curve changes. skip is a node that is about to be removed. +// direction of s) by delta, but the parameters (shifted). The sweep keeps +// everything on either side of the cut exactly as it was relative to its +// neighbours, so only the flows that cross the cut get longer; a flow's +// control vectors are relative to its ends, so no curve changes. skip is a +// node that is about to be removed. func (m *Mutator) shift(g *graph, ax axis, s int, cut float64, delta int, skip string) { for _, n := range g.order { if !shifted(n, ax, s, cut, skip) { @@ -895,9 +896,15 @@ func (m *Mutator) shift(g *graph, ax axis, s int, cut float64, delta int, skip s } } -// shifted reports whether shift moves n: a top-level node past the cut. +// shifted reports whether shift moves n: a top-level node past the cut that +// is not a parameter. A parameter is not on the line the room is made on: it +// belongs to the flow as a whole, and Studio Pro authors draw it above the +// start event (63 of 71 in PedApp and JTSBootLogboek at the start's x, none +// within 60px of the line). Moved, it drifted right with every insert and +// describe printed a @position the script never stated (ako/mxcli#1547). func shifted(n *node, ax axis, s int, cut float64, skip string) bool { - return n.loop == "" && n.id != skip && float64(s)*(float64(n.pos.get(ax))-cut) > 0 + return n.loop == "" && n.id != skip && n.typ != "Microflows$MicroflowParameter" && + float64(s)*(float64(n.pos.get(ax))-cut) > 0 } // pinnedPast returns the first node shift would move that is drawn at a point diff --git a/mdl/backend/mfmutator/splice_parameters_test.go b/mdl/backend/mfmutator/splice_parameters_test.go new file mode 100644 index 0000000000..96c2b74ee4 --- /dev/null +++ b/mdl/backend/mfmutator/splice_parameters_test.go @@ -0,0 +1,50 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mfmutator + +import ( + "testing" + + "github.com/mendixlabs/mxcli/model" +) + +// A parameter is not on the line a fragment is spliced into, so making room +// does not move it (ako/mxcli#1547). shift moved every top-level node past the +// cut, parameters included: one inserted if/else moved a nanoflow's `$Flag` +// from its derived (200, 53) to (539, 53), and from then on describe printed a +// @position the script never stated, the row drifting right with every insert. +// Studio Pro authors draw a parameter above the start event — 63 of 71 in +// PedApp and JTSBootLogboek sit at the start's x, none within 60px of the line +// — and an insert in Studio Pro moves nothing. The control is the activity +// after the gap, which does move. +func TestSplice_MakingRoomLeavesTheParameters(t *testing.T) { + positions := func(m *Mutator) map[string]point { + out := map[string]point{} + for _, n := range m.graph().order { + out[n.id] = n.pos + } + return out + } + objs := append(line(), obj("param", "Microflows$MicroflowParameter", 500, 53)) + for _, c := range []struct { + name string + splice func(m *Mutator) error + }{ + {"an insert", func(m *Mutator) error { return m.InsertAfter(model.ID(uid("a")), oneActivity()) }}, + {"a replace longer than what it replaces", func(m *Mutator) error { return m.Replace(model.ID(uid("a")), twoActivities()) }}, + } { + t.Run(c.name, func(t *testing.T) { + m, _ := newMutator(t, unit(objs, lineFlows())) + if err := c.splice(m); err != nil { + t.Fatal(err) + } + got := positions(m) + if p := got[uid("b")]; p.X <= 420 { + t.Fatalf("control: b is at x=%d, want it moved along past 420 to make room", p.X) + } + if p := got[uid("param")]; p != (point{500, 53}) { + t.Errorf("the parameter moved to (%d, %d), want it left at (500, 53)", p.X, p.Y) + } + }) + } +} diff --git a/mdl/executor/flow_modify_stated_positions_1508_test.go b/mdl/executor/flow_modify_stated_positions_1508_test.go index bb4a3cf40d..9ad10bf23c 100644 --- a/mdl/executor/flow_modify_stated_positions_1508_test.go +++ b/mdl/executor/flow_modify_stated_positions_1508_test.go @@ -160,6 +160,15 @@ func TestCreateOrModify_InsertDoesNotMoveStatedNodes1508(t *testing.T) { if v.objectAt(t, -500, 200, "Microflows$ActionActivity") == "" { t.Errorf("the declare the script draws at (-500, 200) moved:\n%s", describe()) } + // The parameter is not on the line the room was made on: it stays on + // its derived row (ako/mxcli#1547), where it moved to (539, 53). + for _, id := range v.order { + if o := v.objs[id]; afGet(o, "$Type") == "Microflows$MicroflowParameter" { + if x, y := afPoint(o); x != 200 || y != 53 { + t.Errorf("$Flag moved to (%d, %d), want it left at (200, 53):\n%s", x, y, describe()) + } + } + } split, merge, branch := nf1508Branch(t, v, "$Flag") if len(branch) != 2 { t.Fatalf("`if $Flag` has %d activities in its branch, want 2:\n%s", len(branch), describe())