From 7887cb9d0505f7a700d76b3daa54ccc18eedf456 Mon Sep 17 00:00:00 2001 From: "meisam.mahdian" Date: Wed, 11 Nov 2026 14:28:26 +0100 Subject: [PATCH 01/24] fix: use building block action override rebinds a pluggable button (closes #1821) `use building block PlatformDesignSystem.Button (action: ...)` failed with "action override supplied, but the block has no button to rebind" on every PlatformDesignSystem block: they are built from the pluggable pdsbutton, and the target predicate matched only actionbutton, linkbutton and button. A pluggable widget is now a target when its definition names it a button and maps a slot to OnClick. The engine already writes that slot from the Action property the override sets. A click slot alone is not enough: a PDS Link, a Gallery and a Data grid 2 take OnClick too, and on a PDS page header the first clickable widget is the "Back to" link. With no project there is no registry, so only the built-in types match, as before. Co-Authored-By: Claude Code --- ...1-bb-action-override-pluggable-button.json | 1 + .claude/skills/mendix/fragments/SKILL.md | 15 ++-- CHANGELOG.md | 1 + cmd/mxcli/syntax/features_page.go | 2 +- .../15c-fragment-bindings-examples.mdl | 6 +- .../cmd_pages_builder_bb_rebind_test.go | 76 +++++++++++++++++++ mdl/executor/cmd_pages_builder_v3.go | 47 +++++++++++- 7 files changed, 138 insertions(+), 10 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-11-11-bb-action-override-pluggable-button.json diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-11-11-bb-action-override-pluggable-button.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-11-11-bb-action-override-pluggable-button.json new file mode 100644 index 000000000..16c0e7503 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-11-11-bb-action-override-pluggable-button.json @@ -0,0 +1 @@ +{"area": "mdl/executor", "date": "2026-11-11", "symptom": "mendixlabs/mxcli#1821: `use building block PlatformDesignSystem.Button (action: call microflow M.Act) as b_` failed with 'action override supplied, but the block has no button to rebind', on every PlatformDesignSystem block (Marketplace 44.4.0, Mendix 11.12.6, mxcli v0.31.0).", "cause": "The action override's target predicate isButtonWidget matched only the built-in button types (actionbutton, linkbutton, button). The PDS blocks are built from the pluggable pdsbutton, so no widget matched, although the engine already writes a pluggable widget's on-click slot from the same Action property the override sets.", "file": "mdl/executor/cmd_pages_builder_v3.go (isPluggableButtonWidget, the ActionOverride branch of the building-block expansion)", "insight": "A pluggable widget is a button target when its definition names it a button AND maps a slot to OnClick. The click slot alone is not enough: a Gallery and a Data grid 2 take OnClick for a row click and a PDS Link for a link click, and on a PDS page header the first clickable widget is the 'Back to' link, not the primary button. With no project there is no registry, and only the built-in types match, as before.", "refs": ["mendixlabs/mxcli#1821"], "test": "mdl/executor/cmd_pages_builder_bb_rebind_test.go"} diff --git a/.claude/skills/mendix/fragments/SKILL.md b/.claude/skills/mendix/fragments/SKILL.md index 829dbf66d..5f79d67f3 100644 --- a/.claude/skills/mendix/fragments/SKILL.md +++ b/.claude/skills/mendix/fragments/SKILL.md @@ -12,11 +12,11 @@ description: "Define reusable widget groups with CREATE FRAGMENT and place them |---|---| | 24-32 | When to Use This Skill | | 33-40 | What Are Fragments? | -| 41-189 | Syntax Reference | -| 190-259 | Common Patterns | -| 260-306 | Common Mistakes | -| 307-316 | Validation Checklist | -| 317-322 | Related Documentation | +| 41-192 | Syntax Reference | +| 193-262 | Common Patterns | +| 263-309 | Common Mistakes | +| 310-319 | Validation Checklist | +| 320-325 | Related Documentation | # Mendix Fragments Skill @@ -170,7 +170,10 @@ use building block Atlas_Web_Content.List_Cards ``` Binding-point rule (prototype): datasource → the first widget carrying a -datasource; action → the first button widget. For anything more specific, copy +datasource; action → the first button widget. A pluggable button counts as a +button — one whose definition names it a button and takes `OnClick:`, such as +the `pdsbutton` the PlatformDesignSystem blocks are built from; a PDS Link or a +gallery row click does not. For anything more specific, copy the block in with `as prefix_` and use `alter page … set … on prefix_widget`. ### LIST FRAGMENTS diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a277e5b8..5667bed30 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **`use building block … (action: …)` rebinds a pluggable button** (mendixlabs/mxcli#1821) — every block of the PlatformDesignSystem Marketplace module was refused with "action override supplied, but the block has no button to rebind", because the PDS blocks are built from the pluggable `pdsbutton` and the override looked only for the built-in button types. A pluggable widget is now a target when its definition names it a button and takes `OnClick:`, so the action lands on the block's first such button, in its own on-click slot. A PDS Link and a gallery's or data grid's row click are not buttons and are skipped: on a PDS page header the action goes to the primary button, not to the "Back to" link before it. Without a project, only the built-in types match, as before. Reproduced on Mendix 11.12.6 with PlatformDesignSystem 44.4.0. - **`exec` and `check` no longer rescan an MPR v1 project for every lookup** (ako/mxcli#2094, mendixlabs/mxcli#1795) — each statement cost 0.3–0.9 s on a large single-file project, whatever it changed: every typed listing re-read the metadata of the whole `Unit` table, and every lookup by name read every microflow's contents. The reader now keeps the scan, and each unit's name, until a write or another process's commit says the table may have changed. On a 3,400-unit project a `create microflow` went from 0.30 s to 0.11 s, a one-activity `create or modify` from 0.86 s to 0.35 s, and `check` of 150 such statements from 76 s to 15 s. MPR v2 projects already kept this index. - **`mxcli test --local` beside a `run --local --test-endpoint` dev loop runs, and leaves the project buildable** (ako/mxcli#2119) — the test run read the dev loop's `MxTest.RegisterEndpoint` as the project's after-startup microflow and chained its own endpoint to it, to itself: the scratch runtime failed to boot with a `StackOverflowError` and no test ran. Its cleanup then dropped the dev loop's endpoint while the setting still named it, so the dev loop's next build failed with CE1613. A live host's restore record now says what the endpoint replaced, and the test run chains to that; its cleanup removes only its test microflows and leaves the dev loop's endpoint, Java source and setting as it found them. An endpoint whose run died is removed first, as `run` does, and one with no record of what it replaced is refused rather than chained to. When the test run rebuilt the `deployment/` folder both share, the dev loop builds its own back once the run ends (a reload or a restart), so a following `--attach` is not refused for a restart. - **A test run no longer rebuilds or restarts the dev loop it shares a project with** (ako/mxcli#2063, field test on ako/mxcli#2104) — `mxcli test --local`, and `--attach` with a test that has to be built, write their test microflows into the project a `run --local --watch` dev loop is watching, so the dev loop rebuilt for the injection and its cleanup, and applied it by restarting the runtime, which drops every browser session, `--hub` preview included; on a small app the restart itself failed (`EOFException`). The test run now holds the dev loop's watcher (`.mxcli/watch-hold.json`) while it owns the project, `--attach` now puts the project file back byte for byte as `--local` already did, and on release the watcher compares the model with the one the hold began from: the same model is not built at all, and `run status` / `run wait` read it as covered. An edit made before or during the run is still built once it ends, and a hold whose run died is ignored. diff --git a/cmd/mxcli/syntax/features_page.go b/cmd/mxcli/syntax/features_page.go index 8d32b0c7a..e722cf4c7 100644 --- a/cmd/mxcli/syntax/features_page.go +++ b/cmd/mxcli/syntax/features_page.go @@ -862,7 +862,7 @@ LIST IMPACT OF htmlelement; "binding", "rebind", "building block override", "reusable component", }, Syntax: "-- Declare typed params, reference with $name in a datasource/action slot:\nCREATE FRAGMENT Name ($data: datasource, $onEdit: action) AS { … };\n-- Supply values at the use site:\nUSE FRAGMENT Name ($data: , $onEdit: ) [{ payload }]\n-- Building blocks: rebind the outermost datasource / first button:\nUSE BUILDING BLOCK Module.Block (datasource: , action: ) [AS prefix_]", - Example: "CREATE FRAGMENT DataPanel ($data: datasource, $onEdit: action) AS {\n CONTAINER panel (Class: 'card') {\n LISTVIEW lv (DataSource: $data) {\n SLOT content\n ACTIONBUTTON edit (Caption: 'Edit', Action: $onEdit, ButtonStyle: Primary)\n }\n }\n};\n\nCREATE PAGE Module.Orders (Title: 'Orders', Layout: Atlas_Core.Atlas_Default) {\n USE FRAGMENT DataPanel ($data: DATABASE Sales.Order, $onEdit: MICROFLOW Sales.Edit) {\n DYNAMICTEXT heading (Content: 'Orders', RenderMode: H4)\n }\n};\n\n-- Rebind a building block's datasource and primary button:\nUSE BUILDING BLOCK Atlas_Web_Content.List_Cards (datasource: DATABASE Sales.Order, action: CALL MICROFLOW Sales.Open) AS orders_\n\n-- Notes:\n-- * Param kinds: datasource | action. Every declared param must be supplied.\n-- * A microflow value parses as a datasource and is reinterpreted for an action param.\n-- * BB binding-point rule: datasource → first datasource widget; action → first button.", + Example: "CREATE FRAGMENT DataPanel ($data: datasource, $onEdit: action) AS {\n CONTAINER panel (Class: 'card') {\n LISTVIEW lv (DataSource: $data) {\n SLOT content\n ACTIONBUTTON edit (Caption: 'Edit', Action: $onEdit, ButtonStyle: Primary)\n }\n }\n};\n\nCREATE PAGE Module.Orders (Title: 'Orders', Layout: Atlas_Core.Atlas_Default) {\n USE FRAGMENT DataPanel ($data: DATABASE Sales.Order, $onEdit: MICROFLOW Sales.Edit) {\n DYNAMICTEXT heading (Content: 'Orders', RenderMode: H4)\n }\n};\n\n-- Rebind a building block's datasource and primary button:\nUSE BUILDING BLOCK Atlas_Web_Content.List_Cards (datasource: DATABASE Sales.Order, action: CALL MICROFLOW Sales.Open) AS orders_\n\n-- Notes:\n-- * Param kinds: datasource | action. Every declared param must be supplied.\n-- * A microflow value parses as a datasource and is reinterpreted for an action param.\n-- * BB binding-point rule: datasource → first datasource widget; action → first button (a pluggable button such as pdsbutton counts).", SeeAlso: []string{"fragment", "fragment.slot", "fragment.use"}, }) diff --git a/mdl-examples/doctype-tests/15c-fragment-bindings-examples.mdl b/mdl-examples/doctype-tests/15c-fragment-bindings-examples.mdl index 278bb9a48..99d54b6a7 100644 --- a/mdl-examples/doctype-tests/15c-fragment-bindings-examples.mdl +++ b/mdl-examples/doctype-tests/15c-fragment-bindings-examples.mdl @@ -83,7 +83,11 @@ describe page BindTest.P001_Bound_Panel; -- Binding-point rule (prototype): -- datasource → the first widget carrying a datasource -- action → the first button widget (Atlas placeholder buttons ship with --- no action, so the target is matched by widget type) +-- no action, so the target is matched by widget type). A +-- pluggable button counts too — a widget whose definition names +-- it a button and takes `OnClick:`, such as the PDS Button the +-- PlatformDesignSystem blocks are built from. A PDS Link or a +-- gallery row click is not a button and is skipped. -- -- Requires the Atlas_Web_Content.List_Cards building block, which ships with the -- Atlas UI in Mendix 11.x (it is absent from the 10.x Atlas), so this section is diff --git a/mdl/executor/cmd_pages_builder_bb_rebind_test.go b/mdl/executor/cmd_pages_builder_bb_rebind_test.go index 09fdcf880..744f1144d 100644 --- a/mdl/executor/cmd_pages_builder_bb_rebind_test.go +++ b/mdl/executor/cmd_pages_builder_bb_rebind_test.go @@ -53,3 +53,79 @@ func TestBuildingBlockRebindPrefersTheOutermost(t *testing.T) { t.Errorf("rebound %q, want the outermost datasource widget", got) } } + +// pluggableClickRegistry holds definitions shaped like the PlatformDesignSystem +// widgets a PDS block is built from: a button and a link that both source their +// `onClickAction` slot from OnClick, a gallery whose row click does the same, and +// a "button" whose definition maps no click slot at all. +func pluggableClickRegistry() *WidgetRegistry { + onClick := []PropertyMapping{{PropertyKey: "onClickAction", Source: "OnClick", Operation: "action"}} + return &WidgetRegistry{byMDLName: map[string]*WidgetDefinition{ + "PDSBUTTON": {WidgetID: "mendix.pdsbutton.PDSButton", MDLName: "pdsbutton", PropertyMappings: onClick}, + "PDSLINK": {WidgetID: "mendix.pdslink.PDSLink", MDLName: "pdslink", PropertyMappings: onClick}, + "GALLERY": {WidgetID: "com.mendix.widget.web.gallery.Gallery", MDLName: "gallery", PropertyMappings: onClick}, + "NOCLICKBUTTON": {WidgetID: "x.noclickbutton.NoClickButton", MDLName: "noclickbutton"}, + "MODECLICKBUTTON": {WidgetID: "x.modeclickbutton.ModeClickButton", MDLName: "modeclickbutton", Modes: []WidgetMode{{Name: "default", PropertyMappings: onClick}}}, + }} +} + +// TestBuildingBlockActionRebindFindsPluggableButtons: `use building block +// PlatformDesignSystem.Button (action: …)` failed with "the block has no button +// to rebind", because the PDS blocks are built from the pluggable `pdsbutton` +// and the action target matched only the built-in button types. Measured on +// mxcli v0.31.0, Mendix 11.12.6, PlatformDesignSystem 44.4.0. +func TestBuildingBlockActionRebindFindsPluggableButtons(t *testing.T) { + reg := pluggableClickRegistry() + for _, widgetType := range []string{"pdsbutton", "PDSBUTTON", "modeclickbutton"} { + t.Run(widgetType, func(t *testing.T) { + if !isActionTargetWidget(reg, &ast.WidgetV3{Type: widgetType}) { + t.Errorf("%s is not an action target", widgetType) + } + }) + } + // The built-in buttons stay targets, with or without a registry. + for _, widgetType := range []string{"actionbutton", "linkbutton"} { + if !isActionTargetWidget(nil, &ast.WidgetV3{Type: widgetType}) { + t.Errorf("%s is no longer an action target", widgetType) + } + } + // A click slot alone is not a button: a link click or a gallery row click is + // not the block's primary action. Nor is a button that takes no action. + for _, widgetType := range []string{"pdslink", "gallery", "noclickbutton", "container", "unknownbutton"} { + t.Run("not/"+widgetType, func(t *testing.T) { + if isActionTargetWidget(reg, &ast.WidgetV3{Type: widgetType}) { + t.Errorf("%s must not be an action target", widgetType) + } + }) + } + // No project, no definitions: only the built-in button types match, as before. + if isActionTargetWidget(nil, &ast.WidgetV3{Type: "pdsbutton"}) { + t.Error("with no registry, a pdsbutton must not match") + } +} + +// TestBuildingBlockActionRebindSkipsLinksBeforeTheButton pins the binding point +// on a PDS page header: its first clickable widget is a "Back to …" PDS Link, and +// the action belongs on the first button after it. +func TestBuildingBlockActionRebindSkipsLinksBeforeTheButton(t *testing.T) { + reg := pluggableClickRegistry() + back := &ast.WidgetV3{Type: "pdslink", Name: "pDSLink1", Properties: map[string]any{}} + primary := &ast.WidgetV3{Type: "pdsbutton", Name: "pDSButton3", Properties: map[string]any{}} + secondary := &ast.WidgetV3{Type: "pdsbutton", Name: "pDSButton4", Properties: map[string]any{}} + header := &ast.WidgetV3{Type: "container", Name: "container2", + Children: []*ast.WidgetV3{back, {Type: "container", Name: "container6", Children: []*ast.WidgetV3{primary, secondary}}}} + + act := &ast.ActionV3{} + hit := rebindFirst([]*ast.WidgetV3{header}, + func(w *ast.WidgetV3) bool { return isActionTargetWidget(reg, w) }, + func(w *ast.WidgetV3) { w.Properties["Action"] = act }) + if !hit { + t.Fatal("no target found") + } + if primary.Properties["Action"] != act { + t.Errorf("the action did not land on the first PDS button") + } + if back.Properties["Action"] != nil || secondary.Properties["Action"] != nil { + t.Errorf("the action landed on a widget other than the first PDS button") + } +} diff --git a/mdl/executor/cmd_pages_builder_v3.go b/mdl/executor/cmd_pages_builder_v3.go index ebfc599e7..5614b64e7 100644 --- a/mdl/executor/cmd_pages_builder_v3.go +++ b/mdl/executor/cmd_pages_builder_v3.go @@ -3259,8 +3259,12 @@ func (pb *pageBuilder) expandBuildingBlockRef(w *ast.WidgetV3) ([]*ast.WidgetV3, if act, ok := w.Properties["ActionOverride"].(*ast.ActionV3); ok && act != nil { // Action target: the first button-type widget. Atlas blocks ship // placeholder buttons with no action, so match by widget type (not by an - // existing Action property) and set the handler. - hit := rebindFirst(widgets, isButtonWidget, + // existing Action property) and set the handler. A pluggable button (the + // PlatformDesignSystem blocks are built from `pdsbutton`) counts too: its + // on-click slot is sourced from the same Action property, so setting it + // is enough for the engine to write the widget's own action key. + registry := ctx.GetWidgetRegistry() + hit := rebindFirst(widgets, func(t *ast.WidgetV3) bool { return isActionTargetWidget(registry, t) }, func(t *ast.WidgetV3) { t.Properties["Action"] = act }) if !hit { return nil, mdlerrors.NewValidation(fmt.Sprintf( @@ -3270,6 +3274,45 @@ func (pb *pageBuilder) expandBuildingBlockRef(w *ast.WidgetV3) ([]*ast.WidgetV3, return widgets, nil } +// isActionTargetWidget reports whether the `action:` override of `use building +// block` may land on a widget: a built-in button, or a pluggable one. +func isActionTargetWidget(registry *WidgetRegistry, w *ast.WidgetV3) bool { + return isButtonWidget(w) || isPluggableButtonWidget(registry, w) +} + +// isPluggableButtonWidget reports whether a widget is a pluggable button: its +// definition names it a button and maps an on-click action slot to `OnClick:`. +// Both conditions, because a click slot alone is not a button — a Gallery and a +// Data grid 2 take `OnClick:` for a row click, and a PDS Link for a link click, +// and none of those is the block's primary action. registry may be nil (no +// project); then only the built-in button types match. +func isPluggableButtonWidget(registry *WidgetRegistry, w *ast.WidgetV3) bool { + if registry == nil { + return false + } + def, ok := registry.Get(w.Type) + if !ok || !strings.HasSuffix(strings.ToLower(def.MDLName), "button") { + return false + } + takesOnClick := func(mappings []PropertyMapping) bool { + for _, m := range mappings { + if m.Source == "OnClick" { + return true + } + } + return false + } + if takesOnClick(def.PropertyMappings) { + return true + } + for i := range def.Modes { + if takesOnClick(def.Modes[i].PropertyMappings) { + return true + } + } + return false +} + // isButtonWidget reports whether a widget is an action-capable button. // isDataSourceWidget reports whether a widget is one that takes a datasource, // whether or not it currently carries one. The list is the data containers a From 72036f264e8c7c8987c084beca10767622700952 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 11 Nov 2026 19:49:25 +0000 Subject: [PATCH 02/24] fix: `check script.mdl` in a project directory checks references although help calls it syntax-only `mxcli check --help` offered "# Check syntax only (no project needed) mxcli check script.mdl", but run from a directory holding an .mpr the root PersistentPreRun auto-detects it ("Using project: ... (auto-detected in the current directory; pass -p to use another)"), and check treats any project as an implicit --references. There was no way to get the advertised syntax-only check there (ako/mxcli#2135, mendixlabs/mxcli#1817). Auto-detection stays: it is the CLI's documented resolution (mendixlabs/mxcli#1444), and `cd app && mxcli check script.mdl` relying on it keeps resolving references. Instead check gains --no-project, which PersistentPreRun honours for any command registering it; the auto-detect notice on check names it; it is refused together with -p, --references or --post-migration. The help now states the default and the opt-out, and its syntax-only example carries the flag, as do the generated CLAUDE.md template, the check-script command, two skills and the docs-site pages that repeated "no project needed". Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82sv2odH8kVjmGzPosB --- .claude/commands/mendix/check-script.md | 4 +- ...tax-only-example-auto-detects-project.json | 1 + .claude/skills/mendix/check-syntax/SKILL.md | 4 +- .../skills/mendix/xpath-constraints/SKILL.md | 4 +- cmd/mxcli/check_no_project_test.go | 116 ++++++++++++++++++ cmd/mxcli/cmd_check.go | 39 +++++- cmd/mxcli/main.go | 37 +++++- cmd/mxcli/tool_templates.go | 2 +- docs-site/src/ide/vscode-syntax.md | 4 +- docs-site/src/language/script-files.md | 4 +- docs-site/src/migration/generation.md | 4 +- docs-site/src/migration/validation.md | 4 +- docs-site/src/tutorial/skills.md | 4 +- docs-site/src/tutorial/validation.md | 8 +- 14 files changed, 209 insertions(+), 26 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/cmd-mxcli/2026-11-11-check-syntax-only-example-auto-detects-project.json create mode 100644 cmd/mxcli/check_no_project_test.go diff --git a/.claude/commands/mendix/check-script.md b/.claude/commands/mendix/check-script.md index 2ad951176..631a4e295 100644 --- a/.claude/commands/mendix/check-script.md +++ b/.claude/commands/mendix/check-script.md @@ -10,8 +10,8 @@ Validate an MDL script before executing it. ## Commands ```bash -# Syntax check only (no project needed) -mxcli check script.mdl +# Syntax check only (--no-project: do not auto-detect an .mpr in this directory) +mxcli check script.mdl --no-project # Syntax + reference validation (checks entities/modules exist) mxcli check script.mdl -p app.mpr --references diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli/2026-11-11-check-syntax-only-example-auto-detects-project.json b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-11-11-check-syntax-only-example-auto-detects-project.json new file mode 100644 index 000000000..b03f06d05 --- /dev/null +++ b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-11-11-check-syntax-only-example-auto-detects-project.json @@ -0,0 +1 @@ +{"area":"cmd/mxcli","date":"2026-11-11","symptom":"`mxcli check --help` offered `# Check syntax only (no project needed) mxcli check script.mdl`, but run from a directory holding an .mpr it printed `Using project: ...\\project.mpr (auto-detected in the current directory; pass -p to use another)` and resolved references against it; there was no way to get the advertised syntax-only check there","cause":"`rootCmd.PersistentPreRun` auto-discovery sets `-p` before `check` runs, and `check` treats any project as an implicit `--references` (the fix for an unqualified `Check passed!`); the help example was only true outside a project directory","file":"`cmd/mxcli/main.go` (`PersistentPreRun`, `noProjectOptOut`/`skipsProjectDiscovery`), `cmd/mxcli/cmd_check.go` (help, `checkNoProjectConflicts`)","insight":"Two individually right decisions (auto-detect the sole .mpr, mendixlabs/mxcli#1444; a project implies reference checks) composed into a help example that is false exactly where users run it — a project directory, which is also where the generated CLAUDE.md template told agents `check script.mdl # Syntax only`. Changing the default would have silently weakened every `cd app && mxcli check` that relies on the auto-detected refs, so the fix is an opt-out (`--no-project`, honoured by PersistentPreRun for any command that registers it), a notice on the auto-detect line naming it, a refusal when combined with -p/--references/--post-migration, and help/docs whose syntax-only example carries the flag. Note `Flags().Set` by auto-detection marks `-p` Changed, so a conflict check on Changed only works because discovery is skipped first. Test runs real main() in a temp dir with a non-project Stray.mpr (opening it would error); control: discovery forced on under --no-project prints the reported `Using project:` line","refs":["ako/mxcli#2135","mendixlabs/mxcli#1817"]} diff --git a/.claude/skills/mendix/check-syntax/SKILL.md b/.claude/skills/mendix/check-syntax/SKILL.md index bc5159011..b6adcadbf 100644 --- a/.claude/skills/mendix/check-syntax/SKILL.md +++ b/.claude/skills/mendix/check-syntax/SKILL.md @@ -409,8 +409,8 @@ Run `mxcli syntax keywords` for the full list of 320+ reserved keywords. **Always run these checks:** ```bash -# Step 1: Syntax check (no project needed) -./bin/mxcli check script.mdl +# Step 1: Syntax check (--no-project: do not auto-detect an .mpr in this directory) +./bin/mxcli check script.mdl --no-project # Step 2: reference validation (needs project) # Validates microflow bodies, entity/enum references, and widget tree references diff --git a/.claude/skills/mendix/xpath-constraints/SKILL.md b/.claude/skills/mendix/xpath-constraints/SKILL.md index a7bb82dea..002b3f897 100644 --- a/.claude/skills/mendix/xpath-constraints/SKILL.md +++ b/.claude/skills/mendix/xpath-constraints/SKILL.md @@ -460,8 +460,8 @@ grant read * on entity Module.Item to Module.User where [System.owner = '[%Curre Always validate XPath syntax before execution: ```bash -# Syntax check (no project needed) -./bin/mxcli check script.mdl +# Syntax check only (--no-project: do not auto-detect an .mpr in this directory) +./bin/mxcli check script.mdl --no-project # with reference validation (needs project) ./bin/mxcli check script.mdl -p app.mpr --references diff --git a/cmd/mxcli/check_no_project_test.go b/cmd/mxcli/check_no_project_test.go new file mode 100644 index 000000000..36246d57d --- /dev/null +++ b/cmd/mxcli/check_no_project_test.go @@ -0,0 +1,116 @@ +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// ako/mxcli#2135 (mendixlabs/mxcli#1817): `mxcli check --help` offered +// +// # Check syntax only (no project needed) +// mxcli check script.mdl +// +// but run from a directory holding an .mpr, that invocation printed +// "Using project: ...\project.mpr (auto-detected in the current directory; pass +// -p to use another)" and resolved every reference against it — the global +// auto-detection sets -p before check runs — and nothing said how to get the +// syntax-only check the help advertised. Auto-detection stays (it is the CLI's +// documented resolution, mendixlabs/mxcli#1444); --no-project is the opt-out. + +// noProjectFixture is a working directory holding exactly one .mpr, which +// auto-detection would pick, and a syntactically valid script. The .mpr is not +// a project, so any attempt to open it shows up as an error. +func noProjectFixture(t *testing.T) string { + t.Helper() + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "Stray.mpr"), []byte("not a project"), 0o644); err != nil { + t.Fatal(err) + } + script := "create persistent entity Shop.Customer (Name: string(100));\n" + if err := os.WriteFile(filepath.Join(dir, "script.mdl"), []byte(script), 0o644); err != nil { + t.Fatal(err) + } + return dir +} + +func TestCheckNoProject_SkipsAutoDetectedProject(t *testing.T) { + dir := noProjectFixture(t) + + r := runMainStreams(t, dir, "check", "script.mdl", "--no-project") + all := r.stdout + r.stderr + if strings.Contains(all, "Using project:") { + t.Errorf("--no-project still auto-detected the project:\n%s", all) + } + if strings.Contains(all, "Validating references against") { + t.Errorf("--no-project still resolved references:\n%s", all) + } + if r.exitCode != 0 { + t.Errorf("exit %d, want 0 for a syntactically valid script:\n%s", r.exitCode, all) + } + if !strings.Contains(r.stdout, "no project given") { + t.Errorf("verdict does not say no project was used:\n%s", r.stdout) + } +} + +// Control for the test above: the same directory without the flag is the +// reported behaviour, and it is kept — the auto-detected project is used and +// named. If this stops printing "Using project:", the fixture no longer +// exercises auto-detection and the test above proves nothing. +func TestCheckNoProject_DefaultStillAutoDetects(t *testing.T) { + dir := noProjectFixture(t) + + r := runMainStreams(t, dir, "check", "script.mdl") + if !strings.Contains(r.stderr, "Using project:") { + t.Fatalf("default check no longer auto-detects the project:\n%s%s", r.stdout, r.stderr) + } + // The notice is where the user meets the guess, so it names the way out. + if !strings.Contains(r.stderr, "--no-project") { + t.Errorf("auto-detection notice on check does not mention --no-project:\n%s", r.stderr) + } +} + +// --no-project with a flag that needs a project is a contradiction, refused +// before anything is checked rather than letting one silently win. +func TestCheckNoProject_ConflictsWithProjectFlags(t *testing.T) { + dir := noProjectFixture(t) + for _, extra := range [][]string{ + {"-p", "Stray.mpr"}, + {"--references"}, + {"--post-migration"}, + } { + args := append([]string{"check", "script.mdl", "--no-project"}, extra...) + r := runMainStreams(t, dir, args...) + if r.exitCode == 0 { + t.Errorf("%v: exit 0, want a refusal", extra) + } + if !strings.Contains(r.stderr, "--no-project") { + t.Errorf("%v: refusal does not name --no-project:\n%s", extra, r.stderr) + } + if strings.Contains(r.stdout, "Checking syntax") { + t.Errorf("%v: checked the script before refusing:\n%s", extra, r.stdout) + } + } +} + +// The help is the contract: its syntax-only example has to be the invocation +// that is syntax-only wherever it is run, and it has to say a project in the +// current directory is otherwise picked up. +func TestCheckHelp_SyntaxOnlyExampleMatchesBehaviour(t *testing.T) { + long := checkCmd.Long + if strings.Contains(long, "no project needed)\n mxcli check script.mdl\n") { + t.Errorf("help still offers a bare `mxcli check script.mdl` as syntax-only") + } + if !strings.Contains(long, "mxcli check script.mdl --no-project") { + t.Errorf("help has no --no-project example") + } + if !strings.Contains(long, "auto-detect") { + t.Errorf("help does not say a project in the current directory is auto-detected") + } + if checkCmd.Flags().Lookup("no-project") == nil { + t.Errorf("check has no --no-project flag") + } +} diff --git a/cmd/mxcli/cmd_check.go b/cmd/mxcli/cmd_check.go index b1377de35..64e7ce1bd 100644 --- a/cmd/mxcli/cmd_check.go +++ b/cmd/mxcli/cmd_check.go @@ -24,9 +24,15 @@ var checkCmd = &cobra.Command{ Long: `Check an MDL script file for syntax errors and optionally validate references. Without a project it checks syntax and the semantic rules that need no model. -Pass -p and it also resolves every reference — modules, entities, pages, -microflows and icons — against that project; --references is implied by -p and -is kept only for compatibility. +With a project it also resolves every reference — modules, entities, pages, +microflows and icons — against that project; --references is implied by a +project and is kept only for compatibility. + +The project is the one passed with -p or, without -p, the single .mpr in the +current directory, auto-detected as for every mxcli command (the "Using +project:" line says which). Run from a project directory, a plain +"mxcli check script.mdl" therefore checks references too. Pass --no-project to +check syntax and the no-model rules only, wherever it is run. It also resolves MEMBER names, not just the entity they belong to: an attribute named in a create/change activity is looked up on that entity and its @@ -122,12 +128,15 @@ for legacy native widgets that have pluggable replacements — Studio Pro does not auto-migrate these on a Mendix major-version upgrade. Examples: - # Check syntax only (no project needed) - mxcli check script.mdl + # Check syntax only, even in a directory holding an .mpr + mxcli check script.mdl --no-project # Check syntax, resolve references, and type-check expressions mxcli check script.mdl -p app.mpr + # The same, against the .mpr auto-detected in the current directory + mxcli check script.mdl + # Scan the project for legacy native widgets after a Mendix upgrade mxcli check script.mdl -p app.mpr --post-migration @@ -150,12 +159,32 @@ Examples: `, Args: cobra.MinimumNArgs(1), Run: func(cmd *cobra.Command, args []string) { + if err := checkNoProjectConflicts(cmd); err != nil { + fmt.Fprintln(os.Stderr, "Error:", err) + os.Exit(1) + } if code := runCheckFiles(cmd, args); code != 0 { os.Exit(code) } }, } +// checkNoProjectConflicts refuses --no-project together with a flag that asks +// for a project, rather than letting one of the two silently win. -p is only +// "changed" when the user gave it: auto-detection does not run under +// --no-project, so it cannot set it. +func checkNoProjectConflicts(cmd *cobra.Command) error { + if !skipsProjectDiscovery(cmd) { + return nil + } + for _, name := range []string{"project", "references", "post-migration"} { + if cmd.Flags().Changed(name) { + return fmt.Errorf("--%s asks for a project and --%s for none; pass one or the other", name, noProjectFlag) + } + } + return nil +} + // runCheckFile checks one script and returns the exit code: 0 when it passed. func runCheckFile(cmd *cobra.Command, filePath string) int { return runCheckFileAgainst(cmd, filePath, "") diff --git a/cmd/mxcli/main.go b/cmd/mxcli/main.go index f314b1586..4fdf0b372 100644 --- a/cmd/mxcli/main.go +++ b/cmd/mxcli/main.go @@ -125,6 +125,30 @@ func discoverProject(dir string) (path, note string) { } } +// noProjectFlag is the opt-out from project auto-detection, registered on the +// commands that are meaningful without a project (check). The root +// PersistentPreRun honours it on any command that registers it. +const noProjectFlag = "no-project" + +// noProjectOptOut reports whether cmd registers --no-project and, if so, the +// line that tells a user who got an auto-detected project how to avoid it — +// the notice is where they meet the guess, so it is where the way out goes. +func noProjectOptOut(cmd *cobra.Command) (bool, string) { + if cmd.Flags().Lookup(noProjectFlag) == nil { + return false, "" + } + return true, fmt.Sprintf("(pass --%s to %s without a project)", noProjectFlag, cmd.Name()) +} + +// skipsProjectDiscovery reports whether cmd was given --no-project. +func skipsProjectDiscovery(cmd *cobra.Command) bool { + if ok, _ := noProjectOptOut(cmd); !ok { + return false + } + v, _ := cmd.Flags().GetBool(noProjectFlag) + return v +} + var rootCmd = &cobra.Command{ Use: "mxcli", Short: "Mendix CLI - Work with Mendix projects using MDL syntax", @@ -157,10 +181,19 @@ beta language; --mdl 0 starts them in the alpha language, and in the REPL an Version: version, PersistentPreRun: func(cmd *cobra.Command, args []string) { projectPath, _ := cmd.Flags().GetString("project") - if projectPath == "" { + // A command with a --no-project opt-out (check) skips auto-detection + // when it is given: run from a project directory, `check script.mdl` + // otherwise resolves every reference against the .mpr it finds, and + // the syntax-only check its help offered could not be had there at + // all (mendixlabs/mxcli#1817). Without the flag nothing changes. + _, optOut := noProjectOptOut(cmd) + if projectPath == "" && !skipsProjectDiscovery(cmd) { discovered, note := discoverProject(".") if discovered != "" { _ = cmd.Flags().Set("project", discovered) + if optOut != "" { + note += "\n" + optOut + } fmt.Fprintln(os.Stderr, note) } else if note != "" && !shouldSuppressWarning() { fmt.Fprintln(os.Stderr, note) @@ -364,6 +397,8 @@ func init() { checkCmd.Flags().BoolP("references", "r", false, "Validate references against the project (implied by -p; kept for compatibility)") checkCmd.Flags().String("format", "text", "Output format: text, json, sarif") checkCmd.Flags().Bool("post-migration", false, "Scan the project for legacy native widgets that survived a Mendix upgrade (requires -p)") + checkCmd.Flags().Bool(noProjectFlag, false, + "Check syntax and the no-model rules only: do not auto-detect the .mpr in the current directory (conflicts with -p, --references, --post-migration)") // Diff command flags diffCmd.Flags().StringP("format", "f", "unified", "Output format: unified, side, struct") diff --git a/cmd/mxcli/tool_templates.go b/cmd/mxcli/tool_templates.go index cb63e1836..1a1221b65 100644 --- a/cmd/mxcli/tool_templates.go +++ b/cmd/mxcli/tool_templates.go @@ -149,7 +149,7 @@ The mxcli tool is in the PROJECT ROOT, not in system PATH. Always use: ### Validate MDL Scripts `+"```bash"+` -./mxcli check script.mdl # Syntax only +./mxcli check script.mdl --no-project # Syntax only (else the .mpr here is auto-detected) ./mxcli check script.mdl -p %s --references # With refs `+"```"+` diff --git a/docs-site/src/ide/vscode-syntax.md b/docs-site/src/ide/vscode-syntax.md index 19642c536..395ed3496 100644 --- a/docs-site/src/ide/vscode-syntax.md +++ b/docs-site/src/ide/vscode-syntax.md @@ -80,8 +80,8 @@ These diagnostics appear as warnings or errors in the Problems panel, identical The same checks available in the extension can be run from the command line: ```bash -# Syntax check only (no project needed) -mxcli check script.mdl +# Syntax check only (--no-project: do not auto-detect an .mpr in this directory) +mxcli check script.mdl --no-project # Syntax + reference validation mxcli check script.mdl -p app.mpr --references diff --git a/docs-site/src/language/script-files.md b/docs-site/src/language/script-files.md index 350ee5cd5..6de398250 100644 --- a/docs-site/src/language/script-files.md +++ b/docs-site/src/language/script-files.md @@ -98,8 +98,8 @@ EXECUTE SCRIPT './scripts/setup_domain_model.mdl'; You can validate a script without connecting to a project: ```bash -# Syntax only -mxcli check script.mdl +# Syntax only (--no-project: do not auto-detect an .mpr in this directory) +mxcli check script.mdl --no-project # Syntax + reference validation (requires project) mxcli check script.mdl -p app.mpr --references diff --git a/docs-site/src/migration/generation.md b/docs-site/src/migration/generation.md index 242eba7e1..62568e416 100644 --- a/docs-site/src/migration/generation.md +++ b/docs-site/src/migration/generation.md @@ -117,8 +117,8 @@ CREATE PAGE CRM.Customer_Overview ( Validate after each script to catch errors early: ```bash -# Syntax check (fast, no project needed) -mxcli check domain-model.mdl +# Syntax check (fast, no project used) +mxcli check domain-model.mdl --no-project # Reference validation (checks entity/microflow names exist) mxcli check pages.mdl -p app.mpr --references diff --git a/docs-site/src/migration/validation.md b/docs-site/src/migration/validation.md index 91e46a821..c475b5f1b 100644 --- a/docs-site/src/migration/validation.md +++ b/docs-site/src/migration/validation.md @@ -5,8 +5,8 @@ mxcli provides a multi-level validation pipeline. The agent uses these tools to ## Validation Steps ```bash -# 1. Syntax check (fast, no project needed) -mxcli check script.mdl +# 1. Syntax check (fast, no project used) +mxcli check script.mdl --no-project # 2. Reference validation (checks entity/microflow names exist) mxcli check script.mdl -p app.mpr --references diff --git a/docs-site/src/tutorial/skills.md b/docs-site/src/tutorial/skills.md index cfbe7791b..e4ee243b3 100644 --- a/docs-site/src/tutorial/skills.md +++ b/docs-site/src/tutorial/skills.md @@ -118,8 +118,8 @@ A list of errors the AI should avoid. For example, `write-microflows.md` warns a Steps the AI should follow after writing MDL to confirm correctness: ```bash -# Syntax check (no project needed) -./mxcli check script.mdl +# Syntax check only (--no-project: do not auto-detect an .mpr in this directory) +./mxcli check script.mdl --no-project # Syntax + reference validation ./mxcli check script.mdl -p app.mpr --references diff --git a/docs-site/src/tutorial/validation.md b/docs-site/src/tutorial/validation.md index 60b357d16..ce78aec9c 100644 --- a/docs-site/src/tutorial/validation.md +++ b/docs-site/src/tutorial/validation.md @@ -7,9 +7,11 @@ You've created entities, microflows, and pages. Before opening your project in S The fastest check. It parses your MDL script and reports syntax errors without touching any `.mpr` file: ```bash -mxcli check script.mdl +mxcli check script.mdl --no-project ``` +Without `--no-project`, a plain `mxcli check script.mdl` run from a directory holding exactly one `.mpr` auto-detects it (it prints `Using project: ...`) and goes on to Level 2 against it. `--no-project` gives the syntax-only check wherever you run it. + This catches: - Typos in keywords (`CRETE` instead of `CREATE`) @@ -147,8 +149,8 @@ BEGIN END; EOF -# 2. Check syntax (fast, no project needed) -mxcli check changes.mdl +# 2. Check syntax (fast, no project used) +mxcli check changes.mdl --no-project # 3. Check references (needs project, still fast) mxcli check changes.mdl -p app.mpr --references From d88834662389cd5cbb86c4c098547752b4849d68 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 11 Nov 2026 20:04:47 +0000 Subject: [PATCH 03/24] fix: domain model SVG draws associations from their stored anchors Part 1 of ako/mxcli#1841 (mirror of mendixlabs/mxcli#1692). describe --format svg domainmodel drew every association centre to centre and ignored ParentConnection/ChildConnection. Each end now attaches at its stored anchor, an "x;y" percentage of the drawn entity box (clamped onto the box when out of 0..100); an end with no readable anchor keeps the centre-line point. A self-association with anchors is routed round the outside of its box instead of a zero-length line. Parts 2 (crossing-minimising layout) and 3 (microflow auto-layout shape) are feature requests and are not addressed here. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82sv2odH8kVjmGzPosB --- ...vg-ignores-stored-association-anchors.json | 1 + .../1841-domain-model-svg-stored-anchors.mdl | 61 ++++++++ mdl/executor/cmd_domainmodel_svg.go | 136 ++++++++++++++++++ mdl/executor/cmd_domainmodel_svg_test.go | 101 +++++++++++++ 4 files changed, 299 insertions(+) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-11-11-domain-model-svg-ignores-stored-association-anchors.json create mode 100644 mdl-examples/bug-tests/1841-domain-model-svg-stored-anchors.mdl diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-11-11-domain-model-svg-ignores-stored-association-anchors.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-11-11-domain-model-svg-ignores-stored-association-anchors.json new file mode 100644 index 000000000..6efbfcc9b --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-11-11-domain-model-svg-ignores-stored-association-anchors.json @@ -0,0 +1 @@ +{"area":"mdl/executor","date":"2026-11-11","symptom":"`mxcli describe --format svg domainmodel ` draws every association centre to centre; the stored association anchors are ignored (tested with extreme anchor values), so the SVG cannot be used to verify \"corner to corner\" lines","cause":"renderDomainModelSVG computed both line ends from the box centres (edgePointFrom) and never read Association.ParentConnection/ChildConnection, although the backend had parsed them since #872","file":"`mdl/executor/cmd_domainmodel_svg.go` (`svgBox.anchorPoint`, `outward`, `selfLoop`, association loop in `renderDomainModelSVG`)","insight":"**The encoding was already measured — reuse it, do not re-derive it**: #872's two findings (search `anchor`) settled that ParentConnection (FROM) / ChildConnection (TO) are \"x;y\" integer PERCENTAGES of the entity box, 0..100, no -1/auto sentinel, and the loader does not range-check; a blank 11.14.0 app's raw unit still holds `0;54`/`100;54`. So the renderer's job is only `box.x + x/100*w`, applied to the box AS DRAWN (the SVG's widths are its own estimate, and after spread/clamp — a percentage survives that, a pixel offset would not). **nil is the only 'unset'**: every association mxcli writes stores 0;50/100;50, so after the fix almost every real diagram changes, and an mxcli-made FROM-left-of-TO pair now draws from FROM's left edge to TO's right edge, through both boxes — that is the stored data, not a bug in the renderer. **A self-association needs a route, not a line**: centre to centre it was a zero-length line; two anchors on one box joined straight cross the entity, so it walks an outset rectangle the shorter way. Out-of-range anchors are clamped onto the box — an unverified choice (no Studio Pro here), documented in the test. Test the endpoints numerically with a centre-line golden captured BEFORE the change as the unset-anchor control; the anchored cases fail on the old code with the exact reported symptom (every line (400,139)-(190,125) whatever the anchors). Tests `TestDomainModelSVG_AssociationUsesStoredAnchors`, `TestDomainModelSVG_UnsetAnchorsKeepCentreLine`, `TestDomainModelSVG_SelfAssociationUsesStoredAnchors`; example `mdl-examples/bug-tests/1841-domain-model-svg-stored-anchors.mdl`. ako/mxcli#1841, part 1 of upstream #1692","refs":["#1841","#1692","#872"]} diff --git a/mdl-examples/bug-tests/1841-domain-model-svg-stored-anchors.mdl b/mdl-examples/bug-tests/1841-domain-model-svg-stored-anchors.mdl new file mode 100644 index 000000000..82b2f5143 --- /dev/null +++ b/mdl-examples/bug-tests/1841-domain-model-svg-stored-anchors.mdl @@ -0,0 +1,61 @@ +mdl 1; +-- ============================================================================ +-- ako/mxcli#1841 (part 1 of mendixlabs/mxcli#1692) — the domain model SVG +-- draws associations from their stored anchors +-- ============================================================================ +-- +-- Symptom, verbatim: "`mxcli describe --format svg domainmodel ` draws +-- every association centre to centre. The stored association anchors are +-- ignored (tested with extreme anchor values), so the SVG cannot be used to +-- verify "corner to corner" lines." +-- +-- An association's anchors are DomainModels$Association.ParentConnection (FROM +-- entity) and ChildConnection (TO entity), each the string "x;y" — a percentage +-- of the entity box (a blank 11.14.0 app's Studio Pro-authored +-- Administration.AccountPasswordData_Account stores "0;54" / "100;54"). The SVG +-- now attaches each end at that percentage of the drawn box; values outside +-- 0..100 are clamped onto the box. A self-association is routed round the +-- outside of its box from one anchor to the other. +-- +-- Run, then render: +-- mxcli exec 1841-domain-model-svg-stored-anchors.mdl -p app.mpr +-- mxcli describe --format svg -p app.mpr domainmodel Bug1841 +-- +-- Expected, with the entities drawn as below (Customer 90x44 at (100,100), +-- Order at (500,300), Category 90x44 at (100,450)): +-- Order_Customer corner to corner +-- Order_Category mxcli's default 0;50 / 100;50 +-- Category_Parent +-- Before the fix all three were centre-line lines, and Category_Parent was a +-- zero-length line at the box centre. + +create module Bug1841; + +@position(100, 100) +create persistent entity Bug1841.Customer ( + "Name": string(200) +); + +@position(500, 300) +create persistent entity Bug1841.Order ( + "Number": integer +); + +@position(100, 450) +create persistent entity Bug1841.Category ( + "Title": string(100) +); + +-- Corner to corner: Order's top-left to Customer's bottom-right. +@anchor(from: (0, 0), to: (100, 100)) +create association Bug1841.Order_Customer +from Bug1841.Order to Bug1841.Customer; + +-- No anchor given: mxcli stores its default 0;50 / 100;50, and the SVG draws that. +create association Bug1841.Order_Category +from Bug1841.Order to Bug1841.Category; + +-- Self-association: out of the right edge at 30%, back into the bottom at 50%. +@anchor(from: (100, 30), to: (50, 100)) +create association Bug1841.Category_Parent +from Bug1841.Category to Bug1841.Category; diff --git a/mdl/executor/cmd_domainmodel_svg.go b/mdl/executor/cmd_domainmodel_svg.go index 5ccfab481..692bb0de8 100644 --- a/mdl/executor/cmd_domainmodel_svg.go +++ b/mdl/executor/cmd_domainmodel_svg.go @@ -117,6 +117,109 @@ func (b svgBox) edgePointFrom(sx, sy, tx, ty float64) (float64, float64) { return sx + dx*scale, sy + dy*scale } +// anchorPoint is where a stored association anchor sits on the drawn box. +// +// Mendix stores an association's anchors (ParentConnection on the FROM entity, +// ChildConnection on the TO entity) as "x;y", a PERCENTAGE of the entity box +// (see sdk/domainmodel/connection.go for the measurement). The box here is this +// renderer's estimate, not Studio Pro's, so the percentage is applied to the +// box as drawn — the line attaches to the same edge, the same fraction along. +// Mendix's loader does not range-check, so a value outside 0..100 is clamped +// onto the box: a connector attaches to its entity. nil means nothing (or +// nothing readable) was stored, and the caller keeps the centre-line drawing. +func (b svgBox) anchorPoint(p *model.Point) (x, y float64, ok bool) { + if p == nil { + return 0, 0, false + } + fx := math.Max(0, math.Min(100, float64(p.X))) / 100 + fy := math.Max(0, math.Min(100, float64(p.Y))) / 100 + return b.x + fx*b.w, b.y + fy*b.h, true +} + +// outward is the unit normal of the box edge nearest to (x, y), a point on it. +func (b svgBox) outward(x, y float64) (float64, float64) { + nx, ny, best := -1.0, 0.0, x-b.x + if d := b.x + b.w - x; d < best { + nx, ny, best = 1, 0, d + } + if d := y - b.y; d < best { + nx, ny, best = 0, -1, d + } + if d := b.y + b.h - y; d < best { + nx, ny = 0, 1 + } + return nx, ny +} + +// selfLoop routes a self-association from anchor (x1, y1) back to anchor +// (x2, y2) of the same box: a short stub straight out of each anchor's edge, +// joined the shorter way round a rectangle `d` outside the box, so the line +// never crosses the entity it belongs to. +func (b svgBox) selfLoop(x1, y1, x2, y2 float64) [][2]float64 { + const d = 20.0 + r := svgBox{x: b.x - d, y: b.y - d, w: b.w + 2*d, h: b.h + 2*d} + n1x, n1y := b.outward(x1, y1) + n2x, n2y := b.outward(x2, y2) + s1 := [2]float64{x1 + n1x*d, y1 + n1y*d} + s2 := [2]float64{x2 + n2x*d, y2 + n2y*d} + // Position along r's perimeter, clockwise from its top-left corner. + per := 2 * (r.w + r.h) + at := func(p [2]float64) float64 { + switch { + case p[1] <= r.y: + return p[0] - r.x + case p[0] >= r.x+r.w: + return r.w + p[1] - r.y + case p[1] >= r.y+r.h: + return r.w + r.h + r.x + r.w - p[0] + default: + return 2*r.w + r.h + r.y + r.h - p[1] + } + } + corners := []struct { + t float64 + p [2]float64 + }{ + {0, [2]float64{r.x, r.y}}, + {r.w, [2]float64{r.x + r.w, r.y}}, + {r.w + r.h, [2]float64{r.x + r.w, r.y + r.h}}, + {2*r.w + r.h, [2]float64{r.x, r.y + r.h}}, + } + t1, t2 := at(s1), at(s2) + cw := math.Mod(t2-t1+per, per) + clockwise := cw <= per-cw + walk := cw + if !clockwise { + walk = per - cw + } + // The corners passed on the way, each with its distance from s1 along the + // walk; at most four, so sorting them by insertion is enough. + type passed struct { + p [2]float64 + dist float64 + } + var via []passed + for _, c := range corners { + dc := math.Mod(c.t-t1+per, per) + if !clockwise { + dc = math.Mod(t1-c.t+per, per) + } + if dc <= 0 || dc >= walk { + continue + } + i := len(via) + via = append(via, passed{c.p, dc}) + for ; i > 0 && via[i-1].dist > via[i].dist; i-- { + via[i-1], via[i] = via[i], via[i-1] + } + } + pts := [][2]float64{{x1, y1}, s1} + for _, v := range via { + pts = append(pts, v.p) + } + return append(pts, s2, [2]float64{x2, y2}) +} + func svgNum(f float64) string { return fmt.Sprintf("%.0f", f) } func svgText(s string) string { @@ -239,11 +342,44 @@ func renderDomainModelSVG(moduleName string, entities []*domainmodel.Entity, ass } x1, y1 := from.edgePointFrom(fcx, fcy, tcx, tcy) x2, y2 := to.edgePointFrom(tcx, tcy, fcx, fcy) + // The stored anchors say where Studio Pro attaches each end; an end + // with none stored keeps the centre-line point. (ako/mxcli#1841) + ax1, ay1, okA1 := from.anchorPoint(a.ParentConnection) + ax2, ay2, okA2 := to.anchorPoint(a.ChildConnection) + if okA1 { + x1, y1 = ax1, ay1 + } + if okA2 { + x2, y2 = ax2, ay2 + } fromMult, toMult := "*", "1" if a.Type == domainmodel.AssociationTypeReferenceSet { toMult = "*" } fmt.Fprintf(&sb, `%s`, svgText(a.Name)) + if a.ParentID == a.ChildID && okA1 && okA2 { + // A self-association leaves its box and comes back in, round + // the outside; a straight line would cross the entity. + route := from.selfLoop(x1, y1, x2, y2) + sb.WriteString(``) + n := len(route) + writeEndLabel(&sb, x1, y1, route[1][0], route[1][1], fromMult) + writeEndLabel(&sb, x2, y2, route[n-2][0], route[n-2][1], toMult) + mid := route[n/2] + fmt.Fprintf(&sb, `%s`, + svgNum(mid[0]), svgNum(mid[1]-4+offset/16*12), svgText(a.Name)) + sb.WriteString(``) + continue + } fmt.Fprintf(&sb, ``, svgNum(x1), svgNum(y1), svgNum(x2), svgNum(y2)) writeEndLabel(&sb, x1, y1, x2, y2, fromMult) writeEndLabel(&sb, x2, y2, x1, y1, toMult) diff --git a/mdl/executor/cmd_domainmodel_svg_test.go b/mdl/executor/cmd_domainmodel_svg_test.go index 25495d7e0..249e60e99 100644 --- a/mdl/executor/cmd_domainmodel_svg_test.go +++ b/mdl/executor/cmd_domainmodel_svg_test.go @@ -182,3 +182,104 @@ func TestDomainModelSVG_SpreadsTheLayoutSoNamesFit(t *testing.T) { t.Errorf("want the left entity kept at 0 and the right one moved out from 150, got %v", m) } } + +// assocLine returns the endpoints of the one association line drawn for name. +func assocLine(t *testing.T, svg, name string) (x1, y1, x2, y2 int) { + t.Helper() + re := regexp.MustCompile(`` + regexp.QuoteMeta(name) + + `Customer_Parent 100 && p[0] < 190 && p[1] > 100 && p[1] < 144 { + t.Errorf("bend %v lies inside the entity box", p) + } + } +} From 3745b3af081425b94a7bdee922461069d8c567b4 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 11 Nov 2026 20:14:19 +0000 Subject: [PATCH 04/24] fix: a REPL session never sees another process's write to the project (#2121) A long-lived session read the project once and answered from that state: `show microflows` kept the old set, `describe microflow` of a flow created by another process said "microflow not found", `create entity` in a module created elsewhere said "module not found", and on MPR v2 the session's own `create microflow` of a name another process had taken wrote a second document with that name. Before each statement the executor now asks the backend whether something other than its own connection changed the .mpr: PRAGMA data_version (moves only for other connections' commits, so the session's own writes do not trip it and the #2094 caches survive) plus os.SameFile on the path (a git checkout renames a new file into place, which an open SQLite connection never sees; the backend then reopens). On a change the reader drops its unit index, v1 listing/names, name index and content cache, and the executor drops its read caches while keeping its record of the session's own writes. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82sv2odH8kVjmGzPosB --- ...sion-never-sees-another-process-write.json | 1 + CHANGELOG.md | 1 + mdl/backend/connection.go | 10 ++ mdl/backend/modelsdk/backend.go | 41 +++++ mdl/executor/disk_change.go | 59 +++++++ mdl/executor/executor.go | 6 + mdl/executor/external_change_2121_test.go | 159 ++++++++++++++++++ modelsdk/mpr/reader.go | 7 + modelsdk/mpr/reader_disk_change.go | 102 +++++++++++ modelsdk/mpr/reader_disk_change_test.go | 57 +++++++ modelsdk/mpr/writer_deferred.go | 6 + 11 files changed, 449 insertions(+) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-11-11-session-never-sees-another-process-write.json create mode 100644 mdl/executor/disk_change.go create mode 100644 mdl/executor/external_change_2121_test.go create mode 100644 modelsdk/mpr/reader_disk_change.go create mode 100644 modelsdk/mpr/reader_disk_change_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-11-11-session-never-sees-another-process-write.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-11-11-session-never-sees-another-process-write.json new file mode 100644 index 000000000..414512c4b --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-11-11-session-never-sees-another-process-write.json @@ -0,0 +1 @@ +{"area": "mdl/executor", "date": "2026-11-11", "symptom": "ako/mxcli#2121: a REPL session never saw a write by another process (a second mxcli exec, Studio Pro, git): `show microflows` kept the old set, `describe microflow` of a flow created elsewhere said `microflow not found`, and on MPR v2 the session's own `create microflow` of that name wrote a SECOND document with it; on v1 (#2115 already re-read the listing) `create entity` in a module made elsewhere still said `module not found`.", "cause": "Two layers cached with no external-change signal: the v2 reader's unit index (unitCacheValid, cleared only by this reader's own writes) and the executor's executorCache (modules, domain models, hierarchy, name maps), which nothing but a session write ever reset. A git checkout also replaces the .mpr by rename, which an open SQLite connection never sees: it goes on reading the old inode.", "file": "modelsdk/mpr/reader_disk_change.go (SyncWithDisk: PRAGMA data_version + os.SameFile on the .mpr), mdl/backend/modelsdk/backend.go (SyncWithDisk reopens on replacement, refuses while a deferred run holds writes), mdl/executor/disk_change.go (called at the top of Execute; dropProjectReads keeps created/dropped/modifiedDomainModels)", "insight": "Probe, don't re-read: PRAGMA data_version moves only for OTHER connections' commits (the reader holds one connection), so the session's own writes never trip it and #2094's caches survive; file size/mtime would fire on every own write and undo that perf work. data_version cannot see a file renamed into place, so add os.SameFile on the path. The v1 symptom survived #2115 because the stale cache was the executor's, not the reader's — test with two executors (two backends = two SQLite connections) on a copy of both the v1 and the v2 fixture, and assert at the executor, not the reader. Stubbing only the executor-cache drop leaves `module not found` failing on both formats, which is the control that shows both layers are needed. The write-from-stale-state case was a duplicate document (v2 create of a name taken elsewhere), not a lost update: domain-model writes re-read the unit and kept the other process's entity in every measured run. Not covered: a change between one statement's read and its write (the locking proposal), and a v2 change to mprcontents/ with the .mpr untouched.", "refs": ["ako/mxcli#2121", "ako/mxcli#2115", "ako/mxcli#2094"], "test": "mdl/executor/external_change_2121_test.go, modelsdk/mpr/reader_disk_change_test.go"} diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f78b49c0..48a2f831b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **A REPL session sees what another process wrote to the project** (ako/mxcli#2121) — a session read the project once and kept answering from that state, so a write by a second `mxcli`, Studio Pro or a git checkout never showed up: `show microflows` kept the old set, `describe microflow` of a flow made elsewhere said `microflow not found`, `create entity` in a module made elsewhere said `module not found`, and on MPR v2 a `create microflow` of a name another process had taken wrote a second document with that name. Before each statement the session now asks whether another connection committed to the `.mpr` (SQLite's `data_version`) or the file was replaced (a git checkout), and when so drops what it kept and reads afresh; if the file was replaced it is reopened. The probe is one query and one stat, and the session's own writes do not trip it. Not covered: a change made while a statement runs, and on MPR v2 a change to `mprcontents/` that leaves the `.mpr` untouched. - **`describe page` → `exec` no longer rewrites a page over formatting and stored keys MDL cannot state** (ako/mxcli#2106) — on FactoryManagement, 8 pages reported `Replaced page` with nothing changed: a dynamic-classes expression's trailing newlines, an XPath constraint's line breaks and spacing, the order of an enumeration condition's values, an association source's null `SourceVariable` key, and an inherited `createdDate` binding requalified to the specialization. The stored value is now kept where only its form differs, the writer stores the key Studio Pro stores, and a system member resolves to the entity that stores it. A full sweep of the app's 229 pages turns those 8 `Unchanged` and changes no other. - **`describe page` → `exec` keeps a check box's label position, a radio button group's orientation and a show page action's close count** (ako/mxcli#2106) — none had an MDL spelling, so a round trip of a Studio Pro page reset a check box with its label after the control to the default, turned every vertical radio button group horizontal, and stopped a show page button from closing the page it came from, with `mx check` green. `checkbox` takes `LabelPosition: Default|BeforeControl|AfterControl`, `radiobuttons` takes `RenderHorizontal: false`, and `show page` takes `with (NumberOfPagesToClose: n)`; `describe` prints each when it differs from the default. - **`mxcli test --local --watch` keeps applying changes on macOS and Linux mxbuild** (mendixlabs/mxcli#1787, refs mendixlabs/mxcli#1786) — the test runner's rebuild called mxbuild's serve process directly, without the recovery `run --local --watch` has, so the second build of a serve process failed on regenerated pages and every later change reported "could not be applied". The rebuild now goes through the same recovery, which also recognises mxbuild's resource-key form of that failure (`IO_PathNotFound_Path`, as macOS and Linux report it) and skips the web-bundle check, which only matters when a browser is served. diff --git a/mdl/backend/connection.go b/mdl/backend/connection.go index 275dffe99..8d8f33c5a 100644 --- a/mdl/backend/connection.go +++ b/mdl/backend/connection.go @@ -29,6 +29,16 @@ type ConnectionBackend interface { GetMendixVersion() (string, error) } +// DiskChangeSyncer is implemented by a backend that reads a project on disk +// another process can write to (ako/mxcli#2121). SyncWithDisk reports whether +// something other than this connection changed the project since it last +// looked, and when it did, has already dropped what the connection kept, so +// the next read sees the disk. It must be cheap — a session calls it before +// every statement — and must not report the connection's own writes. +type DiskChangeSyncer interface { + SyncWithDisk() (changed bool, err error) +} + // ModuleBackend provides module-level operations. type ModuleBackend interface { // ListModules returns all modules in the project. diff --git a/mdl/backend/modelsdk/backend.go b/mdl/backend/modelsdk/backend.go index 4470d08c5..21926c6c5 100644 --- a/mdl/backend/modelsdk/backend.go +++ b/mdl/backend/modelsdk/backend.go @@ -35,6 +35,7 @@ import ( var _ backend.FullBackend = (*Backend)(nil) var _ backend.WriteStatsReporter = (*Backend)(nil) var _ backend.SourceFileReporter = (*Backend)(nil) +var _ backend.DiskChangeSyncer = (*Backend)(nil) // Backend reads and writes a Mendix project through the modelsdk codec engine. // It embeds `unimplemented` (generated, see gen_unimplemented.go) so any @@ -196,6 +197,46 @@ func (b *Backend) Disconnect() error { return flushErr } +// SyncWithDisk reports whether another process changed the project since this +// connection last looked, having first dropped every view of it the +// connection keeps, so the next read sees the disk (ako/mxcli#2121). A session +// calls it before each statement: one PRAGMA and one stat, not a re-read. +// +// When the .mpr was replaced by a new file (a git checkout renames one into +// place) the connection still holds the old one open, so the project is +// reopened. Not while a deferred run or a simulation holds unwritten units: +// reopening would write them into, or drop them from, a project they were not +// made against, so that is refused with an error instead. +func (b *Backend) SyncWithDisk() (changed bool, err error) { + if b.reader == nil { + return false, nil + } + switch b.reader.SyncWithDisk() { + case mmpr.DiskUnchanged: + return false, nil + case mmpr.DiskChanged: + return true, nil + } + if b.writer != nil && b.writer.HoldsWrites() { + return true, fmt.Errorf("%s was replaced on disk while unwritten changes were held; reconnect to read the new file", b.path) + } + readOnly := b.writer == nil + path := b.path + if err := b.reader.Close(); err != nil { + return true, fmt.Errorf("close the replaced project: %w", err) + } + b.reader, b.writer = nil, nil + if readOnly { + err = b.ConnectReadOnly(path) + } else { + err = b.Connect(path) + } + if err != nil { + return true, fmt.Errorf("reopen %s after it was replaced on disk: %w", path, err) + } + return true, nil +} + // DeferUnitWrites holds unit updates in memory until FlushDeferredWrites, so a // run of statements is written once and judged by its net result // (mmpr.Writer.DeferUnitWrites, ako/mxcli#872). Reads during the run see the diff --git a/mdl/executor/disk_change.go b/mdl/executor/disk_change.go new file mode 100644 index 000000000..0ef397127 --- /dev/null +++ b/mdl/executor/disk_change.go @@ -0,0 +1,59 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "github.com/mendixlabs/mxcli/mdl/backend" +) + +// syncWithDisk is run before each statement: when another process — a second +// mxcli, Studio Pro, a git checkout — changed the project since the session +// last looked, the session drops what it kept of it and reads it afresh +// (ako/mxcli#2121). Without it a long-lived session went on answering from the +// project as it first read it: `show` left out documents made elsewhere, +// `describe` said "not found" for them, and on MPR v2 a `create` of a name +// another process had taken wrote a second document with that name. +// +// The probe is one PRAGMA and one stat (backend.DiskChangeSyncer); the +// session's own writes do not trip it. It is skipped while an access-rule run +// holds deferred writes: the run's statements are one write, and the probe +// runs again before the statement after it. +func (e *Executor) syncWithDisk() error { + if e.backend == nil || e.accessRun != nil || !e.backend.IsConnected() { + return nil + } + syncer, ok := e.backend.(backend.DiskChangeSyncer) + if !ok { + return nil + } + changed, err := syncer.SyncWithDisk() + if !changed { + return err + } + if e.cache != nil { + e.cache.dropProjectReads() + } + if e.logger != nil { + e.logger.Info("project changed on disk by another process; re-reading", "path", e.mprPath) + } + return err +} + +// dropProjectReads forgets what the cache read from the project, keeping what +// it records about this session's own writes (created and dropped documents, +// domain models awaiting finalization), which another process's write does not +// undo. +func (c *executorCache) dropProjectReads() { + c.modulesMu.Lock() + c.modules = nil + c.modulesMu.Unlock() + c.units = nil + c.folders = nil + c.domainModels = nil + c.enumerations = nil + c.hierarchy = nil + c.defaultLang, c.defaultLangLoaded = "", false + c.entityNames = nil + c.microflowNames = nil + c.pageNames = nil +} diff --git a/mdl/executor/executor.go b/mdl/executor/executor.go index 9a98b1ec5..200dc1ec1 100644 --- a/mdl/executor/executor.go +++ b/mdl/executor/executor.go @@ -438,6 +438,12 @@ func (e *Executor) Execute(stmt ast.Statement) error { stmt = s } + // Another process may have written to the project since the last + // statement (ako/mxcli#2121). + if err := e.syncWithDisk(); err != nil { + return err + } + // Announce the MDL command so the PED calls it triggers (reported by the MCP // client) group under it (--mcp-trace, level 2; no-op otherwise). e.tracer.Statement(stmtSummary(stmt)) diff --git a/mdl/executor/external_change_2121_test.go b/mdl/executor/external_change_2121_test.go new file mode 100644 index 000000000..53ae8c918 --- /dev/null +++ b/mdl/executor/external_change_2121_test.go @@ -0,0 +1,159 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +// ako/mxcli#2121: a long-lived session read the project once and went on +// answering from that state, so a write by another process never showed up: +// `show microflows` kept listing the old set, `describe microflow` of a flow +// created elsewhere said "microflow not found", and on MPR v2 the session's own +// `create microflow` of that name wrote a SECOND document with it, because the +// stale unit index said the name was free. On v1 the executor's module cache +// refused `create entity` in a module made elsewhere ("module not found"). +// +// The "other process" is a second executor with its own backend: its own +// SQLite connection to the .mpr, which is what a second mxcli, Studio Pro or a +// git checkout is to the session. +func TestSessionSeesAnotherProcessWrite_Issue2121(t *testing.T) { + for _, f := range []pkgHostFixture{v1Fixture, pedAppFixture} { + t.Run(f.name, func(t *testing.T) { + path := copyHostProject(t, f) + session, sessionOut := connectHost(t, path) + run := func(src string) (string, error) { + t.Helper() + sessionOut.Reset() + err := runPkgMDL(t, session, "mdl 1;\n"+src) + return sessionOut.String(), err + } + + // The session reads first, so whatever it caches is warm. + if _, err := run("create module Lab;"); err != nil { + t.Fatalf("create module: %v", err) + } + if out, err := run("show modules; show microflows in Lab; show entities in Lab;"); err != nil { + t.Fatalf("warm-up reads: %v\n%s", err, out) + } + + other, otherOut := connectHost(t, path) + if err := runPkgMDL(t, other, `mdl 1; +create module ExtMod; +create microflow Lab.StaleProbe () +begin + log info node 'probe' 'one'; +end; +`); err != nil { + t.Fatalf("other process write: %v\n%s", err, otherOut.String()) + } + + out, err := run("show microflows in Lab;") + if err != nil || !strings.Contains(out, "Lab.StaleProbe") { + t.Errorf("show microflows after another process created Lab.StaleProbe: err=%v, output lacks it:\n%s", err, out) + } + out, err = run("describe microflow Lab.StaleProbe;") + if err != nil || !strings.Contains(out, "'one'") { + t.Errorf("describe microflow created elsewhere: err=%v\n%s", err, out) + } + + if err := runPkgMDL(t, other, `mdl 1; +create or modify microflow Lab.StaleProbe () +begin + log info node 'probe' 'two'; +end; +`); err != nil { + t.Fatalf("other process modify: %v\n%s", err, otherOut.String()) + } + out, err = run("describe microflow Lab.StaleProbe;") + if err != nil || !strings.Contains(out, "'two'") { + t.Errorf("describe after another process modified it shows the old content: err=%v\n%s", err, out) + } + + // A session write must not act on the stale state either. + out, err = run(`create microflow Lab.StaleProbe () begin log info node 'probe' 'session'; end;`) + if err == nil { + t.Errorf("create microflow of a name another process took succeeded — the session wrote a duplicate document:\n%s", out) + } + if out, err := run("create persistent entity ExtMod.E1 (A: Integer);"); err != nil { + t.Errorf("create entity in a module another process created: %v\n%s", err, out) + } + + _ = other.Execute(&ast.DisconnectStmt{}) + _ = session.Execute(&ast.DisconnectStmt{}) + check, checkOut := connectHost(t, path) + if err := runPkgMDL(t, check, "show microflows in Lab;"); err != nil { + t.Fatal(err) + } + if n := strings.Count(checkOut.String(), "| Lab.StaleProbe "); n != 1 { + t.Errorf("the project holds %d microflows named Lab.StaleProbe, want 1:\n%s", n, checkOut.String()) + } + }) + } +} + +// A git checkout replaces the .mpr with a new file rather than writing into +// it. The session's SQLite connection still holds the old file open and would +// read it forever; it has to notice the replacement and reopen. v1 only: on v2 +// the documents are separate files, and the replacement of the .mpr alone is +// the case that matters for the connection. +func TestSessionSeesReplacedProjectFile_Issue2121(t *testing.T) { + path := copyHostProject(t, v1Fixture) + session, sessionOut := connectHost(t, path) + if err := runPkgMDL(t, session, "mdl 1;\nshow modules;"); err != nil { + t.Fatal(err) + } + + // Another checkout of the project gets a module, then is moved over the + // session's file, as git does: write a new file, rename it into place. + elsewhere := filepath.Join(t.TempDir(), "App.mpr") + if err := copyPedAppFile(path, elsewhere); err != nil { + t.Fatal(err) + } + other, otherOut := connectHost(t, elsewhere) + if err := runPkgMDL(t, other, "mdl 1;\ncreate module Checkout;"); err != nil { + t.Fatalf("%v\n%s", err, otherOut.String()) + } + _ = other.Execute(&ast.DisconnectStmt{}) + staged := path + ".new" + if err := copyPedAppFile(elsewhere, staged); err != nil { + t.Fatal(err) + } + if err := os.Rename(staged, path); err != nil { + t.Fatal(err) + } + + sessionOut.Reset() + if err := runPkgMDL(t, session, "mdl 1;\nshow modules;"); err != nil { + t.Fatal(err) + } + if !strings.Contains(sessionOut.String(), "Checkout") { + t.Errorf("show modules after the .mpr was replaced still reads the old file:\n%s", sessionOut.String()) + } +} + +// The session's own writes must not be taken for another process's: that +// would throw the caches away after every statement and undo #2094. +func TestOwnWriteIsNotAnExternalChange_Issue2121(t *testing.T) { + for _, f := range []pkgHostFixture{v1Fixture, pedAppFixture} { + t.Run(f.name, func(t *testing.T) { + path := copyHostProject(t, f) + session, out := connectHost(t, path) + if err := runPkgMDL(t, session, "mdl 1;\ncreate module Own;\ncreate persistent entity Own.E (A: Integer);\nshow entities in Own;"); err != nil { + t.Fatalf("%v\n%s", err, out.String()) + } + syncer, ok := session.backend.(interface{ SyncWithDisk() (bool, error) }) + if !ok { + t.Fatalf("the modelsdk backend has no SyncWithDisk") + } + if changed, err := syncer.SyncWithDisk(); err != nil || changed { + t.Errorf("after the session's own writes SyncWithDisk = %v, %v; want false", changed, err) + } + }) + } +} diff --git a/modelsdk/mpr/reader.go b/modelsdk/mpr/reader.go index 0961516c3..091caf445 100644 --- a/modelsdk/mpr/reader.go +++ b/modelsdk/mpr/reader.go @@ -112,6 +112,11 @@ type Reader struct { // hold, read on first use (packaged_modules.go). packagedOnce sync.Once packaged *packagedSet + + // diskSeen is the state of the project on disk this reader last looked + // at (reader_disk_change.go, ako/mxcli#2121). diskMu guards it. + diskMu sync.Mutex + diskSeen diskStamp } // cachedUnit stores metadata about a unit for fast filtering. @@ -227,6 +232,7 @@ func OpenWithOptions(path string, opts OpenOptions) (*Reader, error) { return nil, err } + r.markDiskBaseline() return r, nil } @@ -353,6 +359,7 @@ func OpenWithDB(db *sql.DB, path, contentsDir string) (*Reader, error) { if err := r.verify(); err != nil { return nil, err } + r.markDiskBaseline() return r, nil } diff --git a/modelsdk/mpr/reader_disk_change.go b/modelsdk/mpr/reader_disk_change.go new file mode 100644 index 000000000..27302adc7 --- /dev/null +++ b/modelsdk/mpr/reader_disk_change.go @@ -0,0 +1,102 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mpr + +import ( + "os" +) + +// DiskChange is what SyncWithDisk found. +type DiskChange int + +const ( + // DiskUnchanged: nothing but this reader has written to the project since + // the last look. + DiskUnchanged DiskChange = iota + // DiskChanged: another connection — a second mxcli, Studio Pro — committed + // to the .mpr. The reader has already dropped what it kept. + DiskChanged + // DiskReplaced: the .mpr at the reader's path is no longer the file the + // reader holds open (a git checkout writes a new file and renames it into + // place). The open connection would go on reading the old file, so the + // owner must reopen the project. + DiskReplaced +) + +// diskStamp is what says the project on disk may have changed under a reader +// (ako/mxcli#2121). dataVersion is SQLite's PRAGMA data_version: it moves when +// ANOTHER connection commits, never for this reader's own writes (the reader +// holds one connection, SetMaxOpenConns(1)), so a session's own writes do not +// read as foreign ones and do not throw the caches away (#2094). file is the +// .mpr's identity, for the replacement data_version cannot see. +type diskStamp struct { + dataVersion int64 + file os.FileInfo + ok bool +} + +func (r *Reader) diskStampNow() diskStamp { + var s diskStamp + if r.db == nil { + return s + } + if err := r.db.QueryRow(`PRAGMA data_version`).Scan(&s.dataVersion); err != nil { + return s + } + if r.path != "" { + if fi, err := os.Stat(r.path); err == nil { + s.file = fi + } + } + s.ok = true + return s +} + +// markDiskBaseline records the project's current state as what this reader +// has seen. Called on open. +func (r *Reader) markDiskBaseline() { + r.diskMu.Lock() + r.diskSeen = r.diskStampNow() + r.diskMu.Unlock() +} + +// SyncWithDisk tells whether something other than this reader changed the +// project since the last call (or since the reader was opened), and when it +// did, drops every view of the project the reader keeps, so the next read +// sees what is on disk. It costs one PRAGMA and one stat, not a read of the +// project, so a session can call it before every statement. +// +// What it cannot see: a change on MPR v2 to mprcontents/ alone, with the .mpr +// untouched. Every writer known — mxcli, Studio Pro, a git checkout of a +// commit — moves the Unit table's ContentsHash with the file, and unit bytes +// are read from their file on each read anyway, so only the index of names +// would lag. +func (r *Reader) SyncWithDisk() DiskChange { + now := r.diskStampNow() + r.diskMu.Lock() + seen := r.diskSeen + r.diskSeen = now + r.diskMu.Unlock() + + if seen.file != nil && now.file != nil && !os.SameFile(seen.file, now.file) { + r.dropDiskViews() + return DiskReplaced + } + if !now.ok || !seen.ok || now.dataVersion != seen.dataVersion { + r.dropDiskViews() + return DiskChanged + } + return DiskUnchanged +} + +// dropDiskViews forgets everything the reader keeps that another writer can +// have invalidated without telling it: the unit index, the v1 listing and the +// names it kept by ContentsHash (another writer need not move the hash), the +// name index and cached unit bytes. Overlays are this session's own pending +// bytes and stay. +func (r *Reader) dropDiskViews() { + r.InvalidateCache() + r.v1TypesMu.Lock() + clear(r.v1Headers) + r.v1TypesMu.Unlock() +} diff --git a/modelsdk/mpr/reader_disk_change_test.go b/modelsdk/mpr/reader_disk_change_test.go new file mode 100644 index 000000000..819da8338 --- /dev/null +++ b/modelsdk/mpr/reader_disk_change_test.go @@ -0,0 +1,57 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mpr + +import ( + "os" + "testing" +) + +// ako/mxcli#2121: SyncWithDisk is what a long-lived session asks before each +// statement. It must report another connection's commit, the replacement of +// the .mpr by a new file, and nothing for the reader's own writes — or every +// statement would throw the caches away (#2094). +func TestSyncWithDisk_Issue2121(t *testing.T) { + path := copyV1Fixture(t) + r, err := OpenWithOptions(path, OpenOptions{}) + if err != nil { + t.Fatal(err) + } + defer r.Close() + + if got := r.SyncWithDisk(); got != DiskUnchanged { + t.Fatalf("right after open: %v, want unchanged", got) + } + if _, err := r.db.Exec(`CREATE TABLE probe2121 (x)`); err != nil { + t.Fatal(err) + } + if got := r.SyncWithDisk(); got != DiskUnchanged { + t.Errorf("after the reader's own commit: %v, want unchanged", got) + } + + other := otherProcess(t, path) + if _, err := other.Exec(`INSERT INTO probe2121 VALUES (1)`); err != nil { + t.Fatal(err) + } + if got := r.SyncWithDisk(); got != DiskChanged { + t.Errorf("after another connection's commit: %v, want changed", got) + } + if got := r.SyncWithDisk(); got != DiskUnchanged { + t.Errorf("asked again with nothing new: %v, want unchanged", got) + } + _ = other.Close() + + b, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path+".new", b, 0o600); err != nil { + t.Fatal(err) + } + if err := os.Rename(path+".new", path); err != nil { + t.Fatal(err) + } + if got := r.SyncWithDisk(); got != DiskReplaced { + t.Errorf("after the .mpr was replaced by a new file: %v, want replaced", got) + } +} diff --git a/modelsdk/mpr/writer_deferred.go b/modelsdk/mpr/writer_deferred.go index 4b464330c..47617366b 100644 --- a/modelsdk/mpr/writer_deferred.go +++ b/modelsdk/mpr/writer_deferred.go @@ -50,6 +50,12 @@ func (w *Writer) DeferUnitWrites() { } } +// HoldsWrites reports whether a deferred run or a simulation is open, holding +// unit bytes that are not on disk. +func (w *Writer) HoldsWrites() bool { + return w.deferred != nil || w.simulating +} + // FlushDeferredWrites ends the run: every held unit is written once, through the // ordinary path (reconciliation, elision, the storage-GUID guard), in the order // it was first written. The first error is returned; the remaining units are From 5e9c54c219abeec429fda6530289c762af7ba192 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 11 Nov 2026 20:19:39 +0000 Subject: [PATCH 05/24] fix: auto-layout puts merges inside the activity before them and rejoins handlers backwards The layout measurer and the flow builder disagreed in three shapes (ako/mxcli#1773), so an object landed on or behind its neighbour while `layout flows` reported the flow as already laid out: 1. An IF was measured from its split's centre as if that were its left edge, 45px short, and a decision that opens a run a further 15px short. An if/else followed by an activity in a THEN branch put the enclosing merge inside that activity. An IF/ELSE without a merge (one branch returns, or the tail of a loop body) was measured with one. 2. A `join` was measured as an activity plus an end event although it draws nothing, so a handler row ending in joins reserved about twice its width. A both-return IF at a branch's end likewise gained a phantom end event, and a `merge` label was measured as an activity. 3. A falling-through handler of two or more statements rejoined at a merge placed midway between its activity and the next one, left of the row's last activity, so the edge ran backwards. The merge now goes one gap past the row and the main line waits for it. The #1768 invariant test now runs every shape with no skips on the main line, in loop bodies and in nested branches, and compares each loop's measured width with the box the builder fits around what it built. 10 of 376 doctype-example flows and 4 of 29 fixture flows move; no golden test changed. mx check 0 errors on 11.14.0 after layout flows. Refs ako/mxcli#1773 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82sv2odH8kVjmGzPosB --- ...-if-join-rejoin-disagree-with-builder.json | 1 + docs-wiki/bug-patterns/flow-graph-geometry.md | 10 + .../1773-layout-measurer-if-join-rejoin.mdl | 82 ++++++++ mdl/executor/cmd_microflows_builder.go | 4 + .../cmd_microflows_builder_control.go | 6 +- mdl/executor/cmd_microflows_builder_flows.go | 11 +- .../cmd_microflows_builder_nested_test.go | 2 +- mdl/executor/layout.go | 128 +++++++++---- mdl/executor/layout_handler_lane_1768_test.go | 120 ++++++++---- mdl/executor/layout_lanes.go | 86 ++++++++- mdl/executor/layout_measurer_1773_test.go | 177 ++++++++++++++++++ 11 files changed, 541 insertions(+), 86 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-11-11-measurer-if-join-rejoin-disagree-with-builder.json create mode 100644 mdl-examples/bug-tests/1773-layout-measurer-if-join-rejoin.mdl create mode 100644 mdl/executor/layout_measurer_1773_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-11-11-measurer-if-join-rejoin-disagree-with-builder.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-11-11-measurer-if-join-rejoin-disagree-with-builder.json new file mode 100644 index 000000000..ce1595597 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-11-11-measurer-if-join-rejoin-disagree-with-builder.json @@ -0,0 +1 @@ +{"area":"mdl/executor","date":"2026-11-11","symptom":"Auto-layout put objects on or behind their neighbours and `layout flows` called the flow already laid out (ako/mxcli#1773): an if/else followed by an activity inside a THEN branch put the enclosing merge inside that activity (5px reported; 20px when the decision opens the branch); a handler row ending in `join` reserved about twice its drawn width; a falling-through handler of two or more statements rejoined at a merge left of the row's last activity, so the edge ran backwards. mx check 0 errors in every case.","cause":"Four measurer/builder disagreements. measureIfStatement measured from the split's centre as if it were the left edge (45px short) and gapBetween compensated after the IF only; measureStatements started every run at the left edge although the builder centres a first split where an activity's centre would be (15px short); measureBranch added an end event after any terminal statement, a join and a both-return IF included; and the rejoin merge was placed midway between the activity and the next one by mergeOnEdge while the row ran on past it. An IF/ELSE without a merge (one branch returns, or tailWithoutMerge at a loop body's end) was measured with a merge too.","file":"mdl/executor/layout.go (measureRun, measureIf, gapBetween, lastStmtDrawsEndEvent), mdl/executor/layout_lanes.go (ifHasMerge, ifJoinsNothingAtTail, ifMainWidth, rejoinsPastRow, rejoinAfterRow), mdl/executor/cmd_microflows_builder_flows.go (finishCustomErrorHandler, addErrorHandlerRejoinFlowForState)","insight":"The cheapest exact oracle for a measurer is a loop: fitContainerSize sizes the box from what was BUILT, measureLoopStatement from what was MEASURED, so wrapping any run in `loop … end loop` and comparing the two widths catches every disagreement to the pixel, including over-measures that never overlap anything (the join case). It found two defects the issue did not name (the first-element offset, and a merge counted for merge-less IF/ELSEs). Distinguish the main-line cursor from the drawn extent: an IF without a merge resumes at a point that is not an edge of anything, and folding that point into the extent re-introduced an over-measure at a body's tail. A `merge` label statement advances only half a pitch, so gapBetween's 20px floor over-measured it; MDL refuses join/merge in loops (MDL-FLOW04), but the oracle still applies because a run is measured the same way wherever it is. Moves: 10 of 376 doctype-example flows (02c CX_SYNC_FactoryTelemetry 5465->4315 wide, CX_VAL_Factory, CX_VAL_EmailTemplate, CX_ACT_BatchProcessFactories, CX_ACT_HandleAuthCode; 02 M008_NestedIf, M093_GetCountryLabel; 07 M020, M050, M053) and 4 of 29 fixture flows; no golden test changed. Not this class: vertical measurement (a Bounds height is not symmetric about the line, so an if-without-else inside nested loops pokes out of the outer box, MPR011) and a multi-value CASE outcome's own merge drawn on its first activity (documented at firstCaseGroupShift).","refs":["ako/mxcli#1773","ako/mxcli#1768","ako/mxcli#684"],"test":"mdl/executor/layout_measurer_1773_test.go, mdl/executor/layout_handler_lane_1768_test.go"} diff --git a/docs-wiki/bug-patterns/flow-graph-geometry.md b/docs-wiki/bug-patterns/flow-graph-geometry.md index 3a1d12a62..fb97388c4 100644 --- a/docs-wiki/bug-patterns/flow-graph-geometry.md +++ b/docs-wiki/bug-patterns/flow-graph-geometry.md @@ -43,6 +43,16 @@ outside it — worse than a box that is too wide. Those runs fall back to the conservative measure deliberately. The containment check is the honest test: every child of a looped activity must lie inside the container's box. +**The measurer and the builder must agree, and a loop box says whether they +do.** Whatever follows a body — an IF's closing merge, the next row, a loop's +box — is placed from the body's *measured* width, while the box around a loop +is fitted to what was *built*. Wrapping a run in a loop and comparing the two +widths is an exact oracle that needs no expected coordinates, and it catches +the over-measures no overlap check can see. Every "already laid out" report +from `layout flows` is the builder agreeing with itself, so it proves nothing +about this class. Keep the main-line cursor apart from the drawn extent: an IF +without a merge resumes at a point that is not the edge of anything. + **Wiring: the invalid shapes come from branches that do not merge.** A decision whose non-terminal branch falls off the end of a loop body, a `break` nested inside an `if`, a split followed by a statement — each is a place where a diff --git a/mdl-examples/bug-tests/1773-layout-measurer-if-join-rejoin.mdl b/mdl-examples/bug-tests/1773-layout-measurer-if-join-rejoin.mdl new file mode 100644 index 000000000..35337a185 --- /dev/null +++ b/mdl-examples/bug-tests/1773-layout-measurer-if-join-rejoin.mdl @@ -0,0 +1,82 @@ +mdl 1; +-- ============================================================================ +-- ako/mxcli#1773: the layout measurer disagreed with the flow builder in three +-- shapes, so an object landed on or behind its neighbour, and `layout flows` +-- reported such a flow as already laid out because it uses the same builder. +-- +-- 1. "An IF is measured 45 px short. The measurer counts the split's centre as +-- its left edge. Take an if/else followed by an activity inside a THEN +-- branch: the enclosing merge ends up 5 px inside that activity." +-- A decision that opens a branch was a further 15px short: the builder +-- centres it where an activity's centre would be, and a split is narrower. +-- 2. "A `join` is measured as an activity plus an end event, though it draws +-- nothing. A handler row that ends in joins reserves roughly twice its +-- drawn width." +-- 3. "A fall-through error handler of two or more statements rejoins +-- backwards." The merge stood between the activity and the next one, left +-- of the row's last activity. It now goes one gap past the row, and the +-- main line waits for it. +-- +-- Geometry is invisible below Studio Pro: every flow here passes `mx check` +-- either way. Execute without @position and lint: MPR008 must report nothing. +-- +-- mxcli exec 1773-layout-measurer-if-join-rejoin.mdl -p app.mpr +-- mxcli lint -p app.mpr --rules MPR008,MPR011 +-- +-- Regression coverage: mdl/executor/layout_measurer_1773_test.go and +-- mdl/executor/layout_handler_lane_1768_test.go (now without skips). +-- ============================================================================ + +create or modify module Bug1773; + +create or modify persistent entity Bug1773.Item ( "Code": String(50) ); + +-- Shape 1: before the fix the closing merge of the outer IF stood inside the +-- 'last in then' activity. +create or modify microflow Bug1773.NestedDecisionInBranch ($Flag: Boolean, $Other: Boolean) +begin + if $Other then + if $Flag then + log info node 'Bug1773' 'yes'; + else + log info node 'Bug1773' 'no'; + end if; + log info node 'Bug1773' 'last in then'; + else + log info node 'Bug1773' 'else'; + end if; + log info node 'Bug1773' 'after'; +end; + +-- Shape 2: handler rows that end in a join, in a THEN branch whose closing +-- merge is placed from the branch's measured width. +create or modify microflow Bug1773.HandlerRowsEndingInJoins ($Flag: Boolean) +begin + if $Flag then + retrieve $A from Bug1773.Item on error without rollback begin + log error node 'Bug1773' 'a1'; + join recovered; + end error; + retrieve $B from Bug1773.Item on error without rollback begin + log error node 'Bug1773' 'b1'; + log error node 'Bug1773' 'b2'; + join recovered; + end error; + log info node 'Bug1773' 'between'; + else + log info node 'Bug1773' 'else'; + end if; + merge recovered; + log info node 'Bug1773' 'done'; +end; + +-- Shape 3: a handler of three statements that falls through to the main line. +create or modify microflow Bug1773.LongHandlerRejoins () +begin + retrieve $A from Bug1773.Item on error without rollback begin + log error node 'Bug1773' 'a1'; + log error node 'Bug1773' 'a2'; + log error node 'Bug1773' 'a3'; + end error; + log info node 'Bug1773' 'next'; +end; diff --git a/mdl/executor/cmd_microflows_builder.go b/mdl/executor/cmd_microflows_builder.go index 56512ac31..8be99934b 100644 --- a/mdl/executor/cmd_microflows_builder.go +++ b/mdl/executor/cmd_microflows_builder.go @@ -185,6 +185,10 @@ type flowBuilder struct { // rejoins the normal path, keyed by the activity. describe folds that merge // away into the fall-through form and carries its position here (#750). rejoinMergeAt map[model.ID]*ast.Position + // rejoinPastRowX is the x of the merge an activity's falling-through + // handler row rejoins at when the row is too long to rejoin before the next + // statement (rejoinAfterRow). An @merge in rejoinMergeAt still wins. + rejoinPastRowX map[model.ID]int // labelReg holds the `merge