From bb82348d7f9bd11baabeac3d1cab4b1d0cb107dd Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 03:06:35 +0000 Subject: [PATCH 01/11] docs(fix-issue): restore the microflow loop symptom rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These two rows were written with the #791 / #790 fixes but dropped from that PR so it would stop touching the shared symptom table: every split PR appended at the same anchor, so each merge invalidated the rest and the branch had to be re-synced twice. With both merged there is nothing left to conflict with, so they go back in unchanged. They cover the break/continue serialization gap — including the "dump the microflow and check every *Pointer resolves" recipe for diagnosing a dangling reference without Studio Pro, and the note that a check-time rule existing only as a stopgap for a write-path bug should be removed with the real fix — and the loop-box over-measurement, with the warning not to guess the placement advance for compound elements. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012XR649rKk68z6gBpngu6MA --- .claude/skills/fix-issue.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index 58a93358a..5b7fa27db 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -16,6 +16,8 @@ to the symptom table below, so the next similar issue costs fewer reads. | Symptom | Root cause layer | First file to open | Fix pattern | |---------|-----------------|-------------------|-------------| +| A generated loop box is drawn far wider than the activities inside it (e.g. 880px around 440px of content), leaving a large empty area — "the visualization is poor" | `measureStatements` sums each element's full width **and** adds `HorizontalSpacing` between them, but `HorizontalSpacing` is a centre-to-centre *pitch*: the builder centres each activity on `posX` and advances by exactly that. Counting it on top of each width over-measures a run of n simple activities by `(n-1)*ActivityWidth` | `mdl/executor/layout.go` (`measureStatementsSpan`), used by `addLoopStatement`/`addWhileStatement` in `cmd_microflows_builder_control.go` | For a run of only simple activities the true span is `(n-1)*HorizontalSpacing + ActivityWidth`. **Do not guess the advance for a compound element** (IF/split, nested loop) — its `posX` advance comes from merge geometry (`mergeX + MergeSize + HorizontalSpacing/2`), and guessing under-sizes the box so activities land *outside* it, which is worse than a box that is too wide; those runs fall back to the conservative measure. Verify with a containment check: every child of a LoopedActivity must lie within `[0, width] × [0, height]`. Issue #790 | +| A microflow writes and `mxcli` reports success, but Studio Pro cannot open the project: `System.Collections.Generic.KeyNotFoundException: The given key '' was not present in the dictionary`. Triggered by a `break`/`continue` inside an if/case within a loop | `microflowObjectToGen`'s `default:` branch returns nil for an unhandled object type, so the event object was dropped at serialization while the SequenceFlow pointing at it was written — a DestinationPointer to a GUID that exists nowhere. Exactly the shape already fixed once for `Microflows$ErrorEvent` | `mdl/backend/modelsdk/microflow_write.go` (`microflowObjectToGen`) — compare against `sdk/mpr/writer_microflow.go`, which handles the full set | Add the missing `case *microflows.XxxEvent:` returning the gen element with ID, position and size. **Diagnose without Studio Pro**: dump the microflow (`mxcli bson dump --type microflow --object M.F`), collect every `$ID`, and check every key ending in `Pointer` resolves — a dangling pointer is this bug. When a check-time rule exists only as a stopgap for a write-path bug (MDL051 was), remove it with the real fix, and check whether it covered every affected keyword (MDL051 covered `break`, not `continue`, which is how #791 reached users). Repro `mdl-examples/bug-tests/791-loop-continue-dangling-flow.mdl`. Issue #791, ledger #52 | | `mxcli test` leaves the project mutated: Security Level changed, after-startup pointing at the deleted `MxTest.TestRunner`, and only a `Warning:` line about it. Or: an empty `MxTest` module accumulates after every run | Three defects in one teardown. (a) `getAfterStartup` trimmed quotes *before* trailing punctuation, so a DESCRIBE SETTINGS line ending in `,` yielded `Module.Flow',` and the restore statement was unparseable; (b) cleanup dropped only the microflow, not the module it created; (c) the Security Level was forced OFF and restored to a hardcoded PRODUCTION | `cmd/mxcli/testrunner/runner.go` (`parseSettingValue`, `quoteMDLString`, `projectState`, `setupCommands`, `cleanupCommands`) | Strip trailing `,;` before unquoting; re-emit via `quoteMDLString` (doubling embedded quotes, never backslashes); capture a `projectState` before the first mutation and restore from it; drop the module only when the run created it (a pre-existing `MxTest` is the user's); leave Security Level alone entirely; return cleanup errors instead of printing warnings, and fail the run when they occur. Command lists are pure functions so the restore is testable without a project or Docker. Issues #802/#803/#804 | | `create or modify external entities` silently resets a per-entity setting the user had changed (e.g. allow-create-change-locally) | `applyExternalEntityFields` stamps every field on both the create and the update path, so anything not derivable from the OData contract was overwritten with a default | `mdl/executor/cmd_contract.go` (`applyExternalEntityFields`) | Separate contract-derived fields (Countable/Creatable/Deletable/Skip/Top — refresh from metadata) from local modelling choices (CreateChangeLocally — leave alone; a new entity arrives zero-valued, which is Mendix's default). Issue #782 | | An **external (OData) entity** loses its remote settings on any read-modify-write — `describe external entity` says `is not an external entity (source: )`, and `alter entity … set allow_create_change_locally = true` reports success but the flag stays off. Works under `--engine legacy` | `entityFromGen` recognised only `DomainModels$OqlViewEntitySource`, so the three `Rest$OData*` sources read back as no source at all: `Source` empty, every remote field zeroed. The write path was fine — it switches on `e.Source`, which the read never populated | `mdl/backend/modelsdk/domainmodel.go` (`entityFromGen`'s source switch, `odataKeyFromGen`) | Mirror the legacy parser (`sdk/mpr/parser_domainmodel.go`) for all three flavours: RemoteEntitySource (capabilities + CreateChangeLocally + key), EntityTypeSource (type name + IsOpen + key), PrimitiveCollectionEntitySource (service only). `Updatable` has no gen accessor and the writer does not emit it — leaving it zero is symmetric. **Check the read side first when a write-path field "does not stick"**: a switch on a field the read never fills looks like a write bug. Repro `mdl-examples/bug-tests/782-external-entity-create-change-locally.mdl`. Issue #782 | From 93210064adaa87af2169747eaeb9672373322f11 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 05:26:41 +0000 Subject: [PATCH 02/11] fix(docker): stop `docker build` converting MPRv2 projects to MPRv1 (#808) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `mx update-widgets` rewrites an MPRv2 project into the self-contained MPRv1 storage format: it inlines every unit into the .mpr (adding a Unit.Contents column) and deletes mprcontents/. A command that checks or builds must not mutate the source project's on-disk format — doing so silently desyncs the working tree from a Git repository that tracks the mprcontents/ files, breaks a running `mxcli run --local` watch loop, and has been observed to leave Studio Pro unable to open the project. #763 reported this for `docker check` and PR #764 fixed it there, by wrapping the invocation inside Check with a snapshot/restore. But build.go carried its own bare copy of the same invocation, so the conversion continued through `mxcli docker build`, `docker run` and `docker reload` — all three reporting success while rewriting the project. The guard now lives on the operation rather than on a call site. runUpdateWidgets snapshots the v2 storage, runs the step, and returns the restore func for the caller to defer; both call sites are one line and there is exactly one place in the tree that may exec `update-widgets`. A third caller cannot reintroduce the bug by forgetting the snapshot. Restore stays deferred rather than immediate: the caller's `mx check` and MxBuild have to see the widget-normalized model, or the false CE0463 errors the step exists to suppress come straight back. Only the on-disk format is put back, after the caller is done with the model. Note on scope: the issue reports `mxcli test` as affected, giving this more exposure than #763. It is not — update-widgets sits inside build.go's `if !opts.SkipCheck` block and testrunner invokes `docker build --skip-check`. `run --local` is also unaffected; it goes through mxserve and never calls update-widgets. The three affected commands are `docker build`, `docker run` and `docker reload`, all at their default settings. Tests: unit coverage for the four paths through runUpdateWidgets (v2 conversion undone; v1 needs no snapshot and restore is a no-op; an unsnapshottable v2 project skips the step entirely rather than risking an unrecoverable conversion; a failed step still restores). Removing the protection fails three of the four. Plus an integration counterpart to TestCheck_PreservesMPRv2StorageFormat that drives Build with DryRun — which stops right after the check step, exercising the whole buggy path without paying for a full MxBuild. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012XR649rKk68z6gBpngu6MA --- .claude/skills/fix-issue.md | 2 +- cmd/mxcli/docker/build.go | 17 +- cmd/mxcli/docker/build_integration_test.go | 75 +++++++ cmd/mxcli/docker/check.go | 101 +--------- cmd/mxcli/docker/update_widgets.go | 134 +++++++++++++ cmd/mxcli/docker/update_widgets_test.go | 217 +++++++++++++++++++++ 6 files changed, 441 insertions(+), 105 deletions(-) create mode 100644 cmd/mxcli/docker/build_integration_test.go create mode 100644 cmd/mxcli/docker/update_widgets.go create mode 100644 cmd/mxcli/docker/update_widgets_test.go diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index 58a93358a..ba1fe6d2f 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -107,7 +107,7 @@ to the symptom table below, so the next similar issue costs fewer reads. | CE0463 on a `datagrid` whose column uses `ColumnWidth: manual` + `Size: N` — Studio Pro resets the column `size` to `1` | The MDL `ColumnWidth:` keyword isn't mapped to the schema `width` enum, so `width` stays at its `autoFill` default; `size` only applies when `width=manual`, so the value is inconsistent. Regression from the Stream B keyword-path consolidation (the deleted `datagrid_builder.go` did `colPropString(col.Properties, "ColumnWidth", "autoFill")`) | `mdl/executor/widget_defs.go` `itemPropertyAliases` | Add the MDL→schema alias under `[datagrid]["columns"]`: `"width": {"ColumnWidth"}`. Bump `WidgetDefGeneratorVersion` so stale `.def.json` regenerate. General rule when a column/object-list property's MDL keyword differs from the `.mpk` schema key (not just case), add it to `itemPropertyAliases`; cross-check against the pre-B3 `datagrid_builder.go` `colProp*` calls for any other dropped mappings | | CE0463 on an MDL-created **Combobox** (or other platform widget) at `mxcli docker build/check` time — but the SAME widget passes `mx check` on a project whose installed `.mpk` matches mxcli's embedded template (Mendix 11.6 / combobox 2.5.0). NOT the old incomplete-template bug (#112, fixed — a matching-version combobox is clean) | Widget-VERSION mismatch: mxcli emits the embedded 2.5.0-shaped PropertyTypes, but the project has a NEWER combobox (e.g. 2.8.1) that reorders/regroups properties. `augmentFromMPK` patches presence + enum values but can't restructure the baseline; `GenerateFromMPK` is less faithful still (fails even on a matching version). The designed remediation is `mx update-widgets` (docker build/check runs it before `mx check`) | `cmd/mxcli/docker/check.go` + `build.go` (`updateWidgetsPathArg`) — the update-widgets *invocation*, not the widget emission | The real trap was that `mx update-widgets ` **crashed** (`AddProjectDirAsAllowedPath` → `Path.GetDirectoryName("app.mpr")` = "" → `System.ArgumentNullException`) and some mx builds exit 0 after printing it, so the migration silently no-op'd and CE0463 survived. Pass an **absolute** path to update-widgets (always has a directory component). `mx check` is unaffected. Diagnosis: run the bundled `mx update-widgets ` yourself — if it throws ArgumentNullException on AddProjectDirAsAllowedPath, the path lacks a dir. Faithful multi-version widget emission is the larger fix (#529). Issue #112; repro `mdl-examples/bug-tests/112-combobox-enum-ce0463-widget-version.mdl` | | **CE0463 "widget definition changed" at an `Image` widget** on Mendix 11.7+ (`mx check` without `update-widgets`) — a plain mxcli-authored Image, no custom config. `update-widgets` clears it (and on v2 destroys `mprcontents/`, so it's a data-loss trap) | NOT a version stamp, Type `$ID`s, property order, or missing properties (all ruled out empirically). The embedded `image.json` carried a **spurious default value**: a `WidgetValue.Image` pointing at `Atlas_Core.Content.Mendix` (Atlas's Mendix logo), captured when the template was extracted from a project that had it set. The installed 11.12 Image widget expects that field empty, so MxBuild flags the definition as changed | `modelsdk/widgets/templates/mendix-11.6/image.json` + `sdk/widgets/templates/mendix-11.6/image.json` (line ~63) | Clear the stale default: `"Image": "Atlas_Core.Content.Mendix"` → `"Image": ""` in both engine templates. **Diagnosis method for this whole CE0463 class**: dump the widget BSON, `mx update-widgets` on a COPY, dump again, diff the `CustomWidgets$CustomWidget` subtree **order-independently** (canonicalise key order + mask `$ID`/`TypePointer` blobs). Reordering and generic instance chrome (`LabelTemplate`, `Appearance.DesignProperties`) are cosmetic — a *passing* widget gets reordered/those-added too; the real cause is whatever value/structure survives that normalisation. Other hand-extracted templates likely hide similar stale defaults (audit with the same diff). Repro `mdl-examples/bug-tests/image-ce0463-stale-default.mdl`. DataGrid2 custom-content CE0463 (#600) is a *separate*, more complex delta — same method, own fix | -| `mxcli docker check`/`build` (or a bare `mx update-widgets`) **silently deletes `mprcontents/`** and rewrites an MPRv2 project into single-file v1 — `check` reports **0 errors** and looks successful, but the git working tree diverges from tracked files, a running `mxcli run --local` loop breaks (it watches `mprcontents/`), and Studio Pro may crash on open (`LibGit2RepositoryProvider.WriteBaseFile`). Triggered by following the CE0463 remediation on a `mxcli new` (always v2) project | The pre-check `mx update-widgets` step (run to suppress false CE0463) **performs the conversion**: it inlines every unit into the `.mpr` (`Unit.Contents` column) and deletes `mprcontents/`. The `check` itself is read-only; `update-widgets` is the mutator. docker check/build invoked it with no storage-format protection | `cmd/mxcli/docker/check.go` (`snapshotStorageFormat` / `copyFile` / `copyDir`) + `build.go` | Snapshot `.mpr` + `mprcontents/` to a temp dir before `update-widgets`, `defer restore()` after the check (restore removes the post-conversion single-file `.mpr` residue and puts the v2 tree back); MPRv1 projects need no protection. The check still runs against the widget-normalized model, so CE0463 stays suppressed — only the on-disk format is preserved. **Never tell an agent to run bare `mx update-widgets` on a v2 project** — the synced skills (`create-page.md`, `custom-widgets.md`, `migrate-design-prototype.md`, `download-marketplace-content.md`) + dev `debug-bson.md` route to `mxcli docker check`/`build` (v2-safe) instead. Issue #763 / PR #764 | +| `mxcli docker check`/`build` (or a bare `mx update-widgets`) **silently deletes `mprcontents/`** and rewrites an MPRv2 project into single-file v1 — `check` reports **0 errors** and looks successful, but the git working tree diverges from tracked files, a running `mxcli run --local` loop breaks (it watches `mprcontents/`), and Studio Pro may crash on open (`LibGit2RepositoryProvider.WriteBaseFile`). Triggered by following the CE0463 remediation on a `mxcli new` (always v2) project | The pre-check `mx update-widgets` step (run to suppress false CE0463) **performs the conversion**: it inlines every unit into the `.mpr` (`Unit.Contents` column) and deletes `mprcontents/`. The `check` itself is read-only; `update-widgets` is the mutator. docker check/build invoked it with no storage-format protection | `cmd/mxcli/docker/update_widgets.go` (`runUpdateWidgets` / `snapshotStorageFormat`) — call sites in `check.go` and `build.go` | Snapshot `.mpr` + `mprcontents/` to a temp dir before `update-widgets`, `defer restore()` after the check (restore removes the post-conversion single-file `.mpr` residue and puts the v2 tree back); MPRv1 projects need no protection. The check still runs against the widget-normalized model, so CE0463 stays suppressed — only the on-disk format is preserved. **Never tell an agent to run bare `mx update-widgets` on a v2 project** — the synced skills (`create-page.md`, `custom-widgets.md`, `migrate-design-prototype.md`, `download-marketplace-content.md`) + dev `debug-bson.md` route to `mxcli docker check`/`build` (v2-safe) instead. **Fix the operation, not the call site**: PR #764 wrapped the invocation inside `Check` only, and `Build` had a second bare copy — so `docker build`/`run`/`reload` kept converting projects for another 40 issues, until #808. The snapshot now lives in `runUpdateWidgets`, which is the only place that may exec `update-widgets`; grep for `"update-widgets"` should return exactly one hit. When a mutating external step is guarded, put the guard in a function that also *performs* the step, so a new caller cannot get it wrong. Issues #763 / PR #764, #808 | | Nightly `mx check` reports `CE0117 "Error(s) in expression." at Log message activity 'Log message (warning)'` on Mendix 10.24.19+ but not 10.24.16 or 11.x | Mendix 10.24.19 tightened expression validation: `toString()` is now a type error (toString expects a non-string input). An example called `toString($OrderNumber)` where `$OrderNumber` was already a string parameter | The offending `log warning ... with ({1} = toString($stringVar))` — find via `~/.mxcli/mxbuild/{ver}/modeler/mx check`, then bisect with `drop microflow ...` until CE0117 disappears | Remove the redundant `toString()` wrapper around already-string values. Only wrap non-string values (integers, decimals, dates, enums) in `toString()`. The Mendix 11.x parser is more lenient and lets this slide, but 10.24.19+ rejects it | | A page-level property can't be set — `ALTER PAGE X { SET PopupWidth = 800; }` (or any page-level prop other than Title/Url) fails with `unsupported page-level property: …` | The page-level SET handler only special-cased a couple of properties; everything else fell through to the default error | `mdl/backend/pagemutator/mutator.go` → `applyPageLevelSetMut` (shared by both engines) | Add a `case` writing the field at the top level of the Forms$Page doc via `dSetOrAppend` with the on-disk BSON type (int64 for PopupWidth/PopupHeight, bool for PopupResizable — verify against a Studio Pro page with `mxcli bson dump --format bson`). Page-level prop names are **case-sensitive**. For DESCRIBE roundtrip, emit the values back in the CREATE PAGE header. CREATE-time support: add a generic `IDENTIFIER COLON propertyValueV3` to `pageHeaderPropertyV3` (regen grammar), recognize the keys in `parsePageHeaderV3` (`applyGenericPageHeaderProp`, error on unknown), carry `*int`/`*bool` on `CreatePageStmtV3`, default to 600/600/false in `buildPageV3`, and have both writers honour `page.Popup*` (legacy `sdk/mpr/writer_pages.go` int64; codec `mdl/backend/modelsdk/page_write.go` int32 via gen — tolerated by mx check). The MCP backend has its **own** `mcpPageMutator` (pg content tree, not raw BSON) — page-level SET there reaches `SetWidgetProperty("")`; map it or reject honestly (it rejects, pending a `pg_read_page` probe of the pop-up keys). Issue #661 | | `PopupWidth: 0` / `PopupHeight: 0` rejected ("must be a positive number") on CREATE or ALTER PAGE; user can't make an auto-size pop-up | 0 is actually Studio Pro's **default** for pop-up dimensions (auto-size) — verified live on 11.12: a pg-created PopupLayout page stores 0/0 and `mx check` = 0 errors. Two validators rejected ≤0, and both writers coerced ≤0→600, so even an allowed 0 became 600 | `mdl/visitor/visitor_page_v3.go` (`popupDimensionValue`) + `mdl/backend/pagemutator/mutator.go` (`coercePopupDimension`) + `mdl/executor/cmd_pages_builder_v3.go` (builder default) + `sdk/mpr/writer_pages.go` & `mdl/backend/modelsdk/page_write.go` (`popupDimension`) + `mdl/executor/cmd_pages_describe.go` | Relax both validators to reject only **negative**; default the builder to **0** (not 600, matching Studio Pro); drop the `≤0→600` coercion in both writers (clamp only negatives to 0); have DESCRIBE suppress only the real default 0 (emit an explicit 600). No `*int` needed — 0 is a valid stored value. Bug-test `mdl-examples/bug-tests/713-popup-zero-dimensions.mdl`. Issue #713 | diff --git a/cmd/mxcli/docker/build.go b/cmd/mxcli/docker/build.go index b70b3b317..df63a5158 100644 --- a/cmd/mxcli/docker/build.go +++ b/cmd/mxcli/docker/build.go @@ -100,16 +100,15 @@ func Build(opts BuildOptions) error { if err != nil { fmt.Fprintf(w, " Skipping check: %v\n", err) } else { - // Run update-widgets before check to prevent false CE0463 errors + // Run update-widgets before check to prevent false CE0463 errors. + // runUpdateWidgets preserves the project's on-disk storage format: the bare + // invocation this replaced converted MPRv2 projects to MPRv1 and deleted + // mprcontents/ (mendixlabs/mxcli#808). restore is deferred to Build's exit + // rather than run here, so both `mx check` and MxBuild below see the + // widget-normalized model; only the on-disk format is put back. if !opts.SkipUpdateWidgets { - fmt.Fprintln(w, " Updating widget definitions...") - uwCmd := exec.Command(mxPath, "update-widgets", updateWidgetsPathArg(opts.ProjectPath)) - uwCmd.Stdout = w - uwCmd.Stderr = os.Stderr - PrepareMxCommand(uwCmd) - if err := uwCmd.Run(); err != nil { - fmt.Fprintf(w, " Warning: update-widgets failed (continuing): %v\n", err) - } + restore := runUpdateWidgets(mxPath, opts.ProjectPath, w, os.Stderr) + defer restore() } cmd := exec.Command(mxPath, "check", opts.ProjectPath) diff --git a/cmd/mxcli/docker/build_integration_test.go b/cmd/mxcli/docker/build_integration_test.go new file mode 100644 index 000000000..3eb8b28d7 --- /dev/null +++ b/cmd/mxcli/docker/build_integration_test.go @@ -0,0 +1,75 @@ +// SPDX-License-Identifier: Apache-2.0 + +//go:build integration + +package docker + +import ( + "bytes" + "os" + "os/exec" + "path/filepath" + "testing" + + "github.com/mendixlabs/mxcli/sdk/mpr" +) + +// TestBuild_PreservesMPRv2StorageFormat is the end-to-end guard for +// mendixlabs/mxcli#808, the counterpart to TestCheck_PreservesMPRv2StorageFormat +// (#763). Build's pre-check step ran its own bare `mx update-widgets`, which +// rewrites an MPRv2 project into the self-contained MPRv1 format — inlining every +// unit into the .mpr and deleting mprcontents/. #764 protected Check only, so +// `mxcli docker build`, `docker run` and `docker reload` kept converting projects +// while reporting success. +// +// DryRun stops Build immediately after the check step, so this exercises the whole +// buggy path (update-widgets + mx check) without paying for a full MxBuild. The +// deferred restore still runs on the DryRun return. +// +// Requires a resolvable mx/MxBuild and a JDK 21 (provided by the CI integration +// job); skips otherwise. +func TestBuild_PreservesMPRv2StorageFormat(t *testing.T) { + mxPath, err := ResolveMx("") + if err != nil { + t.Skipf("mx not resolvable: %v", err) + } + if _, err := resolveJDK21(); err != nil { + t.Skipf("JDK 21 not resolvable: %v", err) + } + + // Scaffold a fresh project. `mx create-project` (no template arg) writes App.mpr + // into the working directory and produces MPRv2 storage. + dir := t.TempDir() + scaffold := exec.Command(mxPath, "create-project") + scaffold.Dir = dir + if out, err := scaffold.CombinedOutput(); err != nil { + t.Skipf("mx create-project failed, cannot scaffold fixture: %v\n%s", err, out) + } + mprPath := filepath.Join(dir, "App.mpr") + if _, err := os.Stat(mprPath); err != nil { + t.Skipf("mx create-project did not produce App.mpr: %v", err) + } + + // Precondition: the fixture must be MPRv2, or the test proves nothing. + if v := mprStorageVersion(t, mprPath); v != mpr.MPRVersionV2 { + t.Skipf("scaffolded project is %v, not MPRv2 — nothing to protect", v) + } + + var stdout bytes.Buffer + if err := Build(BuildOptions{ + ProjectPath: mprPath, + DryRun: true, + Stdout: &stdout, + }); err != nil { + t.Fatalf("Build failed: %v\nstdout:\n%s", err, stdout.String()) + } + + // Postcondition: still MPRv2. Without the fix, update-widgets would have left it + // MPRv1 with mprcontents/ deleted. + if v := mprStorageVersion(t, mprPath); v != mpr.MPRVersionV2 { + t.Errorf("Build converted the project to %v; the MPRv2 storage format must be preserved (#808)", v) + } + if _, err := os.Stat(filepath.Join(dir, "mprcontents")); err != nil { + t.Errorf("mprcontents/ missing after Build, storage format was not preserved: %v", err) + } +} diff --git a/cmd/mxcli/docker/check.go b/cmd/mxcli/docker/check.go index 8696bed3b..96ede6750 100644 --- a/cmd/mxcli/docker/check.go +++ b/cmd/mxcli/docker/check.go @@ -34,58 +34,6 @@ type CheckOptions struct { Stderr io.Writer } -// updateWidgetsPathArg returns an absolute form of the .mpr path for the -// `mx update-widgets` invocation. MxToolset's AddProjectDirAsAllowedPath computes -// Path.GetDirectoryName(mprFilePath) to whitelist the project directory; given a -// bare filename (e.g. "app.mpr", as passed by `mxcli docker build -p app.mpr` run -// from the project dir) that returns "" → null and the tool throws -// System.ArgumentNullException, silently skipping the widget migration. That in -// turn leaves CE0463 "widget definition changed" errors unresolved at check time. -// An absolute path always has a directory component. `mx check` is unaffected, so -// only the update-widgets arg is normalized. Falls back to the input if Abs fails. -func updateWidgetsPathArg(p string) string { - if abs, err := filepath.Abs(p); err == nil { - return abs - } - return p -} - -// snapshotStorageFormat backs up the MPRv2 storage files (.mpr index + mprcontents/) -// to a temp directory and returns a restore function that puts them back, undoing -// any v2 -> v1 conversion performed by an intervening `mx update-widgets`. The -// restore function removes the temp directory and is safe to defer; it best-effort -// restores and never panics. mprPath and contentsDir come from an mpr.Reader on a -// project already known to be MPRv2. -func snapshotStorageFormat(mprPath, contentsDir string) (restore func(), err error) { - tmp, err := os.MkdirTemp("", "mxcli-mpr-snapshot-*") - if err != nil { - return nil, err - } - - mprBackup := filepath.Join(tmp, filepath.Base(mprPath)) - if err := copyFile(mprPath, mprBackup); err != nil { - os.RemoveAll(tmp) - return nil, err - } - - contentsBackup := filepath.Join(tmp, "mprcontents") - if err := copyDir(contentsDir, contentsBackup); err != nil { - os.RemoveAll(tmp) - return nil, err - } - - restore = func() { - defer os.RemoveAll(tmp) - // Restore the v2 index file. - _ = copyFile(mprBackup, mprPath) - // update-widgets deletes mprcontents/; drop whatever is there now (nothing, - // after a conversion) and restore the backed-up tree. - _ = os.RemoveAll(contentsDir) - _ = copyDir(contentsBackup, contentsDir) - } - return restore, nil -} - // copyFile copies a single file from src to dst, preserving the source file mode. func copyFile(src, dst string) error { info, err := os.Stat(src) @@ -136,50 +84,13 @@ func Check(opts CheckOptions) error { } fmt.Fprintf(w, "Using mx: %s\n", mxPath) - // `mx update-widgets` rewrites an MPRv2 project into the self-contained MPRv1 - // storage format: it inlines every unit into the .mpr and deletes mprcontents/. - // A `check` must not mutate the on-disk storage format — silently doing so - // desyncs the working tree from a Git repository that tracks the mprcontents/ - // files and can leave Studio Pro unable to open the project. So when the project - // is MPRv2, snapshot the .mpr + mprcontents/ before update-widgets and restore - // them after the check. The check still runs against the widget-normalized model - // (so CE0463 false positives are still suppressed); only the on-disk format is - // preserved. MPRv1 projects are already single-file and need no protection. - if !opts.SkipUpdateWidgets && opts.ProjectPath != "" { - if reader, err := mpr.Open(opts.ProjectPath); err == nil { - isV2 := reader.Version() == mpr.MPRVersionV2 - contentsDir := reader.ContentsDir() - reader.Close() - if isV2 { - restore, snapErr := snapshotStorageFormat(opts.ProjectPath, contentsDir) - if snapErr != nil { - // Can't protect the format — skip update-widgets rather than risk - // an unrecoverable v2 -> v1 conversion. A CE0463 false positive is - // the lesser evil than a silent, unrestorable format change. - fmt.Fprintf(w, "Warning: could not snapshot MPRv2 storage (skipping update-widgets to avoid a v2->v1 conversion): %v\n", snapErr) - opts.SkipUpdateWidgets = true - } else { - defer restore() - } - } - } - } - - // Run mx update-widgets to normalize pluggable widget definitions. - // This prevents false CE0463 ("widget definition changed") errors caused - // by mismatch between widget Object properties and Type PropertyTypes. + // Normalize pluggable widget definitions so `mx check` does not report false + // CE0463 ("widget definition changed") errors. runUpdateWidgets preserves the + // project's on-disk storage format; restore is deferred so the check below still + // runs against the widget-normalized model. if !opts.SkipUpdateWidgets { - fmt.Fprintf(w, "Updating widget definitions in %s...\n", opts.ProjectPath) - uwCmd := exec.Command(mxPath, "update-widgets", updateWidgetsPathArg(opts.ProjectPath)) - uwCmd.Stdout = w - uwCmd.Stderr = stderr - PrepareMxCommand(uwCmd) - if err := uwCmd.Run(); err != nil { - // Non-fatal: warn and continue with check - fmt.Fprintf(w, "Warning: update-widgets failed (continuing with check): %v\n", err) - } else { - fmt.Fprintln(w, "Widget definitions updated.") - } + restore := runUpdateWidgets(mxPath, opts.ProjectPath, w, stderr) + defer restore() } // Run mx check diff --git a/cmd/mxcli/docker/update_widgets.go b/cmd/mxcli/docker/update_widgets.go new file mode 100644 index 000000000..748bc8998 --- /dev/null +++ b/cmd/mxcli/docker/update_widgets.go @@ -0,0 +1,134 @@ +// SPDX-License-Identifier: Apache-2.0 + +package docker + +import ( + "fmt" + "io" + "os" + "os/exec" + "path/filepath" + + "github.com/mendixlabs/mxcli/sdk/mpr" +) + +// updateWidgetsPathArg returns an absolute form of the .mpr path for the +// `mx update-widgets` invocation. MxToolset's AddProjectDirAsAllowedPath computes +// Path.GetDirectoryName(mprFilePath) to whitelist the project directory; given a +// bare filename (e.g. "app.mpr", as passed by `mxcli docker build -p app.mpr` run +// from the project dir) that returns "" → null and the tool throws +// System.ArgumentNullException, silently skipping the widget migration. That in +// turn leaves CE0463 "widget definition changed" errors unresolved at check time. +// An absolute path always has a directory component. `mx check` is unaffected, so +// only the update-widgets arg is normalized. Falls back to the input if Abs fails. +func updateWidgetsPathArg(p string) string { + if abs, err := filepath.Abs(p); err == nil { + return abs + } + return p +} + +// updateWidgetsCmd runs the mx invocation. It is a package variable so tests can +// substitute a stub that simulates the v2 -> v1 conversion without needing mx. +var updateWidgetsCmd = func(mxPath, pathArg string, w, stderr io.Writer) error { + cmd := exec.Command(mxPath, "update-widgets", pathArg) + cmd.Stdout = w + cmd.Stderr = stderr + PrepareMxCommand(cmd) + return cmd.Run() +} + +// runUpdateWidgets runs `mx update-widgets` on the project — normalizing pluggable +// widget definitions so the caller's check/build does not report false CE0463 +// ("widget definition changed") errors — while preserving an MPRv2 project's +// on-disk storage format. +// +// The protection is needed because `mx update-widgets` rewrites an MPRv2 project +// into the self-contained MPRv1 format: it inlines every unit into the .mpr (adding +// a Unit.Contents column) and deletes mprcontents/. A command that checks or builds +// must not mutate the source project's storage format — doing so silently desyncs +// the working tree from a Git repository that tracks the mprcontents/ files, breaks +// a running `mxcli run --local` watch loop, and has been observed to leave Studio +// Pro unable to open the project. So on a v2 project the .mpr + mprcontents/ are +// snapshotted first and put back afterwards. MPRv1 projects are already single-file +// and need no protection. +// +// The caller must `defer restore()` rather than calling it immediately: the check / +// MxBuild step has to run against the widget-normalized model, or the CE0463 false +// positives this step exists to suppress come straight back. Only the on-disk +// format is restored, once the caller is done with the model. +// +// restore is never nil, is safe to defer, and never panics. +// +// This lives on the operation, not on a call site, because it was previously +// implemented in `Check` only — `Build` carried its own bare invocation and kept +// converting projects (mendixlabs/mxcli#763, then #808). +func runUpdateWidgets(mxPath, projectPath string, w, stderr io.Writer) (restore func()) { + restore = func() {} + if projectPath == "" { + return restore + } + + if reader, err := mpr.Open(projectPath); err == nil { + isV2 := reader.Version() == mpr.MPRVersionV2 + contentsDir := reader.ContentsDir() + reader.Close() + if isV2 { + snapRestore, snapErr := snapshotStorageFormat(projectPath, contentsDir) + if snapErr != nil { + // Can't protect the format — skip update-widgets rather than risk an + // unrecoverable v2 -> v1 conversion. A CE0463 false positive is the + // lesser evil compared to a silent, unrestorable format change. + fmt.Fprintf(w, "Warning: could not snapshot MPRv2 storage (skipping update-widgets to avoid a v2->v1 conversion): %v\n", snapErr) + return restore + } + restore = snapRestore + } + } + + fmt.Fprintf(w, "Updating widget definitions in %s...\n", projectPath) + if err := updateWidgetsCmd(mxPath, updateWidgetsPathArg(projectPath), w, stderr); err != nil { + // Non-fatal: warn and let the caller continue. The snapshot is still restored + // by the returned func — a failed run may have converted the project first. + fmt.Fprintf(w, "Warning: update-widgets failed (continuing): %v\n", err) + return restore + } + fmt.Fprintln(w, "Widget definitions updated.") + return restore +} + +// snapshotStorageFormat backs up the MPRv2 storage files (.mpr index + mprcontents/) +// to a temp directory and returns a restore function that puts them back, undoing +// any v2 -> v1 conversion performed by an intervening `mx update-widgets`. The +// restore function removes the temp directory and is safe to defer; it best-effort +// restores and never panics. mprPath and contentsDir come from an mpr.Reader on a +// project already known to be MPRv2. +func snapshotStorageFormat(mprPath, contentsDir string) (restore func(), err error) { + tmp, err := os.MkdirTemp("", "mxcli-mpr-snapshot-*") + if err != nil { + return nil, err + } + + mprBackup := filepath.Join(tmp, filepath.Base(mprPath)) + if err := copyFile(mprPath, mprBackup); err != nil { + os.RemoveAll(tmp) + return nil, err + } + + contentsBackup := filepath.Join(tmp, "mprcontents") + if err := copyDir(contentsDir, contentsBackup); err != nil { + os.RemoveAll(tmp) + return nil, err + } + + restore = func() { + defer os.RemoveAll(tmp) + // Restore the v2 index file. + _ = copyFile(mprBackup, mprPath) + // update-widgets deletes mprcontents/; drop whatever is there now (nothing, + // after a conversion) and restore the backed-up tree. + _ = os.RemoveAll(contentsDir) + _ = copyDir(contentsBackup, contentsDir) + } + return restore, nil +} diff --git a/cmd/mxcli/docker/update_widgets_test.go b/cmd/mxcli/docker/update_widgets_test.go new file mode 100644 index 000000000..6b3661a58 --- /dev/null +++ b/cmd/mxcli/docker/update_widgets_test.go @@ -0,0 +1,217 @@ +// SPDX-License-Identifier: Apache-2.0 + +// mendixlabs/mxcli#808: `mx update-widgets` rewrites an MPRv2 project into the +// self-contained MPRv1 format — it inlines every unit into the .mpr and deletes +// mprcontents/. #763 / PR #764 protected the invocation in check.go with a +// snapshot/restore, but build.go carried its own unprotected copy of the same +// invocation, so `mxcli docker build`, `docker run` and `docker reload` still +// converted the project silently while reporting success. +// +// The fix makes the protection a property of the operation rather than of one +// call site: runUpdateWidgets snapshots the v2 storage, runs the step, and returns +// the restore func for the caller to defer. These tests pin that behaviour, so a +// third call site cannot reintroduce the bug by forgetting the snapshot. +package docker + +import ( + "bytes" + "io" + "os" + "path/filepath" + "testing" + + "github.com/mendixlabs/mxcli/sdk/mpr" +) + +// storageVersion reports the .mpr's detected on-disk storage format. +func storageVersion(t *testing.T, mprPath string) mpr.MPRVersion { + t.Helper() + reader, err := mpr.Open(mprPath) + if err != nil { + t.Fatalf("mpr.Open(%s): %v", mprPath, err) + } + defer reader.Close() + return reader.Version() +} + +// v2Fixture copies the MPRv2 test project (.mpr index + mprcontents/ tree) into a +// temp dir, so a test may convert it without mutating shared testdata. +func v2Fixture(t *testing.T) string { + t.Helper() + dst := t.TempDir() + if err := os.CopyFS(dst, os.DirFS("../../../testdata/expr-checker")); err != nil { + t.Fatalf("copy v2 fixture: %v", err) + } + p := filepath.Join(dst, "minimal.mpr") + if v := storageVersion(t, p); v != mpr.MPRVersionV2 { + t.Fatalf("fixture is %v, not MPRv2 — this test would prove nothing", v) + } + return p +} + +// v1Fixture copies the single-file MPRv1 test project into a temp dir. +func v1Fixture(t *testing.T) string { + t.Helper() + dst := t.TempDir() + if err := os.CopyFS(dst, os.DirFS("../../../sdk/mpr/testdata/v1-project")); err != nil { + t.Fatalf("copy v1 fixture: %v", err) + } + p := filepath.Join(dst, "App.mpr") + if v := storageVersion(t, p); v != mpr.MPRVersionV1 { + t.Fatalf("fixture is %v, not MPRv1", v) + } + return p +} + +// stubUpdateWidgets swaps the mx invocation for the duration of a test. +func stubUpdateWidgets(t *testing.T, fn func(mxPath, pathArg string, w, stderr io.Writer) error) { + t.Helper() + prev := updateWidgetsCmd + updateWidgetsCmd = fn + t.Cleanup(func() { updateWidgetsCmd = prev }) +} + +// convertToV1 mimics what `mx update-widgets` does to an MPRv2 project: rewrite +// the .mpr as a self-contained v1 file and delete the mprcontents/ tree. +func convertToV1(t *testing.T, mprPath string) { + t.Helper() + if err := os.WriteFile(mprPath, []byte("MPRv1-self-contained-inlined"), 0644); err != nil { + t.Fatalf("simulating conversion (.mpr): %v", err) + } + if err := os.RemoveAll(filepath.Join(filepath.Dir(mprPath), "mprcontents")); err != nil { + t.Fatalf("simulating conversion (mprcontents): %v", err) + } +} + +// TestRunUpdateWidgets_RestoresV2AfterConversion is the core regression: whatever +// update-widgets does to the on-disk format, the restore must undo it. +func TestRunUpdateWidgets_RestoresV2AfterConversion(t *testing.T) { + mprPath := v2Fixture(t) + contentsDir := filepath.Join(filepath.Dir(mprPath), "mprcontents") + + orig, err := os.ReadFile(mprPath) + if err != nil { + t.Fatal(err) + } + + ran := false + stubUpdateWidgets(t, func(_, _ string, _, _ io.Writer) error { + ran = true + convertToV1(t, mprPath) + return nil + }) + + var out bytes.Buffer + restore := runUpdateWidgets("mx", mprPath, &out, io.Discard) + if !ran { + t.Fatal("update-widgets was not run on a project whose format could be snapshotted") + } + + // Before restore the caller's check/build step still sees the normalized model — + // that is the whole reason restore is deferred rather than run immediately. + if _, err := os.Stat(contentsDir); !os.IsNotExist(err) { + t.Fatalf("fixture setup wrong: mprcontents/ should be gone at this point (err=%v)", err) + } + + restore() + + if got, err := os.ReadFile(mprPath); err != nil { + t.Fatalf("read restored .mpr: %v", err) + } else if !bytes.Equal(got, orig) { + t.Errorf(".mpr not restored: got %d bytes, want the original %d (#808)", len(got), len(orig)) + } + if v := storageVersion(t, mprPath); v != mpr.MPRVersionV2 { + t.Errorf("project left as %v; MPRv2 storage must be preserved (#808)", v) + } + if entries, err := os.ReadDir(contentsDir); err != nil { + t.Errorf("mprcontents/ not restored: %v", err) + } else if len(entries) == 0 { + t.Error("mprcontents/ restored but empty") + } +} + +// TestRunUpdateWidgets_V1NeedsNoSnapshot: a v1 project is already single-file, so +// there is nothing to protect — the step must still run, and restore must be a +// harmless no-op rather than clobbering the project. +func TestRunUpdateWidgets_V1NeedsNoSnapshot(t *testing.T) { + mprPath := v1Fixture(t) + orig, err := os.ReadFile(mprPath) + if err != nil { + t.Fatal(err) + } + + ran := false + stubUpdateWidgets(t, func(_, _ string, _, _ io.Writer) error { + ran = true + return nil + }) + + var out bytes.Buffer + restore := runUpdateWidgets("mx", mprPath, &out, io.Discard) + if !ran { + t.Error("update-widgets was skipped on an MPRv1 project, which needs no protection") + } + if restore == nil { + t.Fatal("restore is nil — callers defer it unconditionally") + } + restore() + + if got, err := os.ReadFile(mprPath); err != nil { + t.Fatalf("read .mpr: %v", err) + } else if !bytes.Equal(got, orig) { + t.Error("restore modified an MPRv1 project it never snapshotted") + } +} + +// TestRunUpdateWidgets_SkipsStepWhenSnapshotFails is the fail-safe: if the storage +// cannot be backed up, running update-widgets risks an unrecoverable conversion. +// A CE0463 false positive is the lesser evil, so the step must not run at all. +func TestRunUpdateWidgets_SkipsStepWhenSnapshotFails(t *testing.T) { + mprPath := v2Fixture(t) + // Remove mprcontents/ so the snapshot's directory copy fails. The project still + // reads as v2 (detection keys off the absent Unit.Contents column), so this is + // the "v2, but cannot be protected" case rather than a v1 project. + if err := os.RemoveAll(filepath.Join(filepath.Dir(mprPath), "mprcontents")); err != nil { + t.Fatal(err) + } + if v := storageVersion(t, mprPath); v != mpr.MPRVersionV2 { + t.Fatalf("fixture no longer reads as MPRv2 (%v) — test premise broken", v) + } + + ran := false + stubUpdateWidgets(t, func(_, _ string, _, _ io.Writer) error { + ran = true + return nil + }) + + var out bytes.Buffer + restore := runUpdateWidgets("mx", mprPath, &out, io.Discard) + restore() + + if ran { + t.Error("update-widgets ran without a usable snapshot — risks an unrecoverable v2->v1 conversion (#808)") + } + if !bytes.Contains(out.Bytes(), []byte("skipping update-widgets")) { + t.Errorf("the skip must be reported to the user, got:\n%s", out.String()) + } +} + +// TestRunUpdateWidgets_RestoresWhenStepFails: a failed update-widgets is non-fatal, +// but it may still have converted the project before failing, so the snapshot must +// be restored (and its temp dir cleaned up) on that path too. +func TestRunUpdateWidgets_RestoresWhenStepFails(t *testing.T) { + mprPath := v2Fixture(t) + + stubUpdateWidgets(t, func(_, _ string, _, _ io.Writer) error { + convertToV1(t, mprPath) + return io.ErrUnexpectedEOF + }) + + var out bytes.Buffer + restore := runUpdateWidgets("mx", mprPath, &out, io.Discard) + restore() + + if v := storageVersion(t, mprPath); v != mpr.MPRVersionV2 { + t.Errorf("project left as %v after a failed update-widgets; the snapshot must still be restored (#808)", v) + } +} From c00ab83d9b1ecdf3909b17ca8f982b9669ac827a Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 14:19:56 +0000 Subject: [PATCH 03/11] feat(grammar)!: require semicolons on microflow and nanoflow statements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BREAKING CHANGE: every statement inside a microflow or nanoflow body must now end with `;`, including block terminators (`end if;`, `end loop;`, `end while;`, `end case;`). Omitting one is a parse error, where it was previously accepted. Semicolons were optional on all 54 body-statement alternatives, which made statement boundaries ambiguous whenever a keyword can both continue a statement and start one. The concrete case that surfaced it: adding a trailing `commit` modifier to create/change for #779 would let $a = create Mod.E commit $b; bind `commit` to the create rather than starting a commit activity. Diagnosing that per keyword does not scale; requiring the terminator removes the class. Scoped deliberately to microflow/nanoflow BODIES. The document terminator (`end;` / `end` then `/`) stays optional, as it is for pages and every other document type — measured, that variant breaks 88 of 234 example files, 75 of them pages, because block-bodied documents have never required a terminator and three styles coexist in the corpus (168 bare `}`, 56 `};`, 62 `}` + `/`). Tightening that is a separate, much larger decision. Impact measured over all 273 files in mdl-examples: 2 needed fixing, both genuine omissions (`end loop` with no `;`, a `log` line with no `;`), plus a `raise error` inside an on-error block. Example corpus is back at baseline parity (234 pass / 39 fail, 0 regressions). Test fixtures embedding MDL needed the same treatment in enginecompare, executor and visitor. Doc and skill code blocks were extracted and parse-checked against both the old and new parser: 0 regressions, so no documentation migration was required. Diagnostics were already precise for this ("line 143:2 missing ';' at 'return'"), so no error-reporting work was needed. Documented in write-microflows.md, write-nanoflows.md and MDL_QUICK_REFERENCE.md, each stating that the definition terminator remains optional. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012XR649rKk68z6gBpngu6MA --- .claude/skills/mendix/write-microflows.md | 4 +- .claude/skills/mendix/write-nanoflows.md | 4 + docs/01-project/MDL_QUICK_REFERENCE.md | 6 + .../ledger-52-break-in-conditional.mdl | 2 +- .../doctype-tests/02-microflow-examples.mdl | 8 +- mdl/enginecompare/write_alter_test.go | 2 +- mdl/enginecompare/write_microflow_test.go | 14 +-- mdl/executor/validate_microflow_hints_test.go | 16 +-- mdl/grammar/domains/MDLMicroflow.g4 | 108 +++++++++--------- mdl/visitor/visitor_test.go | 4 +- 10 files changed, 90 insertions(+), 78 deletions(-) diff --git a/.claude/skills/mendix/write-microflows.md b/.claude/skills/mendix/write-microflows.md index 5b7185192..61318a030 100644 --- a/.claude/skills/mendix/write-microflows.md +++ b/.claude/skills/mendix/write-microflows.md @@ -95,7 +95,9 @@ end; - Parameters start with `$` prefix - Return variable must be declared or used - Every microflow must end with `return` statement -- Statements end with semicolon `;` +- Every body statement ends with a semicolon `;` — **required**, not optional. This + includes block terminators: `end if;`, `end loop;`, `end while;`, `end case;`. + A missing one is a parse error (`missing ';' at 'return'`), not a warning. - Microflow ends with `/` separator ### Parameter Types diff --git a/.claude/skills/mendix/write-nanoflows.md b/.claude/skills/mendix/write-nanoflows.md index 6cb756bcd..52e1b30fa 100644 --- a/.claude/skills/mendix/write-nanoflows.md +++ b/.claude/skills/mendix/write-nanoflows.md @@ -66,6 +66,10 @@ BEGIN END; ``` +**Every body statement ends with a semicolon `;`** — required, not optional, exactly as +in microflows. That includes block terminators: `end if;`, `end loop;`, `end while;`, +`end case;`. A missing one is a parse error (`missing ';' at 'return'`), not a warning. + ## Naming Convention Nanoflow names use the `NAV_` prefix by convention: diff --git a/docs/01-project/MDL_QUICK_REFERENCE.md b/docs/01-project/MDL_QUICK_REFERENCE.md index 95fdad7c2..ecd593ebb 100644 --- a/docs/01-project/MDL_QUICK_REFERENCE.md +++ b/docs/01-project/MDL_QUICK_REFERENCE.md @@ -258,6 +258,12 @@ return type (`System.ConsumedODataConfiguration` vs ## Microflows - Supported Statements +**Semicolons are mandatory inside a microflow or nanoflow body.** Every statement ends +with `;`, including block terminators (`end if;`, `end loop;`, `end while;`, `end case;`). +Omitting one is a parse error (`missing ';' at 'return'`). The terminator on the +*definition* itself (`end;` / `end` followed by `/`) is unchanged and still optional, as +it is for pages. + | Statement | Syntax | Notes | |-----------|--------|-------| | Variable declaration | `declare $Var type = value;` | Primitives: String, Integer, Boolean, Decimal, DateTime | diff --git a/mdl-examples/bug-tests/ledger-52-break-in-conditional.mdl b/mdl-examples/bug-tests/ledger-52-break-in-conditional.mdl index 94e1265cd..fce56aa77 100644 --- a/mdl-examples/bug-tests/ledger-52-break-in-conditional.mdl +++ b/mdl-examples/bug-tests/ledger-52-break-in-conditional.mdl @@ -34,6 +34,6 @@ begin if $R/Active then break; -- MDL051: crashes mx check (unloadable model) end if; - end loop + end loop; return true; end diff --git a/mdl-examples/doctype-tests/02-microflow-examples.mdl b/mdl-examples/doctype-tests/02-microflow-examples.mdl index b35b58d54..18735093f 100644 --- a/mdl-examples/doctype-tests/02-microflow-examples.mdl +++ b/mdl-examples/doctype-tests/02-microflow-examples.mdl @@ -139,7 +139,7 @@ create or replace microflow MfTest.M001_HelloWorld () returns boolean as $success begin declare $success boolean = true; - log trace node 'TEST' 'List received ' + log trace node 'TEST' 'List received '; return $success; end; / @@ -163,9 +163,9 @@ end; create or replace microflow MfTest.M001_HelloWorld () returns boolean as $success begin - log trace node 'TEST' '> before' + log trace node 'TEST' '> before'; declare $success boolean = true; - log trace node 'TEST' '< After' + log trace node 'TEST' '< After'; return $success; end; / @@ -2010,7 +2010,7 @@ begin log error node 'OrderService' 'Failed to update order status'; change $Order (status = 'ERROR'); commit $Order; - raise error + raise error; }; set $success = true; diff --git a/mdl/enginecompare/write_alter_test.go b/mdl/enginecompare/write_alter_test.go index 035b6f661..d172b062d 100644 --- a/mdl/enginecompare/write_alter_test.go +++ b/mdl/enginecompare/write_alter_test.go @@ -75,7 +75,7 @@ func TestWriteParity_AlterKeepsAccessRule(t *testing.T) { func TestWriteParity_AlterKeepsEventHandler(t *testing.T) { const ent = "MyFirstModule.EvtEnt" setup := []string{ - "CREATE MICROFLOW MyFirstModule.OnEvt () RETURNS BOOLEAN BEGIN RETURN true END", + "CREATE MICROFLOW MyFirstModule.OnEvt () RETURNS BOOLEAN BEGIN RETURN true; END", "CREATE PERSISTENT ENTITY " + ent + " ( Code: string(20), Rank: integer )", "ALTER ENTITY " + ent + " ADD EVENT HANDLER ON BEFORE COMMIT CALL MyFirstModule.OnEvt RAISE ERROR", } diff --git a/mdl/enginecompare/write_microflow_test.go b/mdl/enginecompare/write_microflow_test.go index db46785fe..ea0e1531b 100644 --- a/mdl/enginecompare/write_microflow_test.go +++ b/mdl/enginecompare/write_microflow_test.go @@ -49,9 +49,9 @@ func TestWriteParity_Microflow_ObjectOps(t *testing.T) { // element + parameter mappings (marker-2 list). func TestWriteParity_Microflow_Calls(t *testing.T) { setup := []string{ - "CREATE MICROFLOW MyFirstModule.CTarget () RETURNS BOOLEAN BEGIN RETURN true END", - "CREATE MICROFLOW MyFirstModule.CTargetP (Val: string) RETURNS STRING BEGIN RETURN $Val END", - "CREATE NANOFLOW MyFirstModule.NTarget () RETURNS BOOLEAN BEGIN RETURN true END", + "CREATE MICROFLOW MyFirstModule.CTarget () RETURNS BOOLEAN BEGIN RETURN true; END", + "CREATE MICROFLOW MyFirstModule.CTargetP (Val: string) RETURNS STRING BEGIN RETURN $Val; END", + "CREATE NANOFLOW MyFirstModule.NTarget () RETURNS BOOLEAN BEGIN RETURN true; END", } cases := []struct{ name, stmt, mf string }{ {"MicroflowNoArgs", "CREATE MICROFLOW MyFirstModule.MfCall () BEGIN call microflow MyFirstModule.CTarget(); END", "MfCall"}, @@ -91,10 +91,10 @@ func TestWriteParity_Microflow_Loops(t *testing.T) { cases := []struct{ name, stmt, mf string }{ {"IterateList", "CREATE MICROFLOW MyFirstModule.MfLoop (Items: list of MyFirstModule.LThing) BEGIN " + - "loop $It in $Items begin commit $It; end loop END", "MfLoop"}, + "loop $It in $Items begin commit $It; end loop; END", "MfLoop"}, {"While", "CREATE MICROFLOW MyFirstModule.MfWhile (Item: MyFirstModule.LThing) BEGIN " + - "while $Item/Code != '' begin commit $Item; end while END", "MfWhile"}, + "while $Item/Code != '' begin commit $Item; end while; END", "MfWhile"}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { @@ -188,8 +188,8 @@ func TestWriteParity_Microflow_Retrieve(t *testing.T) { // against legacy, group by group. Skeleton = start → end, boolean return. func TestWriteParity_Microflow(t *testing.T) { cases := []struct{ name, stmt, mf string }{ - {"Skeleton", "CREATE MICROFLOW MyFirstModule.MfEmpty () RETURNS BOOLEAN BEGIN RETURN true END", "MfEmpty"}, - {"Parameters", "CREATE MICROFLOW MyFirstModule.MfParams (Count: integer, Label: string) RETURNS BOOLEAN BEGIN RETURN true END", "MfParams"}, + {"Skeleton", "CREATE MICROFLOW MyFirstModule.MfEmpty () RETURNS BOOLEAN BEGIN RETURN true; END", "MfEmpty"}, + {"Parameters", "CREATE MICROFLOW MyFirstModule.MfParams (Count: integer, Label: string) RETURNS BOOLEAN BEGIN RETURN true; END", "MfParams"}, {"VoidReturn", "CREATE MICROFLOW MyFirstModule.MfVoid () BEGIN END", "MfVoid"}, } for _, c := range cases { diff --git a/mdl/executor/validate_microflow_hints_test.go b/mdl/executor/validate_microflow_hints_test.go index d482d4f19..2b09e7c27 100644 --- a/mdl/executor/validate_microflow_hints_test.go +++ b/mdl/executor/validate_microflow_hints_test.go @@ -170,10 +170,10 @@ func TestValidateMicroflow_AssociationObjectArg(t *testing.T) { // `continue` form reached users as a corrupt project. func TestValidateMicroflow_ConditionalBreakAccepted(t *testing.T) { bodies := []string{ - "loop $R in $L begin if $R/Active then break; end if; end loop", - "loop $R in $L begin if $R/Active then continue; end if; end loop", - "loop $R in $L begin if $R/Active then if $R/Active then break; end if; end if; end loop", - "loop $R in $L begin break; end loop", + "loop $R in $L begin if $R/Active then break; end if; end loop;", + "loop $R in $L begin if $R/Active then continue; end if; end loop;", + "loop $R in $L begin if $R/Active then if $R/Active then break; end if; end if; end loop;", + "loop $R in $L begin break; end loop;", } for _, body := range bodies { t.Run(body, func(t *testing.T) { @@ -231,10 +231,10 @@ func TestValidateMicroflow_DuplicateLoopVariable(t *testing.T) { body string wantMDL bool }{ - {"two loops same iterator", "loop $R in $L begin set $x = 1; end loop loop $R in $L begin set $y = 1; end loop", true}, - {"nested loop reuses outer iterator", "loop $R in $L begin loop $R in $L begin set $x = 1; end loop end loop", true}, - {"distinct iterators are fine", "loop $R in $L begin set $x = 1; end loop loop $C in $L begin set $y = 1; end loop", false}, - {"single loop is fine", "loop $R in $L begin set $x = 1; end loop", false}, + {"two loops same iterator", "loop $R in $L begin set $x = 1; end loop; loop $R in $L begin set $y = 1; end loop;", true}, + {"nested loop reuses outer iterator", "loop $R in $L begin loop $R in $L begin set $x = 1; end loop; end loop;", true}, + {"distinct iterators are fine", "loop $R in $L begin set $x = 1; end loop; loop $C in $L begin set $y = 1; end loop;", false}, + {"single loop is fine", "loop $R in $L begin set $x = 1; end loop;", false}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/mdl/grammar/domains/MDLMicroflow.g4 b/mdl/grammar/domains/MDLMicroflow.g4 index 574536993..d47b0832c 100644 --- a/mdl/grammar/domains/MDLMicroflow.g4 +++ b/mdl/grammar/domains/MDLMicroflow.g4 @@ -116,60 +116,60 @@ microflowBody * not at the grammar level. */ microflowStatement - : annotation* declareStatement SEMICOLON? - | annotation* caseStatement SEMICOLON? - | annotation* inheritanceSplitStatement SEMICOLON? - | annotation* castObjectStatement SEMICOLON? - | annotation* setStatement SEMICOLON? - | annotation* createListStatement SEMICOLON? // Must be before createObjectStatement to match "CREATE LIST OF" - | annotation* createObjectStatement SEMICOLON? - | annotation* changeObjectStatement SEMICOLON? - | annotation* commitStatement SEMICOLON? - | annotation* deleteObjectStatement SEMICOLON? - | annotation* rollbackStatement SEMICOLON? - | annotation* retrieveStatement SEMICOLON? - | annotation* ifStatement SEMICOLON? - | annotation* loopStatement SEMICOLON? - | annotation* whileStatement SEMICOLON? - | annotation* continueStatement SEMICOLON? - | annotation* breakStatement SEMICOLON? - | annotation* returnStatement SEMICOLON? - | annotation* raiseErrorStatement SEMICOLON? - | annotation* logStatement SEMICOLON? - | annotation* callMicroflowStatement SEMICOLON? - | annotation* callNanoflowStatement SEMICOLON? - | annotation* callJavaActionStatement SEMICOLON? - | annotation* callJavaScriptActionStatement SEMICOLON? - | annotation* callWebServiceStatement SEMICOLON? - | annotation* executeDatabaseQueryStatement SEMICOLON? - | annotation* callExternalActionStatement SEMICOLON? - | annotation* showPageStatement SEMICOLON? - | annotation* closePageStatement SEMICOLON? - | annotation* showHomePageStatement SEMICOLON? - | annotation* showMessageStatement SEMICOLON? - | annotation* downloadFileStatement SEMICOLON? - | annotation* throwStatement SEMICOLON? - | annotation* listOperationStatement SEMICOLON? - | annotation* aggregateListStatement SEMICOLON? - | annotation* addToListStatement SEMICOLON? - | annotation* removeFromListStatement SEMICOLON? - | annotation* validationFeedbackStatement SEMICOLON? - | annotation* restCallStatement SEMICOLON? - | annotation* sendRestRequestStatement SEMICOLON? - | annotation* importFromMappingStatement SEMICOLON? - | annotation* exportToMappingStatement SEMICOLON? - | annotation* transformJsonStatement SEMICOLON? - | annotation* callWorkflowStatement SEMICOLON? - | annotation* getWorkflowDataStatement SEMICOLON? - | annotation* getWorkflowsStatement SEMICOLON? - | annotation* getWorkflowActivityRecordsStatement SEMICOLON? - | annotation* workflowOperationStatement SEMICOLON? - | annotation* setTaskOutcomeStatement SEMICOLON? - | annotation* openUserTaskStatement SEMICOLON? - | annotation* notifyWorkflowStatement SEMICOLON? - | annotation* openWorkflowStatement SEMICOLON? - | annotation* lockWorkflowStatement SEMICOLON? - | annotation* unlockWorkflowStatement SEMICOLON? + : annotation* declareStatement SEMICOLON + | annotation* caseStatement SEMICOLON + | annotation* inheritanceSplitStatement SEMICOLON + | annotation* castObjectStatement SEMICOLON + | annotation* setStatement SEMICOLON + | annotation* createListStatement SEMICOLON // Must be before createObjectStatement to match "CREATE LIST OF" + | annotation* createObjectStatement SEMICOLON + | annotation* changeObjectStatement SEMICOLON + | annotation* commitStatement SEMICOLON + | annotation* deleteObjectStatement SEMICOLON + | annotation* rollbackStatement SEMICOLON + | annotation* retrieveStatement SEMICOLON + | annotation* ifStatement SEMICOLON + | annotation* loopStatement SEMICOLON + | annotation* whileStatement SEMICOLON + | annotation* continueStatement SEMICOLON + | annotation* breakStatement SEMICOLON + | annotation* returnStatement SEMICOLON + | annotation* raiseErrorStatement SEMICOLON + | annotation* logStatement SEMICOLON + | annotation* callMicroflowStatement SEMICOLON + | annotation* callNanoflowStatement SEMICOLON + | annotation* callJavaActionStatement SEMICOLON + | annotation* callJavaScriptActionStatement SEMICOLON + | annotation* callWebServiceStatement SEMICOLON + | annotation* executeDatabaseQueryStatement SEMICOLON + | annotation* callExternalActionStatement SEMICOLON + | annotation* showPageStatement SEMICOLON + | annotation* closePageStatement SEMICOLON + | annotation* showHomePageStatement SEMICOLON + | annotation* showMessageStatement SEMICOLON + | annotation* downloadFileStatement SEMICOLON + | annotation* throwStatement SEMICOLON + | annotation* listOperationStatement SEMICOLON + | annotation* aggregateListStatement SEMICOLON + | annotation* addToListStatement SEMICOLON + | annotation* removeFromListStatement SEMICOLON + | annotation* validationFeedbackStatement SEMICOLON + | annotation* restCallStatement SEMICOLON + | annotation* sendRestRequestStatement SEMICOLON + | annotation* importFromMappingStatement SEMICOLON + | annotation* exportToMappingStatement SEMICOLON + | annotation* transformJsonStatement SEMICOLON + | annotation* callWorkflowStatement SEMICOLON + | annotation* getWorkflowDataStatement SEMICOLON + | annotation* getWorkflowsStatement SEMICOLON + | annotation* getWorkflowActivityRecordsStatement SEMICOLON + | annotation* workflowOperationStatement SEMICOLON + | annotation* setTaskOutcomeStatement SEMICOLON + | annotation* openUserTaskStatement SEMICOLON + | annotation* notifyWorkflowStatement SEMICOLON + | annotation* openWorkflowStatement SEMICOLON + | annotation* lockWorkflowStatement SEMICOLON + | annotation* unlockWorkflowStatement SEMICOLON ; declareStatement diff --git a/mdl/visitor/visitor_test.go b/mdl/visitor/visitor_test.go index 5868dd1ff..75254310f 100644 --- a/mdl/visitor/visitor_test.go +++ b/mdl/visitor/visitor_test.go @@ -14,8 +14,8 @@ func TestMicroflowParsing(t *testing.T) { input := `CREATE MICROFLOW MyModule.HelloWorld () RETURNS String BEGIN - DECLARE $greeting String = 'Hello, World!' - RETURN $greeting + DECLARE $greeting String = 'Hello, World!'; + RETURN $greeting; END;` prog, errs := Build(input) From 361c53e66688e66dd38f8dd382bb34dff600f553 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 00:56:41 +0000 Subject: [PATCH 04/11] fix(pages): write a null TitleOverride, unblanking every popup caption (#812) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every Forms$FormSettings / Forms$PageSettings mxcli wrote carried an empty Microflows$TextTemplate as its TitleOverride, where Studio Pro writes null. An empty template is not the absence of an override — it IS an override, to the empty string — so every popup opened by an mxcli-authored button or Show Page action rendered with a blank caption and just the close button. The reporter found 10 broken popups (all mxcli-authored) against 58 correct ones in the same project. The same unconditional write hid a second defect: an override the author did ask for, `show page M.P with title = 'X'`, was discarded. The syntax parses, the visitor sets ast.ShowPageStmt.Title and the builder sets ShowPageAction.OverridePageTitle — but no writer ever read that field. Before this change `grep -rn OverridePageTitle` matched exactly two lines: the struct field and its assignment. Both halves now round-trip. Two traps made this hard to see, and both are recorded in the symptom table. The "must be non-nil" comments cited issue #295 as justification. #295 was about Forms$PageVariable on a PageParameterMapping — a different field — and the conclusion was generalised to TitleOverride without ever being tested. It is false: with the null in place, against Mendix 11.6.6, mxcli docker check -> "The app contains: 0 errors." mxcli docker build -> "Build complete." so the loader accepts it, which is precisely what the old comments said it would not. This repo's own .claude/skills/debug-bson.md already documented `{Key: "TitleOverride", Value: nil}` as the correct Forms$FormSettings shape. Two tests asserted the incorrect behaviour and have been inverted, with the reasoning recorded so the reversal is not silently re-reverted. codec.RegisterTypeDefaults OVERWRITES rather than merges, and Forms$FormSettings was registered twice — in microflow_write.go and widget_write.go. Registrations resolve by init order, so the NullFields entry added beside the microflow writer was clobbered and the null never reached the output. Consolidated to a single registration, with a comment at the surviving site. Scope: the microflow Show Page and widget-action paths, where the evidence is. Navigation writes the same empty template but the reported evidence is about popups, so it is left alone rather than changed on a guess. Verified end-to-end against a real project, not just in unit tests: show page P; -> TitleOverride: null show page P with title = 'X'; -> Microflows$TextTemplate containing 'X' actionbutton (action: show_page P) -> TitleOverride: null plus the mx check and MxBuild runs above. Reverting the fix makes the new tests fail with exactly the BSON shape quoted in the issue. Repro script in mdl-examples/bug-tests/. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012XR649rKk68z6gBpngu6MA --- .claude/skills/fix-issue.md | 1 + .../bug-tests/812-showpage-title-override.mdl | 71 +++++++++ mdl/backend/modelsdk/microflow_write.go | 19 ++- .../modelsdk/showpage_titleoverride_test.go | 145 ++++++++++++++++++ mdl/backend/modelsdk/widget_write.go | 41 +++-- sdk/mpr/showpage_roundtrip_test.go | 47 +++++- sdk/mpr/writer_microflow_actions.go | 29 +++- sdk/mpr/writer_widgets_action.go | 4 +- sdk/mpr/writer_widgets_action_test.go | 30 ++-- 9 files changed, 339 insertions(+), 48 deletions(-) create mode 100644 mdl-examples/bug-tests/812-showpage-title-override.mdl create mode 100644 mdl/backend/modelsdk/showpage_titleoverride_test.go diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index 58a93358a..aeac861e6 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -16,6 +16,7 @@ to the symptom table below, so the next similar issue costs fewer reads. | Symptom | Root cause layer | First file to open | Fix pattern | |---------|-----------------|-------------------|-------------| +| Every popup opened by an **mxcli-authored** button or Show Page action shows a **blank caption** — just the `×` — while Studio Pro / marketplace popups in the same project are fine. Also: `show page M.P with title = 'X'` is silently ignored | `Forms$FormSettings` / `Forms$PageSettings` `TitleOverride` was written as an **empty** `Microflows$TextTemplate` instead of `null`. An empty template is not the absence of an override — it overrides the title with the empty string. The same unconditional write also discarded `ShowPageAction.OverridePageTitle`, so an authored override never reached BSON (`grep -rn OverridePageTitle` matched only the struct field and its assignment) | `sdk/mpr/writer_microflow_actions.go` (`titleOverrideValue`), `sdk/mpr/writer_widgets_action.go`, `mdl/backend/modelsdk/microflow_write.go` (`showPageFormSettingsToGen`), `mdl/backend/modelsdk/widget_write.go` (`formSettingsToGen` + the `Forms$FormSettings`/`Forms$PageSettings` defaults) | Emit `null` when there is no override and a populated `TextTemplate` when there is. **Beware two traps.** (1) The old comments claimed TitleOverride "must be non-nil", reasoning by analogy from #295, which was about `Forms$PageVariable` — a different field; the repo's own `debug-bson.md` already documented `TitleOverride: nil` as correct. When a comment cites an issue as justification, check that the issue is about the same field. (2) `codec.RegisterTypeDefaults` **overwrites rather than merges**, so two registrations for one `$Type` resolve silently by init order — a `NullFields` entry added next to the microflow writer was clobbered by the one in `widget_write.go` and the null never appeared. Grep for duplicate registrations before debugging a default that "does not apply". Repro `mdl-examples/bug-tests/812-showpage-title-override.mdl`. Issue #812 | | `mxcli test` leaves the project mutated: Security Level changed, after-startup pointing at the deleted `MxTest.TestRunner`, and only a `Warning:` line about it. Or: an empty `MxTest` module accumulates after every run | Three defects in one teardown. (a) `getAfterStartup` trimmed quotes *before* trailing punctuation, so a DESCRIBE SETTINGS line ending in `,` yielded `Module.Flow',` and the restore statement was unparseable; (b) cleanup dropped only the microflow, not the module it created; (c) the Security Level was forced OFF and restored to a hardcoded PRODUCTION | `cmd/mxcli/testrunner/runner.go` (`parseSettingValue`, `quoteMDLString`, `projectState`, `setupCommands`, `cleanupCommands`) | Strip trailing `,;` before unquoting; re-emit via `quoteMDLString` (doubling embedded quotes, never backslashes); capture a `projectState` before the first mutation and restore from it; drop the module only when the run created it (a pre-existing `MxTest` is the user's); leave Security Level alone entirely; return cleanup errors instead of printing warnings, and fail the run when they occur. Command lists are pure functions so the restore is testable without a project or Docker. Issues #802/#803/#804 | | `create or modify external entities` silently resets a per-entity setting the user had changed (e.g. allow-create-change-locally) | `applyExternalEntityFields` stamps every field on both the create and the update path, so anything not derivable from the OData contract was overwritten with a default | `mdl/executor/cmd_contract.go` (`applyExternalEntityFields`) | Separate contract-derived fields (Countable/Creatable/Deletable/Skip/Top — refresh from metadata) from local modelling choices (CreateChangeLocally — leave alone; a new entity arrives zero-valued, which is Mendix's default). Issue #782 | | An **external (OData) entity** loses its remote settings on any read-modify-write — `describe external entity` says `is not an external entity (source: )`, and `alter entity … set allow_create_change_locally = true` reports success but the flag stays off. Works under `--engine legacy` | `entityFromGen` recognised only `DomainModels$OqlViewEntitySource`, so the three `Rest$OData*` sources read back as no source at all: `Source` empty, every remote field zeroed. The write path was fine — it switches on `e.Source`, which the read never populated | `mdl/backend/modelsdk/domainmodel.go` (`entityFromGen`'s source switch, `odataKeyFromGen`) | Mirror the legacy parser (`sdk/mpr/parser_domainmodel.go`) for all three flavours: RemoteEntitySource (capabilities + CreateChangeLocally + key), EntityTypeSource (type name + IsOpen + key), PrimitiveCollectionEntitySource (service only). `Updatable` has no gen accessor and the writer does not emit it — leaving it zero is symmetric. **Check the read side first when a write-path field "does not stick"**: a switch on a field the read never fills looks like a write bug. Repro `mdl-examples/bug-tests/782-external-entity-create-change-locally.mdl`. Issue #782 | diff --git a/mdl-examples/bug-tests/812-showpage-title-override.mdl b/mdl-examples/bug-tests/812-showpage-title-override.mdl new file mode 100644 index 000000000..94b264355 --- /dev/null +++ b/mdl-examples/bug-tests/812-showpage-title-override.mdl @@ -0,0 +1,71 @@ +-- Bug #812: Show Page actions got an empty TitleOverride instead of null, +-- blanking every popup's caption. +-- +-- mxcli wrote every Forms$FormSettings / Forms$PageSettings with +-- +-- TitleOverride: { $Type: "Microflows$TextTemplate", Text: {...}, Parameters: [] } +-- +-- where Studio Pro writes `null`. An empty template is not the absence of an +-- override — it IS an override, to the empty string — so every popup opened by an +-- mxcli-authored button or Show Page action rendered with a blank caption and just +-- the close button. The reporter found 10 broken popups (all mxcli-authored) against +-- 58 correct ones in the same project. +-- +-- The same defect hid a second one: `show page M.P with title = 'X'` reached the +-- model (ast.ShowPageStmt.Title -> ShowPageAction.OverridePageTitle) but no writer +-- ever read that field, so an override the author explicitly asked for was dropped. +-- +-- Verify without Studio Pro: +-- +-- mxcli exec 812-showpage-title-override.mdl -p app.mpr +-- mxcli bson dump -p app.mpr --type microflow --object Issue812.OpenPlain +-- -> TitleOverride: null +-- mxcli bson dump -p app.mpr --type microflow --object Issue812.OpenOverridden +-- -> TitleOverride: a Microflows$TextTemplate containing 'Explicit Override' +-- mxcli bson dump -p app.mpr --type page --object Issue812.HostPage +-- -> the button's PageSettings TitleOverride: null +-- +-- The old comments claimed TitleOverride "must be non-nil" because Studio Pro +-- rejects null embedded objects on load. That is false, and was reasoned by analogy +-- from issue #295 (Forms$PageVariable, a different field). Verified against Mendix +-- 11.6.6 with the null in place: +-- +-- mxcli docker check -p app.mpr -> "The app contains: 0 errors." +-- mxcli docker build -p app.mpr -> "Build complete." + + +create module Issue812; +create module role Issue812.User; + +create page Issue812.PopupPage ( + title: 'Popup Title From The Page', + Layout: Atlas_Core.PopupLayout +) +{ + container c1 { } +} +/ + +-- No override: TitleOverride must be null so the page keeps its own title. +create microflow Issue812.OpenPlain () +begin + show page Issue812.PopupPage; +end; +/ + +-- Explicit override: the authored text must survive into the BSON. +create microflow Issue812.OpenOverridden () +begin + show page Issue812.PopupPage with title = 'Explicit Override'; +end; +/ + +-- The button path — the case the issue was actually reported against. There is no +-- MDL syntax for overriding the title from a widget action, so it is always null. +create page Issue812.HostPage (title: 'Host', Layout: Atlas_Core.Atlas_Default) +{ + container c1 { + actionbutton btnOpen (caption: 'Open popup', action: show_page Issue812.PopupPage) + } +} +/ diff --git a/mdl/backend/modelsdk/microflow_write.go b/mdl/backend/modelsdk/microflow_write.go index 3e06e0069..51878c559 100644 --- a/mdl/backend/modelsdk/microflow_write.go +++ b/mdl/backend/modelsdk/microflow_write.go @@ -110,12 +110,12 @@ func init() { codec.RegisterTypeDefaults("Microflows$ResultHandling", codec.TypeDefaults{ NullFields: []string{"ImportMappingCall"}, }) - // A ShowPage FormSettings always carries a TitleOverride (empty Microflows$TextTemplate) - // and each PageParameterMapping a Variable (empty Forms$PageVariable); both are built - // directly. FormSettings' ParameterMappings list empties as marker 2. - codec.RegisterTypeDefaults("Forms$FormSettings", codec.TypeDefaults{ - MandatoryListMarkers: map[string]int32{"ParameterMappings": 2}, - }) + // Each PageParameterMapping carries a Variable (empty Forms$PageVariable), built + // directly. Forms$FormSettings' own defaults — the marker-2 ParameterMappings list + // and the null TitleOverride — are registered ONCE, in widget_write.go: + // RegisterTypeDefaults overwrites rather than merges, so a second registration for + // the same $Type silently wins by init order. A duplicate here is what swallowed + // the TitleOverride null and kept #812 alive after the first attempt at fixing it. } // majorVersion returns the project's Mendix major version (for version-gated BSON). @@ -1279,7 +1279,12 @@ func showPageFormSettingsToGen(a *microflows.ShowPageAction) element.Element { mappings = append(mappings, m) } addPartList(fs, "ParameterMappings", mappings) - addPart(fs, "TitleOverride", emptyTextTemplateToGen()) + // TitleOverride is nil unless the action actually overrides the page title. An + // empty template is not "no override" — it overrides with the empty string, which + // blanked the caption of every popup an mxcli-authored action opened (#812). + if a.OverridePageTitle != nil { + addPart(fs, "TitleOverride", textTemplateToGen(a.OverridePageTitle, nil)) + } return fs } diff --git a/mdl/backend/modelsdk/showpage_titleoverride_test.go b/mdl/backend/modelsdk/showpage_titleoverride_test.go new file mode 100644 index 000000000..37ed23073 --- /dev/null +++ b/mdl/backend/modelsdk/showpage_titleoverride_test.go @@ -0,0 +1,145 @@ +// SPDX-License-Identifier: Apache-2.0 + +// mendixlabs/mxcli#812: every Forms$FormSettings mxcli wrote carried an empty +// Microflows$TextTemplate as its TitleOverride instead of null. An empty template is +// not the absence of an override — it IS an override, to the empty string — so every +// popup opened by an mxcli-authored button or Show Page action rendered with a blank +// caption and only the close button. A scan of one project found 10 broken popups, +// all mxcli-authored, against 58 correct ones from Studio Pro. +// +// The same defect hid a second one: an override the author DID ask for +// (`show page M.P with title = 'X'`) was thrown away, because the empty template was +// written regardless of OverridePageTitle. +// +// These assert the encoded BSON rather than the registry, since the registration is +// only a means to the output — and a duplicate RegisterTypeDefaults call for the same +// $Type silently wins by init order, which is what kept #812 alive after the first +// attempt at fixing it. +package modelsdkbackend + +import ( + "testing" + + bsonv1 "go.mongodb.org/mongo-driver/bson" + + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/modelsdk/codec" + "github.com/mendixlabs/mxcli/modelsdk/element" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// encodeDoc encodes an element and returns it as a decoded bson.D for inspection. +func encodeDoc(t *testing.T, elem element.Element) bsonv1.D { + t.Helper() + raw, err := (&codec.Encoder{}).Encode(elem) + if err != nil { + t.Fatalf("encode: %v", err) + } + var d bsonv1.D + if err := bsonv1.Unmarshal([]byte(raw), &d); err != nil { + t.Fatalf("decode: %v", err) + } + return d +} + +// lookup returns the value of key and whether the key was present at all. +func lookupKey(d bsonv1.D, key string) (any, bool) { + for _, e := range d { + if e.Key == key { + return e.Value, true + } + } + return nil, false +} + +// TestShowPageFormSettings_NoOverrideIsNull is the reported bug. The key must be +// present and null — present, because that is what Studio Pro writes and what this +// repo's own debug-bson.md documents as the correct Forms$FormSettings shape; null, +// because an empty template blanks the caption. +func TestShowPageFormSettings_NoOverrideIsNull(t *testing.T) { + d := encodeDoc(t, showPageFormSettingsToGen(µflows.ShowPageAction{PageName: "Sales.OrderPopup", FormSettingsID: "fs-1"})) + + v, ok := lookupKey(d, "TitleOverride") + if !ok { + t.Fatal("TitleOverride key missing entirely; Studio Pro writes it as an explicit null (#812)") + } + if v != nil { + t.Errorf("TitleOverride = %#v, want nil — an empty template overrides the page "+ + "title with the empty string, blanking the popup caption (#812)", v) + } + // The consolidation must not have dropped the other default. + if mappings, ok := lookupKey(d, "ParameterMappings"); !ok { + t.Error("ParameterMappings marker lost") + } else if arr, _ := mappings.(bsonv1.A); len(arr) == 0 || arr[0] != int32(2) { + t.Errorf("ParameterMappings = %#v, want marker 2", mappings) + } +} + +// TestShowPageFormSettings_OverrideIsPreserved is the second, unreported half. Before +// the fix the authored title reached the model and was then discarded by the writer: +// `grep -rn OverridePageTitle` matched only the struct field and its assignment. +func TestShowPageFormSettings_OverrideIsPreserved(t *testing.T) { + title := &model.Text{ + BaseElement: model.BaseElement{ID: "txt-1", TypeName: "Texts$Text"}, + Translations: map[string]string{"en_US": "Explicit Override"}, + } + d := encodeDoc(t, showPageFormSettingsToGen(µflows.ShowPageAction{PageName: "Sales.OrderPopup", FormSettingsID: "fs-1", OverridePageTitle: title})) + + v, ok := lookupKey(d, "TitleOverride") + if !ok || v == nil { + t.Fatalf("an explicitly authored title override was dropped (#812): %#v", v) + } + sub, isDoc := v.(bsonv1.D) + if !isDoc { + t.Fatalf("TitleOverride = %#v, want a Microflows$TextTemplate document", v) + } + if ty, _ := lookupKey(sub, "$Type"); ty != "Microflows$TextTemplate" { + t.Errorf("TitleOverride $Type = %v, want Microflows$TextTemplate", ty) + } + // The text itself must actually be in there — an empty template would still be a + // TextTemplate, which is exactly how the drop went unnoticed. + if !containsString(sub, "Explicit Override") { + t.Errorf("the override text is absent from the emitted template: %#v", sub) + } +} + +// TestFormSettingsToGen_TitleOverrideIsNull covers the widget-action side — a button +// opening a page, which is the case the reporter actually hit. There is no MDL syntax +// for overriding the title there, so the value is always null. +func TestFormSettingsToGen_TitleOverrideIsNull(t *testing.T) { + d := encodeDoc(t, formSettingsToGen("Sales.OrderPopup")) + + v, ok := lookupKey(d, "TitleOverride") + if !ok { + t.Fatal("TitleOverride key missing entirely (#812)") + } + if v != nil { + t.Errorf("TitleOverride = %#v, want nil — every popup opened by an mxcli-authored "+ + "button showed a blank caption (#812)", v) + } +} + +func containsString(d bsonv1.D, want string) bool { + for _, e := range d { + switch v := e.Value.(type) { + case string: + if v == want { + return true + } + case bsonv1.D: + if containsString(v, want) { + return true + } + case bsonv1.A: + for _, item := range v { + if sub, ok := item.(bsonv1.D); ok && containsString(sub, want) { + return true + } + if s, ok := item.(string); ok && s == want { + return true + } + } + } + } + return false +} diff --git a/mdl/backend/modelsdk/widget_write.go b/mdl/backend/modelsdk/widget_write.go index 8da808cea..3c936d34e 100644 --- a/mdl/backend/modelsdk/widget_write.go +++ b/mdl/backend/modelsdk/widget_write.go @@ -13,7 +13,6 @@ import ( "github.com/mendixlabs/mxcli/modelsdk/element" genCw "github.com/mendixlabs/mxcli/modelsdk/gen/customwidgets" genDm "github.com/mendixlabs/mxcli/modelsdk/gen/domainmodels" - genMf "github.com/mendixlabs/mxcli/modelsdk/gen/microflows" genPg "github.com/mendixlabs/mxcli/modelsdk/gen/pages" genTexts "github.com/mendixlabs/mxcli/modelsdk/gen/texts" "github.com/mendixlabs/mxcli/sdk/microflows" @@ -35,6 +34,14 @@ func init() { codec.RegisterTypeDefaults("Forms$ClientTemplate", codec.TypeDefaults{ MandatoryListMarkers: map[string]int32{"Parameters": 2}, }) + // A widget action's PageSettings never overrides the opened page's title — there + // is no MDL syntax for it — so TitleOverride is always null. It used to be written + // as an empty Microflows$TextTemplate, which overrides the title with the empty + // string: every popup an mxcli-authored button opened showed a blank caption with + // only the close button (#812). + codec.RegisterTypeDefaults("Forms$PageSettings", codec.TypeDefaults{ + NullFields: []string{"TitleOverride"}, + }) // A pluggable-widget XPath datasource (DataGrid2 / Gallery / …) always serializes // SourceVariable as null when unbound, and its GridSortBar always emits an (empty) // SortItems list with marker 2. An older embedded template extracted before these @@ -209,8 +216,19 @@ func init() { codec.RegisterTypeDefaults("Forms$FormAction", codec.TypeDefaults{ MandatoryListMarkers: map[string]int32{"PagesForSpecializations": 2}, }) + // The single Forms$FormSettings registration — shared by the widget-action and + // microflow show-page paths. Keep it here and nowhere else: RegisterTypeDefaults + // overwrites rather than merges, so a second registration for the same $Type + // silently wins by init order. That is what hid #812 — a NullFields entry added + // next to the microflow writer was clobbered by this one. + // + // TitleOverride is null unless the action really overrides the opened page's + // title. An empty Microflows$TextTemplate is not "no override" — it overrides with + // the empty string, blanking the caption of every popup mxcli authored (#812). + // showPageFormSettingsToGen emits the part only when OverridePageTitle is set. codec.RegisterTypeDefaults("Forms$FormSettings", codec.TypeDefaults{ MandatoryListMarkers: map[string]int32{"ParameterMappings": 2}, + NullFields: []string{"TitleOverride"}, }) // create_object action: EntityRef is null when no entity is specified. codec.RegisterTypeDefaults("Forms$CreateObjectClientAction", codec.TypeDefaults{ @@ -1296,27 +1314,20 @@ func microflowSettingsToGen(microflowName string, mappings []*pages.MicroflowPar } // formSettingsToGen builds the Forms$FormSettings (PageSettings) shared by the -// page-opening actions: target page by-name, empty parameter mappings, empty -// title override. +// page-opening actions: target page by-name and empty parameter mappings. +// +// TitleOverride is deliberately left unset. A widget action (a button opening a +// page) has no MDL syntax for overriding the title, so the correct value is always +// null — the page keeps its own title. Setting an empty Microflows$TextTemplate here +// overrode it with the empty string, so every popup opened by an mxcli-authored +// button rendered with a blank caption and just the close button (#812). func formSettingsToGen(pageName string) element.Element { ps := genPg.NewPageSettings() assignID(ps) ps.SetPageQualifiedName(pageName) - ps.SetTitleOverride(emptyTextTemplateToGen()) return ps } -// emptyTextTemplateToGen builds an empty Microflows$TextTemplate. This is the type -// of FormSettings/PageSettings TitleOverride (NOT a Forms$ClientTemplate, which -// Studio Pro's loader rejects with a type-cast error) and it must be non-nil -// (issue #468). Mirrors sdk/mpr.emptyTextTemplate. -func emptyTextTemplateToGen() element.Element { - tt := genMf.NewTextTemplate() - assignID(tt) - tt.SetText(genTexts.NewText()) - return tt -} - // clientActionToGen converts a widget client action. Simple actions are supported; // the page/microflow/nanoflow/create-object actions (which carry settings sub- // objects) are refused loudly for now. diff --git a/sdk/mpr/showpage_roundtrip_test.go b/sdk/mpr/showpage_roundtrip_test.go index d8e402d39..607669af8 100644 --- a/sdk/mpr/showpage_roundtrip_test.go +++ b/sdk/mpr/showpage_roundtrip_test.go @@ -145,10 +145,20 @@ func TestShowPageAction_RoundtripNoParams(t *testing.T) { } } -// TestShowPageAction_TitleOverride_NotNil verifies that FormSettings.TitleOverride is written as -// an embedded Microflows$TextTemplate object, not nil. Studio Pro rejects null embedded objects -// on project load (same class of bug as FormSettings.ParameterMappings.Variable — issue #295). -func TestShowPageAction_TitleOverride_NotNil(t *testing.T) { +// TestShowPageAction_TitleOverride_IsNull replaces an earlier test that asserted the +// opposite. That test reasoned by analogy — "same class of bug as +// FormSettings.ParameterMappings.Variable — issue #295" — but #295 was about +// Forms$PageVariable, a different field, and the conclusion was generalised to +// TitleOverride without ever being observed. +// +// The evidence runs the other way. Studio Pro writes TitleOverride null: a scan of one +// project found 58 correct popups (Studio Pro / marketplace) with null against 10 +// broken ones (mxcli) with an empty template, and this repo's own +// .claude/skills/debug-bson.md documents `{Key: "TitleOverride", Value: nil}` as the +// correct Forms$FormSettings shape. An empty Microflows$TextTemplate is not the +// absence of an override — it overrides the title with the empty string, so every such +// popup rendered with a blank caption and only the close button (#812). +func TestShowPageAction_TitleOverride_IsNull(t *testing.T) { action := µflows.ShowPageAction{ BaseElement: model.BaseElement{ID: "test-action-id"}, PageName: "Sales.Product_NewEdit", @@ -170,9 +180,34 @@ func TestShowPageAction_TitleOverride_NotNil(t *testing.T) { t.Fatal("FormSettings missing") } - to := toMap(formSettings["TitleOverride"]) + raw2, ok := formSettings["TitleOverride"] + if !ok { + t.Fatal("TitleOverride key missing entirely; Studio Pro writes it as an explicit null") + } + if raw2 != nil { + t.Fatalf("TitleOverride = %#v, want nil — an empty template overrides the page "+ + "title with the empty string, blanking the popup caption (#812)", raw2) + } + + // ...and when the action DOES override the title, the authored text must survive. + // Before #812 the empty template was written either way, so this half was silently + // dropped: OverridePageTitle was set by the builder and read by nothing. + withTitle := µflows.ShowPageAction{ + BaseElement: model.BaseElement{ID: "test-action-id"}, + PageName: "Sales.Product_NewEdit", + OverridePageTitle: &model.Text{Translations: map[string]string{"en_US": "Edit Product"}}, + } + data2, err := bson.Marshal(serializeMicroflowAction(withTitle)) + if err != nil { + t.Fatalf("failed to marshal BSON: %v", err) + } + var rawDoc2 map[string]any + if err := bson.Unmarshal(data2, &rawDoc2); err != nil { + t.Fatalf("failed to unmarshal BSON: %v", err) + } + to := toMap(toMap(rawDoc2["FormSettings"])["TitleOverride"]) if to == nil { - t.Fatal("TitleOverride is nil; Studio Pro rejects null Microflows$TextTemplate objects (issue #468)") + t.Fatal("an explicitly authored title override was dropped (#812)") } if got := extractString(to["$Type"]); got != "Microflows$TextTemplate" { t.Fatalf("TitleOverride.$Type = %q, want %q", got, "Microflows$TextTemplate") diff --git a/sdk/mpr/writer_microflow_actions.go b/sdk/mpr/writer_microflow_actions.go index 5ca7c0281..70de49860 100644 --- a/sdk/mpr/writer_microflow_actions.go +++ b/sdk/mpr/writer_microflow_actions.go @@ -448,7 +448,7 @@ func serializeMicroflowAction(action microflows.MicroflowAction) bson.D { {Key: "$Type", Value: "Forms$FormSettings"}, {Key: "Form", Value: a.PageName}, // BY_NAME_REFERENCE (page qualified name) {Key: "ParameterMappings", Value: paramMappings}, - {Key: "TitleOverride", Value: emptyTextTemplate()}, + {Key: "TitleOverride", Value: titleOverrideValue(a.OverridePageTitle)}, } doc = append(doc, bson.E{Key: "FormSettings", Value: formSettings}) doc = append(doc, bson.E{Key: "NumberOfPagesToClose", Value: ""}) @@ -580,10 +580,31 @@ func emptyPageVariable() bson.D { } } +// titleOverrideValue renders FormSettings.TitleOverride: the page's own title is +// used unless the action overrides it, and "no override" is nil — NOT an empty +// Microflows$TextTemplate. +// +// An empty template is not the absence of an override, it *is* an override, to the +// empty string: every popup opened by such an action showed a blank caption with +// only the close button (mendixlabs/mxcli#812). The writers had been emitting one +// unconditionally on a mistaken "must be non-nil" reading of PR #338 / issue #295 — +// which was about Forms$PageVariable, a different field. This repo's own +// .claude/skills/debug-bson.md already documented the correct Forms$FormSettings +// shape as `TitleOverride: nil`. +// +// The same bug hid a second one: an override the author *did* ask for +// (`show page M.P with title = 'X'`) was dropped, because the empty template was +// written regardless of OverridePageTitle. Both cases now round-trip. +func titleOverrideValue(override *model.Text) any { + if override == nil { + return nil + } + return serializeTextTemplate(override, nil) +} + // emptyTextTemplate returns an empty Microflows$TextTemplate embedded object. -// TitleOverride on Forms$FormSettings is a Microflows$TextTemplate (not a scalar), -// so it must be written as an empty object rather than nil — same pattern as -// emptyPageVariable() for Forms$PageVariable (see PR #338 / issue #295). +// Retained for callers that genuinely need an initialized template; do NOT use it +// for TitleOverride — see titleOverrideValue. func emptyTextTemplate() bson.D { return bson.D{ {Key: "$ID", Value: idToBsonBinary(generateUUID())}, diff --git a/sdk/mpr/writer_widgets_action.go b/sdk/mpr/writer_widgets_action.go index 6f9b24b03..4fadebab8 100644 --- a/sdk/mpr/writer_widgets_action.go +++ b/sdk/mpr/writer_widgets_action.go @@ -69,7 +69,7 @@ func serializeClientAction(action pages.ClientAction) bson.D { {Key: "$Type", Value: "Forms$FormSettings"}, {Key: "Form", Value: a.PageName}, // BY_NAME_REFERENCE - qualified name, or empty string if no page {Key: "ParameterMappings", Value: bson.A{int32(2)}}, - {Key: "TitleOverride", Value: emptyTextTemplate()}, + {Key: "TitleOverride", Value: nil}, // no override: the page keeps its own title (#812) } return bson.D{ {Key: "$ID", Value: idToBsonBinary(string(a.ID))}, @@ -93,7 +93,7 @@ func serializeClientAction(action pages.ClientAction) bson.D { {Key: "$Type", Value: "Forms$FormSettings"}, {Key: "Form", Value: a.PageName}, // BY_NAME_REFERENCE - qualified name {Key: "ParameterMappings", Value: bson.A{int32(2)}}, - {Key: "TitleOverride", Value: emptyTextTemplate()}, + {Key: "TitleOverride", Value: nil}, // no override: the page keeps its own title (#812) } return bson.D{ diff --git a/sdk/mpr/writer_widgets_action_test.go b/sdk/mpr/writer_widgets_action_test.go index 718978ee0..6b6b5de46 100644 --- a/sdk/mpr/writer_widgets_action_test.go +++ b/sdk/mpr/writer_widgets_action_test.go @@ -151,24 +151,26 @@ func TestPageClientAction_RequiredFields(t *testing.T) { t.Errorf("FormSettings missing required field %q", "TitleOverride") } - // TitleOverride must be an embedded Microflows$TextTemplate object, not nil. - // Studio Pro rejects null embedded objects on project load (same class of bug as issue #295). + // TitleOverride must be null. A button opening a page has no MDL syntax for + // overriding the opened page's title, so the page always keeps its own. + // + // This assertion was previously inverted, on the reasoning that Studio Pro rejects + // null embedded objects ("same class of bug as issue #295"). #295 was about + // Forms$PageVariable; the conclusion was generalised to TitleOverride without being + // observed. An empty Microflows$TextTemplate is not "no override" — it overrides + // with the empty string, so every popup opened by an mxcli-authored button showed a + // blank caption and only the close button (#812). + found := false for _, e := range formSettings { if e.Key != "TitleOverride" { continue } - to, ok := e.Value.(bson.D) - if !ok || to == nil { - t.Fatalf("TitleOverride must be a bson.D embedded object, got %T (%v)", e.Value, e.Value) - } - var typ string - for _, f := range to { - if f.Key == "$Type" { - typ, _ = f.Value.(string) - } - } - if typ != "Microflows$TextTemplate" { - t.Errorf("TitleOverride.$Type = %q, want %q", typ, "Microflows$TextTemplate") + found = true + if e.Value != nil { + t.Fatalf("TitleOverride = %#v, want nil (#812)", e.Value) } } + if !found { + t.Error("TitleOverride key missing entirely; Studio Pro writes it as an explicit null") + } } From 38766a318d0d509e86e06983fcd5c2b4ddb4ab6e Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 03:52:02 +0000 Subject: [PATCH 05/11] fix(new): resolve the exact Mendix version, and verify what was created MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `mxcli new --version 11.12.2` printed "Step 1/4: Resolving MxBuild 11.12.2..." and then produced a Mendix 11.6.6 project, with no warning that the requested version had been ignored. `mxcli setup mxbuild --version 11.12.2` downloads that version fine, so the CDN was never the problem — the resolver simply never asked. ResolveMxForNewProject delegated to ResolveMxForVersion, whose last resort is AnyCachedMxPath(): any cached mx, of any version. That fallback is defensible when the project already exists and its version is a preference. It is wrong for `new`, where the requested version is not a preference but the definition of the output — `mx create-project` stamps the new project with the version of the binary that ran it, and every later step (init, mxbuild, runtime, run --local) then follows the wrong version. Two changes, because either alone leaves a silent path. Resolution is now exact. localMxForVersion looks only for the requested version: an exact-version Studio Pro install, an exact-version binary in the OS-specific install locations, then the exact-version download cache. PATH is deliberately excluded — an `mx` on PATH carries no version guarantee. Anything else downloads. Windows and macOS still prefer an exact Studio Pro install, so they do not fetch a Linux CDN binary that cannot execute. And `new` now checks its postcondition rather than trusting the resolution: it reopens the created .mpr, compares ProductVersion against --version, and exits with the mismatch, the binary that caused it, and the setup command that fixes it. A resolution bug is invisible without this — which is exactly how the original went unnoticed. Verified by reproducing the reported state (only 11.6.6 cached, 11.12.2 requested): before -> silently created a Mendix 11.6.6 project after -> downloads 11.12.2, creates a Mendix 11.12.2 project and by reverting only the resolver, which now fails loudly instead of silently: Error: requested Mendix 11.12.2 but the created project is 11.6.6. mx create-project stamps the project with the version of the binary that ran it (.../11.6.6/modeler/mx). Run 'mxcli setup mxbuild --version 11.12.2' and try again. Unit tests cover the resolution decision without touching the network: a cached 11.6.6 must not satisfy a request for 11.12.2, an exact match must still be reused rather than re-downloaded, and an empty version matches nothing. Found while building a project to verify #812 in a browser; it cost a full project rebuild before the wrong version was spotted. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012XR649rKk68z6gBpngu6MA --- .claude/skills/fix-issue.md | 1 + cmd/mxcli/cmd_new.go | 23 +++++ cmd/mxcli/docker/check.go | 54 ++++++++-- cmd/mxcli/docker/new_project_version_test.go | 101 +++++++++++++++++++ 4 files changed, 172 insertions(+), 7 deletions(-) create mode 100644 cmd/mxcli/docker/new_project_version_test.go diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index 58a93358a..5fc083569 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -16,6 +16,7 @@ to the symptom table below, so the next similar issue costs fewer reads. | Symptom | Root cause layer | First file to open | Fix pattern | |---------|-----------------|-------------------|-------------| +| `mxcli new --version X` prints "Resolving MxBuild X..." and then produces a project at a **different** Mendix version — silently. Every later step (init, mxbuild, runtime, `run --local`) follows the wrong version | `ResolveMxForNewProject` delegated to `ResolveMxForVersion`, whose last resort is `AnyCachedMxPath()` — *any* cached mx, of any version. That fallback is fine when the project already exists and its version is a preference; for `new` the requested version **is** the output, because `mx create-project` stamps the project with the version of the binary that ran it | `cmd/mxcli/docker/check.go` (`localMxForVersion`, `ResolveMxForNewProject`) + `cmd/mxcli/cmd_new.go` (postcondition) | Resolve **exactly** the requested version for `new` (exact Studio Pro install → exact versioned install path → exact download cache; **not** PATH, which carries no version guarantee), and download otherwise. Then check the postcondition: reopen the created `.mpr`, compare `ProductVersion` to `--version`, and fail loudly on a mismatch — resolution bugs are invisible without it. **Generalisable**: when a flag names the version/identity of the artifact being produced, a "close enough" local substitute is never valid, and the produced artifact should be verified against the request rather than the resolution trusted. Found while reproducing #812 in a browser — cost a full project rebuild before it was noticed | | `mxcli test` leaves the project mutated: Security Level changed, after-startup pointing at the deleted `MxTest.TestRunner`, and only a `Warning:` line about it. Or: an empty `MxTest` module accumulates after every run | Three defects in one teardown. (a) `getAfterStartup` trimmed quotes *before* trailing punctuation, so a DESCRIBE SETTINGS line ending in `,` yielded `Module.Flow',` and the restore statement was unparseable; (b) cleanup dropped only the microflow, not the module it created; (c) the Security Level was forced OFF and restored to a hardcoded PRODUCTION | `cmd/mxcli/testrunner/runner.go` (`parseSettingValue`, `quoteMDLString`, `projectState`, `setupCommands`, `cleanupCommands`) | Strip trailing `,;` before unquoting; re-emit via `quoteMDLString` (doubling embedded quotes, never backslashes); capture a `projectState` before the first mutation and restore from it; drop the module only when the run created it (a pre-existing `MxTest` is the user's); leave Security Level alone entirely; return cleanup errors instead of printing warnings, and fail the run when they occur. Command lists are pure functions so the restore is testable without a project or Docker. Issues #802/#803/#804 | | `create or modify external entities` silently resets a per-entity setting the user had changed (e.g. allow-create-change-locally) | `applyExternalEntityFields` stamps every field on both the create and the update path, so anything not derivable from the OData contract was overwritten with a default | `mdl/executor/cmd_contract.go` (`applyExternalEntityFields`) | Separate contract-derived fields (Countable/Creatable/Deletable/Skip/Top — refresh from metadata) from local modelling choices (CreateChangeLocally — leave alone; a new entity arrives zero-valued, which is Mendix's default). Issue #782 | | An **external (OData) entity** loses its remote settings on any read-modify-write — `describe external entity` says `is not an external entity (source: )`, and `alter entity … set allow_create_change_locally = true` reports success but the flag stays off. Works under `--engine legacy` | `entityFromGen` recognised only `DomainModels$OqlViewEntitySource`, so the three `Rest$OData*` sources read back as no source at all: `Source` empty, every remote field zeroed. The write path was fine — it switches on `e.Source`, which the read never populated | `mdl/backend/modelsdk/domainmodel.go` (`entityFromGen`'s source switch, `odataKeyFromGen`) | Mirror the legacy parser (`sdk/mpr/parser_domainmodel.go`) for all three flavours: RemoteEntitySource (capabilities + CreateChangeLocally + key), EntityTypeSource (type name + IsOpen + key), PrimitiveCollectionEntitySource (service only). `Updatable` has no gen accessor and the writer does not emit it — leaving it zero is symmetric. **Check the read side first when a write-path field "does not stick"**: a switch on a field the read never fills looks like a write bug. Repro `mdl-examples/bug-tests/782-external-entity-create-change-locally.mdl`. Issue #782 | diff --git a/cmd/mxcli/cmd_new.go b/cmd/mxcli/cmd_new.go index c0e5514c1..9ccf45c21 100644 --- a/cmd/mxcli/cmd_new.go +++ b/cmd/mxcli/cmd_new.go @@ -10,6 +10,7 @@ import ( "runtime" "github.com/mendixlabs/mxcli/cmd/mxcli/docker" + "github.com/mendixlabs/mxcli/sdk/mpr" "github.com/spf13/cobra" ) @@ -115,6 +116,28 @@ Examples: } fmt.Printf(" Created %s\n", mprPath) + // The project is stamped with the version of the binary that created it, not + // with --version. Resolving the wrong binary therefore yields a project at a + // version the user never asked for, and every later step (init, mxbuild, + // runtime) silently follows it. Check the postcondition rather than trusting + // the resolution: a mismatch here means the model is wrong, so fail loudly + // instead of handing back something that merely looks finished. + if reader, err := mpr.Open(mprPath); err == nil { + created := reader.ProjectVersion().ProductVersion + reader.Close() + if created != "" && created != mendixVersion { + fmt.Fprintf(os.Stderr, + "Error: requested Mendix %s but the created project is %s.\n", + mendixVersion, created) + fmt.Fprintf(os.Stderr, + " mx create-project stamps the project with the version of the binary that ran it (%s).\n", mxPath) + fmt.Fprintf(os.Stderr, + " Run 'mxcli setup mxbuild --version %s' and try again.\n", mendixVersion) + os.Exit(1) + } + fmt.Printf(" Mendix version: %s\n", created) + } + // Step 3: Initialize tooling if !skipInit { fmt.Printf("\nStep 3/4: Initializing AI tooling...\n") diff --git a/cmd/mxcli/docker/check.go b/cmd/mxcli/docker/check.go index 8696bed3b..a5bc86ac8 100644 --- a/cmd/mxcli/docker/check.go +++ b/cmd/mxcli/docker/check.go @@ -286,17 +286,57 @@ func ResolveMxForVersion(mxbuildPath, preferredVersion string) (string, error) { return "", fmt.Errorf("mx not found; specify --mxbuild-path pointing to Mendix installation directory") } +// localMxForVersion returns a locally available mx binary that is EXACTLY the +// requested version, or "" if there is none. +// +// Unlike ResolveMxForVersion it never substitutes a different version. That +// substitution is reasonable when the project already exists and its version is a +// preference; it is wrong when the version IS the request — see +// ResolveMxForNewProject. +// +// Looks in the same places, minus the "any version will do" fallbacks: an +// exact-version Studio Pro install, an exact-version binary in the OS-specific +// install locations, then the exact-version download cache. PATH is deliberately +// excluded: an `mx` on PATH carries no version guarantee. +func localMxForVersion(version string) string { + if version == "" { + return "" + } + if studioProDir := ResolveStudioProDir(version); studioProDir != "" { + for _, name := range mxBinaryNames() { + candidate := filepath.Join(studioProDir, "modeler", name) + if info, err := os.Stat(candidate); err == nil && !info.IsDir() { + return candidate + } + } + } + if matches := globVersionedMatches(mendixSearchPaths(mxBinaryName())); len(matches) > 0 { + if exact := exactVersionedPath(matches, version); exact != "" { + return exact + } + } + return CachedMxPath(version) +} + // ResolveMxForNewProject finds the mx binary for use by mxcli new. -// On Windows and macOS it prefers an installed Studio Pro to avoid downloading -// Linux CDN binaries that won't execute on those platforms. On Linux (and as a -// fallback) it downloads mxbuild from the CDN and derives mx from the same dir. +// +// The requested version is not a preference here, it is the definition of the +// output: `mx create-project` stamps the new project with the version of the binary +// that created it. So only an EXACT match may be reused; anything else is +// downloaded. Delegating to ResolveMxForVersion was wrong, because its last resort +// is AnyCachedMxPath() — with only 11.6.6 cached, `mxcli new --version 11.12.2` +// printed "Resolving MxBuild 11.12.2..." and then produced an 11.6.6 project, with +// no warning that the requested version had been ignored (mendixlabs/mxcli#808 era +// finding; the CDN had the version all along). +// +// On Windows and macOS an exact-version Studio Pro is still preferred, so those +// platforms do not download a Linux CDN binary that cannot execute. func ResolveMxForNewProject(version string, progressWriter io.Writer) (string, error) { - // Fast path: Studio Pro or cached download already present - if mxPath, err := ResolveMxForVersion("", version); err == nil { + if mxPath := localMxForVersion(version); mxPath != "" { return mxPath, nil } - // Slow path: download mxbuild from CDN (works on Linux; on macOS/Windows - // this is only reached if Studio Pro is not installed) + // Not available locally at the requested version: download it (works on Linux; + // on macOS/Windows this is only reached when Studio Pro is not installed). mxbuildPath, err := DownloadMxBuild(version, progressWriter) if err != nil { return "", err diff --git a/cmd/mxcli/docker/new_project_version_test.go b/cmd/mxcli/docker/new_project_version_test.go new file mode 100644 index 000000000..3cf6cd2ec --- /dev/null +++ b/cmd/mxcli/docker/new_project_version_test.go @@ -0,0 +1,101 @@ +// SPDX-License-Identifier: Apache-2.0 + +// `mxcli new --version X` silently produced a project at a DIFFERENT version. +// +// ResolveMxForNewProject delegated to ResolveMxForVersion, whose last resort is +// AnyCachedMxPath() — any cached mx, of any version. That fallback is defensible for +// check/build, where the project already exists and its version is a preference. It is +// wrong for `new`, where the requested version is not a preference but the definition +// of the output: `mx create-project` stamps the project with ITS OWN version, so a +// mismatched binary produces a project the user did not ask for. +// +// Observed: with only 11.6.6 cached, `mxcli new --version 11.12.2` printed +// "Resolving MxBuild 11.12.2..." and produced a Mendix 11.6.6 project, with no +// warning. `mxcli setup mxbuild --version 11.12.2` downloaded it fine, so the CDN was +// never the problem — the resolver just never asked. +package docker + +import ( + "os" + "path/filepath" + "testing" +) + +// cacheFakeMx plants a fake cached mx binary for each version under a fake HOME. +func cacheFakeMx(t *testing.T, home string, versions ...string) map[string]string { + t.Helper() + paths := make(map[string]string, len(versions)) + for _, v := range versions { + modelerDir := filepath.Join(home, ".mxcli", "mxbuild", v, "modeler") + if err := os.MkdirAll(modelerDir, 0755); err != nil { + t.Fatal(err) + } + bin := filepath.Join(modelerDir, mxBinaryName()) + if err := os.WriteFile(bin, []byte("fake"), 0755); err != nil { + t.Fatal(err) + } + paths[v] = bin + } + return paths +} + +func isolateResolution(t *testing.T, home string) { + t.Helper() + setTestHomeDir(t, home) + setTestApplicationsDir(t, t.TempDir()) // no real macOS Studio Pro + t.Setenv("PATH", t.TempDir()) // no mx on PATH +} + +// TestLocalMxForVersion_RefusesAnotherVersion is the bug: a cached 11.6.6 must not be +// offered up when 11.12.2 was requested. +func TestLocalMxForVersion_RefusesAnotherVersion(t *testing.T) { + home := t.TempDir() + isolateResolution(t, home) + cacheFakeMx(t, home, "11.6.6") + + if got := localMxForVersion("11.12.2"); got != "" { + t.Errorf("localMxForVersion(11.12.2) = %q with only 11.6.6 cached; a different "+ + "version is not a substitute — mx create-project stamps the project with its "+ + "own version, so this silently produces the wrong project", got) + } +} + +// TestLocalMxForVersion_AcceptsExactMatch: an exact match must still be reused, so a +// cached version is not re-downloaded. +func TestLocalMxForVersion_AcceptsExactMatch(t *testing.T) { + home := t.TempDir() + isolateResolution(t, home) + want := cacheFakeMx(t, home, "11.6.6", "11.12.2")["11.12.2"] + + if got := localMxForVersion("11.12.2"); got != want { + t.Errorf("localMxForVersion(11.12.2) = %q, want the exact cached binary %q", got, want) + } +} + +// TestLocalMxForVersion_EmptyVersion: with no version requested there is nothing to +// match, so nothing local qualifies and the caller must decide. +func TestLocalMxForVersion_EmptyVersion(t *testing.T) { + home := t.TempDir() + isolateResolution(t, home) + cacheFakeMx(t, home, "11.6.6") + + if got := localMxForVersion(""); got != "" { + t.Errorf("localMxForVersion(\"\") = %q, want \"\" — no requested version means no match", got) + } +} + +// TestResolveMxForNewProject_UsesExactCachedVersion covers the wiring: the exact match +// short-circuits before any download is attempted. +func TestResolveMxForNewProject_UsesExactCachedVersion(t *testing.T) { + home := t.TempDir() + isolateResolution(t, home) + want := cacheFakeMx(t, home, "11.6.6", "11.12.2")["11.12.2"] + + got, err := ResolveMxForNewProject("11.12.2", os.Stderr) + if err != nil { + t.Fatalf("ResolveMxForNewProject: %v", err) + } + if got != want { + t.Errorf("ResolveMxForNewProject(11.12.2) = %q, want %q", got, want) + } +} From 77de33c98fef52009d63fe34981326818b07807a Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 04:03:44 +0000 Subject: [PATCH 06/11] docs(skills): verify a fix at the layer its symptom lives in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds .claude/skills/verify-in-runtime.md and the checklist rule that routes to it, so browser-level verification is a normal step for the bugs that need it rather than something rediscovered per incident. The motivating pattern: twice this week a green test suite coexisted with a live bug. In #812 a duplicate codec.RegisterTypeDefaults for the same $Type silently clobbered the fix — registrations overwrite rather than merge and resolve by init order — so the unit test passed while the emitted BSON stayed wrong. In #808 the integration test had only ever skipped, for want of an mx binary, and a skip reads exactly like a pass. A green suite is evidence about the layer it tests and nothing more. #812 is the case the new tier exists for: every popup opened by an mxcli-authored button rendered a blank caption, yet the BSON was structurally valid, mx check reported 0 errors and MxBuild completed. Nothing below the browser could see it, because the defect WAS the rendering — an empty Microflows$TextTemplate is not "no title override", it is an override to the empty string. The trigger rule is deliberately narrow: use the layer the symptom lives in. Parser to unit test, BSON to a test on the encoded document, on-disk effects to an integration test, and only render-time behaviour to the runtime tier. Of four fixes this week exactly one qualified; #808, #779 and the semicolon change did not. The skill records the procedure and, more usefully, the traps that cost the most time: cache MxBuild for the version under test explicitly; create the scratch project at a SHORT path, because Mendix throws PathTooLongException on deep ones and in Go tests that turns into a silent skip (set TMPDIR); install the Playwright package but never the browsers; and A/B against a pre-fix binary, since a run that only shows the fixed state demonstrates the app works, not that the change is why. Also the mundane one that nearly landed in a commit: restore files with absolute paths after a mutation test, and finish with git status. Adds a companion checklist item requiring every bug fix to demonstrate its test detects the bug, by reverting the fix and watching it fail. Contributor-only: the top-level .claude/skills/ are not synced into user projects by mxcli init (only .claude/skills/mendix/ is). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012XR649rKk68z6gBpngu6MA --- .claude/skills/fix-issue.md | 11 +- .claude/skills/verify-in-runtime.md | 183 ++++++++++++++++++++++++++++ CLAUDE.md | 3 + 3 files changed, 196 insertions(+), 1 deletion(-) create mode 100644 .claude/skills/verify-in-runtime.md diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index 58a93358a..512f925cd 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -8,7 +8,16 @@ to the symptom table below, so the next similar issue costs fewer reads. 1. Match the issue symptom to a row in the table — go straight to that file. 2. Follow the fix pattern for that row. 3. Write a failing test first, then implement. -4. After the fix: **add a new row** to the table if the symptom is not already covered. +4. **Verify at the layer the symptom lives in.** Parser → unit test. BSON we write → + unit test on the encoded document. Files on disk after `mx` runs → integration test + (`-tags integration`). The **rendered app's behaviour or appearance** → boot it and + assert in a browser: [`verify-in-runtime.md`](./verify-in-runtime.md). A page can + serialize to valid-looking BSON, pass `mx check`, build cleanly, and still render + wrong. +5. **Prove the test detects the bug**: revert the fix and confirm it fails with the + reported symptom. A test that has only ever run against fixed code has not been + shown to detect anything. +6. After the fix: **add a new row** to the table if the symptom is not already covered. --- diff --git a/.claude/skills/verify-in-runtime.md b/.claude/skills/verify-in-runtime.md new file mode 100644 index 000000000..f18c7ac71 --- /dev/null +++ b/.claude/skills/verify-in-runtime.md @@ -0,0 +1,183 @@ +# Verify a Fix in the Running App + +A contributor workflow for proving a fix in a **real Mendix app in a real browser**, +rather than at the BSON or parser layer. + +This is the outermost verification tier. It is slow (~5 min the first time, ~2 min +after) and it is not needed for most fixes — read *When This Is Required* before +reaching for it. + +## When This Is Required + +Use the layer where the symptom actually lives: + +| Symptom lives in | Sufficient proof | +|------------------|------------------| +| the parser / grammar | unit test | +| the BSON we write | unit test on the encoded document | +| files on disk after `mx` runs | integration test (`-tags integration`) | +| **the rendered app's behaviour or appearance** | **this skill** | + +**The rule: if the symptom is a property of the *running app* rather than of the file +we write, no unit or BSON test can prove the fix.** A page can serialize to perfectly +correct-looking BSON and still render wrong. + +Worked example — mendixlabs/mxcli#812. Every popup opened by an mxcli-authored button +showed a blank caption. The BSON was structurally valid, `mx check` reported 0 errors, +and MxBuild completed. Nothing below the browser could see the defect, because the +defect *was* the rendering: an empty `Microflows$TextTemplate` is not "no title +override", it is an override to the empty string. + +Counter-examples from the same week, where this skill would have been waste: +`#808` (MPRv2 → v1 conversion — visible on disk, integration test), `#779` (Commit +flag — visible in BSON round-trip, unit test), mandatory semicolons (parser). + +## The Procedure + +### 1. Cache MxBuild for the version you want + +```bash +mxcli setup mxbuild --version 11.12.2 +``` + +Do this **first**, explicitly. Test against the version users report against, not +whatever happens to be cached. + +### 2. Create a scratch project — at a SHORT path + +```bash +mxcli new PopupDemo --version 11.12.2 --output-dir /root/pd +``` + +> **Trap: `PathTooLongException`.** Mendix's package extractor fails on deep paths. +> A scratchpad path like +> `/tmp/claude-...//scratchpad/proj` is already too long and +> `mx create-project` dies. Use a short root (`/root/pd`). The same applies to Go +> tests: set `TMPDIR=/root/t` so `t.TempDir()` stays short, or the scaffolding fails +> and the test **skips** — which is indistinguishable from passing. + +Confirm you got the version you asked for; the connect banner prints it: + +```bash +echo "" > /root/pd/noop.mdl +mxcli exec /root/pd/noop.mdl -p /root/pd/PopupDemo.mpr | grep -i connected +# Connected to: /root/pd/PopupDemo.mpr (Mendix 11.12.2) +``` + +### 3. Author the smallest model that shows the symptom + +Keep it to the widgets/flows involved. A popup-caption repro is one popup page and one +button: + +```mdl +create or replace page MyFirstModule.OrderPopup ( + title: 'Order Details Popup', + Layout: Atlas_Core.PopupLayout +) { container c1 { dynamictext dtBody (content: 'Body of the popup') } } +/ +create or replace page MyFirstModule.Home_Web ( + title: 'Home', Layout: Atlas_Core.Atlas_Default +) { + container c1 { + actionbutton btnOpen (caption: 'Open popup', action: show_page MyFirstModule.OrderPopup) + } +} +/ +``` + +### 4. Provision and boot + +```bash +mxcli run --local -p /root/pd/PopupDemo.mpr --setup --ensure-db # once +mxcli run --local -p /root/pd/PopupDemo.mpr # boot (blocks) +``` + +`--setup` caches the runtime and provisions Postgres without booting. Run the boot in +the background and wait for the ready line rather than sleeping: + +```bash +until grep -qE 'App is running at|Error:|Exception' run.log; do sleep 3; done +``` + +First boot downloads the runtime and bundles the web client (~2–4 min). Later boots +are fast. + +### 5. Assert in the browser + +Chromium is pre-installed at `/opt/pw-browsers/chromium`; install the Playwright +package only (never `playwright install`): + +```bash +PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD=1 npm install playwright +``` + +Drive it with `NO_PROXY='*'` so the agent proxy does not intercept localhost. Assert +on **text content**, and print a JSON result so the outcome is unambiguous: + +```js +const browser = await chromium.launch({ executablePath: '/opt/pw-browsers/chromium' }); +// ... click, wait for the dialog, then: +const title = await dialog.locator('.modal-header .modal-title').innerText(); +console.log(JSON.stringify({ captionText: title, captionIsBlank: title.trim() === '' })); +``` + +### 6. A/B against the broken build — this is the actual proof + +**A run that only shows the fixed state proves nothing.** It shows the app works, not +that your change is why. Build a binary from before the fix, rewrite the same model +with it, restart, and re-observe: + +```bash +git checkout HEAD~1 -- +go build -o /tmp/mxcli-buggy ./cmd/mxcli +git checkout HEAD -- # restore immediately + +/tmp/mxcli-buggy exec demo.mdl -p app.mpr # rewrite the model, broken +# restart the app, re-run the browser assertion +``` + +The #812 result, which is what a finished verification looks like: + +| build | `TitleOverride` | `headerText` | caption | +|---|---|---|---| +| fixed | `null` | `"×\nOrder Details Popup"` | `"Order Details Popup"` | +| pre-fix | empty `TextTemplate` | `"×"` | `""` | + +Only the second row proves the fix is the cause. + +> **Trap: restoring after the mutation.** Use **absolute paths** when copying files +> back. A relative `cp` after a `cd` silently fails and leaves the reverted code in +> your tree — it will be committed. Always finish with `git status`. + +## Why This Tier Exists + +Two bugs in one week had a green test suite while broken: + +- **#812** — a duplicate `codec.RegisterTypeDefaults` for the same `$Type` silently + clobbered the fix (registrations overwrite rather than merge, resolved by init + order). The unit test passed; the BSON was still wrong. Caught only by dumping the + actual document. +- **#808** — the integration test had *only ever skipped*, for want of an `mx` binary. + A skip reads exactly like a pass in CI output. + +So: **a green suite is evidence about the layer it tests, and nothing more.** When a +test can silently become a no-op, prefer `t.Fatal` over `t.Skipf` once the +prerequisite is present — a missing `mx` is a legitimate skip; a present `mx` plus +failed scaffolding is a failure. + +## Cleanup + +```bash +pkill -f 'mxbuild --serve'; pkill -f runtimelauncher +rm -rf /root/pd +``` + +Cached MxBuild is ~1.4 GB per version. Keep the versions you test against; drop the +rest if disk gets tight (`~/.mxcli/mxbuild/`). + +## Related + +- `.claude/skills/fix-issue.md` — symptom table; start there +- `.claude/skills/debug-bson.md` — when the symptom *is* in the BSON +- `.claude/skills/mendix/run-local.md` — full `run --local` reference (flags, + `--screenshot`, hot reload) diff --git a/CLAUDE.md b/CLAUDE.md index 8e38b1588..6d89df489 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -287,6 +287,8 @@ When reviewing pull requests or validating work before commit, verify these item - [ ] **Fix-issue skill consulted** — read `.claude/skills/fix-issue.md` before diagnosing; match symptom to table before opening files - [ ] **Symptom table updated** — new symptom/layer/file mapping added to `.claude/skills/fix-issue.md` if not already covered - [ ] **Test written first** — failing test exists before implementation (parser test in `sdk/mpr/`, backend mutation test in `mdl/backend/mpr/`, executor handler test in `mdl/executor/` using `MockBackend`) +- [ ] **Verified at the layer the symptom lives in** — a test proves something about the layer it exercises and nothing more. Parser/grammar → unit test. BSON we write → unit test on the encoded document. Files on disk after `mx` runs → integration test (`-tags integration`). **The rendered app's behaviour or appearance → `.claude/skills/verify-in-runtime.md`** (boot with `run --local`, assert in Playwright). A page can serialize to valid-looking BSON, pass `mx check`, build cleanly, and still render wrong — that was #812. +- [ ] **Fix proven to be the cause** — revert the fix (or stub the guard) and confirm the test fails with the reported symptom. A test that only passes against fixed code has not been shown to detect anything; two bugs this week had a green suite while live (#812 a clobbered `RegisterTypeDefaults`, #808 an integration test that had only ever skipped) ### Overlap & duplication - [ ] Check `docs/11-proposals/` for existing proposals covering the same functionality @@ -575,6 +577,7 @@ Full syntax tables for all MDL statements (microflows, pages, security, navigati - `docs/03-development/PAGE_BSON_SERIALIZATION.md` - Page/widget BSON format, type mappings, required defaults - `docs/03-development/WIDGET_BSON_VERSION_COMPATIBILITY.md` - What's version-resilient vs version-fragile in widget BSON output, and how to onboard a new Mendix minor (e.g. 11.10) - `.claude/skills/debug-bson.md` - Workflow for debugging BSON serialization issues with `mx` tool (includes the "Studio Pro Update Widget" diff methodology that closed CE0463) +- `.claude/skills/verify-in-runtime.md` - Proving a fix in a real app in a real browser (`run --local` + Playwright). For symptoms that only exist at render time, where valid-looking BSON and a clean `mx check` prove nothing — see #812 - `cmd/mxcli/lsp.go` - LSP server implementation (hover, definition, diagnostics, completion, symbols) - `cmd/mxcli/init.go` - `mxcli init` command (project initialization + VS Code extension install) - `cmd/mxcli/docker/oql.go` - OQL query execution against running Mendix runtime via M2EE admin API From 2767afbe72dd44cef1da6d4a71c08c207386120f Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 04:25:48 +0000 Subject: [PATCH 07/11] docs(fix-issue): append symptom rows at the end of the table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every bug fix touches .claude/skills/fix-issue.md, and new rows went in directly under the table header. Two branches fixing unrelated bugs therefore inserted at the same line, which git cannot merge — a conflict by construction rather than by bad luck. It cost five separate resolution rounds in one week (#59-#62 three times, then #68 and #69), and each round risks silently dropping a row: one earlier round was "resolved" by deleting a PR's rows entirely and restoring them in a follow-up. Appending puts each branch's insert at a different offset, which git merges without help. The table is unordered — it is looked up by matching a symptom, not read top to bottom — so position carries no meaning and appending costs nothing. Recorded in both places a contributor might look: the skill's How to Use, with the reasoning so it is not "fixed" back, and the PR checklist item in CLAUDE.md. Existing rows are deliberately left where they are. Reordering them would conflict with every open branch at once, which is the problem this change exists to avoid. Folded into this branch rather than opened separately: it edits the same How to Use block, so a sixth branch would have manufactured exactly the conflict it is meant to prevent. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012XR649rKk68z6gBpngu6MA --- .claude/skills/fix-issue.md | 14 ++++++++++++++ CLAUDE.md | 2 +- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index 512f925cd..82409a814 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -18,6 +18,20 @@ to the symptom table below, so the next similar issue costs fewer reads. reported symptom. A test that has only ever run against fixed code has not been shown to detect anything. 6. After the fix: **add a new row** to the table if the symptom is not already covered. + **Append it at the END of the table, never at the top.** + +> **Why the end matters.** Every bug fix touches this one file, and for a long time +> new rows went in directly under the header. Two branches fixing two unrelated bugs +> therefore inserted at the *same line*, and git cannot merge that — it is a conflict +> by construction, not by bad luck. It cost five separate resolution rounds in one +> week, and each round risks dropping someone's row. +> +> Appending puts each branch's insert at a different offset, which git merges without +> help. The table is unordered — it is looked up by matching a symptom, not by +> reading top to bottom — so position carries no meaning and appending costs nothing. +> +> The rows above pre-date this convention and are left as they are; reordering them +> would conflict with every open branch at once, which is the problem, not the fix. --- diff --git a/CLAUDE.md b/CLAUDE.md index 6d89df489..87de43888 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -285,7 +285,7 @@ When reviewing pull requests or validating work before commit, verify these item ### Bug fixes - [ ] **Fix-issue skill consulted** — read `.claude/skills/fix-issue.md` before diagnosing; match symptom to table before opening files -- [ ] **Symptom table updated** — new symptom/layer/file mapping added to `.claude/skills/fix-issue.md` if not already covered +- [ ] **Symptom table updated** — new symptom/layer/file mapping added to `.claude/skills/fix-issue.md` if not already covered. **Append at the END of the table**: every fix touches this file, so inserting at the top means two concurrent fixes conflict at the same line by construction (five resolution rounds in one week). The table is looked up by matching a symptom, not read in order, so position carries no meaning - [ ] **Test written first** — failing test exists before implementation (parser test in `sdk/mpr/`, backend mutation test in `mdl/backend/mpr/`, executor handler test in `mdl/executor/` using `MockBackend`) - [ ] **Verified at the layer the symptom lives in** — a test proves something about the layer it exercises and nothing more. Parser/grammar → unit test. BSON we write → unit test on the encoded document. Files on disk after `mx` runs → integration test (`-tags integration`). **The rendered app's behaviour or appearance → `.claude/skills/verify-in-runtime.md`** (boot with `run --local`, assert in Playwright). A page can serialize to valid-looking BSON, pass `mx check`, build cleanly, and still render wrong — that was #812. - [ ] **Fix proven to be the cause** — revert the fix (or stub the guard) and confirm the test fails with the reported symptom. A test that only passes against fixed code has not been shown to detect anything; two bugs this week had a green suite while live (#812 a clobbered `RegisterTypeDefaults`, #808 an integration test that had only ever skipped) From 68f290a4362f261dc9aa650bb6a64614df3b503b Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 14:22:24 +0000 Subject: [PATCH 08/11] fix(microflows): declare the Commit values Mendix actually defines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CommitType declared two values that do not exist in the Mendix metamodel: "YesWithEvents" and "NoEvent". The canonical set is Yes / YesWithoutEvents / No (modelsdk/gen/microflows CommitEnum, generated from Mendix reflection data, and docs/05-mdl-specification/10-bson-mapping.md). Nothing crashed, because the read path passes the stored string through untouched — but the named constants matched nothing in a real project, so any code switching on them fell through to its default. The giveaway was mdl/backend/mcp, whose mfCommitType had to translate CommitTypeNoEvent into "YesWithoutEvents" to produce a correct PED value, mapping a constant named "no events" onto "yes, without events". Replaces the set with the canonical three and simplifies mfCommitType, which no longer needs the translation. A new test pins CommitType against the generated CommitEnum in both directions, so neither a missing value nor an invented one can drift back in. Groundwork for mendixlabs/mxcli#779 (exposing the flag in DESCRIBE MICROFLOW), which needs a trustworthy value set to render. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012XR649rKk68z6gBpngu6MA --- mdl/backend/mcp/microflow.go | 4 +- mdl/backend/mcp/microflow_test.go | 11 ++--- sdk/microflows/commit_type_test.go | 60 ++++++++++++++++++++++++++++ sdk/microflows/microflows_actions.go | 13 ++++-- 4 files changed, 77 insertions(+), 11 deletions(-) create mode 100644 sdk/microflows/commit_type_test.go diff --git a/mdl/backend/mcp/microflow.go b/mdl/backend/mcp/microflow.go index 5a1a149ee..64eb87067 100644 --- a/mdl/backend/mcp/microflow.go +++ b/mdl/backend/mcp/microflow.go @@ -789,9 +789,9 @@ func memberChangeType(t microflows.MemberChangeType) string { // mfCommitType maps a CommitType onto the PED commit enum (Yes / YesWithoutEvents / No). func mfCommitType(c microflows.CommitType) string { switch c { - case microflows.CommitTypeYes, microflows.CommitTypeYesWithEvents: + case microflows.CommitTypeYes: return "Yes" - case microflows.CommitTypeNoEvent: + case microflows.CommitTypeYesWithoutEvents: return "YesWithoutEvents" default: return "No" diff --git a/mdl/backend/mcp/microflow_test.go b/mdl/backend/mcp/microflow_test.go index 1f4a37216..30f774ff5 100644 --- a/mdl/backend/mcp/microflow_test.go +++ b/mdl/backend/mcp/microflow_test.go @@ -438,11 +438,12 @@ func TestMapObjectTree_Loop(t *testing.T) { func TestMfCommitType(t *testing.T) { cases := map[microflows.CommitType]string{ - microflows.CommitTypeYes: "Yes", - microflows.CommitTypeYesWithEvents: "Yes", - microflows.CommitTypeNoEvent: "YesWithoutEvents", - microflows.CommitTypeNo: "No", - microflows.CommitType(""): "No", + microflows.CommitTypeYes: "Yes", + microflows.CommitTypeYesWithoutEvents: "YesWithoutEvents", + microflows.CommitTypeNo: "No", + // An unset flag is Mendix's default, No — the same as an unrecognised one. + microflows.CommitType(""): "No", + microflows.CommitType("Nonsense"): "No", } for in, want := range cases { if got := mfCommitType(in); got != want { diff --git a/sdk/microflows/commit_type_test.go b/sdk/microflows/commit_type_test.go new file mode 100644 index 000000000..8c8416904 --- /dev/null +++ b/sdk/microflows/commit_type_test.go @@ -0,0 +1,60 @@ +// SPDX-License-Identifier: Apache-2.0 + +// CommitType previously declared two values Mendix does not define — +// "YesWithEvents" and "NoEvent". Nothing crashed, because the read path passes the +// stored string straight through, but the named constants matched nothing in a real +// project: any code switching on them silently fell through to the default. The +// giveaway was mdl/backend/mcp, which had to translate CommitTypeNoEvent into +// "YesWithoutEvents" to stay correct. +// +// This test pins the set to the generated metamodel so a hand-written constant can +// never drift from the enum again. +package microflows + +import ( + "testing" + + genMf "github.com/mendixlabs/mxcli/modelsdk/gen/microflows" +) + +func TestCommitType_MatchesMetamodelEnum(t *testing.T) { + // The generated CommitEnum is derived from Mendix's own reflection data and is + // the authority for what may appear in a BSON Commit field. + want := map[string]bool{ + string(genMf.CommitEnumYes): true, + string(genMf.CommitEnumYesWithoutEvents): true, + string(genMf.CommitEnumNo): true, + } + + got := map[string]bool{ + string(CommitTypeYes): true, + string(CommitTypeYesWithoutEvents): true, + string(CommitTypeNo): true, + } + + for v := range want { + if !got[v] { + t.Errorf("metamodel defines Commit value %q with no CommitType constant", v) + } + } + for v := range got { + if !want[v] { + t.Errorf("CommitType declares %q, which is not a Mendix Commit value — "+ + "code switching on it can never match a real project", v) + } + } +} + +// TestCommitType_DefaultIsNo documents the zero value: an action built without an +// explicit flag must serialize as Mendix's default, No — not as an empty string. +func TestCommitType_DefaultIsNo(t *testing.T) { + var zero CommitType + if zero == CommitTypeNo { + t.Fatal("test premise wrong: the zero value is not literally CommitTypeNo") + } + // Callers must set the flag explicitly; the builder does. This asserts the + // constant's spelling, which is what the BSON writer emits. + if string(CommitTypeNo) != "No" { + t.Errorf("CommitTypeNo = %q, want %q", CommitTypeNo, "No") + } +} diff --git a/sdk/microflows/microflows_actions.go b/sdk/microflows/microflows_actions.go index 758409691..7a2d77c0f 100644 --- a/sdk/microflows/microflows_actions.go +++ b/sdk/microflows/microflows_actions.go @@ -91,11 +91,16 @@ const ( // CommitType represents how objects are committed. type CommitType string +// Mendix defines exactly three values (Microflows$Commit). "YesWithEvents" and +// "NoEvent" were never among them — they matched nothing in a real project and +// forced mfCommitType to translate CommitTypeNoEvent into "YesWithoutEvents" to +// stay correct. The set below is the canonical one; see +// modelsdk/gen/microflows/enums.go (CommitEnum) and +// docs/05-mdl-specification/10-bson-mapping.md. const ( - CommitTypeYes CommitType = "Yes" - CommitTypeNo CommitType = "No" - CommitTypeYesWithEvents CommitType = "YesWithEvents" - CommitTypeNoEvent CommitType = "NoEvent" + CommitTypeYes CommitType = "Yes" + CommitTypeYesWithoutEvents CommitType = "YesWithoutEvents" + CommitTypeNo CommitType = "No" ) // Retrieve Actions From d55832d45dc880284bf713a58121033ddc0588c0 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 14:27:38 +0000 Subject: [PATCH 09/11] feat(microflows): express the Commit flag on create/change activities (#779) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MDL had no syntax for the Commit setting, and the builder hardcoded CommitTypeNo at both call sites. Every create/change authored through MDL was therefore written as Commit: No regardless of intent — and a describe → edit → re-exec round-trip silently turned committing activities into non-committing ones. That is a data-fidelity bug, not just a missing feature: it is exactly the object-orphaning failure mode #779 reports chasing in Studio Pro. Syntax follows the existing `refresh` modifier on change, spelling all three values of Mendix's Microflows$Commit enum: $Order = create Sales.Order (Number = 'X'); -- No (default) $Order = create Sales.Order (Number = 'X') commit; -- Yes $Order = create Sales.Order (Number = 'X') commit without events; change $Order (Status = 'Paid') commit refresh; The default stays unwritten, so existing scripts are unaffected and turning the flag on is a one-word diff. `COMMIT` both modifies a create/change and starts a standalone commit activity, so the two could be confused. Mandatory statement semicolons make it decidable: the modifier cannot absorb a following `commit $Var` across a terminator. Two tests pin that specifically — a create followed by a commit activity, and a create carrying the modifier AND followed by one. Wired grammar → AST (CommitFlag) → visitor (buildCommitClause) → builder (commitTypeOf). DESCRIBE emission is the next commit; until then the flag is authorable and persisted but not yet rendered. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012XR649rKk68z6gBpngu6MA --- mdl/ast/ast_microflow.go | 17 ++- .../cmd_microflows_builder_actions.go | 18 ++- mdl/grammar/domains/MDLMicroflow.g4 | 18 ++- mdl/visitor/visitor_commit_flag_test.go | 128 ++++++++++++++++++ mdl/visitor/visitor_microflow_statements.go | 15 ++ 5 files changed, 190 insertions(+), 6 deletions(-) create mode 100644 mdl/visitor/visitor_commit_flag_test.go diff --git a/mdl/ast/ast_microflow.go b/mdl/ast/ast_microflow.go index e37775d40..ac4f48b4d 100644 --- a/mdl/ast/ast_microflow.go +++ b/mdl/ast/ast_microflow.go @@ -219,21 +219,34 @@ type ChangeItem struct { Value Expression // Value expression } -// CreateObjectStmt represents: $Var = CREATE Entity (assignments) [ON ERROR ...] +// CommitFlag is the Commit setting on a create/change activity, matching Mendix's +// Microflows$Commit enum. The zero value is CommitNo, which is Mendix's default and +// is therefore omitted from DESCRIBE output. +type CommitFlag int + +const ( + CommitNo CommitFlag = iota // no COMMIT clause + CommitYes // COMMIT + CommitYesWithoutEvents // COMMIT WITHOUT EVENTS +) + +// CreateObjectStmt represents: $Var = CREATE Entity (assignments) [COMMIT [WITHOUT EVENTS]] [ON ERROR ...] type CreateObjectStmt struct { Variable string // Variable name (without $ prefix) EntityType QualifiedName // Entity type Changes []ChangeItem // SET assignments + Commit CommitFlag // Commit setting (default CommitNo) ErrorHandling *ErrorHandlingClause // Optional ON ERROR clause Annotations *ActivityAnnotations // Optional @position, @caption, @color, @annotation } func (s *CreateObjectStmt) isMicroflowStatement() {} -// ChangeObjectStmt represents: CHANGE $Var (assignments) +// ChangeObjectStmt represents: CHANGE $Var (assignments) [COMMIT [WITHOUT EVENTS]] [REFRESH] type ChangeObjectStmt struct { Variable string // Variable name Changes []ChangeItem // SET assignments + Commit CommitFlag // Commit setting (default CommitNo) RefreshInClient bool // Whether to refresh in client Annotations *ActivityAnnotations // Optional @position, @caption, @color, @annotation } diff --git a/mdl/executor/cmd_microflows_builder_actions.go b/mdl/executor/cmd_microflows_builder_actions.go index d7cb7b391..684ecd01e 100644 --- a/mdl/executor/cmd_microflows_builder_actions.go +++ b/mdl/executor/cmd_microflows_builder_actions.go @@ -86,13 +86,27 @@ func (fb *flowBuilder) addChangeVariableAction(s *ast.MfSetStmt) model.ID { return activity.ID } +// commitTypeOf maps the AST's Commit modifier onto the stored Mendix enum. Both +// create and change previously hardcoded CommitTypeNo, so any project authored or +// round-tripped through MDL had its commit flags silently cleared (#779). +func commitTypeOf(f ast.CommitFlag) microflows.CommitType { + switch f { + case ast.CommitYes: + return microflows.CommitTypeYes + case ast.CommitYesWithoutEvents: + return microflows.CommitTypeYesWithoutEvents + default: + return microflows.CommitTypeNo + } +} + // addCreateObjectAction creates a CREATE OBJECT statement. func (fb *flowBuilder) addCreateObjectAction(s *ast.CreateObjectStmt) model.ID { action := µflows.CreateObjectAction{ BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, ErrorHandlingType: fb.ehType(s.ErrorHandling), OutputVariable: s.Variable, - Commit: microflows.CommitTypeNo, + Commit: commitTypeOf(s.Commit), } // Set entity reference as qualified name (BY_NAME_REFERENCE) entityQN := "" @@ -238,7 +252,7 @@ func (fb *flowBuilder) addChangeObjectAction(s *ast.ChangeObjectStmt) model.ID { BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, ErrorHandlingType: fb.ehType(nil), ChangeVariable: s.Variable, - Commit: microflows.CommitTypeNo, + Commit: commitTypeOf(s.Commit), RefreshInClient: s.RefreshInClient || len(s.Changes) == 0, } diff --git a/mdl/grammar/domains/MDLMicroflow.g4 b/mdl/grammar/domains/MDLMicroflow.g4 index d47b0832c..4bc34edd4 100644 --- a/mdl/grammar/domains/MDLMicroflow.g4 +++ b/mdl/grammar/domains/MDLMicroflow.g4 @@ -212,13 +212,27 @@ setStatement ; // $NewProduct = CREATE MfTest.Product (Name = $Name, Code = $Code); +// $NewProduct = CREATE MfTest.Product (Name = $Name) COMMIT; createObjectStatement - : (VARIABLE EQUALS)? CREATE nonListDataType (LPAREN memberAssignmentList? RPAREN)? onErrorClause? + : (VARIABLE EQUALS)? CREATE nonListDataType (LPAREN memberAssignmentList? RPAREN)? commitClause? onErrorClause? ; // CHANGE $Product (Name = $NewName, ModifiedDate = [%CurrentDateTime%]); +// CHANGE $Product (Name = $NewName) COMMIT WITHOUT EVENTS REFRESH; changeObjectStatement - : CHANGE VARIABLE (LPAREN memberAssignmentList? RPAREN)? REFRESH? + : CHANGE VARIABLE (LPAREN memberAssignmentList? RPAREN)? commitClause? REFRESH? + ; + +// The Commit flag on a create/change activity: Mendix's Microflows$Commit enum. +// Absent = No (the default, so it is omitted from DESCRIBE output); COMMIT = Yes; +// COMMIT WITHOUT EVENTS = YesWithoutEvents. +// +// This is a modifier, NOT the standalone `COMMIT $Var` activity (commitStatement). +// The two are told apart by what follows: the activity always names a variable. +// Because a body statement must be terminated (see microflowStatement), a stray +// `commit $Other` on the next line cannot be absorbed into this clause. +commitClause + : COMMIT (WITHOUT EVENTS)? ; // Shared by SET, LOOP, aggregate expressions, and validation feedback targets. diff --git a/mdl/visitor/visitor_commit_flag_test.go b/mdl/visitor/visitor_commit_flag_test.go new file mode 100644 index 000000000..ef196b2a7 --- /dev/null +++ b/mdl/visitor/visitor_commit_flag_test.go @@ -0,0 +1,128 @@ +// SPDX-License-Identifier: Apache-2.0 + +// mendixlabs/mxcli#779: MDL had no way to express the Commit flag on a create or +// change activity, and the builder hardcoded CommitTypeNo — so authoring or +// round-tripping a microflow through MDL silently cleared every commit flag in it. +// That is the object-orphaning failure mode the reporter hit. +// +// These tests cover the parse half: `COMMIT` / `COMMIT WITHOUT EVENTS` reach the AST, +// and the modifier is never confused with the standalone `COMMIT $Var` activity. +package visitor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +func flowBody(t *testing.T, body string) []ast.MicroflowStatement { + t.Helper() + src := "create microflow M.F ($C: M.Contract)\nbegin\n " + body + "\nend;" + prog, errs := Build(src) + if len(errs) > 0 { + t.Fatalf("parse %q: %v", body, errs) + } + mf, ok := prog.Statements[0].(*ast.CreateMicroflowStmt) + if !ok { + t.Fatalf("expected CreateMicroflowStmt, got %T", prog.Statements[0]) + } + return mf.Body +} + +func TestCommitClause_Create(t *testing.T) { + tests := []struct { + name string + body string + want ast.CommitFlag + }{ + {"absent is No", "$O = create M.Order (Number = 'X');", ast.CommitNo}, + {"commit", "$O = create M.Order (Number = 'X') commit;", ast.CommitYes}, + {"commit without events", "$O = create M.Order (Number = 'X') commit without events;", ast.CommitYesWithoutEvents}, + {"no member list", "$O = create M.Order commit;", ast.CommitYes}, + // The error handler still follows the modifier. + {"with on error", "$O = create M.Order (Number = 'X') commit on error rollback;", ast.CommitYes}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + stmts := flowBody(t, tc.body) + c, ok := stmts[0].(*ast.CreateObjectStmt) + if !ok { + t.Fatalf("expected CreateObjectStmt, got %T", stmts[0]) + } + if c.Commit != tc.want { + t.Errorf("Commit = %v, want %v", c.Commit, tc.want) + } + }) + } +} + +func TestCommitClause_Change(t *testing.T) { + tests := []struct { + name string + body string + want ast.CommitFlag + refresh bool + }{ + {"absent is No", "change $C (Status = 'P');", ast.CommitNo, false}, + {"commit", "change $C (Status = 'P') commit;", ast.CommitYes, false}, + {"commit without events", "change $C (Status = 'P') commit without events;", ast.CommitYesWithoutEvents, false}, + // Ordering: the commit modifier precedes refresh, and both must survive. + {"commit then refresh", "change $C (Status = 'P') commit refresh;", ast.CommitYes, true}, + {"without events then refresh", "change $C (Status = 'P') commit without events refresh;", ast.CommitYesWithoutEvents, true}, + {"refresh alone still works", "change $C (Status = 'P') refresh;", ast.CommitNo, true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + stmts := flowBody(t, tc.body) + c, ok := stmts[0].(*ast.ChangeObjectStmt) + if !ok { + t.Fatalf("expected ChangeObjectStmt, got %T", stmts[0]) + } + if c.Commit != tc.want { + t.Errorf("Commit = %v, want %v", c.Commit, tc.want) + } + if c.RefreshInClient != tc.refresh { + t.Errorf("RefreshInClient = %v, want %v", c.RefreshInClient, tc.refresh) + } + }) + } +} + +// TestCommitClause_NotConfusedWithCommitActivity is the ambiguity guard. `COMMIT` +// both modifies a create/change and starts a standalone commit activity; the two +// must stay distinct. Mandatory statement semicolons are what make this decidable — +// without the terminator the modifier would greedily absorb the next statement. +func TestCommitClause_NotConfusedWithCommitActivity(t *testing.T) { + stmts := flowBody(t, "$O = create M.Order (Number = 'X');\n commit $O;") + if len(stmts) != 2 { + t.Fatalf("expected 2 statements, got %d", len(stmts)) + } + c, ok := stmts[0].(*ast.CreateObjectStmt) + if !ok { + t.Fatalf("statement 0 = %T, want *ast.CreateObjectStmt", stmts[0]) + } + if c.Commit != ast.CommitNo { + t.Errorf("the following commit activity was absorbed as a modifier: Commit = %v", c.Commit) + } + cm, ok := stmts[1].(*ast.MfCommitStmt) + if !ok { + t.Fatalf("statement 1 = %T, want *ast.MfCommitStmt", stmts[1]) + } + if cm.Variable != "O" { + t.Errorf("commit activity variable = %q, want %q", cm.Variable, "O") + } +} + +// A create carrying the modifier AND followed by a commit activity: both survive. +func TestCommitClause_ModifierAndActivityTogether(t *testing.T) { + stmts := flowBody(t, "$O = create M.Order (Number = 'X') commit;\n commit $C;") + if len(stmts) != 2 { + t.Fatalf("expected 2 statements, got %d", len(stmts)) + } + if c := stmts[0].(*ast.CreateObjectStmt); c.Commit != ast.CommitYes { + t.Errorf("create Commit = %v, want CommitYes", c.Commit) + } + if cm, ok := stmts[1].(*ast.MfCommitStmt); !ok || cm.Variable != "C" { + t.Errorf("statement 1 = %T (%+v), want a commit activity on $C", stmts[1], stmts[1]) + } +} diff --git a/mdl/visitor/visitor_microflow_statements.go b/mdl/visitor/visitor_microflow_statements.go index c74bff7af..88bd14914 100644 --- a/mdl/visitor/visitor_microflow_statements.go +++ b/mdl/visitor/visitor_microflow_statements.go @@ -998,6 +998,8 @@ func buildCreateObjectStatement(ctx parser.ICreateObjectStatementContext) *ast.C stmt.Changes = buildMemberAssignmentList(memberList) } + stmt.Commit = buildCommitClause(createCtx.CommitClause()) + // Check for ON ERROR clause if errClause := createCtx.OnErrorClause(); errClause != nil { stmt.ErrorHandling = buildOnErrorClause(errClause) @@ -1006,6 +1008,18 @@ func buildCreateObjectStatement(ctx parser.ICreateObjectStatementContext) *ast.C return stmt } +// buildCommitClause maps the optional COMMIT modifier on a create/change activity. +// Grammar: COMMIT (WITHOUT EVENTS)? — absent means Mendix's default, No. +func buildCommitClause(ctx parser.ICommitClauseContext) ast.CommitFlag { + if ctx == nil { + return ast.CommitNo + } + if cc, ok := ctx.(*parser.CommitClauseContext); ok && cc.EVENTS() != nil { + return ast.CommitYesWithoutEvents + } + return ast.CommitYes +} + // buildChangeObjectStatement converts CHANGE statement context to ChangeObjectStmt. // Grammar: CHANGE VARIABLE (LPAREN memberAssignmentList? RPAREN)? // Example: CHANGE $Product (Name = $NewName, ModifiedDate = [%CurrentDateTime%]); @@ -1026,6 +1040,7 @@ func buildChangeObjectStatement(ctx parser.IChangeObjectStatementContext) *ast.C if memberList := changeCtx.MemberAssignmentList(); memberList != nil { stmt.Changes = buildMemberAssignmentList(memberList) } + stmt.Commit = buildCommitClause(changeCtx.CommitClause()) stmt.RefreshInClient = changeCtx.REFRESH() != nil return stmt From 50515be76ef6ec4ad60d7ed95105ef0f98adef0a Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 14:34:43 +0000 Subject: [PATCH 10/11] feat(microflows): emit the Commit flag in DESCRIBE MICROFLOW (#779) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reported symptom: DESCRIBE rendered a create/change activity with Commit enabled identically to one without, so transaction boundaries were invisible to automated analysis, linting and impact tooling — and confirming them meant opening Studio Pro activity by activity. The value was already read (both engines populate CreateObjectAction.Commit and ChangeObjectAction.Commit); the formatter simply never looked at it. It now renders the modifier introduced in the previous commit, omitting it for Mendix's default (No) so existing describe output is unchanged for the common case. Canonical order on change is `commit [without events]` then `refresh`, matching the grammar, so re-executing the output rebuilds the same activity. Round-trip is asserted rather than assumed: TestFormatAction_CommitRoundTrips formats each of the three values, re-parses the emitted MDL through the visitor, and requires the flag to come back identical — which is #779's second acceptance criterion. A formatter emitting syntax the grammar cannot read would be worse than not emitting it at all. Adds mdl-examples/doctype-tests/779-commit-flag.mdl covering all three values on both create and change, with and without member lists, and the modifier sitting next to a standalone COMMIT activity. Docs: MDL_QUICK_REFERENCE.md create/change rows, write-microflows.md CREATE and CHANGE sections, and the `microflow.object-operations` syntax topic — each distinguishing the modifier from the standalone COMMIT activity, which is the easy thing to confuse. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012XR649rKk68z6gBpngu6MA --- .claude/skills/mendix/write-microflows.md | 19 +++ cmd/mxcli/syntax/features_microflow.go | 6 +- docs/01-project/MDL_QUICK_REFERENCE.md | 4 +- .../doctype-tests/779-commit-flag.mdl | 75 ++++++++ mdl/executor/cmd_microflows_format_action.go | 30 +++- .../cmd_microflows_format_commit_test.go | 161 ++++++++++++++++++ 6 files changed, 284 insertions(+), 11 deletions(-) create mode 100644 mdl-examples/doctype-tests/779-commit-flag.mdl create mode 100644 mdl/executor/cmd_microflows_format_commit_test.go diff --git a/.claude/skills/mendix/write-microflows.md b/.claude/skills/mendix/write-microflows.md index 61318a030..524532123 100644 --- a/.claude/skills/mendix/write-microflows.md +++ b/.claude/skills/mendix/write-microflows.md @@ -542,6 +542,18 @@ $NewProduct = create Test.Product ( - Closing `)` followed by semicolon - Syntax aligned with CALL MICROFLOW/CALL JAVA ACTION +**The Commit flag** (Studio Pro's "Commit" dropdown) is the optional `commit` +modifier after the member list: + +```mdl +$Order = create Sales.Order (Number = $Nr); -- Commit: No (default) +$Order = create Sales.Order (Number = $Nr) commit; -- Commit: Yes +$Order = create Sales.Order (Number = $Nr) commit without events;-- Commit: YesWithoutEvents +``` + +Omit it for the default. This is a **modifier on the create**, not the separate +`commit $Var;` activity — see COMMIT Object below for that one. + ### CHANGE Object ```mdl @@ -549,8 +561,15 @@ change $Product ( Name = $NewName, ModifiedDate = [%CurrentDateTime%]); +-- Commit the changed object as part of the change activity +change $Product (Name = $NewName) commit; +change $Product (Name = $NewName) commit without events; + -- Refresh the changed object in the client change $Product (Name = $NewName) refresh; + +-- Both: commit comes first +change $Product (Name = $NewName) commit refresh; ``` **Note**: Only specify attributes you want to change. Syntax aligned with CREATE. diff --git a/cmd/mxcli/syntax/features_microflow.go b/cmd/mxcli/syntax/features_microflow.go index 89c4133aa..77e497a57 100644 --- a/cmd/mxcli/syntax/features_microflow.go +++ b/cmd/mxcli/syntax/features_microflow.go @@ -82,10 +82,10 @@ func init() { Keywords: []string{ "create object", "change object", "commit", "rollback", "delete", "save", "persist", "modify object", - "with events", "refresh", + "with events", "refresh", "commit flag", "without events", }, - Syntax: "$Obj = CREATE Module.Entity (Attr = value);\nCHANGE $Obj (Attr = value);\nCOMMIT $Obj;\nCOMMIT $Obj WITH EVENTS;\nCOMMIT $Obj REFRESH;\nCOMMIT $Obj WITH EVENTS REFRESH;\nDELETE $Obj;\nROLLBACK $Obj;", - Example: "$NewOrder = CREATE MyModule.Order (\n OrderNumber = 'ORD-001',\n Quantity = $Quantity,\n CreateDate = [%CurrentDateTime%]\n);\n\nCHANGE $NewOrder (MyModule.Order_Customer = $Customer);\nCOMMIT $NewOrder WITH EVENTS;\nDELETE $OldOrder;\nROLLBACK $DraftOrder;", + Syntax: "$Obj = CREATE Module.Entity (Attr = value) [COMMIT [WITHOUT EVENTS]];\nCHANGE $Obj (Attr = value) [COMMIT [WITHOUT EVENTS]] [REFRESH];\nCOMMIT $Obj;\nCOMMIT $Obj WITH EVENTS;\nCOMMIT $Obj REFRESH;\nCOMMIT $Obj WITH EVENTS REFRESH;\nDELETE $Obj;\nROLLBACK $Obj;\n\n-- The COMMIT modifier on CREATE/CHANGE is the activity's Commit setting\n-- (omitted = No). The standalone COMMIT $Obj is a separate activity.", + Example: "$NewOrder = CREATE MyModule.Order (\n OrderNumber = 'ORD-001',\n Quantity = $Quantity,\n CreateDate = [%CurrentDateTime%]\n) COMMIT;\n\nCHANGE $NewOrder (MyModule.Order_Customer = $Customer) COMMIT REFRESH;\nCHANGE $Draft (Status = 'Imported') COMMIT WITHOUT EVENTS;\nDELETE $OldOrder;\nROLLBACK $DraftOrder;", SeeAlso: []string{"microflow.retrieve", "microflow.variables"}, }) diff --git a/docs/01-project/MDL_QUICK_REFERENCE.md b/docs/01-project/MDL_QUICK_REFERENCE.md index ecd593ebb..2f42c4c6b 100644 --- a/docs/01-project/MDL_QUICK_REFERENCE.md +++ b/docs/01-project/MDL_QUICK_REFERENCE.md @@ -270,8 +270,8 @@ it is for pages. | Entity declaration | `declare $entity Module.Entity;` | No AS keyword, no = empty | | List declaration | `declare $list list of Module.Entity = empty;` | | | Assignment | `set $Var = expression;` | Variable must be declared first | -| Create object | `$Var = create Module.Entity (attr = value);` | | -| Change object | `change $entity (attr = value) [refresh];` | `refresh` updates the changed object in the client | +| Create object | `$Var = create Module.Entity (attr = value) [commit [without events]];` | `commit` = Commit Yes, `commit without events` = YesWithoutEvents; omitted = No (the default) | +| Change object | `change $entity (attr = value) [commit [without events]] [refresh];` | `commit` as above, before `refresh`; `refresh` updates the changed object in the client | | Commit | `commit $entity [with events] [refresh];` | | | Delete | `delete $entity;` | | | Rollback | `rollback $entity [refresh];` | Reverts uncommitted changes | diff --git a/mdl-examples/doctype-tests/779-commit-flag.mdl b/mdl-examples/doctype-tests/779-commit-flag.mdl new file mode 100644 index 000000000..d952a249f --- /dev/null +++ b/mdl-examples/doctype-tests/779-commit-flag.mdl @@ -0,0 +1,75 @@ +-- Issue #779: the Commit flag on create/change activities. +-- +-- Before this change MDL could not express the flag at all: the builder hardcoded +-- Commit: No at both call sites, so every create/change authored through MDL was +-- written as non-committing regardless of intent, and DESCRIBE MICROFLOW rendered a +-- committing activity identically to a non-committing one. A describe → edit → +-- re-exec round-trip therefore silently cleared commit boundaries — the +-- object-orphaning failure mode the issue reports chasing in Studio Pro. +-- +-- Three values, matching Mendix's Microflows$Commit enum: +-- +-- (no modifier) -> No (the default, omitted from DESCRIBE) +-- commit -> Yes +-- commit without events -> YesWithoutEvents +-- +-- Verify the round-trip: +-- +-- mxcli exec 779-commit-flag.mdl -p app.mpr +-- mxcli -p app.mpr -c "describe microflow Issue779.CommitVariants" +-- +-- The DESCRIBE output must carry the same modifiers as the source below, and be +-- re-executable as-is. + +create module Issue779; +create module role Issue779.User; + +@position(100, 100) +create persistent entity Issue779.Order ( + Number: string(50), + Status: string(20) +); + +create microflow Issue779.CommitVariants () +returns boolean +begin + -- Default: no modifier, Commit = No. Unchanged from before, so existing + -- scripts and existing DESCRIBE output are unaffected. + $Draft = create Issue779.Order (Number = 'ORD-0001', Status = 'Draft'); + + -- Commit = Yes. Enabling the flag is a one-word diff. + $Placed = create Issue779.Order (Number = 'ORD-0002', Status = 'Placed') commit; + + -- Commit = YesWithoutEvents: persists without firing before/after commit events. + $Imported = create Issue779.Order (Number = 'ORD-0003', Status = 'Imported') commit without events; + + -- No member list, still takes the modifier. + $Blank = create Issue779.Order commit; + + -- change carries the same modifier, before the existing `refresh`. + change $Draft (Status = 'Confirmed') commit; + change $Placed (Status = 'Shipped') commit without events; + change $Imported (Status = 'Reconciled') commit refresh; + + -- refresh alone still parses exactly as it did. + change $Blank (Status = 'Void') refresh; + + return true; +end; +/ + +-- The modifier must not be confused with the standalone COMMIT activity. These are +-- two different things on adjacent lines; mandatory statement semicolons are what +-- make them decidable. +create microflow Issue779.ModifierVsActivity () +returns boolean +begin + $A = create Issue779.Order (Number = 'ORD-0004'); + commit $A; + + $B = create Issue779.Order (Number = 'ORD-0005') commit; + commit $B with events; + + return true; +end; +/ diff --git a/mdl/executor/cmd_microflows_format_action.go b/mdl/executor/cmd_microflows_format_action.go index 57bfdbc53..88e445230 100644 --- a/mdl/executor/cmd_microflows_format_action.go +++ b/mdl/executor/cmd_microflows_format_action.go @@ -19,6 +19,24 @@ import ( // formatActivity formats a single microflow activity as an MDL statement. +// commitModifier renders the Commit flag of a create/change activity as the MDL +// modifier, or "" for Mendix's default (No) so the common case stays unwritten. +// The leading space is included, since the caller appends it directly after the +// member list. +// +// Before #779 this was never emitted: DESCRIBE could not distinguish a committing +// create from a non-committing one, and re-executing its output cleared the flag. +func commitModifier(c microflows.CommitType) string { + switch c { + case microflows.CommitTypeYes: + return " commit" + case microflows.CommitTypeYesWithoutEvents: + return " commit without events" + default: + return "" + } +} + // escapeExpressionValue escapes raw control characters inside string literals // of a Mendix expression value so it can be safely embedded in MDL output. // The lexer's STRING_LITERAL rule forbids raw \r and \n inside single-quoted @@ -256,9 +274,9 @@ func formatAction( } members = append(members, fmt.Sprintf("%s = %s", memberName, escapeExpressionValue(m.Value))) } - return fmt.Sprintf("$%s = create %s (%s);", outputVar, entityName, strings.Join(members, ", ")) + return fmt.Sprintf("$%s = create %s (%s)%s;", outputVar, entityName, strings.Join(members, ", "), commitModifier(a.Commit)) } - return fmt.Sprintf("$%s = create %s;", outputVar, entityName) + return fmt.Sprintf("$%s = create %s%s;", outputVar, entityName, commitModifier(a.Commit)) case *microflows.ChangeObjectAction: varName := a.ChangeVariable @@ -287,14 +305,14 @@ func formatAction( members = append(members, fmt.Sprintf("%s = %s", memberName, escapeExpressionValue(m.Value))) } if a.RefreshInClient { - return fmt.Sprintf("change $%s (%s) refresh;", varName, strings.Join(members, ", ")) + return fmt.Sprintf("change $%s (%s)%s refresh;", varName, strings.Join(members, ", "), commitModifier(a.Commit)) } - return fmt.Sprintf("change $%s (%s);", varName, strings.Join(members, ", ")) + return fmt.Sprintf("change $%s (%s)%s;", varName, strings.Join(members, ", "), commitModifier(a.Commit)) } if a.RefreshInClient { - return fmt.Sprintf("change $%s refresh;", varName) + return fmt.Sprintf("change $%s%s refresh;", varName, commitModifier(a.Commit)) } - return fmt.Sprintf("change $%s;", varName) + return fmt.Sprintf("change $%s%s;", varName, commitModifier(a.Commit)) case *microflows.CommitObjectsAction: varName := a.CommitVariable diff --git a/mdl/executor/cmd_microflows_format_commit_test.go b/mdl/executor/cmd_microflows_format_commit_test.go new file mode 100644 index 000000000..3080a7c10 --- /dev/null +++ b/mdl/executor/cmd_microflows_format_commit_test.go @@ -0,0 +1,161 @@ +// SPDX-License-Identifier: Apache-2.0 + +// mendixlabs/mxcli#779: DESCRIBE MICROFLOW could not distinguish a create/change +// activity with Commit enabled from one without — both rendered identically, so +// transaction boundaries were invisible to automated analysis and to anyone not +// willing to open Studio Pro. +// +// The round-trip half matters as much as the display: whatever DESCRIBE emits must +// re-parse to the same flag, or describe → edit → re-exec silently rewrites the +// project's commit boundaries. +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/visitor" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +func TestCommitModifier(t *testing.T) { + tests := []struct { + in microflows.CommitType + want string + }{ + {microflows.CommitTypeYes, " commit"}, + {microflows.CommitTypeYesWithoutEvents, " commit without events"}, + // The default is omitted, so existing describe output is unchanged for the + // overwhelmingly common case and enabling it is a one-word diff. + {microflows.CommitTypeNo, ""}, + {microflows.CommitType(""), ""}, + } + for _, tc := range tests { + if got := commitModifier(tc.in); got != tc.want { + t.Errorf("commitModifier(%q) = %q, want %q", tc.in, got, tc.want) + } + } +} + +func TestFormatAction_CreateEmitsCommit(t *testing.T) { + tests := []struct { + name string + commit microflows.CommitType + want string + }{ + {"default omits the modifier", microflows.CommitTypeNo, + "$Order = create Sales.Order (Number = 'X');"}, + {"commit", microflows.CommitTypeYes, + "$Order = create Sales.Order (Number = 'X') commit;"}, + {"commit without events", microflows.CommitTypeYesWithoutEvents, + "$Order = create Sales.Order (Number = 'X') commit without events;"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + a := µflows.CreateObjectAction{ + EntityQualifiedName: "Sales.Order", + OutputVariable: "Order", + Commit: tc.commit, + InitialMembers: []*microflows.MemberChange{ + {AttributeQualifiedName: "Sales.Order.Number", Value: "'X'"}, + }, + } + if got := formatAction(nil, a, nil, nil); got != tc.want { + t.Errorf("formatAction =\n %q\nwant\n %q", got, tc.want) + } + }) + } +} + +func TestFormatAction_CreateNoMembersEmitsCommit(t *testing.T) { + a := µflows.CreateObjectAction{ + EntityQualifiedName: "Sales.Order", + OutputVariable: "Order", + Commit: microflows.CommitTypeYes, + } + want := "$Order = create Sales.Order commit;" + if got := formatAction(nil, a, nil, nil); got != want { + t.Errorf("formatAction = %q, want %q", got, want) + } +} + +func TestFormatAction_ChangeEmitsCommit(t *testing.T) { + tests := []struct { + name string + commit microflows.CommitType + refresh bool + want string + }{ + {"default", microflows.CommitTypeNo, false, "change $Order (Status = 'Paid');"}, + {"commit", microflows.CommitTypeYes, false, "change $Order (Status = 'Paid') commit;"}, + {"commit without events", microflows.CommitTypeYesWithoutEvents, false, + "change $Order (Status = 'Paid') commit without events;"}, + // Canonical order: commit precedes refresh, matching the grammar. + {"commit and refresh", microflows.CommitTypeYes, true, + "change $Order (Status = 'Paid') commit refresh;"}, + {"refresh alone", microflows.CommitTypeNo, true, + "change $Order (Status = 'Paid') refresh;"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + a := µflows.ChangeObjectAction{ + ChangeVariable: "Order", + Commit: tc.commit, + RefreshInClient: tc.refresh, + Changes: []*microflows.MemberChange{ + {AttributeQualifiedName: "Sales.Order.Status", Value: "'Paid'"}, + }, + } + if got := formatAction(nil, a, nil, nil); got != tc.want { + t.Errorf("formatAction =\n %q\nwant\n %q", got, tc.want) + } + }) + } +} + +// TestFormatAction_CommitRoundTrips is the acceptance criterion from #779: what +// DESCRIBE emits must parse back to the same flag. A formatter that renders a +// modifier the grammar cannot read would be worse than not rendering it at all. +func TestFormatAction_CommitRoundTrips(t *testing.T) { + for _, commit := range []microflows.CommitType{ + microflows.CommitTypeNo, + microflows.CommitTypeYes, + microflows.CommitTypeYesWithoutEvents, + } { + t.Run(string(commit), func(t *testing.T) { + emitted := formatAction(nil, µflows.CreateObjectAction{ + EntityQualifiedName: "Sales.Order", + OutputVariable: "Order", + Commit: commit, + InitialMembers: []*microflows.MemberChange{ + {AttributeQualifiedName: "Sales.Order.Number", Value: "'X'"}, + }, + }, nil, nil) + + src := "create microflow M.F ()\nbegin\n " + emitted + "\nend;" + prog, errs := visitor.Build(src) + if len(errs) > 0 { + t.Fatalf("DESCRIBE output does not re-parse: %q\n%v", emitted, errs) + } + mf := prog.Statements[0].(*ast.CreateMicroflowStmt) + got := mf.Body[0].(*ast.CreateObjectStmt).Commit + + want := ast.CommitNo + switch commit { + case microflows.CommitTypeYes: + want = ast.CommitYes + case microflows.CommitTypeYesWithoutEvents: + want = ast.CommitYesWithoutEvents + } + if got != want { + t.Errorf("round-trip lost the flag: emitted %q, re-parsed as %v, want %v", emitted, got, want) + } + // Guard the reported symptom directly: two different flags must not + // produce identical MDL. + if commit != microflows.CommitTypeNo && !strings.Contains(emitted, "commit") { + t.Errorf("Commit=%q rendered without a commit modifier: %q", commit, emitted) + } + }) + } +} From 6067f41d343964ed873781049a8e5ce0d0df48db Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 04:57:40 +0000 Subject: [PATCH 11/11] =?UTF-8?q?docs(web):=20document=20the=20full=20Clau?= =?UTF-8?q?de=20Code=20web=20workflow=20(repo=20=E2=86=92=20hub=20key=20?= =?UTF-8?q?=E2=86=92=20env=20=E2=86=92=20prompt)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The existing web docs were scattered (bootstrap prompt + a run-local aside) and didn't describe the end-to-end path. Add a dedicated getting-started page and wire the surrounding workflow together. - docs-site: new tutorial page "Developing on the Web (Claude Code)" covering the whole workflow — create a GitHub repo, get a hub key from hub.mxcli.org for browser preview, create a Claude Code environment (MXCLI_HUB_KEY + network policy), paste the bootstrap prompt, iterate with the warm loop / --hub / screenshots, and how sessions self-bootstrap after idle. Wired into SUMMARY. - docs-site: the bootstrap prompt now creates a FINDINGS.md and logs to it, commits it, and includes an optional --hub browser-preview step. - CONTRIBUTING.md: document the end-to-end web dogfooding loop (build a real app in Claude Code web, keep a FINDINGS.md, feed it back) under Agentic Code Testing. - README: rewrite the "From the web or an iPad" section into a numbered workflow and link the new page. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01JXnEgoc2NQP1Y2TWMCMXC4 --- CONTRIBUTING.md | 24 +++ README.md | 16 +- docs-site/src/SUMMARY.md | 1 + docs-site/src/tools/bootstrap-prompt.md | 17 ++- docs-site/src/tutorial/claude-code-web.md | 171 ++++++++++++++++++++++ 5 files changed, 222 insertions(+), 7 deletions(-) create mode 100644 docs-site/src/tutorial/claude-code-web.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1c3a13913..b8654f259 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -297,6 +297,30 @@ mxcli's primary value is that AI agents (Claude Code, Cursor, etc.) can generate | Claude uses outdated patterns | Update CLI help text (`Short`/`long`/`Example` in Cobra) | | Error messages are unhelpful | Improve error text with hints | +### End-to-end testing on the web (build a real app + keep a findings doc) + +The highest-signal way to exercise mxcli is to **build a real Mendix app with it in +[Claude Code on the web](https://claude.ai/code)** and record what breaks. This is +the same loop the maintainers dogfood with (e.g. the `mxcli-traceops` / +`mxcli-timeregistration` test apps), and it works from a laptop or an iPad — no +local install. + +1. Create an **empty GitHub repo** and a Claude Code environment on it (set + `MXCLI_HUB_KEY` for browser preview; allow outbound HTTPS to GitHub Releases, + Mendix's CDN, and `hub.mxcli.org`). +2. Paste the **[bootstrap prompt](https://mendixlabs.github.io/mxcli/tools/bootstrap-prompt.html)** + — it installs the toolchain, creates the app, starts a `FINDINGS.md`, commits, + and boots the app. +3. Ask the agent to build real features (a domain model, CRUD pages, microflows), + and verify with `mxcli run --local` + `mxcli oql` and a real `mx check`. + +**Keep a `FINDINGS.md`** at the repo root as you go — one entry per surprise or +defect: the exact MDL, the command, the error, the Mendix + mxcli versions, and how +you verified it. A finding that reproduces against a real `mx check` is often enough +to drive a fix straight into a symptom row in `.claude/skills/fix-issue.md`. The full +workflow is documented in +**[Developing on the Web (Claude Code)](https://mendixlabs.github.io/mxcli/tutorial/claude-code-web.html)**. + ### PR Section Include in your PR: diff --git a/README.md b/README.md index 8f0038199..8e750c9c2 100644 --- a/README.md +++ b/README.md @@ -128,9 +128,19 @@ This downloads MxBuild, creates a blank Mendix project, sets up AI tooling and a ### From the web or an iPad (empty repo, no local install) -No machine with a CLI? Open an **empty repo** in [Claude Code on the web](https://claude.ai/code) (works on an iPad) and paste the **bootstrap prompt** — the agent provisions the whole project (creates the app, wires the Dev Container + AI tooling, provisions the database) and commits it so future sessions self-bootstrap. This is the recommended web/iPad path; you don't need to pick a GitHub template. - -> See **[Bootstrap Prompt](https://mendixlabs.github.io/mxcli/tools/bootstrap-prompt.html)** for the exact copy-paste text. In short: it runs `mxcli new` → `mxcli init` → commits the config → `mxcli run --local --setup --ensure-db` so the app comes up testable. +No machine with a CLI? Build a Mendix app **entirely in the browser** with +[Claude Code on the web](https://claude.ai/code) (works on an iPad) — mxcli +provisions the whole toolchain in the cloud container and commits it so future +sessions self-bootstrap. You don't need to pick a GitHub template. The full +workflow: + +1. **Create an empty GitHub repo** — this is where the app lives and where sessions run. +2. **(Optional) Get a hub key** for a browser preview: open , sign in with GitHub, and **Create a hub key**. +3. **Create a Claude Code environment** on the repo. Set `MXCLI_HUB_KEY=` (from step 2) and allow outbound HTTPS to GitHub Releases, Mendix's CDN, and `hub.mxcli.org`. +4. **Paste the bootstrap prompt** into a session. The agent installs mxcli, runs `mxcli new` → `mxcli init`, starts a `FINDINGS.md`, commits the config, and boots the app with `mxcli run --local --setup --ensure-db` so it comes up testable. +5. **Iterate** with the warm loop below; add `--hub https://hub.mxcli.org` for a shareable browser preview. + +> **[Developing on the Web (Claude Code)](https://mendixlabs.github.io/mxcli/tutorial/claude-code-web.html)** walks the whole workflow, and **[Bootstrap Prompt](https://mendixlabs.github.io/mxcli/tools/bootstrap-prompt.html)** has the exact copy-paste text. Then iterate with the **warm local dev loop** — a Docker-free ~1-second edit→test cycle: diff --git a/docs-site/src/SUMMARY.md b/docs-site/src/SUMMARY.md index f55ef7904..4cdff47a9 100644 --- a/docs-site/src/SUMMARY.md +++ b/docs-site/src/SUMMARY.md @@ -17,6 +17,7 @@ - [Installation](tutorial/installation.md) - [Opening Your First Project](tutorial/opening-project.md) - [The REPL](tutorial/repl.md) +- [Developing on the Web (Claude Code)](tutorial/claude-code-web.md) - [Exploring a Project](tutorial/exploring.md) - [SHOW MODULES, SHOW ENTITIES](tutorial/show-commands.md) - [DESCRIBE, SEARCH](tutorial/describe-search.md) diff --git a/docs-site/src/tools/bootstrap-prompt.md b/docs-site/src/tools/bootstrap-prompt.md index 49e573c63..5d85e75d1 100644 --- a/docs-site/src/tools/bootstrap-prompt.md +++ b/docs-site/src/tools/bootstrap-prompt.md @@ -36,11 +36,20 @@ This is an empty repo. Provision it as a Mendix app developed with mxcli: SessionStart hook to `.claude/settings.json` that self-bootstraps future sessions. 4. Bring prerequisites up: `./mxcli run --local --setup --ensure-db -p App.mpr` (caches MxBuild + runtime, starts Postgres, creates the app database). -5. COMMIT everything now — `App.mpr`, `.devcontainer/`, and `.claude/` (including the - SessionStart hook) — so that after idle reaping the next session bootstraps from - files, not from re-running this prompt. -6. Boot and verify: `./mxcli run --local -p App.mpr` in the background, then confirm +5. Create a `FINDINGS.md` at the repo root and keep appending to it as you work. + Log anything surprising or broken: an mxcli command that errored, a workaround you + applied, a `mxcli check` that passed but a real `mx check` later flagged. Note the + Mendix + mxcli versions and how each finding was verified. This is durable context + for the next session, and the most useful thing to share back to improve mxcli. +6. COMMIT everything now — `App.mpr`, `.devcontainer/`, `.claude/` (including the + SessionStart hook), and `FINDINGS.md` — so that after idle reaping the next session + bootstraps from files, not from re-running this prompt. +7. Boot and verify: `./mxcli run --local -p App.mpr` in the background, then confirm the app answers HTTP 200 at http://localhost:8080/ and report. +8. (Optional) For a browser preview from this cloud session, run + `./mxcli run --hub https://hub.mxcli.org -p App.mpr` and report the preview URL it + prints. This needs `MXCLI_HUB_KEY` set on the environment (see the workflow page); + without it, continue as a normal local run. (Optional) Seed the domain model, pages, and microflows from this prototype: . ```` diff --git a/docs-site/src/tutorial/claude-code-web.md b/docs-site/src/tutorial/claude-code-web.md new file mode 100644 index 000000000..69bc203f6 --- /dev/null +++ b/docs-site/src/tutorial/claude-code-web.md @@ -0,0 +1,171 @@ +# Developing on the Web (Claude Code) + +You can build a Mendix app with mxcli **entirely in the browser** — no local CLI, +no Studio Pro, no machine setup. [Claude Code on the web](https://claude.ai/code) +runs the agent in a cloud container against a GitHub repository; mxcli provisions +the whole toolchain inside that container on first run and commits the result so +future sessions come back up on their own. It works from a laptop, and from an +iPad or phone. + +This page walks the **entire workflow**: + +1. [Create a GitHub repository](#1-create-a-github-repository) +2. [Get a hub key for browser preview](#2-get-a-hub-key-for-browser-preview) (optional) +3. [Create a Claude Code environment](#3-create-a-claude-code-environment) +4. [Start the session with the bootstrap prompt](#4-start-the-session-with-the-bootstrap-prompt) +5. [Iterate — the warm loop, preview, and screenshots](#5-iterate) + +> **New to how Claude Code on the web works** (sessions, environments, network +> policies)? See Anthropic's guide: +> . + +## 1. Create a GitHub repository + +Create a **new, empty** repository on GitHub (private is fine). This is where the +app will live and where every Claude Code session runs. You don't need a template +or any starter files — the bootstrap prompt starts from a truly empty repo and +runs the *current* mxcli, so there is nothing to keep up to date. + +If you already have a Mendix project in a repo, you can skip the bootstrap prompt +and use `mxcli init` instead (step 4 notes how). + +## 2. Get a hub key for browser preview + +*Optional, but recommended — it's what lets you actually see the running app in a +browser tab from a web session.* + +A cloud session has no localhost you can open, so mxcli can reverse-tunnel the +running app out to the **mxcli hub** and give you a public preview URL (see +[External browser preview](../tools/run-local.md)). +The hosted hub at `hub.mxcli.org` is GitHub-authenticated, so each preview is +private to you — which means you need a per-user **hub key**: + +1. Open in any browser and **sign in with GitHub**. +2. Click **Create a hub key** and copy it. +3. Keep it for step 3 — you'll set it as `MXCLI_HUB_KEY`. + +The key is durable (no expiry, survives hub restarts); you set it once and it +stays valid until you revoke it from the same page. If you'd rather run your own +hub, see [`mxcli tunnel-hub`](../tools/run-local.md). + +You can skip this step and add the key later — without it, the app still builds +and runs in the container; you just won't have a browser preview URL. + +## 3. Create a Claude Code environment + +In Claude Code on the web, create an **environment** pointed at the repository +from step 1. The environment is the reusable container definition for your +sessions; configure two things on it: + +**Environment variables** + +| Variable | Value | Why | +|----------|-------|-----| +| `MXCLI_HUB_KEY` | the hub key from step 2 | mxcli reads it automatically so `run --hub` registers previews as you. Survives container reaping. | + +**Network policy** + +The bootstrap prompt downloads a few things on first run, so the environment's +network policy must allow outbound HTTPS to: + +- **`github.com` / `objects.githubusercontent.com`** — the prebuilt `mxcli` binary + from GitHub Releases. +- **Mendix's CDN** — MxBuild and the runtime (`mxcli` fetches the version your app + needs). +- **`hub.mxcli.org`** — the browser-preview tunnel (only if you're using step 2). + +A policy that permits general outbound HTTPS covers all of these. If your +organization restricts egress, allow-list those hosts. See the +[network policy docs](https://code.claude.com/docs/en/claude-code-on-the-web) for +where to configure this. + +> **Even faster startup:** if you control the environment image, pre-installing +> `mxcli` (and pre-caching MxBuild + the runtime) makes step 4 nearly instant and +> removes the one network-dependent step. This is the most robust setup for a team. + +## 4. Start the session with the bootstrap prompt + +Open a session on the environment and paste the **bootstrap prompt**. It tells the +agent to set up the complete toolchain, create the app, wire the AI tooling and +Dev Container, **start a findings document**, boot the app, and commit everything +so the next session self-bootstraps. + +👉 **[Copy the bootstrap prompt](../tools/bootstrap-prompt.md)** — it's a single +paste. In short, the agent will: + +- ensure `mxcli` is available (pre-installed, or download the `nightly` binary); +- `mxcli new App --version ` (or `mxcli init` if an `.mpr` already exists); +- `mxcli init --tool claude` — adds a **SessionStart hook** so future sessions come + back up automatically; +- `mxcli run --local --setup --ensure-db` — cache MxBuild + runtime, start + Postgres, create the app database; +- create a **`FINDINGS.md`** (see below) and start logging to it; +- **commit** `App.mpr`, `.devcontainer/`, and `.claude/` so the steady state is + file-driven; +- boot `mxcli run --local` and verify the app answers HTTP 200. + +You can append your own goal to the end of the prompt — e.g. *"Then seed a domain +model, pages, and microflows for a time-registration app"*, or link a design +prototype to seed from. + +### Keep a findings document + +The bootstrap prompt asks the agent to create a **`FINDINGS.md`** at the repo root +and to append to it as it works. Treat it as a lab notebook for the session: + +- anything **surprising or broken** — an mxcli command that errored, a workaround + you had to apply, a check that passed but a real `mx check` later flagged; +- the **Mendix / mxcli versions** in play and how each finding was verified; +- decisions and dead-ends, so a later session (or a teammate) doesn't repeat them. + +This is high-leverage for two reasons. It gives *your next session* durable +context that survives container reaping, and — because mxcli is fast-moving alpha +— a clear findings doc is the single most useful thing you can share back to +improve mxcli. If something misbehaves, capture the exact MDL, the command, and +the error in `FINDINGS.md`; that's often enough for a fix. + +## 5. Iterate + +Once the app is up, use the **warm local dev loop** — a Docker-free, ~1-second +edit→test cycle: + +```bash +mxcli run --local -p App.mpr --watch --screenshot # hot-reload + a PNG per change +``` + +Edit the model with MDL and the running loop hot-applies it: + +```bash +mxcli exec change.mdl -p App.mpr # the loop picks it up and reloads +``` + +To **see the app in a browser** from the web session, add `--hub` (this is where +the key from step 2 pays off): + +```bash +mxcli run --hub https://hub.mxcli.org -p App.mpr # prints a shareable preview URL +``` + +`--hub` implies `--local`, so you get the warm loop *and* a public preview URL in +one command — edit here, hot-apply, refresh the tab. + +See [mxcli run --local](../tools/run-local.md) for `--watch`, `--ensure-db`, +`--setup`, the screenshot flags, and the full `--hub` reference. + +## After idle: sessions self-bootstrap + +Cloud containers are reaped when idle, so a resumed session starts from the +committed files, not from re-pasting the prompt. Because step 4 ran +`mxcli init`, `.claude/settings.json` carries a **SessionStart hook** that runs +`mxcli run --local --setup --ensure-db` on every new session — it re-caches +MxBuild + runtime and re-provisions the database, leaving the session ready to +`run --local`. You never re-paste the bootstrap prompt; it's a one-time seed. + +## Next steps + +- [The bootstrap prompt](../tools/bootstrap-prompt.md) — the exact copy-paste text + and the rules that make it robust. +- [Local Dev Loop](../tools/run-local.md) — the warm loop, browser preview, and + screenshots in depth. +- [Claude Code Integration](claude-code.md) — how skills, `CLAUDE.md`, and slash + commands shape what the agent does with your project (applies to web and local).