From 70d589fb249818e95f1143f289b106a18a1514bc Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 13:00:50 +0000 Subject: [PATCH 01/17] ci: run the no-compile checks first and gate every job on them An unformatted Go file failed Lint Go ~24 min into build-and-test, while every integration leg ran regardless: ~100 runner-minutes for a known-red result, and at the 20-concurrent-job cap it delayed other PRs too. A quick-checks job now runs fmt-check, check-findings, check-wiki-pages, check-skill-index and check-test-timeouts on a bare checkout (no parser, no submodules, no Go cache; each takes about a second, verified on a fresh worktree, with an unformatted-file control). Every other job needs it. go vet and check-tunnel-deps load the package graph, so they stay in build-and-test, with vet moved ahead of Test. integration-passed still fails when quick-checks makes the integration legs skip. ci-local follows the same order and set; lint-go is replaced by its two halves, fmt-check and vet. Refs #1887 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82simPxcdrRu9PUSPcQ --- .github/workflows/push-test.yml | 83 ++++++++++++++++++++++++--------- Makefile | 11 +++-- 2 files changed, 66 insertions(+), 28 deletions(-) diff --git a/.github/workflows/push-test.yml b/.github/workflows/push-test.yml index 6a8c42e8e..b563b4839 100644 --- a/.github/workflows/push-test.yml +++ b/.github/workflows/push-test.yml @@ -29,6 +29,51 @@ permissions: contents: read jobs: + # Checks that need no generated parser and no compile, run first and gating + # every other job (ako/mxcli#1887). An unformatted file used to fail ~24 min + # into build-and-test while all the integration legs ran anyway, ~100 + # runner-minutes for a known-red result, and at the 20-concurrent-job cap that + # also delayed every other PR. Each check takes about a second; the job's cost + # is runner start-up, so it saves no setup it does not need: no submodules, no + # Go module or build cache (gofmt reads source only), no ANTLR. + # + # Only checks proven to run on a bare checkout belong here: `go vet` and + # check-tunnel-deps load the package graph, which includes the generated + # parser, so they stay in build-and-test. + quick-checks: + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@v7 + - uses: actions/setup-go@v7 + with: + # The toolchain matters: gofmt's output differs between Go releases + # (the mismatch fixed in #1759), so this must match the other jobs. + go-version: '1.27.3' + cache: false + - name: Check gofmt + run: make fmt-check + - name: Check bug findings + # One JSON object per line, an area, a date, and either the four + # structured fields or a raw row. DuckDB rejects a whole file on one bad + # line, so a typo in an appended finding takes out every query over that + # area. Also prints how far docs-wiki/bug-patterns/ has fallen behind. + run: make check-findings + - name: Check wiki page list + # The table of contents must describe the wiki, both directions. It + # drifted for three months before anyone noticed. + run: make check-wiki-pages + - name: Check the skill section indexes + # Each SKILL.md opens with a generated line-numbered index so an agent + # can read one section instead of the file — measured on BENCH-001 as + # ~68% of a session's re-read cost. A hand-kept table of contents is + # the drift this repo has paid for twice, so the generator owns it. + run: make check-skill-index + - name: Check every go test sets -timeout + # Without one Go applies 10m per test binary, and a package that passes + # in seconds alone can exceed it under `./...` load (mendixlabs/mxcli#1291). + run: make check-test-timeouts + # The tunnel seam has a !linux half (stub + its tests) that the ubuntu job can # only compile, never run. This job actually executes it on real Windows and # macOS runners, so "--hub fails with an actionable message" is a tested claim @@ -37,6 +82,7 @@ jobs: # Neither package depends on the generated ANTLR parser, so this needs no # grammar step and stays fast. tunnel-seam-cross-platform: + needs: quick-checks strategy: fail-fast: false matrix: @@ -96,6 +142,7 @@ jobs: # blocked. This job runs the Windows-only regression tests on a real Windows # runner so neither half can come back. windows-process-regression: + needs: quick-checks runs-on: windows-latest steps: - uses: actions/checkout@v7 @@ -136,6 +183,7 @@ jobs: fi build-and-test: + needs: quick-checks runs-on: ubuntu-latest steps: - uses: actions/checkout@v7 @@ -159,6 +207,10 @@ jobs: ANTLR4_TOOLS_ANTLR_VERSION: '4.13.2' - name: Build run: make build + - name: Vet + # Before Test, which takes ~12 min. gofmt runs in quick-checks; vet + # needs the generated parser, so it is the first thing after the build. + run: make vet - name: Test run: make test - name: Check tunnel stays Linux-only @@ -180,26 +232,8 @@ jobs: # #574 case: the widget must attach the session's CSRF token to # same-origin fetches and never to any other host. run: make check-skill-pack-js - - name: Check bug findings - # One JSON object per line, an area, a date, and either the four - # structured fields or a raw row. DuckDB rejects a whole file on one bad - # line, so a typo in an appended finding takes out every query over that - # area. Also prints how far docs-wiki/bug-patterns/ has fallen behind. - run: make check-findings - - name: Check wiki page list - # The table of contents must describe the wiki, both directions. It - # drifted for three months before anyone noticed. - run: make check-wiki-pages - - name: Check the skill section indexes - # Each SKILL.md opens with a generated line-numbered index so an agent - # can read one section instead of the file — measured on BENCH-001 as - # ~68% of a session's re-read cost. A hand-kept table of contents is - # the drift this repo has paid for twice, so the generator owns it. - run: make check-skill-index - - name: Check every go test sets -timeout - # Without one Go applies 10m per test binary, and a package that passes - # in seconds alone can exceed it under `./...` load (mendixlabs/mxcli#1291). - run: make check-test-timeouts + # check-findings, check-wiki-pages, check-skill-index and + # check-test-timeouts run in quick-checks. - name: Check skill MDL blocks run: ./scripts/check-skill-mdl.sh ./bin/mxcli .claude/skills/mendix - name: Check docs-site MDL blocks @@ -220,9 +254,8 @@ jobs: # The tables on docs-site/src/language/versions.md are generated from # the language-change and deprecation registries (ako/mxcli#714). run: make check-migration-reference - # The integration tests are the `integration` job below. - - name: Lint Go - run: make lint-go + # The integration tests are the `integration` job below. Lint Go was here: + # its fmt-check is in quick-checks and its vet runs before Test. - name: Vulnerability scan run: | go install golang.org/x/vuln/cmd/govulncheck@latest @@ -250,6 +283,7 @@ jobs: # tag — `make test` in build-and-test runs those. integration: name: integration (${{ matrix.suite }}${{ matrix.shard && format(' {0}', matrix.shard) || '' }}) + needs: quick-checks runs-on: ubuntu-latest # Each suite's go test has its own -timeout (Makefile); this is the job's # backstop, with room for the setup steps. @@ -318,6 +352,9 @@ jobs: # `concurrency` above) could not end until this job got a runner — and the # concurrency group held the replacement run pending meanwhile: ~5 min on # PR #1885 (run 41406249166), at the 20-job cap (ako/mxcli#1886). + # + # When quick-checks fails, the integration legs are skipped, and this gate + # then fails on `integration: skipped` — red, never a vacuous pass. integration-passed: if: ${{ !cancelled() }} needs: integration diff --git a/Makefile b/Makefile index f64a174fa..b97f51bbe 100644 --- a/Makefile +++ b/Makefile @@ -454,8 +454,9 @@ check-widget-versions: build # Lint all code (Go + TypeScript) lint: lint-go lint-ts check-conformance check-test-timeouts -# What the build-and-test job in .github/workflows/push-test.yml runs, in its -# order, so a push is proven against the gate it will meet rather than against +# What the quick-checks and build-and-test jobs in .github/workflows/push-test.yml +# run, in their order (the no-compile checks first, then vet before test -- +# ako/mxcli#1887), so a push is proven against the gate it will meet rather than against # `build test lint`, which covers less of it: check-mdl, check-findings, # check-skill-index and the rest run only in CI unless someone remembers them. # check-skill-mdl is a superset of the job's three skill/docs MDL steps. @@ -463,9 +464,9 @@ lint: lint-go lint-ts check-conformance check-test-timeouts # Left out: govulncheck (the job installs it @latest, over the network) and the # integration jobs, which need mxbuild -- `make test-integration` runs those. # A step added to the job belongs here too. -ci-local: build test check-tunnel-deps check-mdl check-skill-pack-js check-findings \ - check-wiki-pages check-skill-index check-test-timeouts check-skill-mdl \ - check-conformance check-migration-reference lint-go +ci-local: fmt-check check-findings check-wiki-pages check-skill-index check-test-timeouts \ + build vet test check-tunnel-deps check-mdl check-skill-pack-js check-skill-mdl \ + check-conformance check-migration-reference @echo "ci-local passed (build-and-test, without govulncheck)" # Lint Go code From 5655b9ff25152fdc33d5dffcb169968757bb2f24 Mon Sep 17 00:00:00 2001 From: Ako Date: Mon, 2 Nov 2026 13:03:36 +0000 Subject: [PATCH 02/17] fix(describe): print the concurrency error message in the default language (#1869) describableMessage chose the alphabetically first language, so on an en_US project with de_DE enabled describe printed the German text, and a drop + create from that output stored it under en_US. It now uses the rule every other described text follows (#702): the project default, then en_US, then the first non-empty one. pickTextTranslation is split into pickTextLanguage so the describer can name the languages it did not print. Co-Authored-By: Claude Opus 5.5 --- ...ow-concurrency-message-wrong-language.json | 1 + CHANGELOG.md | 1 + mdl/executor/cmd_microflows_show.go | 26 ++++++++------ mdl/executor/describe_language.go | 26 +++++++++----- .../microflow_carried_properties_test.go | 36 +++++++++++++++++++ 5 files changed, 72 insertions(+), 18 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-11-02-describe-microflow-concurrency-message-wrong-language.json diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-11-02-describe-microflow-concurrency-message-wrong-language.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-11-02-describe-microflow-concurrency-message-wrong-language.json new file mode 100644 index 000000000..5fc018d11 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-11-02-describe-microflow-concurrency-message-wrong-language.json @@ -0,0 +1 @@ +{"area": "mdl/executor", "date": "2026-11-02", "symptom": "On an en_US project with de_DE also enabled, describe microflow prints the concurrency error message in German ('-- also translated into en_US'); drop + create from that output stores the German text under en_US, so the English message is gone. check, exec and mx check are clean; a show message text in the same flow describes and rebuilds correctly", "cause": "describableMessage (cmd_microflows_show.go) chose the alphabetically first language (de_DE < en_US) instead of the project-default rule every other described text uses (pickTextTranslation, #702)", "file": "mdl/executor/cmd_microflows_show.go, describe_language.go", "insight": "A describer that picks its own language for one property is a write on the round trip, the same class as #705: exec stores what describe prints in the project default. The existing test passed only because en_US < nl_NL sorts the default first; a language test needs a pair where the default does NOT sort first, plus a control with the default flipped. Grep for any other sort.Strings over Translations keys in a describer", "refs": ["ako/mxcli#1869", "mendixlabs/mxcli#1711", "#702"]} diff --git a/CHANGELOG.md b/CHANGELOG.md index 79de42570..a2464b0fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **`describe microflow` prints the concurrency error message in the project's default language** (ako/mxcli#1869, mendixlabs/mxcli#1711) — on an `en_US` project with `de_DE` enabled it printed the German text, and a drop and create from that output stored it under `en_US` too, so the English message was gone while `check`, `exec` and `mx check` stayed clean. It now picks the language the way every other described text does: the project default, then `en_US`, then the first non-empty one. - **The catalog's XPath rows name their widget** (ako/mxcli#1879) — `xpath_expressions.ComponentId` for a page or snippet data source was a Go byte dump (`{0 [57 221 …]}`) and nothing tied the row to its widget. It is now a GUID, and the new `WidgetId` / `WidgetName` columns (`widget_id` / `widget_name` in `xpath_expressions()`) name the widget by the id `widgets` gives it. The catalog schema moves to version 32, so a cached catalog is rebuilt once. - **`create or replace page` can clear a data grid column caption** (ako/mxcli#1877) — `Caption: ''` over a stored caption reported "Unchanged page" and kept it, because the translation carry added the stored text back to an empty header written with no translation. The header is now written as one empty translation, which the carry leaves alone; an empty translation where the stored text has none is dropped, so a Studio Pro empty caption still rewrites as unchanged. This also repairs pages that still carry a leaked column name (#535). - **A custom-content data grid column with an empty caption no longer shows its MDL name as the header** (ako/mxcli#535) — through the generic `pluggablewidget 'com.mendix.widget.web.datagrid.Datagrid'` form, `column colTriage (Caption: '') { … }` (or no caption) wrote `colTriage` as the header text, which end users saw; Mendix stores no name for a column. It now writes an empty header, as the keyword `datagrid` form already did; it builds clean (11.15.0) and Data grid 2 renders it blank. diff --git a/mdl/executor/cmd_microflows_show.go b/mdl/executor/cmd_microflows_show.go index a254fd3cb..799534a14 100644 --- a/mdl/executor/cmd_microflows_show.go +++ b/mdl/executor/cmd_microflows_show.go @@ -327,7 +327,7 @@ func describeMicroflowMode(ctx *ExecContext, name ast.QualifiedName, opts descri // them while `diff-local` printed all three. That is duplicate-resolver // drift in one file; every header property added from here on has to go // through a shared helper, or the next one diverges the same way. - lines = append(lines, microflowDocumentPropertyLines(targetMf)...) + lines = append(lines, microflowDocumentPropertyLines(ctx, targetMf)...) // BEGIN block lines = append(lines, "begin") @@ -685,7 +685,7 @@ func renderMicroflowMDL( lines = append(lines, exposeClauseLines(mf)...) if flowType == "microflow" { - lines = append(lines, microflowDocumentPropertyLines(mf)...) + lines = append(lines, microflowDocumentPropertyLines(ctx, mf)...) } else if flowType == "nanoflow" && mf.ExportLevel == types.ExportLevelAPI { // A nanoflow has an export level and nothing else of the three: the // wrapped Microflow's zero AllowConcurrentExecution would otherwise @@ -1710,7 +1710,7 @@ func microflowBodyWarnings( // gave: every document in every marketplace module measured stores "Hidden" and // allows concurrent execution, so emitting them unconditionally would add three // lines to every describe in order to say nothing. -func microflowDocumentPropertyLines(mf *microflows.Microflow) []string { +func microflowDocumentPropertyLines(ctx *ExecContext, mf *microflows.Microflow) []string { var lines []string if mf.URL != "" { lines = append(lines, fmt.Sprintf("url '%s'", escapeMDLString(mf.URL))) @@ -1739,7 +1739,7 @@ func microflowDocumentPropertyLines(mf *microflows.Microflow) []string { // dropping it would describe a microflow that fails CE4899 — and // flag the languages a replay would not carry, the same honesty rule // DESCRIBE applies to a range bounded by another attribute. - text, langs := describableMessage(mf.ConcurrencyErrorMessage) + text, langs := describableMessage(mf.ConcurrencyErrorMessage, describeDefaultLanguage(ctx)) line += fmt.Sprintf(" error message '%s'", escapeMDLString(text)) if len(langs) > 0 { line += fmt.Sprintf(" -- also translated into %s; replaying this line keeps only the one shown", @@ -1753,16 +1753,22 @@ func microflowDocumentPropertyLines(mf *microflows.Microflow) []string { // describableMessage returns the text DESCRIBE prints and the other languages // the stored message carries, sorted. -func describableMessage(t *model.Text) (string, []string) { +// +// The text is chosen by pickTextLanguage, the rule every other described +// text follows (#702): the project's default language, else en_US, else the +// first non-empty one. Taking the alphabetically first language printed de_DE +// on an en_US project, and a rebuild from that line stored the German text under +// en_US (#1869). +func describableMessage(t *model.Text, defaultLang string) (string, []string) { + shown, _ := pickTextLanguage(t, defaultLang) var langs []string for lang := range t.Translations { - langs = append(langs, lang) + if lang != shown { + langs = append(langs, lang) + } } sort.Strings(langs) - if len(langs) == 0 { - return "", nil - } - return t.Translations[langs[0]], langs[1:] + return t.Translations[shown], langs } // allDeclared reports whether every id is a crossed merge the description diff --git a/mdl/executor/describe_language.go b/mdl/executor/describe_language.go index 2cf48407c..4101f02b6 100644 --- a/mdl/executor/describe_language.go +++ b/mdl/executor/describe_language.go @@ -70,16 +70,26 @@ func selectTranslationText(items []any, preferredLang string) string { // page Title, enumeration value Caption). // As there, a preferred language that is present but empty wins. func pickTextTranslation(t *model.Text, preferredLang string) string { + if lang, ok := pickTextLanguage(t, preferredLang); ok { + return t.Translations[lang] + } + return "" +} + +// pickTextLanguage is pickTextTranslation's choice as a language code, for a +// caller that must also name the languages it did not print. ok is false when +// no translation qualifies. +func pickTextLanguage(t *model.Text, preferredLang string) (string, bool) { if t == nil || len(t.Translations) == 0 { - return "" + return "", false } if preferredLang != "" { - if v, ok := t.Translations[preferredLang]; ok { - return v + if _, ok := t.Translations[preferredLang]; ok { + return preferredLang, true } } - if v := t.Translations[fallbackLanguageCode]; v != "" { - return v + if t.Translations[fallbackLanguageCode] != "" { + return fallbackLanguageCode, true } // Last resort: any stored translation beats reporting a caption Studio Pro // plainly shows as empty. Pick it by sorted language code — iterating the map @@ -91,11 +101,11 @@ func pickTextTranslation(t *model.Text, preferredLang string) string { } sort.Strings(langs) for _, lang := range langs { - if v := t.Translations[lang]; v != "" { - return v + if t.Translations[lang] != "" { + return lang, true } } - return "" + return "", false } // authoringLanguage is the language a bare MDL string (Caption:, Title:, diff --git a/mdl/executor/microflow_carried_properties_test.go b/mdl/executor/microflow_carried_properties_test.go index 7885a4948..b4439a07a 100644 --- a/mdl/executor/microflow_carried_properties_test.go +++ b/mdl/executor/microflow_carried_properties_test.go @@ -192,3 +192,39 @@ func TestDescribeMicroflow_FlagsUntranslatableMessage(t *testing.T) { t.Errorf("describe did not flag the language a replay would drop:\n%s", got) } } + +// TestDescribeMicroflow_MessageInDefaultLanguage: the concurrency message is +// described in the PROJECT's default language, as every other text is (#702). +// Picking the alphabetically first one printed de_DE on an en_US project, and a +// drop + create from that output wrote the German text under en_US, so the +// English message was gone with nothing reporting it (#1869). +// +// Both projects store the same two languages; only the default differs. The +// second case is the control: a describer that always prints en_US passes the +// first and fails it. +func TestDescribeMicroflow_MessageInDefaultLanguage(t *testing.T) { + for _, tc := range []struct { + defaultLang, want, other string + }{ + {"en_US", "Another save is running.", "de_DE"}, + {"de_DE", "Ein anderes Speichern läuft.", "en_US"}, + } { + t.Run(tc.defaultLang, func(t *testing.T) { + got := renderMicroflowMDL(ctxWithDefaultLanguage(tc.defaultLang), "microflow", µflows.Microflow{ + Name: "SaveOrder", + AllowConcurrentExecution: false, + ConcurrencyErrorMessage: &model.Text{Translations: map[string]string{ + "en_US": "Another save is running.", + "de_DE": "Ein anderes Speichern läuft.", + }}, + }, ast.QualifiedName{Module: "MyModule", Name: "SaveOrder"}, nil, nil, nil) + + if !strings.Contains(got, "error message '"+tc.want+"'") { + t.Errorf("describe did not print the %s message %q:\n%s", tc.defaultLang, tc.want, got) + } + if !strings.Contains(got, "also translated into "+tc.other) { + t.Errorf("describe did not flag %s as the other language:\n%s", tc.other, got) + } + }) + } +} From da719de73cc9fced013af303a21df33fc9acaa7f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 13:21:19 +0000 Subject: [PATCH 03/17] TEMP CONTROL: add an unformatted Go file (to be reverted) Proves on a live run that quick-checks fails fast and no other job starts (ako/mxcli#1887). Reverted by the next commit. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82simPxcdrRu9PUSPcQ --- cmd/mxcli/zz_fmt_control.go | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 cmd/mxcli/zz_fmt_control.go diff --git a/cmd/mxcli/zz_fmt_control.go b/cmd/mxcli/zz_fmt_control.go new file mode 100644 index 000000000..63ad9e006 --- /dev/null +++ b/cmd/mxcli/zz_fmt_control.go @@ -0,0 +1,5 @@ +package main + +// CONTROL for ako/mxcli#1887, reverted in the next commit: deliberately not +// gofmt-clean, so quick-checks must fail and gate every other job. +func zzFmtControl( ) int { return 1 } From 40ee1ecbcd70563875d64eae1873da9926005f63 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 13:24:36 +0000 Subject: [PATCH 04/17] Revert the TEMP CONTROL unformatted file The control did its job on run 41452346684: quick-checks failed at gofmt 23 s in, every other job was skipped without a runner, and integration-passed went red (ako/mxcli#1887). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82simPxcdrRu9PUSPcQ --- cmd/mxcli/zz_fmt_control.go | 5 ----- 1 file changed, 5 deletions(-) delete mode 100644 cmd/mxcli/zz_fmt_control.go diff --git a/cmd/mxcli/zz_fmt_control.go b/cmd/mxcli/zz_fmt_control.go deleted file mode 100644 index 63ad9e006..000000000 --- a/cmd/mxcli/zz_fmt_control.go +++ /dev/null @@ -1,5 +0,0 @@ -package main - -// CONTROL for ako/mxcli#1887, reverted in the next commit: deliberately not -// gofmt-clean, so quick-checks must fail and gate every other job. -func zzFmtControl( ) int { return 1 } From 8a7d181deb7c0ddb9247651af340af401bf093e7 Mon Sep 17 00:00:00 2001 From: Ako Date: Mon, 2 Nov 2026 15:27:15 +0000 Subject: [PATCH 05/17] fix(test): no MDL-V1-SLASH / MDL-DEPR001 per test on a headerless suite (#1855) `mxcli test` on an mdl 0 `.test.mdl` printed one MDL-V1-SLASH and one MDL-DEPR001 per test, about the script the runner generated rather than the author's file: the per-test generators kept `CREATE OR REPLACE MICROFLOW` and a `/` after every `END;` under mdl 0, and mdl 0 warns on both. The canonical spelling is valid under every version, so the generators now always write `CREATE OR MODIFY` and `END;` alone; only the header follows the suite. Closes #1855 Co-Authored-By: Claude Opus 5.5 --- ...headerless-suite-warns-slash-per-test.json | 1 + cmd/mxcli/testrunner/endpoint.go | 4 +-- cmd/mxcli/testrunner/generator.go | 34 ++++++++----------- cmd/mxcli/testrunner/generator_endpoint.go | 6 ++-- .../testrunner/generator_endpoint_test.go | 8 ++--- cmd/mxcli/testrunner/language_header_test.go | 32 +++++++++++++---- cmd/mxcli/testrunner/parser_test.go | 4 +-- 7 files changed, 51 insertions(+), 38 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/cmd-mxcli/2026-11-02-test-headerless-suite-warns-slash-per-test.json diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli/2026-11-02-test-headerless-suite-warns-slash-per-test.json b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-11-02-test-headerless-suite-warns-slash-per-test.json new file mode 100644 index 000000000..e451eedd7 --- /dev/null +++ b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-11-02-test-headerless-suite-warns-slash-per-test.json @@ -0,0 +1 @@ +{"area": "cmd/mxcli", "date": "2026-11-02", "symptom": "`mxcli test` on a headerless (mdl 0) `.test.mdl` printed one MDL-V1-SLASH and one MDL-DEPR001 warning per test, before any result — 104 warnings for a 52-test suite — although the `/` separators are what the test format requires. A suite with `mdl 1;` printed none, and `mxcli check` of the test file printed none either", "cause": "The warnings were about the script the runner generated, not the author's file. `GenerateTestFlows` and `GenerateTestRunner` kept the mdl 0 spellings — `CREATE OR REPLACE MICROFLOW` and a `/` line after every `END;` — on the theory that a headerless suite should get the script it always had; but mdl 0 warns on both, and the injecting `mxcli exec` printed those warnings verbatim. #943 had fixed the endpoint script and #847 the check rendering, both by writing the canonical form; the per-test generators were the last caller still switching on the version", "file": "`cmd/mxcli/testrunner/generator.go` (`createFlow` is now a constant `CREATE OR MODIFY MICROFLOW`; `writeFlowEnd` writes `END;` alone under every version)", "insight": "The canonical spelling is valid under mdl 0 as well, so the generators never needed a version switch for it — only the header follows the suite. Test generated scripts through `visitor.Build` and assert `LanguageNotes` and `Deprecations` are empty, with bodies valid under both versions so any note is the generator's; the previous test (`TestGeneratorsHeaderlessSuiteUnchanged`) only asserted the script parsed, which the warning-laden one did. Runtime control on an 11.12.2 app, same headerless file: main 2 MDL-V1-SLASH + 2 MDL-DEPR001, fix 0, both runs 2/2 PASS", "refs": ["ako/mxcli#1855", "mendixlabs/mxcli#1697", "ako/mxcli#943"], "rules": ["MDL-V1-SLASH", "MDL-DEPR001"], "ce": []} diff --git a/cmd/mxcli/testrunner/endpoint.go b/cmd/mxcli/testrunner/endpoint.go index cf1acea01..de63a6cca 100644 --- a/cmd/mxcli/testrunner/endpoint.go +++ b/cmd/mxcli/testrunner/endpoint.go @@ -101,7 +101,7 @@ func GenerateEndpointMDL(chainAfterStartup string) string { b.WriteString("/** Registers the mxcli test endpoint at boot. Runs no tests. */\n") b.WriteString("@applyentityaccess\n") - b.WriteString(createFlow(langver.V1) + " " + endpointStartupFlow + " ()\n") + b.WriteString(createFlow + " " + endpointStartupFlow + " ()\n") b.WriteString("RETURNS Boolean AS $Registered\n") b.WriteString("BEGIN\n") b.WriteString(" $Registered = CALL JAVA ACTION " + endpointRegisterAction + "();\n") @@ -112,7 +112,7 @@ func GenerateEndpointMDL(chainAfterStartup string) string { b.WriteString(" $Chained = CALL MICROFLOW " + chainAfterStartup + "();\n") } b.WriteString(" RETURN $Registered;\n") - writeFlowEnd(&b, langver.V1) + writeFlowEnd(&b) return b.String() } diff --git a/cmd/mxcli/testrunner/generator.go b/cmd/mxcli/testrunner/generator.go index 5e2e8fa40..bae60d8d9 100644 --- a/cmd/mxcli/testrunner/generator.go +++ b/cmd/mxcli/testrunner/generator.go @@ -32,17 +32,12 @@ func writeScriptHeader(b *strings.Builder, v langver.Version) { } } -// createFlow is the statement that (re)defines a generated microflow. Under -// mdl 0 it stays `CREATE OR REPLACE`, the spelling the generators have always -// written; mdl 1 spells the same thing `CREATE OR MODIFY` (`or replace` is -// the deprecated alias MDL-DEPR001 there), and a generated script should not -// warn about a spelling its author never wrote. -func createFlow(v langver.Version) string { - if v >= langver.V1 { - return "CREATE OR MODIFY MICROFLOW" - } - return "CREATE OR REPLACE MICROFLOW" -} +// createFlow is the statement that (re)defines a generated microflow: +// `CREATE OR MODIFY` under every version. Under mdl 1 `or replace` is the +// deprecated alias (MDL-DEPR001), and mdl 0 reads both spellings the same way +// but warns on the old one too — so writing it made `mxcli test` print a +// warning per test about a script the author never wrote (ako/mxcli#1855). +const createFlow = "CREATE OR MODIFY MICROFLOW" // writeBodyLines writes a test body's lines into a generated flow, indented — // except a line that starts inside a string literal, whose leading whitespace @@ -95,14 +90,13 @@ func scanLiteralState(line string, inString, inComment bool, v langver.Version) return inString, inComment } -// writeFlowEnd closes a generated microflow. Under mdl 0 that is `END;` and -// the `/` line the generators have always written; mdl 1 refuses the `/` -// (MDL-V1-SLASH), and `;` alone ends the statement. -func writeFlowEnd(b *strings.Builder, v langver.Version) { +// writeFlowEnd closes a generated microflow with `END;` alone. `;` ends the +// statement under every version; a `/` after it is refused under mdl 1 and drew +// an MDL-V1-SLASH warning under mdl 0, once per generated microflow — the test +// format's own separator, reported back to its author as if they had written +// it as a terminator (ako/mxcli#1855). +func writeFlowEnd(b *strings.Builder) { b.WriteString("END;\n") - if v < langver.V1 { - b.WriteString("/\n") - } } // GenerateTestRunner generates the MDL for a TestRunner microflow from parsed test cases. @@ -118,7 +112,7 @@ func GenerateTestRunner(suite *TestSuite) string { // Module creation (idempotent — only if MxTest module doesn't exist) b.WriteString("CREATE MODULE MxTest;\n\n") - b.WriteString(createFlow(v) + " MxTest.TestRunner ()\n") + b.WriteString(createFlow + " MxTest.TestRunner ()\n") b.WriteString("RETURNS Boolean AS $AllPassed\n") b.WriteString("BEGIN\n") b.WriteString(" DECLARE $AllPassed Boolean = true;\n") @@ -140,7 +134,7 @@ func GenerateTestRunner(suite *TestSuite) string { b.WriteString(fmt.Sprintf(" LOG INFO NODE 'MXTEST' 'MXTEST:END:%s';\n", escapeMDLString(suite.Name))) b.WriteString(" RETURN $AllPassed;\n") - writeFlowEnd(&b, v) + writeFlowEnd(&b) return b.String() } diff --git a/cmd/mxcli/testrunner/generator_endpoint.go b/cmd/mxcli/testrunner/generator_endpoint.go index f7dc93bcb..0f9d35306 100644 --- a/cmd/mxcli/testrunner/generator_endpoint.go +++ b/cmd/mxcli/testrunner/generator_endpoint.go @@ -60,7 +60,7 @@ func GenerateTestFlows(suite *TestSuite) string { // writeTestFlow writes one test's microflow. func writeTestFlow(b *strings.Builder, tc TestCase, v langver.Version) { fmt.Fprintf(b, "/** %s */\n", escapeMDLComment(tc.Name)) - fmt.Fprintf(b, "%s %s ()\n", createFlow(v), testFlowName(tc)) + fmt.Fprintf(b, "%s %s ()\n", createFlow, testFlowName(tc)) b.WriteString("RETURNS String AS $Verdict\n") b.WriteString("BEGIN\n") fmt.Fprintf(b, " DECLARE $Verdict String = '%s';\n", verdictPass) @@ -76,7 +76,7 @@ func writeTestFlow(b *strings.Builder, tc TestCase, v langver.Version) { } b.WriteString(" RETURN $Verdict;\n") - writeFlowEnd(b, v) + writeFlowEnd(b) } // writeSetupCalls writes the @setup microflow calls that precede a test's body. @@ -370,7 +370,7 @@ func attachOnError(lines []string, v langver.Version, handler []string) []string func bodyParsePrefix(v langver.Version) string { var wrap strings.Builder writeScriptHeader(&wrap, v) - wrap.WriteString(createFlow(v) + " " + mxTestModule + ".Body ()\nBEGIN\n") + wrap.WriteString(createFlow + " " + mxTestModule + ".Body ()\nBEGIN\n") return wrap.String() } diff --git a/cmd/mxcli/testrunner/generator_endpoint_test.go b/cmd/mxcli/testrunner/generator_endpoint_test.go index 852186471..fce3ee206 100644 --- a/cmd/mxcli/testrunner/generator_endpoint_test.go +++ b/cmd/mxcli/testrunner/generator_endpoint_test.go @@ -18,14 +18,14 @@ func TestGenerateTestFlowsOneMicroflowPerTest(t *testing.T) { mdl := GenerateTestFlows(suite) for _, want := range []string{ - "CREATE OR REPLACE MICROFLOW MxTest.Test_test_1 ()", - "CREATE OR REPLACE MICROFLOW MxTest.Test_test_2 ()", + "CREATE OR MODIFY MICROFLOW MxTest.Test_test_1 ()", + "CREATE OR MODIFY MICROFLOW MxTest.Test_test_2 ()", } { if !strings.Contains(mdl, want) { t.Errorf("generated MDL is missing %q", want) } } - if n := strings.Count(mdl, "CREATE OR REPLACE MICROFLOW"); n != 2 { + if n := strings.Count(mdl, "CREATE OR MODIFY MICROFLOW"); n != 2 { t.Errorf("got %d microflows, want one per test (2)", n) } } @@ -168,7 +168,7 @@ func TestGenerateTestFlowsEscapesNameInComment(t *testing.T) { }}} mdl := GenerateTestFlows(suite) - head := mdl[:strings.Index(mdl, "CREATE OR REPLACE MICROFLOW")] + head := mdl[:strings.Index(mdl, "CREATE OR MODIFY MICROFLOW")] if strings.Count(head, "*/") != 1 { t.Errorf("the test name closed the javadoc block early:\n%s", head) } diff --git a/cmd/mxcli/testrunner/language_header_test.go b/cmd/mxcli/testrunner/language_header_test.go index abb733582..de824b42b 100644 --- a/cmd/mxcli/testrunner/language_header_test.go +++ b/cmd/mxcli/testrunner/language_header_test.go @@ -169,20 +169,38 @@ func TestGeneratorsHonourTheHeader(t *testing.T) { } } -// A headerless suite's scripts are unchanged: no header, and the `/` lines -// mdl 0 has always had. -func TestGeneratorsHeaderlessSuiteUnchanged(t *testing.T) { +// A headerless suite's scripts get no header, and draw nothing about text the +// runner wrote itself. They used to end every microflow with a `/` line and +// open it with `create or replace`, so `mxcli test` printed an MDL-V1-SLASH and +// an MDL-DEPR001 per test — 104 warnings for a 52-test suite — about a script +// the author never saw (ako/mxcli#1855). The bodies here are valid under both +// versions, so any note is the generator's. +func TestGeneratorsHeaderlessSuiteDrawsNoNotes(t *testing.T) { tests, err := parseMDLTests(strings.TrimPrefix(headeredTestFile, "mdl 1;\n"), "h.test.mdl") if err != nil { t.Fatal(err) } suite := &TestSuite{Name: "h", Tests: tests} - for _, script := range []string{GenerateTestRunner(suite), GenerateTestFlows(suite)} { + for name, script := range map[string]string{ + "GenerateTestRunner": GenerateTestRunner(suite), + "GenerateTestFlows": GenerateTestFlows(suite), + } { if strings.HasPrefix(script, "mdl ") { - t.Errorf("headerless suite got a header:\n%s", script) + t.Errorf("%s: headerless suite got a header:\n%s", name, script) + } + prog, errs := visitor.Build(script) + if len(errs) > 0 { + t.Errorf("%s output does not parse: %v\n%s", name, errs, script) + continue } - if _, errs := visitor.Build(script); len(errs) > 0 { - t.Errorf("output does not parse: %v", errs) + if prog.LanguageVersion != langver.V0 { + t.Errorf("%s output parses as mdl %d, want mdl 0", name, prog.LanguageVersion) + } + for _, n := range prog.LanguageNotes { + t.Errorf("%s draws %s at line %d: %s", name, n.Code, n.Line, n.Message) + } + for _, d := range prog.Deprecations { + t.Errorf("%s writes the deprecated spelling %s at line %d", name, d.Code, d.Line) } } } diff --git a/cmd/mxcli/testrunner/parser_test.go b/cmd/mxcli/testrunner/parser_test.go index 881cc14a9..07f067ebd 100644 --- a/cmd/mxcli/testrunner/parser_test.go +++ b/cmd/mxcli/testrunner/parser_test.go @@ -172,8 +172,8 @@ func TestGenerateTestRunner(t *testing.T) { mdl := GenerateTestRunner(suite) // Check that it contains key patterns - if !contains(mdl, "CREATE OR REPLACE MICROFLOW MxTest.TestRunner") { - t.Error("Missing CREATE OR REPLACE MICROFLOW") + if !contains(mdl, "CREATE OR MODIFY MICROFLOW MxTest.TestRunner") { + t.Error("Missing CREATE OR MODIFY MICROFLOW") } if !contains(mdl, "RETURNS Boolean") { t.Error("Missing RETURNS Boolean") From 1f295260063566d6967f4ef66dba061469a8ff12 Mon Sep 17 00:00:00 2001 From: Ako Date: Mon, 2 Nov 2026 15:27:19 +0000 Subject: [PATCH 06/17] fix(test): no "Module 'MxTest' already exists" on a clean project (#1856) The endpoint runner injects the endpoint script, which creates the MxTest module, and then the test-flow script, which began with its own `CREATE MODULE MxTest;`. exec answered the second create with "Module 'MxTest' already exists" on every run. Every caller of GenerateTestFlows runs it into a project that already holds the module (the --local boot after the endpoint script; --watch and --attach into a project hosting the endpoint), so the statement is dropped. Closes #1856 Co-Authored-By: Claude Opus 5.5 --- ...ts-mxtest-already-exists-on-clean-project.json | 1 + cmd/mxcli/testrunner/generator_endpoint.go | 8 +++++++- cmd/mxcli/testrunner/generator_endpoint_test.go | 15 +++++++++++++++ 3 files changed, 23 insertions(+), 1 deletion(-) create mode 100644 .claude/skills/fix-issue/findings/cmd-mxcli/2026-11-02-test-prints-mxtest-already-exists-on-clean-project.json diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli/2026-11-02-test-prints-mxtest-already-exists-on-clean-project.json b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-11-02-test-prints-mxtest-already-exists-on-clean-project.json new file mode 100644 index 000000000..43235be44 --- /dev/null +++ b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-11-02-test-prints-mxtest-already-exists-on-clean-project.json @@ -0,0 +1 @@ +{"area": "cmd/mxcli", "date": "2026-11-02", "symptom": "`mxcli test tests/ --local` printed \"Module 'MxTest' already exists\" on a project that had no MxTest module before the run, between \"Created microflow: MxTest.RegisterEndpoint\" and the first test microflow", "cause": "The endpoint runner injects two scripts in sequence: `GenerateEndpointMDL` (which creates the module) and then `GenerateTestFlows`, which began with its own `CREATE MODULE MxTest;`. The second create found the module the first had just made, and exec's no-op message for an existing module went straight to the user's terminal. The message was true about the second script and false about the project", "file": "`cmd/mxcli/testrunner/generator_endpoint.go` (`GenerateTestFlows` no longer emits `CREATE MODULE`; every caller — the --local boot, `reinjectTests` under --watch, `runAttached` — runs it into a project that already holds the module)", "insight": "Read the run's own output for messages about the runner's scripts rather than the project: the line names a fact (the module exists) whose cause is mxcli's injection order, so it reads as the user's project being dirty. Before dropping the statement, check every caller's precondition — --attach and --watch reinject the test-flow script alone, and both run into a project hosting the endpoint, so the module is always there; if it were not, the microflow create fails loudly rather than the module being silently minted. The test pins both halves: the flow script creates no module, and the endpoint script still does. Runtime control on an 11.12.2 app: main printed the line once per run, the fix zero, both 2/2 PASS", "refs": ["ako/mxcli#1856", "mendixlabs/mxcli#1698"], "rules": [], "ce": []} diff --git a/cmd/mxcli/testrunner/generator_endpoint.go b/cmd/mxcli/testrunner/generator_endpoint.go index f7dc93bcb..ed05fafb2 100644 --- a/cmd/mxcli/testrunner/generator_endpoint.go +++ b/cmd/mxcli/testrunner/generator_endpoint.go @@ -39,11 +39,17 @@ const ( // - Every test gets its own variable scope, so the suffix-renaming the // monolith needs to keep `$result` in test 1 from colliding with `$result` // in test 2 is simply not required here. +// +// The script does not create the MxTest module: every caller runs it into a +// project that already holds one — straight after GenerateEndpointMDL, or under +// --watch and --attach into a project hosting the endpoint. A `CREATE MODULE` +// here made exec print "Module 'MxTest' already exists" on every run, on a +// project that had no MxTest module before it (ako/mxcli#1856). func GenerateTestFlows(suite *TestSuite) string { var b strings.Builder v := suiteVersion(suite) writeScriptHeader(&b, v) - b.WriteString("CREATE MODULE " + mxTestModule + ";\n\n") + b.WriteString("\n") for _, tc := range suite.Tests { // A test with an uncompilable @expect gets no microflow. The runner // reports it as an ERROR from the parse message, which is more useful diff --git a/cmd/mxcli/testrunner/generator_endpoint_test.go b/cmd/mxcli/testrunner/generator_endpoint_test.go index 852186471..13178db8f 100644 --- a/cmd/mxcli/testrunner/generator_endpoint_test.go +++ b/cmd/mxcli/testrunner/generator_endpoint_test.go @@ -242,3 +242,18 @@ func TestEndpointCleanupRestoreIsAlwaysFirst(t *testing.T) { } } } + +// The test-flow script runs after the endpoint script, which creates the MxTest +// module — and under --watch and --attach into a project that already holds it. +// A `CREATE MODULE` of its own is never needed, and exec answers it with +// "Module 'MxTest' already exists", which every `mxcli test --local` printed on +// a project that had no such module before the run (ako/mxcli#1856). +func TestGenerateTestFlowsDoesNotCreateTheModule(t *testing.T) { + suite := &TestSuite{Tests: []TestCase{{ID: "test_1", Name: "a", MDL: "$r = CALL MICROFLOW Mod.A();"}}} + if mdl := GenerateTestFlows(suite); strings.Contains(strings.ToUpper(mdl), "CREATE MODULE") { + t.Errorf("test-flow script creates the module the endpoint script already created:\n%s", mdl) + } + if !strings.Contains(GenerateEndpointMDL(""), "CREATE MODULE "+mxTestModule+";") { + t.Error("the endpoint script no longer creates the module; the test-flow script depends on it") + } +} From 54aab8eb4da3b420594e2d111975fafc191083a0 Mon Sep 17 00:00:00 2001 From: Ako Date: Mon, 2 Nov 2026 15:37:27 +0000 Subject: [PATCH 07/17] =?UTF-8?q?fix(visitor):=20store=20a=20doc=20comment?= =?UTF-8?q?=20above=20alter=20entity=20=E2=80=A6=20add=20attribute=20(#186?= =?UTF-8?q?3)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `/** … */` comment above `alter entity M.E add attribute X: …;` was read by nothing: the alter visitor only reads a comment written inside the statement, so exec reported success and the attribute had no documentation, with only a later check warning MDL089. The statement's comment now documents the attribute when the alter (or a bulk `alter entities`) adds exactly one attribute with no comment of its own. Two adds, or an attribute already documented, keep the comment detached so check still warns. Co-Authored-By: Claude Opus 5.5 --- ...above-alter-entity-add-attribute-lost.json | 1 + CHANGELOG.md | 1 + .../1863-alter-add-attribute-doc-comment.mdl | 17 ++++ .../visitor_alter_add_attribute_doc_test.go | 81 +++++++++++++++++++ mdl/visitor/visitor_doc_comment_placement.go | 40 ++++++++- 5 files changed, 137 insertions(+), 3 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-visitor/2026-11-02-doc-comment-above-alter-entity-add-attribute-lost.json create mode 100644 mdl-examples/bug-tests/1863-alter-add-attribute-doc-comment.mdl create mode 100644 mdl/visitor/visitor_alter_add_attribute_doc_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-visitor/2026-11-02-doc-comment-above-alter-entity-add-attribute-lost.json b/.claude/skills/fix-issue/findings/mdl-visitor/2026-11-02-doc-comment-above-alter-entity-add-attribute-lost.json new file mode 100644 index 000000000..facaf17e3 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-visitor/2026-11-02-doc-comment-above-alter-entity-add-attribute-lost.json @@ -0,0 +1 @@ +{"area": "mdl/visitor", "date": "2026-11-02", "symptom": "ako/mxcli#1863 / mendixlabs/mxcli#1705: `/** Why */` above `alter entity M.E add attribute X: String(200);` — exec reports \"Added attribute\", describe shows X with no documentation; only a later `check` warns MDL089", "cause": "The alter visitor reads a doc comment written INSIDE the alter (before `add`, or after `attribute`); the statement-level comment, the place a user writes it, was recorded as detached by exitStatementDocs and applied to nothing", "file": "mdl/visitor/visitor_doc_comment_placement.go (adoptsAddedAttributeDoc)", "fix": "exitStatementDocs gives the statement's doc comment to the added attribute when the alter (or bulk `alter entities`) adds exactly one attribute with no comment of its own; otherwise it stays detached and MDL089 still warns", "test": "mdl/visitor TestAlterAddAttributeStatementDocComment (alter entity + alter entities, controls: attribute's own comment wins, two adds stay detached). Fails on main with documentation \"\" and one detached comment; end-to-end on PedApp 11.13 describe shows `/** Why */` above X after the fix and none before", "insight": "The 2026-07-30 fix (traceops #27) made the comment parse between clauses and believed the gap closed, but tested only the clause position. MDL089 lists exactly the statement-level comments nothing reads, so it doubles as a census of this bug class: any statement whose MDL089 warning a user would reasonably expect to be stored is a drop, not a misplacement", "refs": ["ako/mxcli#1863", "mendixlabs/mxcli#1705"]} diff --git a/CHANGELOG.md b/CHANGELOG.md index bfe78c19b..04e4c81a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **A doc comment above `alter entity … add attribute` is stored on the attribute** (ako/mxcli#1863, mendixlabs/mxcli#1705) — `/** Why */` written above the statement was read by nothing: exec reported "Added attribute" and the attribute had no documentation, with only a later `check` warning MDL089. It now documents the attribute the statement adds, as it does above a `create entity` attribute, for `alter entities` too. When the statement adds two attributes, or the attribute has its own comment, the comment is still not stored and `check` still warns. - **`describe microflow` prints the concurrency error message in the project's default language** (ako/mxcli#1869, mendixlabs/mxcli#1711) — on an `en_US` project with `de_DE` enabled it printed the German text, and a drop and create from that output stored it under `en_US` too, so the English message was gone while `check`, `exec` and `mx check` stayed clean. It now picks the language the way every other described text does: the project default, then `en_US`, then the first non-empty one. - **The catalog's XPath rows name their widget** (ako/mxcli#1879) — `xpath_expressions.ComponentId` for a page or snippet data source was a Go byte dump (`{0 [57 221 …]}`) and nothing tied the row to its widget. It is now a GUID, and the new `WidgetId` / `WidgetName` columns (`widget_id` / `widget_name` in `xpath_expressions()`) name the widget by the id `widgets` gives it. The catalog schema moves to version 32, so a cached catalog is rebuilt once. - **`create or replace page` can clear a data grid column caption** (ako/mxcli#1877) — `Caption: ''` over a stored caption reported "Unchanged page" and kept it, because the translation carry added the stored text back to an empty header written with no translation. The header is now written as one empty translation, which the carry leaves alone; an empty translation where the stored text has none is dropped, so a Studio Pro empty caption still rewrites as unchanged. This also repairs pages that still carry a leaked column name (#535). diff --git a/mdl-examples/bug-tests/1863-alter-add-attribute-doc-comment.mdl b/mdl-examples/bug-tests/1863-alter-add-attribute-doc-comment.mdl new file mode 100644 index 000000000..cdd9792d9 --- /dev/null +++ b/mdl-examples/bug-tests/1863-alter-add-attribute-doc-comment.mdl @@ -0,0 +1,17 @@ +mdl 1; +-- ============================================================================ +-- ako/mxcli#1863 (mendixlabs/mxcli#1705) — a doc comment above +-- `alter entity … add attribute` was not stored. Exec reported "Added +-- attribute 'Reason'" and the attribute had no documentation; only `check` +-- warned (MDL089). +-- +-- After the fix `describe entity Bug1863.Ticket` prints `/** Why it was +-- closed */` above Reason, and `check` reports no MDL089. +-- ============================================================================ + +create module Bug1863; + +create persistent entity Bug1863.Ticket (Title: String(200)); + +/** Why it was closed */ +alter entity Bug1863.Ticket add attribute Reason: String(200); diff --git a/mdl/visitor/visitor_alter_add_attribute_doc_test.go b/mdl/visitor/visitor_alter_add_attribute_doc_test.go new file mode 100644 index 000000000..1d37ef510 --- /dev/null +++ b/mdl/visitor/visitor_alter_add_attribute_doc_test.go @@ -0,0 +1,81 @@ +// SPDX-License-Identifier: Apache-2.0 + +package visitor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +// ako/mxcli#1863 (mendixlabs/mxcli#1705): a doc comment above an `alter +// entity … add attribute` documents the attribute it adds, as it would above a +// `create entity` attribute. The statement's comment was read by nothing — the +// alter only looked inside itself, before `add` — so exec reported success and +// the text was lost, with only a later check warning MDL089. +func TestAlterAddAttributeStatementDocComment(t *testing.T) { + addedDocs := func(prog *ast.Program) []string { + var out []string + for _, s := range prog.Statements { + switch st := s.(type) { + case *ast.AlterEntityStmt: + if st.Attribute != nil { + out = append(out, st.Attribute.Documentation) + } + case *ast.AlterEntitiesStmt: + for _, a := range st.Actions { + if a.Attribute != nil { + out = append(out, a.Attribute.Documentation) + } + } + } + } + return out + } + for _, tc := range []struct { + name string + src string + docs []string + detached int + }{{ + name: "alter entity", + src: "/** Why */\nalter entity M.E add attribute X: String(200);\n", + docs: []string{"Why"}, + }, { + name: "alter entities", + src: "/** Why */\nalter entities in M add attribute X: String(200);\n", + docs: []string{"Why"}, + }, { + // The attribute's own comment wins; the statement's is still lost. + name: "control: attribute already documented", + src: "/** Outer */\nalter entity M.E add attribute /** Inner */ X: String(200);\n", + docs: []string{"Inner"}, + detached: 1, + }, { + // Two attributes: which one the comment is for is a guess, so it + // stays a check warning rather than being attached to either. + name: "control: two attributes added", + src: "/** Which */\nalter entity M.E add attribute X: String(200) add attribute Y: Integer;\n", + docs: []string{"", ""}, + detached: 1, + }} { + t.Run(tc.name, func(t *testing.T) { + prog, errs := Build(tc.src) + if len(errs) > 0 { + t.Fatalf("parse: %v", errs) + } + got := addedDocs(prog) + if len(got) != len(tc.docs) { + t.Fatalf("added attributes' docs = %q, want %q", got, tc.docs) + } + for i := range got { + if got[i] != tc.docs[i] { + t.Errorf("attribute %d documentation = %q, want %q", i, got[i], tc.docs[i]) + } + } + if n := len(prog.DetachedDocComments); n != tc.detached { + t.Errorf("detached doc comments = %+v, want %d", prog.DetachedDocComments, tc.detached) + } + }) + } +} diff --git a/mdl/visitor/visitor_doc_comment_placement.go b/mdl/visitor/visitor_doc_comment_placement.go index 3a423cadf..6268187f7 100644 --- a/mdl/visitor/visitor_doc_comment_placement.go +++ b/mdl/visitor/visitor_doc_comment_placement.go @@ -40,7 +40,7 @@ func (b *Builder) exitStatementDocs(ctx *parser.StatementContext) { return // nothing built: a syntax error, reported already } head, line := statementHead(ctx) - stores := storesDocumentation(ctx, built) + stores := storesDocumentation(ctx, built) || adoptsAddedAttributeDoc(ctx, built) if stores { for _, d := range b.detachedDocs[b.docsAwaitingNext:] { d.Next, d.NextLine = head, line @@ -68,8 +68,8 @@ func (b *Builder) detachedDocComments() []ast.DetachedDocComment { // storesDocumentation reports whether a statement keeps its doc comment: it is // a create, and what it builds has a documentation property the comment sets. -// No other statement reads the statement's doc comment (an `alter entity … -// add attribute` reads the one written inside the alter, before `add`). +// No other statement reads the statement's doc comment, except an alter that +// adds one attribute (adoptsAddedAttributeDoc). func storesDocumentation(ctx *parser.StatementContext, built []ast.Statement) bool { ddl := ctx.DdlStatement() if ddl == nil || ddl.CreateStatement() == nil { @@ -92,6 +92,40 @@ func storesDocumentation(ctx *parser.StatementContext, built []ast.Statement) bo return false } +// adoptsAddedAttributeDoc gives the statement's doc comment to the attribute an +// `alter entity` / `alter entities` adds, as a comment above a create entity +// attribute documents it (ako/mxcli#1863). Only when the statement adds exactly +// one attribute that has no comment of its own: with two, which one the text +// was written for is a guess, and an attribute's own comment wins — both stay +// detached, so check still warns (MDL089). +func adoptsAddedAttributeDoc(ctx *parser.StatementContext, built []ast.Statement) bool { + doc := ctx.DocComment() + if doc == nil { + return false + } + var added []*ast.Attribute + collect := func(s *ast.AlterEntityStmt) { + if s.Operation == ast.AlterEntityAddAttribute && s.Attribute != nil { + added = append(added, s.Attribute) + } + } + for _, s := range built { + switch st := s.(type) { + case *ast.AlterEntityStmt: + collect(st) + case *ast.AlterEntitiesStmt: + for _, a := range st.Actions { + collect(a) + } + } + } + if len(added) != 1 || added[0].Documentation != "" { + return false + } + added[0].Documentation = extractDocComment(doc.GetText()) + return true +} + // statementHead is the start of the statement as written — its keywords and // name, up to the first bracket, `;` or line end — and its line, for a message // that points at it. From 1cf67b042aa752d3f325cfb2e699ec6dcf0e7686 Mon Sep 17 00:00:00 2001 From: Ako Date: Mon, 2 Nov 2026 15:41:03 +0000 Subject: [PATCH 08/17] fix(test): repeat a failed build's errors above the summary (#1834) A suite with one test that does not build ended in "Passed: 0, Errors: 1, Skipped: 108": the build error was on its ERROR row, one line among a hundred identical "not run" rows. resultsForBuildFailure now marks the result, and PrintResults repeats each rejected test's errors, with its file, directly above the summary line. Co-Authored-By: Claude Opus 5.5 --- cmd/mxcli/testrunner/build_attribution.go | 7 ++- .../testrunner/build_error_first_test.go | 59 +++++++++++++++++++ cmd/mxcli/testrunner/results.go | 38 ++++++++++++ 3 files changed, 102 insertions(+), 2 deletions(-) create mode 100644 cmd/mxcli/testrunner/build_error_first_test.go diff --git a/cmd/mxcli/testrunner/build_attribution.go b/cmd/mxcli/testrunner/build_attribution.go index e17b44e66..e35042828 100644 --- a/cmd/mxcli/testrunner/build_attribution.go +++ b/cmd/mxcli/testrunner/build_attribution.go @@ -64,7 +64,7 @@ func resultsForBuildFailure(err error, suite *TestSuite) (*SuiteResult, error) { return nil, err } if results := resultsFromFailedBuild(bf.BuildErrors(), suite); results != nil { - return &SuiteResult{Name: suite.Name, Tests: results, Started: time.Now()}, nil + return &SuiteResult{Name: suite.Name, Tests: results, Started: time.Now(), BuildFailed: true}, nil } _, other := attributeBuildProblems(bf.BuildErrors(), suite) return nil, fmt.Errorf("%w%s", err, buildFailureHint(other)) @@ -138,9 +138,12 @@ func buildProblemMessage(problems []docker.BuildProblem) string { } parts = append(parts, msg) } - return "the assertion could not be built: " + strings.Join(parts, "; ") + return buildProblemPrefix + strings.Join(parts, "; ") } +// buildProblemPrefix starts the message of a test MxBuild rejected. +const buildProblemPrefix = "the assertion could not be built: " + // resultsFromFailedBuild turns a failed build into one result per test, so a run // that cannot boot still reports something per test instead of nothing at all. // diff --git a/cmd/mxcli/testrunner/build_error_first_test.go b/cmd/mxcli/testrunner/build_error_first_test.go new file mode 100644 index 000000000..0921eb259 --- /dev/null +++ b/cmd/mxcli/testrunner/build_error_first_test.go @@ -0,0 +1,59 @@ +// SPDX-License-Identifier: Apache-2.0 + +package testrunner + +import ( + "strings" + "testing" +) + +// A suite whose one test did not build ended in "Passed: 0, Errors: 1, +// Skipped: 108", the CE0161 that caused it one row among 108 identical "not +// run" rows — so the run read as if every test had failed for no reason +// (ako/mxcli#1834). The build error is now repeated in a block of its own +// directly above the summary line, where the reader's eye lands. +func TestPrintResultsLeadsTheSummaryWithTheBuildError(t *testing.T) { + ids := []string{"test_1", "test_2", "test_3", "test_4"} + suite := suiteOf(ids...) + suite.Tests[1].SourceFile = "tests/orders.test.mdl" + err := buildFailure(failedBuild(problem("CE0161", "Error(s) in XPath constraint.", + "MxTest", "Microflow 'Test_test_2'", "Retrieve object(s) activity 'Retrieve list of Order from database'"))) + result, err := resultsForBuildFailure(err, suite) + if err != nil || result == nil { + t.Fatalf("resultsForBuildFailure: %v", err) + } + + var out strings.Builder + PrintResults(&out, result, false) + s := out.String() + + block := strings.Index(s, "Build failed") + summary := strings.Index(s, "Total:") + lastSkip := strings.LastIndex(s, "SKIP") + if block < 0 { + t.Fatalf("no build-failure block in the output:\n%s", s) + } + if !(lastSkip < block && block < summary) { + t.Errorf("build-failure block at %d, want after the last row (%d) and before the summary (%d):\n%s", + block, lastSkip, summary, s) + } + tail := s[block:summary] + for _, want := range []string{"name of test_2", "tests/orders.test.mdl", "CE0161", "Error(s) in XPath constraint."} { + if !strings.Contains(tail, want) { + t.Errorf("build-failure block is missing %q:\n%s", want, tail) + } + } +} + +// A run that built prints no such block — the control for the test above. +func TestPrintResultsNoBuildBlockWhenTheBuildSucceeded(t *testing.T) { + result := &SuiteResult{Name: "s", Tests: []TestResult{ + {Name: "a", Status: StatusPass}, + {Name: "b", Status: StatusError, Message: "calling the test endpoint: boom"}, + }} + var out strings.Builder + PrintResults(&out, result, false) + if strings.Contains(out.String(), "Build failed") { + t.Errorf("a run that built printed a build-failure block:\n%s", out.String()) + } +} diff --git a/cmd/mxcli/testrunner/results.go b/cmd/mxcli/testrunner/results.go index 593574229..c7ba721e4 100644 --- a/cmd/mxcli/testrunner/results.go +++ b/cmd/mxcli/testrunner/results.go @@ -70,6 +70,10 @@ type SuiteResult struct { Tests []TestResult Duration time.Duration Started time.Time + // BuildFailed is set when MxBuild rejected a generated test microflow, so no + // test ran: the ERROR rows carry the build errors and every other row is a + // SKIP. PrintResults repeats those errors above the summary. + BuildFailed bool } // PassCount returns the number of passing tests. @@ -393,6 +397,8 @@ func PrintResults(w io.Writer, result *SuiteResult, color bool) { "Run with --require-assertions to make that an error.\n", n) } + printBuildFailure(w, result) + fmt.Fprintf(w, "%s\n", strings.Repeat("-", 60)) errors := result.ErrorCount() fmt.Fprintf(w, "Total: %d Passed: %d Failed: %d", @@ -421,6 +427,38 @@ func PrintResults(w io.Writer, result *SuiteResult, color bool) { } } +// printBuildFailure repeats a failed build's errors directly above the summary. +// +// The ERROR rows already carry them, but a suite with one test that does not +// build has every other test as a SKIP row, and the error was one line among a +// hundred identical "not run" lines: the run ended in "Passed: 0, Errors: 1, +// Skipped: 108" and read as if every test had failed for no reason +// (ako/mxcli#1834). The summary is where the reader's eye lands, so the cause +// goes right above it. +func printBuildFailure(w io.Writer, result *SuiteResult) { + if !result.BuildFailed { + return + } + var failed []TestResult + for _, t := range result.Tests { + if t.Status == StatusError { + failed = append(failed, t) + } + } + if len(failed) == 0 { + return + } + fmt.Fprintf(w, "%s\n", strings.Repeat("-", 60)) + fmt.Fprintf(w, "Build failed: MxBuild rejected %d test(s), so no test ran.\n", len(failed)) + for _, t := range failed { + where := t.Name + if t.SourceFile != "" { + where += " (" + t.SourceFile + ")" + } + fmt.Fprintf(w, " %s\n %s\n", where, strings.TrimPrefix(t.Message, buildProblemPrefix)) + } +} + // assertionErrorResult turns a test whose assertions could not be compiled into // an ERROR result. // From 985e232d29567dc6977daa50673cc10b70e569b1 Mon Sep 17 00:00:00 2001 From: Ako Date: Mon, 2 Nov 2026 15:41:06 +0000 Subject: [PATCH 09/17] fix(test): retry the boot's build once when mxbuild --serve times out (#1834) A serve process that stopped answering ended `mxcli test --local` in "serve build request ... context deadline exceeded", and the next run passed with nothing changed. StartLocalApp now restarts the serve process and builds once more on a timeout (net.Error.Timeout), and only then: a failed build or a refused connection is returned as it is. Closes #1834 Co-Authored-By: Claude Opus 5.5 --- ...-buried-and-serve-timeout-not-retried.json | 1 + cmd/mxcli/docker/localapp.go | 2 +- cmd/mxcli/docker/serve_timeout_retry.go | 39 +++++++ cmd/mxcli/docker/serve_timeout_retry_test.go | 109 ++++++++++++++++++ 4 files changed, 150 insertions(+), 1 deletion(-) create mode 100644 .claude/skills/fix-issue/findings/cmd-mxcli/2026-11-02-test-build-error-buried-and-serve-timeout-not-retried.json create mode 100644 cmd/mxcli/docker/serve_timeout_retry.go create mode 100644 cmd/mxcli/docker/serve_timeout_retry_test.go diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli/2026-11-02-test-build-error-buried-and-serve-timeout-not-retried.json b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-11-02-test-build-error-buried-and-serve-timeout-not-retried.json new file mode 100644 index 000000000..0c3b41c22 --- /dev/null +++ b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-11-02-test-build-error-buried-and-serve-timeout-not-retried.json @@ -0,0 +1 @@ +{"area": "cmd/mxcli/testrunner", "date": "2026-11-02", "symptom": "A suite with one test that does not build (CE0161 in an XPath that passes `mxcli check`) ended `mxcli test --local` in \"Passed: 0, Errors: 1, Skipped: 108\", and read as if every test had failed for no reason. Separately, a `mxbuild --serve` that stopped answering ended the run in \"local runtime: build: serve build request to http://127.0.0.1:6544/build: ... context deadline exceeded\", and the next run passed with nothing changed", "cause": "The build error was attributed (since mendixlabs/mxcli#1104) but printed only on its ERROR row, among one SKIP row per other test that each repeated \"not run: another test in this run failed to build\" — the cause was present and buried. The boot's build called `ServeServer.Build` once, so a wedged serve process was reported as the run's error although a fresh process builds the same project fine", "file": "`cmd/mxcli/testrunner/results.go` (`printBuildFailure`, `SuiteResult.BuildFailed` set by `resultsForBuildFailure`); `cmd/mxcli/docker/serve_timeout_retry.go` (`buildRetryingServeTimeout`, used by `StartLocalApp`)", "insight": "Surfacing is about position, not presence: the error had been in the output for months. Put it directly above the summary line, where the eye lands at the end of a long run. For the retry, retry only on a timeout (`net.Error.Timeout()`), once, on a FRESH process — a failed build is the model's verdict and a refused connection is a dead process, and retrying either hides it. The fake timeout in the unit test is cross-checked against a real `http.Client{Timeout}` error wrapped as `ServeServer.Build` wraps it, so the detection cannot pass against a shape net/http never produces. Runtime control on 11.12.2 with a real CE0161 test: main printed the error on its row only, the fix also in the block above the summary", "refs": ["ako/mxcli#1834", "mendixlabs/mxcli#1685", "mendixlabs/mxcli#1104"], "rules": [], "ce": ["CE0161"]} diff --git a/cmd/mxcli/docker/localapp.go b/cmd/mxcli/docker/localapp.go index c224e28e2..64730a419 100644 --- a/cmd/mxcli/docker/localapp.go +++ b/cmd/mxcli/docker/localapp.go @@ -248,7 +248,7 @@ func StartLocalApp(opts LocalAppOptions) (*LocalApp, error) { } app.serve = serve - build, err := serve.Build(BuildRequest{Target: TargetDeploy, ProjectFilePath: opts.ProjectPath}) + build, err := buildRetryingServeTimeout(serve, BuildRequest{Target: TargetDeploy, ProjectFilePath: opts.ProjectPath}, w) if err != nil { app.Stop() return nil, fmt.Errorf("build: %w", err) diff --git a/cmd/mxcli/docker/serve_timeout_retry.go b/cmd/mxcli/docker/serve_timeout_retry.go new file mode 100644 index 000000000..9e6509f6b --- /dev/null +++ b/cmd/mxcli/docker/serve_timeout_retry.go @@ -0,0 +1,39 @@ +// SPDX-License-Identifier: Apache-2.0 + +package docker + +import ( + "errors" + "fmt" + "io" + "net" +) + +// buildRetryingServeTimeout runs a boot's build, and when the serve process did +// not answer in time, restarts it and builds once more. +// +// A timed-out request is not a verdict on the model: a `mxbuild --serve` that +// stops answering is process state, and a fresh process builds the same project +// fine — reported as `mxcli test --local` ending in "serve build request ... +// context deadline exceeded", with the next run passing unchanged +// (ako/mxcli#1834). Once, not until it works: a second timeout is reported. Any +// other outcome is returned as it is — a failed build is the model's answer, +// and a refused connection is a dead process a restart would only paper over. +func buildRetryingServeTimeout(serve serveProcess, req BuildRequest, w io.Writer) (*BuildResult, error) { + res, err := serve.Build(req) + if err == nil || !isTimeout(err) { + return res, err + } + fmt.Fprintf(w, " mxbuild --serve did not answer the build request (%v); restarting it and building once more...\n", err) + if rerr := serve.Restart(); rerr != nil { + return nil, fmt.Errorf("%w (restarting mxbuild --serve to retry failed: %v)", err, rerr) + } + return serve.Build(req) +} + +// isTimeout reports whether err is a request that ran out of time, as net/http +// reports an elapsed Client.Timeout. +func isTimeout(err error) bool { + var ne net.Error + return errors.As(err, &ne) && ne.Timeout() +} diff --git a/cmd/mxcli/docker/serve_timeout_retry_test.go b/cmd/mxcli/docker/serve_timeout_retry_test.go new file mode 100644 index 000000000..856edaf02 --- /dev/null +++ b/cmd/mxcli/docker/serve_timeout_retry_test.go @@ -0,0 +1,109 @@ +// SPDX-License-Identifier: Apache-2.0 + +package docker + +import ( + "errors" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + "time" +) + +// timeoutErr is what net/http returns when the client's Timeout elapses: a +// *url.Error whose Timeout() is true ("context deadline exceeded (Client.Timeout +// exceeded while awaiting headers)"). +type timeoutErr struct{} + +func (timeoutErr) Error() string { + return "context deadline exceeded (Client.Timeout exceeded while awaiting headers)" +} +func (timeoutErr) Timeout() bool { return true } +func (timeoutErr) Temporary() bool { return true } + +func serveTimeout() error { + return fmt.Errorf("serve build request to http://127.0.0.1:6544: %w", + &url.Error{Op: "Post", URL: "http://127.0.0.1:6544/build", Err: timeoutErr{}}) +} + +// A `mxbuild --serve` that stops answering made `mxcli test --local` end in +// "serve build request ... context deadline exceeded", and the next run passed +// with nothing changed (ako/mxcli#1834). The boot's build now restarts the +// serve process and builds once more on a timeout. +func TestBuildRetryingServeTimeout_RetriesOnceOnAFreshProcess(t *testing.T) { + f := &scriptedServe{build: func(proc, nth int) (*BuildResult, error) { + if proc == 0 { + return nil, serveTimeout() + } + return buildOK(true), nil + }} + var out strings.Builder + res, err := buildRetryingServeTimeout(f, BuildRequest{Target: TargetDeploy}, &out) + if err != nil { + t.Fatalf("err = %v, want the retry's success", err) + } + if !res.OK() { + t.Errorf("result = %+v, want the retry's", res) + } + if f.restarts != 1 || len(f.reqs) != 2 { + t.Errorf("restarts = %d, builds = %d; want one restart and one retry", f.restarts, len(f.reqs)) + } + if !strings.Contains(out.String(), "did not answer") { + t.Errorf("the retry was silent: %q", out.String()) + } +} + +// Once, not until it works: a second timeout is reported, with the first. +func TestBuildRetryingServeTimeout_SecondTimeoutIsReported(t *testing.T) { + f := &scriptedServe{build: func(proc, nth int) (*BuildResult, error) { return nil, serveTimeout() }} + _, err := buildRetryingServeTimeout(f, BuildRequest{}, &strings.Builder{}) + if err == nil || !strings.Contains(err.Error(), "deadline exceeded") { + t.Fatalf("err = %v, want the timeout", err) + } + if f.restarts != 1 || len(f.reqs) != 2 { + t.Errorf("restarts = %d, builds = %d; want exactly one retry", f.restarts, len(f.reqs)) + } +} + +// Anything but a timeout is not retried: a failed build is a verdict, and a +// refused connection is a dead process a restart would only hide. Also the +// control for the retry test. +func TestBuildRetryingServeTimeout_OtherOutcomesAreNotRetried(t *testing.T) { + cases := map[string]func(int, int) (*BuildResult, error){ + "success": func(int, int) (*BuildResult, error) { return buildOK(true), nil }, + "failure": func(int, int) (*BuildResult, error) { + return &BuildResult{Status: "Failure", Message: "The project cannot be deployed"}, nil + }, + "refused": func(int, int) (*BuildResult, error) { + return nil, &url.Error{Op: "Post", URL: "x", Err: errors.New("connection refused")} + }, + } + for name, build := range cases { + f := &scriptedServe{build: build} + _, _ = buildRetryingServeTimeout(f, BuildRequest{}, &strings.Builder{}) + if f.restarts != 0 || len(f.reqs) != 1 { + t.Errorf("%s: restarts = %d, builds = %d; want no retry", name, f.restarts, len(f.reqs)) + } + } +} + +// The fake above is only as good as its resemblance to the real error, so the +// detection is also checked against what net/http actually returns when a +// Client.Timeout elapses, wrapped the way ServeServer.Build wraps it. +func TestIsTimeout_RealClientTimeout(t *testing.T) { + release := make(chan struct{}) + srv := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { <-release })) + defer srv.Close() + defer close(release) + + _, err := (&http.Client{Timeout: 20 * time.Millisecond}).Post(srv.URL+"/build", "application/json", nil) + if err == nil { + t.Fatal("request did not time out") + } + if wrapped := fmt.Errorf("serve build request to %s: %w", srv.URL, err); !isTimeout(wrapped) { + t.Errorf("isTimeout(%v) = false, want true", wrapped) + } +} From 2c1a82a8490d32a1fca7feb756b45df6d01dd542 Mon Sep 17 00:00:00 2001 From: Ako Date: Mon, 2 Nov 2026 15:46:56 +0000 Subject: [PATCH 10/17] fix(pages): store a dynamic-text parameter's dateFormat in Mendix's spelling (#1864) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MDL-WIDGET18 accepts a parameter's dateFormat and enumFormat in any case, but formattingInfoFromParamFormat copied the value verbatim, so `dateFormat: custom` was stored as "custom" — not a Mendix enum value — and the runtime showed the default format. The converter now canonicalises both through pages.CanonicalDateFormat and the new CanonicalEnumFormat, as the date picker's path already did. The converter feeds every ClientTemplateParameter FormattingInfo (dynamic text, Data grid 2 column templates, the ALTER PAGE column path), so all of them are covered. Co-Authored-By: Claude Opus 5.5 --- ...-dateformat-lowercase-stored-verbatim.json | 1 + CHANGELOG.md | 1 + .../1864-dynamictext-dateformat-lowercase.mdl | 20 +++++++++++++ mdl/executor/cmd_pages_builder_v3_widgets.go | 11 ++++++-- mdl/executor/dynamictext_format_test.go | 28 +++++++++++++++++++ sdk/pages/formatting.go | 10 +++++++ 6 files changed, 69 insertions(+), 2 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-11-02-dynamictext-param-dateformat-lowercase-stored-verbatim.json create mode 100644 mdl-examples/bug-tests/1864-dynamictext-dateformat-lowercase.mdl diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-11-02-dynamictext-param-dateformat-lowercase-stored-verbatim.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-11-02-dynamictext-param-dateformat-lowercase-stored-verbatim.json new file mode 100644 index 000000000..fcd5e48d2 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-11-02-dynamictext-param-dateformat-lowercase-stored-verbatim.json @@ -0,0 +1 @@ +{"area": "mdl/executor", "date": "2026-11-02", "symptom": "ako/mxcli#1864 / mendixlabs/mxcli#1706: `ContentParams: ({1} = Starts format (dateFormat: custom, customDateFormat: 'd MMM yyyy, HH:mm'))` — check and exec clean, but the runtime shows the default date format (the report's `diff` churn did not reproduce on PedApp 11.13: a create-or-replace rerun diffs clean with the pre-fix binary too)", "cause": "MDL-WIDGET18 accepts dateFormat/enumFormat case-insensitively, but formattingInfoFromParamFormat copied the value verbatim, so the BSON held \"DateFormat\": \"custom\" — not a Mendix enum value. The date picker's path (inputFormattingInfo) already went through pages.CanonicalDateFormat; the template-parameter path never did", "file": "mdl/executor/cmd_pages_builder_v3_widgets.go (formattingInfoFromParamFormat), sdk/pages/formatting.go (CanonicalEnumFormat)", "fix": "Canonicalise dateFormat via pages.CanonicalDateFormat and enumFormat via the new pages.CanonicalEnumFormat. The converter is the one source for every ClientTemplateParameter FormattingInfo (dynamic text, Data grid 2 column templates, ALTER PAGE column path), so all of them are covered", "test": "mdl/executor TestFormattingInfoFromParamFormat_EnumCase (custom, datetime, TIME, enumformat image; control Custom). Fails on main with \"custom\" stored; bson dump on PedApp 11.13 shows DateFormat \"custom\" before the fix", "insight": "The report read as a drop (\"stored with only customDateFormat\") but the value was stored — in a spelling Mendix reads as the default. A validator that lowercases before checking an enum and a writer that does not is the shape to grep for: compare every strings.ToLower in a check against the matching store, since the existing test passed `Custom` capitalised and so never exercised the gap", "refs": ["ako/mxcli#1864", "mendixlabs/mxcli#1706"]} diff --git a/CHANGELOG.md b/CHANGELOG.md index bfe78c19b..37273b9b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **`dateFormat: custom` on a dynamic-text parameter is stored as Custom** (ako/mxcli#1864, mendixlabs/mxcli#1706) — `check` accepts a parameter's `dateFormat` and `enumFormat` in any case, but the writer stored the value as written, and `"custom"` is not a value Mendix knows, so the runtime showed the default format. The values are now stored in Mendix's spelling (`Custom`, `DateTime`, `Image`) on dynamic text and on Data grid 2 column templates. - **`describe microflow` prints the concurrency error message in the project's default language** (ako/mxcli#1869, mendixlabs/mxcli#1711) — on an `en_US` project with `de_DE` enabled it printed the German text, and a drop and create from that output stored it under `en_US` too, so the English message was gone while `check`, `exec` and `mx check` stayed clean. It now picks the language the way every other described text does: the project default, then `en_US`, then the first non-empty one. - **The catalog's XPath rows name their widget** (ako/mxcli#1879) — `xpath_expressions.ComponentId` for a page or snippet data source was a Go byte dump (`{0 [57 221 …]}`) and nothing tied the row to its widget. It is now a GUID, and the new `WidgetId` / `WidgetName` columns (`widget_id` / `widget_name` in `xpath_expressions()`) name the widget by the id `widgets` gives it. The catalog schema moves to version 32, so a cached catalog is rebuilt once. - **`create or replace page` can clear a data grid column caption** (ako/mxcli#1877) — `Caption: ''` over a stored caption reported "Unchanged page" and kept it, because the translation carry added the stored text back to an empty header written with no translation. The header is now written as one empty translation, which the carry leaves alone; an empty translation where the stored text has none is dropped, so a Studio Pro empty caption still rewrites as unchanged. This also repairs pages that still carry a leaked column name (#535). diff --git a/mdl-examples/bug-tests/1864-dynamictext-dateformat-lowercase.mdl b/mdl-examples/bug-tests/1864-dynamictext-dateformat-lowercase.mdl new file mode 100644 index 000000000..c1f4f724e --- /dev/null +++ b/mdl-examples/bug-tests/1864-dynamictext-dateformat-lowercase.mdl @@ -0,0 +1,20 @@ +mdl 1; +-- ============================================================================ +-- ako/mxcli#1864 (mendixlabs/mxcli#1706) — `dateFormat: custom` on a +-- dynamic-text parameter. Check accepts the value in any case, but it was +-- stored as written: "DateFormat": "custom" is not a Mendix value, so the +-- runtime showed the default format. +-- +-- After the fix `mxcli bson dump --type page --object Bug1864.Shift_View` +-- shows "DateFormat": "Custom" (before: "custom"). +-- ============================================================================ + +create module Bug1864; + +create persistent entity Bug1864.Shift (Starts: DateTime); + +create page Bug1864.Shift_View (Title: 'Shift', Layout: Atlas_Core.Atlas_Default, Params: ($Shift: Bug1864.Shift)) { + dataview dvShift (DataSource: $Shift) { + dynamictext textStarts (Content: '{1}', ContentParams: ({1} = Starts format (dateFormat: custom, customDateFormat: 'd MMM yyyy, HH:mm'))) + } +}; diff --git a/mdl/executor/cmd_pages_builder_v3_widgets.go b/mdl/executor/cmd_pages_builder_v3_widgets.go index 6037af191..e55137e5a 100644 --- a/mdl/executor/cmd_pages_builder_v3_widgets.go +++ b/mdl/executor/cmd_pages_builder_v3_widgets.go @@ -1195,12 +1195,19 @@ func formattingInfoFromParamFormat(f *ast.ParamFormatV3) *pages.FormattingInfo { } case "groupdigits": fi.GroupDigits = strings.EqualFold(p.Value, "true") + // Check accepts the enums in any case, so store Mendix's spelling: + // a stored "custom" is not Custom, and the runtime shows the default + // format (ako/mxcli#1864). case "dateformat": - fi.DateFormat = p.Value + if v, ok := pages.CanonicalDateFormat(p.Value); ok { + fi.DateFormat = v + } case "customdateformat": fi.CustomDateFormat = p.Value case "enumformat": - fi.EnumFormat = p.Value + if v, ok := pages.CanonicalEnumFormat(p.Value); ok { + fi.EnumFormat = v + } } } return fi diff --git a/mdl/executor/dynamictext_format_test.go b/mdl/executor/dynamictext_format_test.go index 88079c4c9..8e630a8b4 100644 --- a/mdl/executor/dynamictext_format_test.go +++ b/mdl/executor/dynamictext_format_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/sdk/pages" ) // TestFormattingInfoFromParamFormat: a format block starts from the Mendix @@ -43,6 +44,33 @@ func TestFormattingInfoFromParamFormat(t *testing.T) { } } +// TestFormattingInfoFromParamFormat_EnumCase: check accepts the enum keys in +// any case (MDL-WIDGET18 lowercases before looking them up), so the converter +// must store Mendix's spelling. `dateFormat: custom` was stored as "custom", +// which Mendix does not read as Custom: the runtime showed the default format +// and every rerun diffed as modified (ako/mxcli#1864, mendixlabs/mxcli#1706). +func TestFormattingInfoFromParamFormat_EnumCase(t *testing.T) { + tests := []struct { + key, value, want string + got func(fi *pages.FormattingInfo) string + }{ + {"dateformat", "custom", "Custom", func(fi *pages.FormattingInfo) string { return fi.DateFormat }}, + {"dateformat", "datetime", "DateTime", func(fi *pages.FormattingInfo) string { return fi.DateFormat }}, + {"dateformat", "TIME", "Time", func(fi *pages.FormattingInfo) string { return fi.DateFormat }}, + {"enumformat", "image", "Image", func(fi *pages.FormattingInfo) string { return fi.EnumFormat }}, + // Control: the canonical spelling passes through unchanged. + {"dateformat", "Custom", "Custom", func(fi *pages.FormattingInfo) string { return fi.DateFormat }}, + } + for _, tt := range tests { + fi := formattingInfoFromParamFormat(&ast.ParamFormatV3{Props: []ast.ParamFormatProp{ + {Key: tt.key, Value: tt.value}, {Key: "customdateformat", Value: "d MMM yyyy, HH:mm"}, + }}) + if got := tt.got(fi); got != tt.want { + t.Errorf("%s: %s stored as %q, want %q", tt.key, tt.value, got, tt.want) + } + } +} + func dtWidget(params []ast.ParamAssignmentV3, props map[string]any) *ast.WidgetV3 { all := map[string]any{"ContentParams": params} for k, v := range props { diff --git a/sdk/pages/formatting.go b/sdk/pages/formatting.go index 7336e2b2b..f85e5a8ec 100644 --- a/sdk/pages/formatting.go +++ b/sdk/pages/formatting.go @@ -20,6 +20,16 @@ func CanonicalDateFormat(s string) (string, bool) { return v, ok } +// enumFormats maps a lowercase EnumFormat value to the stored enum value. +var enumFormats = map[string]string{"text": "Text", "image": "Image"} + +// CanonicalEnumFormat returns the stored spelling of a FormattingInfo +// EnumFormat value (Text, Image), matched case-insensitively. +func CanonicalEnumFormat(s string) (string, bool) { + v, ok := enumFormats[strings.ToLower(s)] + return v, ok +} + // formattingProperties lists, per widget storage type, the FormattingInfo // fields MDL sets on it, in canonical spelling and describe order. // From 2431f381acf3e64a6c112c196837fce8adceca5f Mon Sep 17 00:00:00 2001 From: Ako Date: Mon, 2 Nov 2026 15:39:12 +0000 Subject: [PATCH 11/17] fix(test): each --local hsqldb run starts on an empty test database (#1853) `mxcli test --local --db-type hsqldb` booted on the file database the previous run left in deployment/data/database/hsqldb/_test, so data a previous run committed or seeded stayed, an idempotent init skipped seeding, and tests failed on stale data. LocalAppOptions gains FreshFileDB, which removes that one database before the boot; only the test runner sets it, so `run --local --db-type hsqldb` keeps the developer's data. Closes #1853 Co-Authored-By: Claude Opus 5.5 --- ...est-hsqldb-database-kept-between-runs.json | 1 + .../skills/mendix/test-microflows/SKILL.md | 30 ++++--- cmd/mxcli/docker/localapp.go | 31 ++++++- cmd/mxcli/docker/localapp_freshdb_test.go | 87 +++++++++++++++++++ cmd/mxcli/main.go | 2 +- cmd/mxcli/testrunner/localapp_dbtype_test.go | 14 +++ cmd/mxcli/testrunner/localapp_options.go | 7 +- docs-site/src/tools/running-tests.md | 5 +- 8 files changed, 159 insertions(+), 18 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/cmd-mxcli/2026-11-02-test-hsqldb-database-kept-between-runs.json create mode 100644 cmd/mxcli/docker/localapp_freshdb_test.go diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli/2026-11-02-test-hsqldb-database-kept-between-runs.json b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-11-02-test-hsqldb-database-kept-between-runs.json new file mode 100644 index 000000000..ff3ab0c5e --- /dev/null +++ b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-11-02-test-hsqldb-database-kept-between-runs.json @@ -0,0 +1 @@ +{"area": "cmd/mxcli/docker", "date": "2026-11-02", "symptom": "`mxcli test --local --db-type hsqldb` kept `deployment/data/database/hsqldb/_test` between runs. Data a previous run committed (an after-startup seed, an `@cleanup none` test) was still there on the next run, so an idempotent init skipped seeding and tests failed on stale data. Measured on 11.12.2: a test asserting an empty table PASSed on run 1 and FAILed on run 2 after another test committed one row", "cause": "The file database is created by the runtime the first time it boots and opened as-is afterwards. `prepareLocalAppDatabase` had nothing to do for it, so nothing ever removed it; the `_test` suffix kept it off the dev loop's data but not off the previous test run's", "file": "`cmd/mxcli/docker/localapp.go` (`LocalAppOptions.FreshFileDB`, `removeFileDatabase` — refuses a name that is not one path element); `cmd/mxcli/testrunner/localapp_options.go` sets it for the file database", "insight": "Scope the reset to the caller whose database is its own scratch: the option lives on `LocalAppOptions` but only the test runner sets it, so `run --local --db-type hsqldb` (the developer's data) is untouched, and the test pins that the dev database beside the scratch one survives. The name is joined onto the deployment dir and removed recursively, so it is refused unless it is a single path element. Runtime control: same two-test file, main run 2 FAIL (count 1), fix both runs PASS. PostgreSQL's `_test` database has the same carry-over (measured: run 2 FAIL) and is not reset by this change", "refs": ["ako/mxcli#1853", "mendixlabs/mxcli#1695"], "rules": [], "ce": []} diff --git a/.claude/skills/mendix/test-microflows/SKILL.md b/.claude/skills/mendix/test-microflows/SKILL.md index aa2e18162..f8b4ced05 100644 --- a/.claude/skills/mendix/test-microflows/SKILL.md +++ b/.claude/skills/mendix/test-microflows/SKILL.md @@ -12,18 +12,18 @@ description: "Write and run MDL-based microflow tests with `mxcli test` — anno |---|---| | 33-39 | Reference files | | 40-50 | When to Use This Skill | -| 51-180 | Prerequisites | -| 181-223 | Test File Formats | -| 224-239 | string Operations | -| 240-263 | Running Tests | -| 264-457 | How It Works | -| 458-496 | Check a test file before you run it | -| 497-618 | Writing Good Tests | -| 619-633 | Test File Organization | -| 634-646 | Interpreting Failures | -| 647-668 | CI Integration | -| 669-675 | Related Skills | -| 676-678 | Warm test loop (`mxcli test --local [--watch]`, `--attach`, `run --local --test-endpoint`) | +| 51-182 | Prerequisites | +| 183-225 | Test File Formats | +| 226-241 | string Operations | +| 242-265 | Running Tests | +| 266-459 | How It Works | +| 460-498 | Check a test file before you run it | +| 499-620 | Writing Good Tests | +| 621-635 | Test File Organization | +| 636-648 | Interpreting Failures | +| 649-670 | CI Integration | +| 671-677 | Related Skills | +| 678-680 | Warm test loop (`mxcli test --local [--watch]`, `--attach`, `run --local --test-endpoint`) | # Test Microflows Skill @@ -81,8 +81,10 @@ mxcli test tests/ -p app.mpr --local --db-type hsqldb It is still a scratch database (`_test`), separate from the one `run --local --db-type hsqldb` uses, so a dev loop on the file database keeps its -data and its file lock. `--db-type` applies only to `--local`; it is refused with -`--attach` (which uses the attached app's database) and on the Docker path. +data and its file lock. Each run starts it **empty** — the previous run's files are +removed before the boot — so seeded data never carries over between runs. +`--db-type` applies only to `--local`; it is refused with `--attach` (which uses +the attached app's database) and on the Docker path. The **deployment directory is shared**, and not by choice: mxbuild always writes it to `/deployment` and has no option to move it. That used to blank the diff --git a/cmd/mxcli/docker/localapp.go b/cmd/mxcli/docker/localapp.go index c224e28e2..86c419daa 100644 --- a/cmd/mxcli/docker/localapp.go +++ b/cmd/mxcli/docker/localapp.go @@ -36,6 +36,10 @@ type LocalAppOptions struct { // EnsureDB provisions the local Postgres + database when missing instead of // only checking reachability. EnsureDB bool + // FreshFileDB removes the built-in file database's data before the boot, so + // the runtime creates it empty. For a caller whose database is its own + // scratch (the test runner), never for one holding the developer's data. + FreshFileDB bool // SkipBuild boots against whatever is already in DeployDir. SkipBuild bool // RuntimeLogPath tees the runtime JVM output and the runtime's own @@ -159,13 +163,17 @@ func (o *LocalAppOptions) applyDefaults() { // prepareLocalAppDatabase makes the configured database usable before the boot: // provisioning it with EnsureDB, otherwise checking that its server answers. -// The built-in file database needs neither: the runtime creates its files. +// The built-in file database needs neither: the runtime creates its files — +// afresh when FreshFileDB removed the previous ones. func prepareLocalAppDatabase(opts *LocalAppOptions, w io.Writer) error { if opts.DB.IsFileBased() { if opts.EnsureDB { return fmt.Errorf("EnsureDB provisions PostgreSQL; the built-in file database " + "needs no provisioning") } + if opts.FreshFileDB { + return removeFileDatabase(opts.DeployDir, opts.DB.Name, w) + } return nil } if opts.EnsureDB { @@ -182,6 +190,27 @@ func prepareLocalAppDatabase(opts *LocalAppOptions, w io.Writer) error { return nil } +// removeFileDatabase deletes the runtime's file database called name, which +// lives under /data/database/hsqldb/. +// +// The name must be a single path element: it is joined onto the deployment +// directory and the result removed recursively, so anything else is refused +// rather than resolved. A database that does not exist yet is not an error. +func removeFileDatabase(deployDir, name string, w io.Writer) error { + if name == "" || name == "." || name == ".." || filepath.Base(name) != name { + return fmt.Errorf("refusing to reset file database %q: the name must be a single path element", name) + } + dir := filepath.Join(deployDir, "data", "database", "hsqldb", name) + if _, err := os.Stat(dir); os.IsNotExist(err) { + return nil + } + if err := os.RemoveAll(dir); err != nil { + return fmt.Errorf("resetting file database %s: %w", dir, err) + } + fmt.Fprintf(w, " Removed the previous run's file database %q; the runtime creates it empty\n", name) + return nil +} + // StartLocalApp builds the project with mxbuild and boots the standalone // runtime against the result — the same sequence as `mxcli run --local`, minus // everything a headless caller does not need (no web client bundle, no hub, no diff --git a/cmd/mxcli/docker/localapp_freshdb_test.go b/cmd/mxcli/docker/localapp_freshdb_test.go new file mode 100644 index 000000000..4a8473154 --- /dev/null +++ b/cmd/mxcli/docker/localapp_freshdb_test.go @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: Apache-2.0 + +package docker + +import ( + "io" + "os" + "path/filepath" + "testing" +) + +// `mxcli test --local --db-type hsqldb` booted on the file database the +// previous run left, so data an after-startup flow seeded under an earlier +// model stayed, an idempotent init skipped seeding, and tests failed on stale +// data (ako/mxcli#1853). FreshFileDB removes the run's database before boot — +// and only that one: the dev loop's database beside it is not the run's. +func TestPrepareLocalAppDatabase_FreshFileDBRemovesOnlyItsOwnDatabase(t *testing.T) { + proj := t.TempDir() + o := LocalAppOptions{ + ProjectPath: filepath.Join(proj, "App.mpr"), + DB: DBConfig{Type: RuntimeDatabaseType(DBTypeHSQLDB), Name: "app_test"}, + FreshFileDB: true, + } + o.applyDefaults() + stale := filepath.Join(o.DeployDir, "data", "database", "hsqldb", "app_test", "app_test.script") + devDB := filepath.Join(o.DeployDir, "data", "database", "hsqldb", "app", "app.script") + for _, f := range []string{stale, devDB} { + if err := os.MkdirAll(filepath.Dir(f), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(f, []byte("INSERT INTO ..."), 0o644); err != nil { + t.Fatal(err) + } + } + + if err := prepareLocalAppDatabase(&o, io.Discard); err != nil { + t.Fatalf("prepareLocalAppDatabase: %v", err) + } + if _, err := os.Stat(filepath.Dir(stale)); !os.IsNotExist(err) { + t.Errorf("the previous run's database is still there (stat err %v); the runtime boots on its data", err) + } + if _, err := os.Stat(devDB); err != nil { + t.Errorf("the dev loop's database was touched: %v", err) + } +} + +// Without FreshFileDB — `run --local --db-type hsqldb`, the developer's own +// data — nothing is removed. Also the control for the test above. +func TestPrepareLocalAppDatabase_FileDBKeptByDefault(t *testing.T) { + proj := t.TempDir() + o := LocalAppOptions{ + ProjectPath: filepath.Join(proj, "App.mpr"), + DB: DBConfig{Type: RuntimeDatabaseType(DBTypeHSQLDB), Name: "app"}, + } + o.applyDefaults() + f := filepath.Join(o.DeployDir, "data", "database", "hsqldb", "app", "app.script") + if err := os.MkdirAll(filepath.Dir(f), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(f, nil, 0o644); err != nil { + t.Fatal(err) + } + if err := prepareLocalAppDatabase(&o, io.Discard); err != nil { + t.Fatalf("prepareLocalAppDatabase: %v", err) + } + if _, err := os.Stat(f); err != nil { + t.Errorf("the database was removed without FreshFileDB: %v", err) + } +} + +// The name becomes a path under the deployment directory. One that is not a +// single path element is refused rather than resolved, so a reset can never +// reach above the hsqldb directory. +func TestPrepareLocalAppDatabase_FreshFileDBRefusesAPathName(t *testing.T) { + for _, name := range []string{"", ".", "..", "../app", "a/b"} { + o := LocalAppOptions{ + ProjectPath: filepath.Join(t.TempDir(), "App.mpr"), + DB: DBConfig{Type: RuntimeDatabaseType(DBTypeHSQLDB), Name: name}, + FreshFileDB: true, + } + o.applyDefaults() + o.DB.Name = name // applyDefaults may fill an empty name; test the raw value + if err := prepareLocalAppDatabase(&o, io.Discard); err == nil { + t.Errorf("name %q: reset accepted, want a refusal", name) + } + } +} diff --git a/cmd/mxcli/main.go b/cmd/mxcli/main.go index 2af995617..99c81d006 100644 --- a/cmd/mxcli/main.go +++ b/cmd/mxcli/main.go @@ -439,7 +439,7 @@ func init() { testRunCmd.Flags().String("configuration", "", "With --local, which project configuration's constant values to run the tests with (default: the only one, or \"Default\") — the same resolution 'mxcli run --local' uses, so a suite sees the same constants either way") testRunCmd.Flags().StringArray("constant", nil, "With --local, set a constant for THIS RUN only: Module.Name=value (repeatable). Never written to the project. The value is visible in shell history and in `ps` — for a value that must not be, see docs/11-proposals/PROPOSAL_constant_values.md") testRunCmd.Flags().String("mxbuild-path", "", "With --local, the mxbuild to build with, overriding resolution (Studio Pro's bundled mxbuild on macOS/Windows, the cached CDN download on Linux); also settable as MXCLI_MXBUILD_PATH") - testRunCmd.Flags().String("db-type", "", "With --local, the database the scratch app boots on: postgresql (default) or hsqldb (the runtime's built-in file database — no server and no provisioning, so it works without PostgreSQL). Same values as 'run --local --db-type'") + testRunCmd.Flags().String("db-type", "", "With --local, the database the scratch app boots on: postgresql (default) or hsqldb (the runtime's built-in file database — no server and no provisioning, so it works without PostgreSQL; each run starts it empty). Same values as 'run --local --db-type'") registerTestPortFlags(testRunCmd) testRunCmd.Flags().BoolP("verbose", "v", false, "Show all runtime log output") testRunCmd.Flags().BoolP("color", "", false, "Use colored output") diff --git a/cmd/mxcli/testrunner/localapp_dbtype_test.go b/cmd/mxcli/testrunner/localapp_dbtype_test.go index c385bd5f3..9c9e01057 100644 --- a/cmd/mxcli/testrunner/localapp_dbtype_test.go +++ b/cmd/mxcli/testrunner/localapp_dbtype_test.go @@ -91,3 +91,17 @@ func TestResolveTestDBType(t *testing.T) { } } } + +// A test run's file database is its own scratch, so it starts empty: kept +// between runs, data an earlier run's after-startup seeded made an idempotent +// init skip seeding and the tests fail on it (ako/mxcli#1853). +func TestLocalAppOptions_HSQLDBStartsFresh(t *testing.T) { + got := localAppOptions(RunOptions{ProjectPath: "/tmp/app/App.mpr", DBType: docker.DBTypeHSQLDB}, + "log", nil, io.Discard) + if !got.FreshFileDB { + t.Error("FreshFileDB is false; the run boots on the previous run's data") + } + if pg := localAppOptions(RunOptions{ProjectPath: "/tmp/app/App.mpr"}, "log", nil, io.Discard); pg.FreshFileDB { + t.Error("FreshFileDB set for PostgreSQL, where it means nothing") + } +} diff --git a/cmd/mxcli/testrunner/localapp_options.go b/cmd/mxcli/testrunner/localapp_options.go index 314b7a8cf..62eb546d2 100644 --- a/cmd/mxcli/testrunner/localapp_options.go +++ b/cmd/mxcli/testrunner/localapp_options.go @@ -36,7 +36,12 @@ func localAppOptions(opts RunOptions, logPath string, env []string, w io.Writer) // StartLocalApp carrying the bundle across the boot (FINDINGS §62). DB: db, // Provisioning is PostgreSQL's; the file database is created by the runtime. - EnsureDB: !db.IsFileBased(), + EnsureDB: !db.IsFileBased(), + // The file database is this run's scratch, so it starts empty. Kept + // between runs, data a previous run's after-startup seeded under an + // earlier model made an idempotent init skip seeding, and the tests + // failed on stale data (ako/mxcli#1853). + FreshFileDB: db.IsFileBased(), SkipBuild: opts.SkipBuild, Env: env, ConstantOverrides: opts.ConstantOverrides, diff --git a/docs-site/src/tools/running-tests.md b/docs-site/src/tools/running-tests.md index b457152ab..ea3896664 100644 --- a/docs-site/src/tools/running-tests.md +++ b/docs-site/src/tools/running-tests.md @@ -29,7 +29,10 @@ runtime's built-in file database instead: no server and no provisioning. It takes the same values as `run --local --db-type` (`postgresql`, the default, or `hsqldb`), keeps the `_test` scratch name so it never shares files with a `run --local --db-type hsqldb` dev loop, and is refused with `--attach` and on -the Docker path, which do not boot a database of their own. +the Docker path, which do not boot a database of their own. Each run starts on an +**empty** file database: the previous run's `_test` files are removed +before the boot, so data an after-startup flow seeded under an earlier model +cannot make an idempotent init skip seeding. ```bash mxcli test tests/ -p app.mpr --local --db-type hsqldb From eda66b0b92588f2a4d63d4a78daa8e12758cf7ba Mon Sep 17 00:00:00 2001 From: Ako Date: Mon, 2 Nov 2026 16:17:40 +0000 Subject: [PATCH 12/17] fix(check): refuse like/between/in/is null instead of dropping them (#1835) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The MDL expression rule is shared with OQL and admits LIKE, NOT LIKE, BETWEEN, IN and MATCH, but the visitor returned only the left operand for them: `retrieve … where Name like 'A-%'` was stored as `[Name]`, and check and exec both passed before the build failed with CE0161. In an expression `$s like 'A%'` was stored as `$s`. The visitor now keeps the comparison as written, and check reports MDL091 (already exec-enforced) naming the operator — CE0161 in a retrieve constraint, CE0117 in an expression — with the XPath/expression spelling of the same test. IS NULL / IS NOT NULL are reported the same way. Measured on mxbuild 11.12.2: every OQL form fails; starts-with(), not(contains()), >= and <=, = or =, = empty, startsWith() and a qualified enumeration value (the issue's other shape, already written as 'VALUE') build clean. Mirror of mendixlabs/mxcli#1686. Co-Authored-By: Claude Opus 5.5 --- ...26-11-02-xpath-oql-comparison-dropped.json | 1 + CHANGELOG.md | 1 + mdl/executor/validate_microflow.go | 2 + mdl/executor/validate_oql_comparisons.go | 106 +++++++++++++++++ mdl/executor/xpath_oql_comparison_test.go | 112 ++++++++++++++++++ mdl/visitor/visitor_microflow_expression.go | 42 +++++++ 6 files changed, 264 insertions(+) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-11-02-xpath-oql-comparison-dropped.json create mode 100644 mdl/executor/validate_oql_comparisons.go create mode 100644 mdl/executor/xpath_oql_comparison_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-11-02-xpath-oql-comparison-dropped.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-11-02-xpath-oql-comparison-dropped.json new file mode 100644 index 000000000..13fb9899e --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-11-02-xpath-oql-comparison-dropped.json @@ -0,0 +1 @@ +{"area": "mdl/executor", "date": "2026-11-02", "symptom": "ako/mxcli#1835 (mendixlabs/mxcli#1686, 11.12.4): `retrieve $L from Mod.Item where Name like 'A-%'` passed check and exec, then failed the build with CE0161. The reported second shape, `Status = Mod.ENUM_X.VALUE`, no longer reproduced: the writer already stores 'VALUE' and it builds clean on 11.12.2", "cause": "the MDL expression rule is shared with OQL and admits LIKE / NOT LIKE / BETWEEN / IN / MATCH, but visitor buildComparisonExpression returned only the left operand for them, so the constraint was stored as `[Name]` (and `where N between 1 and 5` as `[N]`); in a microflow expression `$s like 'A%'` was stored as `$s`. IS NULL / IS NOT NULL were kept as a BinaryExpr but nothing refused them", "file": "mdl/visitor/visitor_microflow_expression.go (oqlOnlyComparisonOperator), mdl/executor/validate_oql_comparisons.go (checkXPathOQLComparisons, checkExpressionOQLComparisons)", "fix": "the visitor keeps the comparison as written (SourceExpr over a BinaryExpr with the lowercased operator); check reports MDL091 (exec-enforced) naming the operator, CE0161 in a retrieve constraint and CE0117 in an expression, with the XPath/expression spelling of the same test", "test": "mdl/executor/xpath_oql_comparison_test.go — like/not like/between/in in a constraint, like in a declare, check and exec agreeing; controls starts-with, contains, >= and <=, a literal containing 'like', an enum key. Revert: every bad case got [] and the constraint serialized as \"Name\". Measured on mxbuild 11.12.2: [Name like 'A-%'], [Name not like …], [N between 1 and 5], [N in (1, 2)], [Name is null] CE0161; $s like / is null / in (…) CE0117; starts-with, not(contains()), >= and <=, = or =, = empty, Status = Mod.Enum.Value, startsWith() clean", "insight": "A check cannot see what the visitor never put in the AST: the report read as a missing rule, but the operator was gone before any rule ran, so the stored constraint differed from the script and describe printed the truncated form. When a grammar rule is shared with another language (here OQL), audit every alternative the visitor ignores — each one is a silent drop. describe's own XPath reader drops the same operators on the way out (a stored [Name like 'A-%'] describes as `where Name`)", "refs": ["ako/mxcli#1835", "mendixlabs/mxcli#1686"]} diff --git a/CHANGELOG.md b/CHANGELOG.md index bfe78c19b..6c2d9221f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **`like`, `between`, `in` and `is null` in a retrieve constraint or expression are refused instead of dropped** (ako/mxcli#1835, mendixlabs/mxcli#1686) — the expression grammar admits these OQL comparisons, but the parser kept only the left operand, so `where Name like 'A-%'` was stored as `[Name]`: `check` and `exec` passed and the build failed with CE0161. `check` and `exec` now report **MDL091** naming the operator — CE0161 in an XPath constraint, CE0117 in an expression — with the XPath spelling of the same test (`starts-with()`/`contains()`, `>=` and `<=`, `or`, `= empty`). A qualified enumeration value (`Status = Mod.Enum.Value`) already builds clean. - **`describe microflow` prints the concurrency error message in the project's default language** (ako/mxcli#1869, mendixlabs/mxcli#1711) — on an `en_US` project with `de_DE` enabled it printed the German text, and a drop and create from that output stored it under `en_US` too, so the English message was gone while `check`, `exec` and `mx check` stayed clean. It now picks the language the way every other described text does: the project default, then `en_US`, then the first non-empty one. - **The catalog's XPath rows name their widget** (ako/mxcli#1879) — `xpath_expressions.ComponentId` for a page or snippet data source was a Go byte dump (`{0 [57 221 …]}`) and nothing tied the row to its widget. It is now a GUID, and the new `WidgetId` / `WidgetName` columns (`widget_id` / `widget_name` in `xpath_expressions()`) name the widget by the id `widgets` gives it. The catalog schema moves to version 32, so a cached catalog is rebuilt once. - **`create or replace page` can clear a data grid column caption** (ako/mxcli#1877) — `Caption: ''` over a stored caption reported "Unchanged page" and kept it, because the translation carry added the stored text back to an empty header written with no translation. The header is now written as one empty translation, which the carry leaves alone; an empty translation where the stored text has none is dropped, so a Studio Pro empty caption still rewrites as unchanged. This also repairs pages that still carry a leaked column name (#535). diff --git a/mdl/executor/validate_microflow.go b/mdl/executor/validate_microflow.go index b802b818c..5d335fece 100644 --- a/mdl/executor/validate_microflow.go +++ b/mdl/executor/validate_microflow.go @@ -428,6 +428,7 @@ func (v *microflowValidator) walkBody(body []ast.MicroflowStatement) { v.checkXPathAssociationEmpty(stmt.Variable, xp) v.checkXPathFunctionNames(stmt.Variable, xp) v.checkXPathFunctionArgumentOperators(stmt.Variable, stmt.Where) + v.checkXPathOQLComparisons(stmt.Variable, stmt.Where) v.checkXPathIdConstraint(stmt.Variable, xp) v.checkXPathVariableTraversal(stmt.Variable, xp) } @@ -604,6 +605,7 @@ func (v *microflowValidator) checkExprFunctions(label string, expr ast.Expressio // The one per-expression hook shared by microflows and nanoflows, so the // JSON-number check rides along rather than keeping a third list of sites. v.checkLocaleNumberInJSON(label, expr) + v.checkExpressionOQLComparisons(label, expr) src := microflowExprSource(expr) if src == "" { return diff --git a/mdl/executor/validate_oql_comparisons.go b/mdl/executor/validate_oql_comparisons.go new file mode 100644 index 000000000..d7fd7de66 --- /dev/null +++ b/mdl/executor/validate_oql_comparisons.go @@ -0,0 +1,106 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// oqlComparisonFixes maps each OQL comparison the MDL expression grammar admits +// (it is shared with OQL) to the XPath spelling of the same test. Neither an +// XPath constraint nor a Mendix expression has any of them: measured on mxbuild +// 11.12.2, `[Name like 'A-%']` is CE0161 while `[starts-with(Name, 'A-')]` +// builds clean (mendixlabs/mxcli#1686). Until this rule the visitor dropped the +// operator and its operands, so the constraint was stored as `[Name]` — the +// same CE0161, with nothing in check to say why. +var oqlComparisonFixes = map[string]string{ + "like": "Use starts-with(Name, 'A-'), ends-with(Name, '-A') or contains(Name, 'A') — `like` is OQL.", + "not like": "Negate the XPath function: not(starts-with(Name, 'A-')) or not(contains(Name, 'A')) — `like` is OQL.", + "between": "Compare both bounds: N >= 1 and N <= 5 — `between` is OQL.", + "not between": "Compare outside the bounds: N < 1 or N > 5 — `between` is OQL.", + "in": "Compare each value: N = 1 or N = 2 — `in (…)` is OQL.", + "match": "Use contains() or starts-with() — `match` is OQL.", + "is null": "Compare with empty: Name = empty — `is null` is OQL.", + "is not null": "Compare with empty: Name != empty — `is not null` is OQL.", +} + +// oqlComparisonExpressionFixes is oqlComparisonFixes for a Mendix expression, +// whose string functions are spelled startsWith/endsWith/contains. +var oqlComparisonExpressionFixes = map[string]string{ + "like": "Use startsWith($S, 'A-'), endsWith($S, '-A') or contains($S, 'A') — `like` is OQL.", + "not like": "Negate the function: not(startsWith($S, 'A-')) — `like` is OQL.", + "between": "Compare both bounds: $N >= 1 and $N <= 5 — `between` is OQL.", + "not between": "Compare outside the bounds: $N < 1 or $N > 5 — `between` is OQL.", + "in": "Compare each value: $N = 1 or $N = 2 — `in (…)` is OQL.", + "match": "Use contains() or startsWith() — `match` is OQL.", + "is null": "Compare with empty: $S = empty — `is null` is OQL.", + "is not null": "Compare with empty: $S != empty — `is not null` is OQL.", +} + +// oqlOnlyComparisons returns the OQL comparison operators an expression uses, +// in the order they appear. +func oqlOnlyComparisons(expr ast.Expression) []string { + var out []string + var walk func(ast.Expression) + walk = func(e ast.Expression) { + switch n := e.(type) { + case *ast.BinaryExpr: + if _, ok := oqlComparisonFixes[strings.ToLower(n.Operator)]; ok { + out = append(out, strings.ToLower(n.Operator)) + } + walk(n.Left) + walk(n.Right) + case *ast.UnaryExpr: + walk(n.Operand) + case *ast.ParenExpr: + walk(n.Inner) + case *ast.SourceExpr: + walk(n.Expression) + case *ast.FunctionCallExpr: + for _, a := range n.Arguments { + walk(a) + } + case *ast.IfThenElseExpr: + walk(n.Condition) + walk(n.ThenExpr) + walk(n.ElseExpr) + case *ast.XPathPathExpr: + for _, s := range n.Steps { + walk(s.Expr) + for _, p := range s.Predicates { + walk(p) + } + } + } + } + walk(expr) + return out +} + +// checkXPathOQLComparisons flags (MDL091) an OQL comparison in a retrieve +// constraint — MDL091 because it is the same mistake as `startsWith()` there: +// a construct from the language next door, CE0161 at build time. +func (v *microflowValidator) checkXPathOQLComparisons(variable string, where ast.Expression) { + for _, op := range oqlOnlyComparisons(where) { + v.addViolation("MDL091", linter.SeverityError, + fmt.Sprintf("retrieve '$%s' constraint uses `%s`, which is OQL — XPath does not have it, and mxbuild "+ + "reports CE0161 \"Error(s) in XPath constraint\"", variable, op), + oqlComparisonFixes[op]) + } +} + +// checkExpressionOQLComparisons is the same rule in a Mendix expression, where +// the build reports CE0117 instead. The visitor dropped these operators there +// too: `declare $b Boolean = $s like 'A%'` was stored as `$s`. +func (v *microflowValidator) checkExpressionOQLComparisons(label string, expr ast.Expression) { + for _, op := range oqlOnlyComparisons(expr) { + v.addViolation("MDL091", linter.SeverityError, + fmt.Sprintf("%s uses `%s`, which is OQL — a Mendix expression does not have it, and mxbuild "+ + "reports CE0117 \"Error(s) in expression\"", label, op), + oqlComparisonExpressionFixes[op]) + } +} diff --git a/mdl/executor/xpath_oql_comparison_test.go b/mdl/executor/xpath_oql_comparison_test.go new file mode 100644 index 000000000..cec0c2ecc --- /dev/null +++ b/mdl/executor/xpath_oql_comparison_test.go @@ -0,0 +1,112 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +func mdl091Messages(t *testing.T, src string) []string { + t.Helper() + prog, errs := visitor.Build(src) + if len(errs) > 0 { + t.Fatalf("parse errors: %v", errs) + } + var msgs []string + for _, vi := range ValidateMicroflow(prog.Statements[0].(*ast.CreateMicroflowStmt)) { + if vi.RuleID == "MDL091" { + msgs = append(msgs, vi.Message) + } + } + return msgs +} + +// mendixlabs/mxcli#1686: `like` in a retrieve constraint passed check and exec, +// then failed the build with CE0161 — the visitor had dropped the operator and +// stored `[Name]`. The siblings the shared OQL rule admits were dropped the same +// way (measured on mxbuild 11.12.2: `where N between 1 and 5` stored `[N]`). +func TestValidateMicroflow_XPathOQLComparison(t *testing.T) { + cases := []struct { + name string + where string + want string // the operator MDL091 must name, "" for none + }{ + {"like", "Name like 'A-%'", "like"}, + {"like beside a comparison", "N = 1 and Name like 'A-%'", "like"}, + {"not like", "Name not like 'A-%'", "not like"}, + {"between", "N between 1 and 5", "between"}, + {"in", "N in (1, 2)", "in"}, + // Controls: the XPath spellings of the same tests, measured clean. + {"starts-with", "starts-with(Name, 'A-')", ""}, + {"contains", "contains(Name, 'A')", ""}, + {"bounds", "N >= 1 and N <= 5", ""}, + {"like inside a literal is text", "Name = 'a like b'", ""}, + {"enum key", "Status = 'VALUE'", ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + msgs := mdl091Messages(t, "create microflow M.F ()\nreturns list of M.Item\nbegin\n retrieve $L from M.Item where "+tc.where+";\n return $L;\nend;") + if tc.want == "" { + if len(msgs) > 0 { + t.Fatalf("MDL091 fired on a valid constraint %q: %v", tc.where, msgs) + } + return + } + joined := strings.Join(msgs, "\n") + if !strings.Contains(joined, "`"+tc.want+"`") || !strings.Contains(joined, "CE0161") { + t.Fatalf("want MDL091 naming `%s` and CE0161 for %q, got %v", tc.want, tc.where, msgs) + } + }) + } +} + +// The expression side of the same drop: `$s like 'A%'` was stored as `$s`. +func TestValidateMicroflow_ExpressionOQLComparison(t *testing.T) { + bad := mdl091Messages(t, "create microflow M.F ($s: String) returns Boolean\nbegin\n declare $b Boolean = $s like 'A%';\n return $b;\nend;") + if len(bad) != 1 || !strings.Contains(bad[0], "CE0117") || !strings.Contains(bad[0], "`like`") { + t.Fatalf("want one MDL091 naming `like` and CE0117, got %v", bad) + } + if ok := mdl091Messages(t, "create microflow M.F ($s: String) returns Boolean\nbegin\n declare $b Boolean = startsWith($s, 'A');\n return $b;\nend;"); len(ok) > 0 { + t.Fatalf("MDL091 fired on startsWith(): %v", ok) + } +} + +// The visitor keeps the comparison as written instead of dropping everything +// after the left operand. +func TestBuildExpression_KeepsOQLComparison(t *testing.T) { + prog, errs := visitor.Build("create microflow M.F ()\nreturns list of M.Item\nbegin\n retrieve $L from M.Item where Name like 'A-%';\n return $L;\nend;") + if len(errs) > 0 { + t.Fatalf("parse errors: %v", errs) + } + mf := prog.Statements[0].(*ast.CreateMicroflowStmt) + ret := mf.Body[0].(*ast.RetrieveStmt) + if got := expressionToXPath(ret.Where); got != "Name like 'A-%'" { + t.Fatalf("constraint serialized as %q, want the comparison as written", got) + } +} + +// check and exec must agree: the report had both accepting the constraint. +func TestCheckExecAgree_RetrieveConstraintLike(t *testing.T) { + const head = `mdl 1; +create module ModG; +create persistent entity ModG.Item (Name: String(50)); +create microflow ModG.SUB_Like () +begin + retrieve $L from ModG.Item where %s; +end;` + exec, _, dir := openPedAppCopy(t) + bad := strings.Replace(head, "%s", "Name like 'A-%'", 1) + got := strings.Join(agreeCheck(t, exec, dir, bad), "\n") + if !strings.Contains(got, "MDL091") || !strings.Contains(got, "`like`") { + t.Errorf("check -p must report MDL091 for `like`, reported:\n%s", got) + } + if err := agreeExec(t, exec, bad); err == nil || !strings.Contains(err.Error(), "MDL091") { + t.Errorf("exec must refuse `like` with MDL091, got: %v", err) + } + exec2, _, dir2 := openPedAppCopy(t) + assertAgree(t, exec2, dir2, strings.Replace(head, "%s", "starts-with(Name, 'A-')", 1), "") +} diff --git a/mdl/visitor/visitor_microflow_expression.go b/mdl/visitor/visitor_microflow_expression.go index f5da92ee8..fe73419f5 100644 --- a/mdl/visitor/visitor_microflow_expression.go +++ b/mdl/visitor/visitor_microflow_expression.go @@ -144,9 +144,51 @@ func buildComparisonExpression(ctx parser.IComparisonExpressionContext) ast.Expr } } + // LIKE / BETWEEN / IN / MATCH are OQL; the rule admits them because the + // expression grammar is shared with OQL, but neither a Mendix expression nor + // an XPath constraint has them. Returning only `left` here dropped the + // operator and its operands without a word: `where Name like 'A-%'` was + // stored as `[Name]` and failed the build with CE0161 while check passed + // (mendixlabs/mxcli#1686). Keep the operator so the check can refuse it — + // see oqlOnlyComparisons. The comparison is kept as written: Right is the + // first operand after the operator (the pattern, or BETWEEN's low bound) and + // nil for IN, so the source is what any writer stores. + if op := oqlOnlyComparisonOperator(compCtx); op != "" { + var right ast.Expression + if len(addExprs) >= 2 { + right = buildAdditiveExpression(addExprs[1]) + } + var cmp ast.Expression = &ast.BinaryExpr{Left: left, Operator: op, Right: right} + if src := strings.TrimSpace(extractExpressionText(compCtx)); src != "" { + cmp = &ast.SourceExpr{Expression: cmp, Source: src} + } + return cmp + } + return left } +// oqlOnlyComparisonOperator names the OQL comparison a comparisonExpression +// carries — "like", "not like", "between", "not between", "in" or "match" — +// or "" when it carries none of them. +func oqlOnlyComparisonOperator(c *parser.ComparisonExpressionContext) string { + not := "" + if c.NOT() != nil { + not = "not " + } + switch { + case c.LIKE() != nil: + return not + "like" + case c.BETWEEN() != nil: + return not + "between" + case c.IN() != nil: + return "in" + case c.MATCH() != nil: + return "match" + } + return "" +} + // buildAdditiveExpression handles + and - expressions. func buildAdditiveExpression(ctx parser.IAdditiveExpressionContext) ast.Expression { if ctx == nil { From 69cffcc49e03bbedcd2bfd6e204f814b86f598fd Mon Sep 17 00:00:00 2001 From: Ako Date: Mon, 2 Nov 2026 16:21:11 +0000 Subject: [PATCH 13/17] fix(describe): print a pop-up page's width and height when they are 0 (#1881) describe page suppressed PopupWidth/PopupHeight at 0, Studio Pro's auto-size, so a reviewer reading DESCRIBE could not tell an auto-size pop-up from one whose size was never set (mendixlabs/mxcli#1713). On a page whose layout is a ModalPopup layout both are now printed, 0 included; other pages still print only a non-zero size. The layout is resolved by FormCall.Form's qualified name: Studio Pro pages carry no FormCall.Layout ID, so keying on it did nothing on a real page. Verified on the Studio Pro-authored testapp page Administration.ChangePasswordForm (Atlas_Core.PopupLayout, 0/0), including describe -> exec -> describe as a fixed point. Co-Authored-By: Claude Opus 5.5 --- ...ribe-page-omits-zero-popup-dimensions.json | 1 + .claude/skills/mendix/alter-page/SKILL.md | 4 +- .claude/skills/mendix/create-page/SKILL.md | 22 +++--- CHANGELOG.md | 1 + mdl/executor/cmd_pages_describe.go | 55 ++++++++++++--- .../describe_page_popup_dimensions_test.go | 68 +++++++++++++++++++ 6 files changed, 130 insertions(+), 21 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-11-02-describe-page-omits-zero-popup-dimensions.json create mode 100644 mdl/executor/describe_page_popup_dimensions_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-11-02-describe-page-omits-zero-popup-dimensions.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-11-02-describe-page-omits-zero-popup-dimensions.json new file mode 100644 index 000000000..006d0d993 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-11-02-describe-page-omits-zero-popup-dimensions.json @@ -0,0 +1 @@ +{"area": "mdl/executor", "date": "2026-11-02", "symptom": "`describe page` of a pop-up page (Atlas_Core.PopupLayout) printed neither `PopupWidth:` nor `PopupHeight:` when both were 0, so an auto-size pop-up read the same as one whose size was never set; a review gate built on DESCRIBE passed unconfigured pages", "cause": "#713 made DESCRIBE suppress the default 0 everywhere, which is lossless for exec but erases a value a reviewer reads; and the obvious way to know the layout (FormCall.Layout, a binary ID) is absent on Studio Pro pages, which store FormCall.Form as a qualified name", "file": "mdl/executor/cmd_pages_describe.go (popupDimensionProps, layoutTypeByName)", "insight": "Print the dimensions, 0 included, only where they mean something: a page whose layout's LayoutType is ModalPopup; elsewhere keep suppressing the default. Resolve the layout by FormCall.Form's qualified name, not by ID: the first fix keyed on FormCall.Layout passed review-by-reading and did nothing on a real page, caught only by describing a Studio Pro-authored page (testapp Administration.ChangePasswordForm, 0/0 on PopupLayout). Field names PopupWidth/PopupHeight measured on testapp pages, present on every page, popup or not.", "refs": ["mendixlabs/mxcli#1713", "ako/mxcli#1881", "mdl/executor/describe_page_popup_dimensions_test.go"]} diff --git a/.claude/skills/mendix/alter-page/SKILL.md b/.claude/skills/mendix/alter-page/SKILL.md index e16b4afe8..3823fd35d 100644 --- a/.claude/skills/mendix/alter-page/SKILL.md +++ b/.claude/skills/mendix/alter-page/SKILL.md @@ -227,8 +227,8 @@ so a silent write would build cleanly and then fail to open. | `Title` | Page-level only (case-sensitive) | String | `set (Title: 'Edit Customer')` | | `Documentation` | Page-level only (case-sensitive) | String (`''` clears) | `set (Documentation: 'Coordinator triage step.')` | | `layout` | Page-level only | Qualified name | `set layout = Atlas_Core.Atlas_Default` | -| `PopupWidth` | Page-level only (case-sensitive) | Positive integer (pixels) | `set (PopupWidth: 800)` | -| `PopupHeight` | Page-level only (case-sensitive) | Positive integer (pixels) | `set (PopupHeight: 480)` | +| `PopupWidth` | Page-level only (case-sensitive) | Integer ≥ 0 (pixels; 0 = auto-size) | `set (PopupWidth: 800)` | +| `PopupHeight` | Page-level only (case-sensitive) | Integer ≥ 0 (pixels; 0 = auto-size) | `set (PopupHeight: 480)` | | `PopupResizable` | Page-level only (case-sensitive) | Boolean | `set (PopupResizable: true)` | | `Class` | Page-level (case-sensitive, no ON) | CSS class string | `set (Class: 'container-fluid bg-light')` | | `Style` | Page-level (case-sensitive, no ON) | Inline CSS string | `set (Style: 'min-height: 100vh')` | diff --git a/.claude/skills/mendix/create-page/SKILL.md b/.claude/skills/mendix/create-page/SKILL.md index a50eff181..ed7fc7798 100644 --- a/.claude/skills/mendix/create-page/SKILL.md +++ b/.claude/skills/mendix/create-page/SKILL.md @@ -12,15 +12,15 @@ description: "CREATE PAGE syntax reference — parameters, variables, layouts, a |---|---| | 28-43 | Reference files | | 44-46 | Overview | -| 47-220 | Syntax | -| 221-291 | Basic Examples | -| 292-316 | Modifying Existing Pages | -| 317-420 | Conditional Visibility and Editability | -| 421-585 | Known Limitations | -| 586-594 | Tips | -| 595-621 | Related Commands | -| 622-646 | PLUGGABLEWIDGET Escape Hatch | -| 647-650 | See Also | +| 47-222 | Syntax | +| 223-293 | Basic Examples | +| 294-318 | Modifying Existing Pages | +| 319-422 | Conditional Visibility and Editability | +| 423-587 | Known Limitations | +| 588-596 | Tips | +| 597-623 | Related Commands | +| 624-648 | PLUGGABLEWIDGET Escape Hatch | +| 649-652 | See Also | # CREATE PAGE - MDL Syntax Guide @@ -73,7 +73,9 @@ pages. **Pop-up dimensions** (`PopupWidth` / `PopupHeight` / `PopupResizable`) apply when the page is opened in a pop-up. They are optional — omitting them uses the Mendix defaults -(600 × 600, not resizable). Unlike the other header keywords, these property names are +(0 × 0, which is auto-size, and not resizable). `describe page` prints the width and +height of a page on a pop-up layout even when they are 0, so a reviewer can tell an +auto-size pop-up from one whose size was never checked. Unlike the other header keywords, these property names are **case-sensitive** and must be written exactly as shown. They can also be changed later with `alter page … { set (PopupWidth: …); }` (see the alter-page skill). diff --git a/CHANGELOG.md b/CHANGELOG.md index bfe78c19b..53eb3ff74 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **`describe page` prints a pop-up page's width and height when they are 0** (ako/mxcli#1881, mendixlabs/mxcli#1713) — on a page whose layout is a pop-up (`ModalPopup`, e.g. `Atlas_Core.PopupLayout`), `PopupWidth: 0` and `PopupHeight: 0` (auto-size) were left out, so a reviewer reading DESCRIBE could not tell an auto-size pop-up from one never configured. They are printed now and round-trip through `create or modify page`. Other pages still print only a non-zero size. - **`describe microflow` prints the concurrency error message in the project's default language** (ako/mxcli#1869, mendixlabs/mxcli#1711) — on an `en_US` project with `de_DE` enabled it printed the German text, and a drop and create from that output stored it under `en_US` too, so the English message was gone while `check`, `exec` and `mx check` stayed clean. It now picks the language the way every other described text does: the project default, then `en_US`, then the first non-empty one. - **The catalog's XPath rows name their widget** (ako/mxcli#1879) — `xpath_expressions.ComponentId` for a page or snippet data source was a Go byte dump (`{0 [57 221 …]}`) and nothing tied the row to its widget. It is now a GUID, and the new `WidgetId` / `WidgetName` columns (`widget_id` / `widget_name` in `xpath_expressions()`) name the widget by the id `widgets` gives it. The catalog schema moves to version 32, so a cached catalog is rebuilt once. - **`create or replace page` can clear a data grid column caption** (ako/mxcli#1877) — `Caption: ''` over a stored caption reported "Unchanged page" and kept it, because the translation carry added the stored text back to an empty header written with no translation. The header is now written as one empty translation, which the carry leaves alone; an empty translation where the stored text has none is dropped, so a Studio Pro empty caption still rewrites as unchanged. This also repairs pages that still carry a leaked column name (#535). diff --git a/mdl/executor/cmd_pages_describe.go b/mdl/executor/cmd_pages_describe.go index a618be018..5ef9207e8 100644 --- a/mdl/executor/cmd_pages_describe.go +++ b/mdl/executor/cmd_pages_describe.go @@ -81,6 +81,8 @@ func describePage(ctx *ExecContext, name ast.QualifiedName) error { } } + popupLayout := layoutTypeByName(ctx, layoutName) == pages.LayoutTypeModalPopup + // @excluded annotation if foundPage.Excluded { fmt.Fprintln(ctx.Output, "@excluded") @@ -102,16 +104,8 @@ func describePage(ctx *ExecContext, name ast.QualifiedName) error { if foundPage.URL != "" { props = append(props, fmt.Sprintf("Url: %s", mdlQuote(ctx, foundPage.URL))) } - // Pop-up dimensions (issues #661, #713) — emit only non-default values so - // the CREATE PAGE header round-trips. Studio Pro's default is 0/0 (auto-size); - // an explicit non-zero value (e.g. 600) is emitted so it round-trips. if rawData != nil { - if w := toInt(rawData["PopupWidth"]); w != 0 { - props = append(props, fmt.Sprintf("PopupWidth: %d", w)) - } - if hgt := toInt(rawData["PopupHeight"]); hgt != 0 { - props = append(props, fmt.Sprintf("PopupHeight: %d", hgt)) - } + props = append(props, popupDimensionProps(rawData, popupLayout)...) if r, ok := rawData["PopupResizable"].(bool); ok && r { props = append(props, "PopupResizable: true") } @@ -637,6 +631,49 @@ func getBsonArrayMaps(v any) []map[string]any { } } +// popupDimensionProps renders a page's pop-up width and height for the CREATE +// PAGE header (issues #661, #713). On a pop-up layout they are always printed, +// 0 included: 0 is Studio Pro's auto-size, and leaving it out made a page set +// to auto-size read the same as one never looked at (mendixlabs/mxcli#1713). +// Elsewhere the dimensions do nothing, so only a non-default value is printed. +func popupDimensionProps(rawData map[string]any, popupLayout bool) []string { + var props []string + if w := toInt(rawData["PopupWidth"]); w != 0 || popupLayout { + props = append(props, fmt.Sprintf("PopupWidth: %d", w)) + } + if h := toInt(rawData["PopupHeight"]); h != 0 || popupLayout { + props = append(props, fmt.Sprintf("PopupHeight: %d", h)) + } + return props +} + +// layoutTypeByName returns the type of the layout with this qualified name, +// or "" when it cannot be read. A page stores its layout as FormCall.Form, a +// qualified name, so the layout is found by name rather than by ID. +func layoutTypeByName(ctx *ExecContext, qualifiedName string) pages.LayoutType { + if qualifiedName == "" { + return "" + } + layouts, err := layoutHeaders(ctx.Backend) + if err != nil { + return "" + } + h, err := getHierarchy(ctx) + if err != nil { + return "" + } + for _, l := range layouts { + if h.GetQualifiedName(l.ContainerID, l.Name) != qualifiedName { + continue + } + if layout, err := ctx.Backend.GetLayout(l.ID); err == nil && layout != nil { + return layout.LayoutType + } + return "" + } + return "" +} + // resolveLayoutName resolves a layout ID to its qualified name. func resolveLayoutName(ctx *ExecContext, layoutID model.ID) string { layouts, err := layoutHeaders(ctx.Backend) diff --git a/mdl/executor/describe_page_popup_dimensions_test.go b/mdl/executor/describe_page_popup_dimensions_test.go new file mode 100644 index 000000000..7d9d18722 --- /dev/null +++ b/mdl/executor/describe_page_popup_dimensions_test.go @@ -0,0 +1,68 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +// DESCRIBE PAGE left out a pop-up page's width and height when they were 0, +// Studio Pro's auto-size, so a page set to auto-size read the same as one +// whose size was never looked at (mendixlabs/mxcli#1713). The pages are +// Studio Pro-authored: Administration.ChangePasswordForm sits on +// Atlas_Core.PopupLayout (ModalPopup) and stores PopupWidth/PopupHeight 0/0. +func TestDescribePage_PopupLayoutPrintsZeroDimensions(t *testing.T) { + x, out := openTestAppCopy(t) + describe := func(name string) string { + t.Helper() + out.Reset() + if err := x.Execute(&ast.DescribeStmt{ObjectType: ast.DescribePage, Name: ast.QualifiedName{Module: "Administration", Name: name}}); err != nil { + t.Fatalf("describe %s: %v", name, err) + } + return out.String() + } + + popup := describe("ChangePasswordForm") + if !strings.Contains(popup, "Layout: Atlas_Core.PopupLayout") { + t.Fatalf("fixture moved: ChangePasswordForm is no longer on the pop-up layout:\n%s", popup) + } + for _, want := range []string{"PopupWidth: 0", "PopupHeight: 0"} { + if !strings.Contains(popup, want) { + t.Errorf("describe of a pop-up page does not print %q (#1713):\n%s", want, popup) + } + } + + // And they round-trip: executing the description changes nothing. + if err := x.ExecuteProgram(reparse(t, popup)); err != nil { + t.Fatalf("exec of the description: %v\n%s", err, popup) + } + if again := describe("ChangePasswordForm"); again != popup { + t.Errorf("describe -> exec -> describe is not a fixed point:\n--- first ---\n%s\n--- second ---\n%s", popup, again) + } + + // Control: on a page that does not open as a pop-up the dimensions do + // nothing, and the default stays unprinted. + if page := describe("Account_Overview"); strings.Contains(page, "PopupWidth") || strings.Contains(page, "PopupHeight") { + t.Errorf("describe of a non-pop-up page prints its default pop-up size:\n%s", page) + } +} + +func TestPopupDimensionProps(t *testing.T) { + for _, tc := range []struct { + raw map[string]any + popup bool + want string + }{ + {map[string]any{"PopupWidth": int32(0), "PopupHeight": int32(0)}, true, "PopupWidth: 0|PopupHeight: 0"}, + {map[string]any{"PopupWidth": int32(0), "PopupHeight": int32(0)}, false, ""}, + {map[string]any{"PopupWidth": int32(1000), "PopupHeight": int32(0)}, false, "PopupWidth: 1000"}, + {map[string]any{"PopupWidth": int32(1200), "PopupHeight": int32(480)}, true, "PopupWidth: 1200|PopupHeight: 480"}, + } { + if got := strings.Join(popupDimensionProps(tc.raw, tc.popup), "|"); got != tc.want { + t.Errorf("popupDimensionProps(%v, popup=%v) = %q, want %q", tc.raw, tc.popup, got, tc.want) + } + } +} From 8cb4f97e109bbe0b2a702c1e04095d66f77121d8 Mon Sep 17 00:00:00 2001 From: Ako Date: Mon, 2 Nov 2026 16:27:22 +0000 Subject: [PATCH 14/17] fix(diff-local): list untracked units as new against the working copy (#1867) `git diff --name-status ` lists tracked files only, so units added under mprcontents/ without `git add` -- a new module's pages and flows -- were missing from diff-local and its "new" count (mendixlabs/mxcli#1709). In single-ref mode diff-local now also asks `git ls-files --others --exclude-standard` and reports each untracked .mxunit as new. A range compares two commits and is unchanged. Co-Authored-By: Claude Opus 5.5 --- ...02-diff-local-untracked-units-not-new.json | 1 + CHANGELOG.md | 1 + mdl/executor/cmd_diff_local.go | 16 +++++ .../cmd_diff_local_unrendered_test.go | 3 + mdl/executor/cmd_diff_local_untracked_test.go | 62 +++++++++++++++++++ 5 files changed, 83 insertions(+) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-11-02-diff-local-untracked-units-not-new.json create mode 100644 mdl/executor/cmd_diff_local_untracked_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-11-02-diff-local-untracked-units-not-new.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-11-02-diff-local-untracked-units-not-new.json new file mode 100644 index 000000000..d8e91ec87 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-11-02-diff-local-untracked-units-not-new.json @@ -0,0 +1 @@ +{"area": "mdl/executor", "date": "2026-11-02", "symptom": "`diff-local --ref HEAD` (MPR v2) reported `0 new, 3 modified` while dozens of untracked .mxunit files sat under mprcontents/ — a new module's pages and flows, never `git add`ed, were missing", "cause": "findChangedMxunitFiles asked only `git diff --name-status `, which lists tracked files; an untracked file is invisible to it", "file": "mdl/executor/cmd_diff_local.go (findChangedMxunitFiles: `git ls-files --others --exclude-standard` in single-ref mode)", "insight": "Against the working tree the question is 'what is in the folder that is not at ref', and git splits the answer between diff (tracked) and ls-files --others (untracked). Ask both; a range compares two commits and must not look at the working tree. Stubbed-git tests must answer the new command, or they fail with exit 128. Real-git test: commit a unit, write a second one untracked, diff HEAD → want 1 new.", "refs": ["mendixlabs/mxcli#1709", "ako/mxcli#1867", "mdl/executor/cmd_diff_local_untracked_test.go"]} diff --git a/CHANGELOG.md b/CHANGELOG.md index bfe78c19b..35f1c161e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **`diff-local` against the working copy lists untracked units as new** (ako/mxcli#1867, mendixlabs/mxcli#1709) — documents added under `mprcontents/` but not yet `git add`ed, such as a new module's pages and flows, were missing from the diff and from the `new` count. A range (`--ref a..b`) still compares only the two commits. - **`describe microflow` prints the concurrency error message in the project's default language** (ako/mxcli#1869, mendixlabs/mxcli#1711) — on an `en_US` project with `de_DE` enabled it printed the German text, and a drop and create from that output stored it under `en_US` too, so the English message was gone while `check`, `exec` and `mx check` stayed clean. It now picks the language the way every other described text does: the project default, then `en_US`, then the first non-empty one. - **The catalog's XPath rows name their widget** (ako/mxcli#1879) — `xpath_expressions.ComponentId` for a page or snippet data source was a Go byte dump (`{0 [57 221 …]}`) and nothing tied the row to its widget. It is now a GUID, and the new `WidgetId` / `WidgetName` columns (`widget_id` / `widget_name` in `xpath_expressions()`) name the widget by the id `widgets` gives it. The catalog schema moves to version 32, so a cached catalog is rebuilt once. - **`create or replace page` can clear a data grid column caption** (ako/mxcli#1877) — `Caption: ''` over a stored caption reported "Unchanged page" and kept it, because the translation carry added the stored text back to an empty header written with no translation. The header is now written as one empty translation, which the carry leaves alone; an empty translation where the stored text has none is dropped, so a Studio Pro empty caption still rewrites as unchanged. This also repairs pages that still carry a leaked column name (#535). diff --git a/mdl/executor/cmd_diff_local.go b/mdl/executor/cmd_diff_local.go index 04642b0c8..549402458 100644 --- a/mdl/executor/cmd_diff_local.go +++ b/mdl/executor/cmd_diff_local.go @@ -178,6 +178,22 @@ func findChangedMxunitFiles(_ *ExecContext, projectDir, contentsName, ref string }) } + // Against the working tree, a unit git has not been told about is still a + // new document: `git diff ` lists tracked files only, so a module + // added without `git add` was missing from "new" (mendixlabs/mxcli#1709). + // A range compares two commits and has no working tree to look at. + if _, _, isRange, _ := parseRefRange(ref); !isRange { + untracked, err := gitInProject(projectDir, "ls-files", "--others", "--exclude-standard", "--", contentsName).Output() + if err != nil { + return nil, mdlerrors.NewBackend("git ls-files", err) + } + for line := range strings.SplitSeq(string(untracked), "\n") { + if filePath := strings.TrimSpace(line); strings.HasSuffix(filePath, ".mxunit") { + changes = append(changes, gitChange{Status: "A", FilePath: filePath}) + } + } + } + return changes, nil } diff --git a/mdl/executor/cmd_diff_local_unrendered_test.go b/mdl/executor/cmd_diff_local_unrendered_test.go index 6fbbaef4f..90272274f 100644 --- a/mdl/executor/cmd_diff_local_unrendered_test.go +++ b/mdl/executor/cmd_diff_local_unrendered_test.go @@ -93,6 +93,9 @@ func diffLocalAfter(t *testing.T, setup, change string, opts DiffOptions) string if len(args) > 0 && args[0] == "diff" { return exec.Command("cat", listing) } + if len(args) > 0 && args[0] == "ls-files" { // the listing already has the new units + return exec.Command("true") + } if len(args) == 2 && args[0] == "show" { if f, ok := files[args[1]]; ok { return exec.Command("cat", f) diff --git a/mdl/executor/cmd_diff_local_untracked_test.go b/mdl/executor/cmd_diff_local_untracked_test.go new file mode 100644 index 000000000..c9aa2430f --- /dev/null +++ b/mdl/executor/cmd_diff_local_untracked_test.go @@ -0,0 +1,62 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// A unit the working copy has added but git has not been told about is still a +// new document. `git diff ` lists tracked files only, so diff-local +// against the working tree reported "0 new" while new modules, pages and flows +// sat untracked under mprcontents/ (mendixlabs/mxcli#1709). +func TestDiffLocal_UntrackedUnitIsNew(t *testing.T) { + root, proj := gitProjectRepo(t) + gitIn(t, root, "checkout", "-q", "--", ".") // drop gitProjectRepo's tracked edit + const added = "7c7c7c7c-1111-2222-3333-444444444444" + src := filepath.Join(proj, "mprcontents", "5a", "5a", gitUnit+".mxunit") + data, err := os.ReadFile(src) + if err != nil { + t.Fatal(err) + } + dst := filepath.Join(proj, "mprcontents", "7c", "7c", added+".mxunit") + if err := os.MkdirAll(filepath.Dir(dst), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(dst, data, 0o644); err != nil { + t.Fatal(err) + } + + ctx, out := gitDiffContext(filepath.Join(proj, "mprcontents")) + if err := diffLocal(ctx, "HEAD", DiffOptions{}); err != nil { + t.Fatal(err) + } + if got := out.String(); !strings.Contains(got, "Summary: 1 new, 0 modified, 0 deleted") { + t.Errorf("an untracked unit is not reported as new (#1709):\n%s", got) + } +} + +// A range compares two commits, so the working tree's untracked files are no +// part of it. +func TestDiffLocal_RangeIgnoresUntrackedUnits(t *testing.T) { + root, proj := gitProjectRepo(t) + gitIn(t, root, "commit", "-q", "-am", "after") + dst := filepath.Join(proj, "mprcontents", "7c", "7c", "7c7c7c7c-1111-2222-3333-444444444444.mxunit") + if err := os.MkdirAll(filepath.Dir(dst), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(dst, []byte("not bson"), 0o644); err != nil { + t.Fatal(err) + } + + ctx, out := gitDiffContext(filepath.Join(proj, "mprcontents")) + if err := diffLocal(ctx, "HEAD~1..HEAD", DiffOptions{}); err != nil { + t.Fatal(err) + } + if got := out.String(); !strings.Contains(got, "Summary: 0 new, 1 modified, 0 deleted") { + t.Errorf("a range picked up an untracked working-tree unit:\n%s", got) + } +} From ad2e2cb556e22ad3f3f43e7d04a4af26a28b8bdd Mon Sep 17 00:00:00 2001 From: Ako Date: Mon, 2 Nov 2026 16:29:15 +0000 Subject: [PATCH 15/17] fix(check): refuse an unparenthesised if-expression as an operand (#1868) `declare $L String = 'x' + if $c then 'a' else 'b';` passed check and exec, then mx check failed with CE0117: Mendix's expression grammar takes an if-expression only where a whole expression goes, MDL's anywhere. check now reports MDL-IFOPERAND01, exec-enforced, for an IfThenElseExpr that is the direct operand of a binary operator or a unary minus, in microflows and nanoflows alike (the shared per-expression hook), and suggests the parenthesised form. Measured on mxbuild 11.12.2, one microflow per shape: `+ if`, `and if`, `= if`, `-if` and the same in a return are CE0117; the parenthesised form, an if whose else branch takes the `+`, an if as a function or not() argument and an if as an else branch build clean. Mirror of mendixlabs/mxcli#1710. Co-Authored-By: Claude Opus 5.5 --- ...11-02-if-expression-as-operand-ce0117.json | 1 + CHANGELOG.md | 1 + mdl/executor/if_operand_test.go | 99 +++++++++++++++++++ mdl/executor/validate.go | 3 + mdl/executor/validate_if_operand.go | 94 ++++++++++++++++++ mdl/executor/validate_microflow.go | 1 + 6 files changed, 199 insertions(+) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-11-02-if-expression-as-operand-ce0117.json create mode 100644 mdl/executor/if_operand_test.go create mode 100644 mdl/executor/validate_if_operand.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-11-02-if-expression-as-operand-ce0117.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-11-02-if-expression-as-operand-ce0117.json new file mode 100644 index 000000000..19e7b86c8 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-11-02-if-expression-as-operand-ce0117.json @@ -0,0 +1 @@ +{"area": "mdl/executor", "date": "2026-11-02", "symptom": "ako/mxcli#1868 (mendixlabs/mxcli#1710, 11.12.4): `declare $L String = 'x' + if $c then 'a' else 'b';` passed check (\"Syntax OK\", \"Check passed!\") and exec, and mx check failed CE0117", "cause": "MDL's expression grammar admits an if-expression at any primary position, Mendix's only where a whole expression goes; the writer stores the source as written, so the unparenthesised operand reached the model. No rule compared the two grammars", "file": "mdl/executor/validate_if_operand.go (ifOperands, checkIfOperand, called from checkExprFunctions so microflows and nanoflows share it), mdl/executor/validate.go (execEnforcedMicroflowRules)", "fix": "MDL-IFOPERAND01 error, exec-enforced: an IfThenElseExpr as a direct operand of a binary operator or a unary minus, with the parenthesised form as the fix", "test": "mdl/executor/if_operand_test.go — +, and, =, unary minus, return, nanoflow, check/exec agreement; controls parenthesised, else-branch absorbing the +, function argument, not() argument, if as else branch. Revert (hook call commented out): the five bad shapes and the nanoflow and agreement tests failed. Measured on mxbuild 11.12.2, one microflow per shape and mx check -j to attribute each error: every bad shape CE0117, every control clean", "insight": "mx check prints the element (\"Create variable activity 'Create Boolean variable'\"), not the microflow, so a one-microflow-per-shape matrix is unattributable from the console; `mx check -j out.json` carries document-name per error. The boundary was not the operator but the position: `if … else 'b' + 'x'` builds because the else branch takes the `+`, so a rule on `+ if` in the source text would have been right by accident and wrong for `-if`", "refs": ["ako/mxcli#1868", "mendixlabs/mxcli#1710"]} diff --git a/CHANGELOG.md b/CHANGELOG.md index bfe78c19b..187e4c597 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **An `if … then … else …` used as an operand without parentheses is refused** (ako/mxcli#1868, mendixlabs/mxcli#1710) — `'x' + if $c then 'a' else 'b'` passed `check` and `exec`, and the build failed with CE0117: Mendix accepts an if-expression only where a whole expression goes. `check` and `exec` now report **MDL-IFOPERAND01** for an if-expression as the operand of `+`, `and`, `=` or any other operator, or of a unary minus, and suggest the parenthesised form `'x' + (if $c then 'a' else 'b')`, which builds clean. - **`describe microflow` prints the concurrency error message in the project's default language** (ako/mxcli#1869, mendixlabs/mxcli#1711) — on an `en_US` project with `de_DE` enabled it printed the German text, and a drop and create from that output stored it under `en_US` too, so the English message was gone while `check`, `exec` and `mx check` stayed clean. It now picks the language the way every other described text does: the project default, then `en_US`, then the first non-empty one. - **The catalog's XPath rows name their widget** (ako/mxcli#1879) — `xpath_expressions.ComponentId` for a page or snippet data source was a Go byte dump (`{0 [57 221 …]}`) and nothing tied the row to its widget. It is now a GUID, and the new `WidgetId` / `WidgetName` columns (`widget_id` / `widget_name` in `xpath_expressions()`) name the widget by the id `widgets` gives it. The catalog schema moves to version 32, so a cached catalog is rebuilt once. - **`create or replace page` can clear a data grid column caption** (ako/mxcli#1877) — `Caption: ''` over a stored caption reported "Unchanged page" and kept it, because the translation carry added the stored text back to an empty header written with no translation. The header is now written as one empty translation, which the carry leaves alone; an empty translation where the stored text has none is dropped, so a Studio Pro empty caption still rewrites as unchanged. This also repairs pages that still carry a leaked column name (#535). diff --git a/mdl/executor/if_operand_test.go b/mdl/executor/if_operand_test.go new file mode 100644 index 000000000..2e080df7b --- /dev/null +++ b/mdl/executor/if_operand_test.go @@ -0,0 +1,99 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +func ifOperandMessages(t *testing.T, src string) []string { + t.Helper() + prog, errs := visitor.Build(src) + if len(errs) > 0 { + t.Fatalf("parse errors: %v", errs) + } + var msgs []string + switch s := prog.Statements[0].(type) { + case *ast.CreateMicroflowStmt: + for _, vi := range ValidateMicroflow(s) { + if vi.RuleID == ifOperandRule { + msgs = append(msgs, vi.Message) + } + } + case *ast.CreateNanoflowStmt: + for _, vi := range ValidateNanoflow(s) { + if vi.RuleID == ifOperandRule { + msgs = append(msgs, vi.Message) + } + } + } + return msgs +} + +// mendixlabs/mxcli#1710: `'x' + if $c then 'a' else 'b'` passed check and exec +// and failed the build with CE0117. Every row was measured on mxbuild 11.12.2. +func TestValidateMicroflow_IfOperand(t *testing.T) { + cases := []struct { + name string + body string + want bool + }{ + {"right of +", "declare $L String = 'x' + if $c then 'a' else 'b';\n return true;", true}, + {"right of and", "declare $L Boolean = $c and if $c then true else false;\n return $L;", true}, + {"right of =", "declare $L Boolean = $c = if $c then true else false;\n return $L;", true}, + {"unary minus", "declare $L Integer = -if $c then 1 else 2;\n return true;", true}, + {"in a return", "return 'x' + if $c then 'a' else 'b' = 'xa';", true}, + // Controls, measured clean. + {"parenthesised", "declare $L String = 'x' + (if $c then 'a' else 'b');\n return true;", false}, + {"else takes the +", "declare $L String = if $c then 'a' else 'b' + 'x';\n return true;", false}, + {"function argument", "declare $L String = toUpperCase(if $c then 'a' else 'b');\n return true;", false}, + {"not() argument", "declare $L Boolean = not(if $c then true else false);\n return $L;", false}, + {"if in the else branch", "declare $L String = if $c then 'a' else if $c then 'b' else 'c';\n return true;", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + msgs := ifOperandMessages(t, "create microflow M.F ($c: Boolean) returns Boolean\nbegin\n "+tc.body+"\nend;") + if got := len(msgs) > 0; got != tc.want { + t.Fatalf("%s fired=%v, want %v: %v", ifOperandRule, got, tc.want, msgs) + } + if tc.want && !strings.Contains(strings.Join(msgs, "\n"), "CE0117") { + t.Errorf("message must name CE0117: %v", msgs) + } + }) + } +} + +// Nanoflows share the per-expression hook. +func TestValidateNanoflow_IfOperand(t *testing.T) { + msgs := ifOperandMessages(t, "create nanoflow M.N ($c: Boolean) returns String\nbegin\n declare $L String = 'x' + if $c then 'a' else 'b';\n return $L;\nend;") + if len(msgs) != 1 { + t.Fatalf("want one %s in a nanoflow, got %v", ifOperandRule, msgs) + } +} + +// check and exec must agree: the report had both accepting the expression. +func TestCheckExecAgree_IfOperand(t *testing.T) { + const head = `mdl 1; +create module ModG; +create microflow ModG.SUB_IfOperand ($c: Boolean) +returns String +begin + declare $L String = %s; + return $L; +end;` + exec, _, dir := openPedAppCopy(t) + bad := strings.Replace(head, "%s", "'x' + if $c then 'a' else 'b'", 1) + got := strings.Join(agreeCheck(t, exec, dir, bad), "\n") + if !strings.Contains(got, ifOperandRule) { + t.Errorf("check -p must report %s, reported:\n%s", ifOperandRule, got) + } + if err := agreeExec(t, exec, bad); err == nil || !strings.Contains(err.Error(), ifOperandRule) { + t.Errorf("exec must refuse with %s, got: %v", ifOperandRule, err) + } + exec2, _, dir2 := openPedAppCopy(t) + assertAgree(t, exec2, dir2, strings.Replace(head, "%s", "'x' + (if $c then 'a' else 'b')", 1), "") +} diff --git a/mdl/executor/validate.go b/mdl/executor/validate.go index 66d8fb7e0..24a1329ee 100644 --- a/mdl/executor/validate.go +++ b/mdl/executor/validate.go @@ -2132,6 +2132,9 @@ var execEnforcedMicroflowRules = map[string]bool{ // System.changedBy is CE1050 "Unknown association", measured on 11.12.5 // for every entity shape, microflow and nanoflow (mendixlabs/mxcli#1612). "MDL-CHANGE01": true, + // MDL-IFOPERAND01: an if-expression as an operator's operand without + // parentheses is CE0117, measured on 11.12.2 (mendixlabs/mxcli#1710). + "MDL-IFOPERAND01": true, } // validateMicroflowRules runs the MDL0xx microflow rule set (ValidateMicroflow) diff --git a/mdl/executor/validate_if_operand.go b/mdl/executor/validate_if_operand.go new file mode 100644 index 000000000..05810fe63 --- /dev/null +++ b/mdl/executor/validate_if_operand.go @@ -0,0 +1,94 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// ifOperandRule is an inline `if … then … else …` used as the operand of an +// operator without parentheses. Mendix's expression grammar admits an +// if-expression only where a whole expression goes; MDL's parses it anywhere. +// Measured on mxbuild 11.12.2 (mendixlabs/mxcli#1710): +// +// 'x' + if $c then 'a' else 'b' CE0117 "Error(s) in expression" +// $c and if $c then true else false CE0117 +// $c = if $c then true else false CE0117 +// -if $c then 1 else 2 CE0117 +// return 'x' + if $c then 'a' else 'b' CE0117 +// 'x' + (if $c then 'a' else 'b') clean — parenthesised +// if $c then 'a' else 'b' + 'x' clean — the else branch takes the `+` +// toUpperCase(if $c then 'a' else 'b') clean — a function argument +// not(if $c then true else false) clean +// if $c then 'a' else if $c then … clean — an if as the else branch +const ifOperandRule = "MDL-IFOPERAND01" + +// ifOperands returns the operators that take an unparenthesised if-expression +// as an operand, in the order they appear. +func ifOperands(expr ast.Expression) []string { + var out []string + var walk func(ast.Expression) + walk = func(e ast.Expression) { + switch n := e.(type) { + case *ast.BinaryExpr: + if isBareIf(n.Left) || isBareIf(n.Right) { + out = append(out, strings.ToLower(n.Operator)) + } + walk(n.Left) + walk(n.Right) + case *ast.UnaryExpr: + // not(…) is always written with its parentheses. + if !strings.EqualFold(n.Operator, "not") && isBareIf(n.Operand) { + out = append(out, "unary "+n.Operator) + } + walk(n.Operand) + case *ast.ParenExpr: + walk(n.Inner) + case *ast.SourceExpr: + walk(n.Expression) + case *ast.FunctionCallExpr: + for _, a := range n.Arguments { + walk(a) + } + case *ast.IfThenElseExpr: + walk(n.Condition) + walk(n.ThenExpr) + walk(n.ElseExpr) + } + } + walk(expr) + return out +} + +// isBareIf reports an if-expression that is not inside parentheses. A +// SourceExpr is only a wrapper, unless its source carries the parentheses. +func isBareIf(e ast.Expression) bool { + for { + switch n := e.(type) { + case *ast.SourceExpr: + if strings.HasPrefix(strings.TrimSpace(n.Source), "(") { + return false + } + e = n.Expression + case *ast.IfThenElseExpr: + return true + default: + return false + } + } +} + +// checkIfOperand flags an if-expression used as an operand without +// parentheses (mendixlabs/mxcli#1710). +func (v *microflowValidator) checkIfOperand(label string, expr ast.Expression) { + for _, op := range ifOperands(expr) { + v.addViolation(ifOperandRule, linter.SeverityError, + fmt.Sprintf("%s uses an `if … then … else …` as an operand of `%s` without parentheses — Mendix "+ + "rejects it (CE0117 \"Error(s) in expression\")", label, op), + "Put the if-expression in parentheses: `'x' + (if $c then 'a' else 'b')`.") + } +} diff --git a/mdl/executor/validate_microflow.go b/mdl/executor/validate_microflow.go index b802b818c..72d8186a6 100644 --- a/mdl/executor/validate_microflow.go +++ b/mdl/executor/validate_microflow.go @@ -604,6 +604,7 @@ func (v *microflowValidator) checkExprFunctions(label string, expr ast.Expressio // The one per-expression hook shared by microflows and nanoflows, so the // JSON-number check rides along rather than keeping a third list of sites. v.checkLocaleNumberInJSON(label, expr) + v.checkIfOperand(label, expr) src := microflowExprSource(expr) if src == "" { return From 448f6336d56048875c27e59fd70f12990b13614a Mon Sep 17 00:00:00 2001 From: Ako Date: Mon, 2 Nov 2026 16:31:32 +0000 Subject: [PATCH 16/17] fix(security): modify a demo user in place (#1859) create or modify demo user removed an existing user and added it back, which appended it: it moved to the end of the list and every later user shifted an index, so `mxcli diff` showed other demo users' names, passwords and entities changing (mendixlabs/mxcli#1701). AddDemoUser already updates a user by name in place, so the remove is dropped. The test mock of AddDemoUser now honours that contract instead of always appending. Co-Authored-By: Claude Opus 5.5 --- ...ate-or-modify-demo-user-reorders-list.json | 1 + CHANGELOG.md | 1 + mdl/executor/cmd_security_write.go | 8 ++-- .../demo_user_modify_in_place_test.go | 43 +++++++++++++++++++ .../issue707_describe_roundtrip_test.go | 7 +++ 5 files changed, 56 insertions(+), 4 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-11-02-create-or-modify-demo-user-reorders-list.json create mode 100644 mdl/executor/demo_user_modify_in_place_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-11-02-create-or-modify-demo-user-reorders-list.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-11-02-create-or-modify-demo-user-reorders-list.json new file mode 100644 index 000000000..8c4dcf82a --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-11-02-create-or-modify-demo-user-reorders-list.json @@ -0,0 +1 @@ +{"area": "mdl/executor", "date": "2026-11-02", "symptom": "`create or modify demo user 'x'` on an existing user made `mxcli diff` show DemoUsers[1].Password, DemoUsers[1].UserName, DemoUsers[2].Entity … changing for users the statement never named; after exec the modified user was last in the list", "cause": "execCreateDemoUser modified a user by RemoveDemoUser + AddDemoUser, which appends — the user moved to the end and every later user shifted one index; AddDemoUser already updates by user name in place", "file": "mdl/executor/cmd_security_write.go (execCreateDemoUser: drop the RemoveDemoUser call)", "insight": "A remove-then-add 'update' of a list member is a reorder, and an index-keyed diff blames every sibling. Use the backend's update-in-place path. The test mock of AddDemoUser appended unconditionally, which hid that the backend contract is add-or-update; make mocks honour the contract they stand in for.", "refs": ["mendixlabs/mxcli#1701", "ako/mxcli#1859", "mdl/executor/demo_user_modify_in_place_test.go"]} diff --git a/CHANGELOG.md b/CHANGELOG.md index bfe78c19b..112db885f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **`create or modify demo user` updates the user in place** (ako/mxcli#1859, mendixlabs/mxcli#1701) — modifying an existing demo user removed it and added it back at the end of the list, so `mxcli diff` showed every later demo user's name, password and entity changing. The user now keeps its position and its siblings are untouched. - **`describe microflow` prints the concurrency error message in the project's default language** (ako/mxcli#1869, mendixlabs/mxcli#1711) — on an `en_US` project with `de_DE` enabled it printed the German text, and a drop and create from that output stored it under `en_US` too, so the English message was gone while `check`, `exec` and `mx check` stayed clean. It now picks the language the way every other described text does: the project default, then `en_US`, then the first non-empty one. - **The catalog's XPath rows name their widget** (ako/mxcli#1879) — `xpath_expressions.ComponentId` for a page or snippet data source was a Go byte dump (`{0 [57 221 …]}`) and nothing tied the row to its widget. It is now a GUID, and the new `WidgetId` / `WidgetName` columns (`widget_id` / `widget_name` in `xpath_expressions()`) name the widget by the id `widgets` gives it. The catalog schema moves to version 32, so a cached catalog is rebuilt once. - **`create or replace page` can clear a data grid column caption** (ako/mxcli#1877) — `Caption: ''` over a stored caption reported "Unchanged page" and kept it, because the translation carry added the stored text back to an empty header written with no translation. The header is now written as one empty translation, which the carry leaves alone; an empty translation where the stored text has none is dropped, so a Studio Pro empty caption still rewrites as unchanged. This also repairs pages that still carry a leaked column name (#535). diff --git a/mdl/executor/cmd_security_write.go b/mdl/executor/cmd_security_write.go index 3b8eccd8a..ab996d4a1 100644 --- a/mdl/executor/cmd_security_write.go +++ b/mdl/executor/cmd_security_write.go @@ -1377,7 +1377,7 @@ func execCreateDemoUser(ctx *ExecContext, s *ast.CreateDemoUserStmt) error { if !s.CreateOrModify { return mdlerrors.NewAlreadyExists("demo user", s.UserName) } - // Additive: merge roles, update password. Drop and re-create with merged roles. + // Additive: merge roles, update password, in place. mergedRoles := du.UserRoles existingSet := make(map[string]bool) for _, r := range mergedRoles { @@ -1400,9 +1400,9 @@ func execCreateDemoUser(ctx *ExecContext, s *ast.CreateDemoUserStmt) error { reportUnchanged(ctx, "demo user: "+s.UserName) return nil } - if err := ctx.Backend.RemoveDemoUser(ps.ID, s.UserName); err != nil { - return mdlerrors.NewBackend("update demo user", err) - } + // AddDemoUser updates an existing user in place. Removing it first + // moved it to the end of the list, so every user after it shifted + // an index and showed up in the diff (mendixlabs/mxcli#1701). if err := ctx.Backend.AddDemoUser(ps.ID, s.UserName, password, entity, mergedRoles); err != nil { return mdlerrors.NewBackend("update demo user", err) } diff --git a/mdl/executor/demo_user_modify_in_place_test.go b/mdl/executor/demo_user_modify_in_place_test.go new file mode 100644 index 000000000..57b21c614 --- /dev/null +++ b/mdl/executor/demo_user_modify_in_place_test.go @@ -0,0 +1,43 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/sdk/security" +) + +// CREATE OR MODIFY DEMO USER on an existing user removed it and added it back, +// so it moved to the end of the list and every user after it shifted one +// index: `mxcli diff` showed DemoUsers[1].UserName, DemoUsers[2].Entity … as +// changed for users the statement never named (mendixlabs/mxcli#1701). +func TestCreateOrModifyDemoUser_ModifiesInPlace(t *testing.T) { + ctx, _, ps := demoUserCtx(t, + &security.DemoUser{UserName: "alice", Password: "Pa55word!", Entity: "Administration.Account", UserRoles: []string{"User"}}, + &security.DemoUser{UserName: "bob", Password: "Pa55word!", Entity: "Administration.Account", UserRoles: []string{"User"}}, + &security.DemoUser{UserName: "carol", Password: "Pa55word!", Entity: "Administration.Account", UserRoles: []string{"User"}}, + ) + alice := ps.DemoUsers[0] + + assertNoError(t, execCreateDemoUser(ctx, &ast.CreateDemoUserStmt{ + UserName: "alice", Password: "Pa55word!", UserRoles: []string{"Administrator"}, CreateOrModify: true, + })) + + var order []string + for _, du := range ps.DemoUsers { + order = append(order, du.UserName) + } + if got := strings.Join(order, ","); got != "alice,bob,carol" { + t.Errorf("modifying alice reordered the demo users: %s (#1701)", got) + } + if ps.DemoUsers[0] != alice { + t.Errorf("alice was replaced by a new element instead of updated in place (#1701)") + } + // Control: the modification itself landed. + if got := strings.Join(alice.UserRoles, ","); got != "User,Administrator" { + t.Errorf("control: alice's roles = %s, want User,Administrator", got) + } +} diff --git a/mdl/executor/issue707_describe_roundtrip_test.go b/mdl/executor/issue707_describe_roundtrip_test.go index 296dcd9fb..f8d7f0696 100644 --- a/mdl/executor/issue707_describe_roundtrip_test.go +++ b/mdl/executor/issue707_describe_roundtrip_test.go @@ -394,7 +394,14 @@ func demoUserCtx(t *testing.T, users ...*security.DemoUser) (*ExecContext, *byte ps.DemoUsers = kept return nil }, + // Adds, or updates by user name in place — the backend's contract. AddDemoUserFunc: func(_ model.ID, name, pw, entity string, roles []string) error { + for _, du := range ps.DemoUsers { + if strings.EqualFold(du.UserName, name) { + du.Password, du.Entity, du.UserRoles = pw, entity, roles + return nil + } + } ps.DemoUsers = append(ps.DemoUsers, &security.DemoUser{UserName: name, Password: pw, Entity: entity, UserRoles: roles}) return nil }, From 242c770714fdf7c622e549a6979d38c3ca2e8369 Mon Sep 17 00:00:00 2001 From: Ako Date: Mon, 2 Nov 2026 16:43:43 +0000 Subject: [PATCH 17/17] fix(test): each --local run also starts on an empty PostgreSQL test database The hsqldb half of #1853 removed the file database before the boot; PostgreSQL's `_test` database had the same carry-over (a test asserting an empty table failed on the second run). FreshFileDB becomes FreshDB, and on PostgreSQL it runs `DROP OWNED BY CURRENT_USER` as the app role inside the scratch database once it is ensured: the database stays, and the dev loop's database owned by the same role is out of reach. The test runner sets it for every --local run. Refs #1853 Co-Authored-By: Claude Opus 5.5 --- ...t-postgres-database-kept-between-runs.json | 1 + .../skills/mendix/test-microflows/SKILL.md | 30 +++++---- cmd/mxcli/docker/localapp.go | 65 ++++++++++++++++--- cmd/mxcli/docker/localapp_freshdb_test.go | 55 ++++++++++++++-- cmd/mxcli/main.go | 2 +- cmd/mxcli/testrunner/localapp_dbtype_test.go | 21 +++--- cmd/mxcli/testrunner/localapp_options.go | 10 +-- docs-site/src/tools/running-tests.md | 12 ++-- 8 files changed, 145 insertions(+), 51 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/cmd-mxcli/2026-11-02-test-postgres-database-kept-between-runs.json diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli/2026-11-02-test-postgres-database-kept-between-runs.json b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-11-02-test-postgres-database-kept-between-runs.json new file mode 100644 index 000000000..a36dc425e --- /dev/null +++ b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-11-02-test-postgres-database-kept-between-runs.json @@ -0,0 +1 @@ +{"area": "cmd/mxcli/docker", "date": "2026-11-02", "symptom": "`mxcli test --local` on PostgreSQL (the default) kept the `_test` scratch database between runs, as the file database did before ako/mxcli#1853: a test asserting an empty table PASSed on run 1 and FAILed on run 2 after an `@cleanup none` test committed one row (11.12.2, PostgreSQL 15)", "cause": "`EnsureDatabase` creates the role and database only when missing and is otherwise a no-op, so a second run boots on whatever the first left. Nothing in the boot ever emptied it", "file": "`cmd/mxcli/docker/localapp.go` (`LocalAppOptions.FreshDB`, renamed from FreshFileDB, now also covers PostgreSQL via `emptyPostgresDatabase`); `cmd/mxcli/testrunner/localapp_options.go` sets it for every --local run", "insight": "Empty in place rather than drop and recreate: `DROP OWNED BY CURRENT_USER`, as the app role, connected to the scratch database, removes only objects in that database, so the dev loop's database owned by the same role cannot be reached, and nothing depends on CREATEDB or a superuser at the moment the data is already gone. Public survives (owned by pg_database_owner from PG15; by postgres before) and the role can still create in it. Measured before choosing: 88 relations to 0 on a database the runtime had booted on. Runtime control: main run 2 FAIL, fix both runs PASS, and the dev database `tapp` kept its 87 tables across both runs", "refs": ["ako/mxcli#1853"], "rules": [], "ce": []} diff --git a/.claude/skills/mendix/test-microflows/SKILL.md b/.claude/skills/mendix/test-microflows/SKILL.md index f8b4ced05..aec814a13 100644 --- a/.claude/skills/mendix/test-microflows/SKILL.md +++ b/.claude/skills/mendix/test-microflows/SKILL.md @@ -12,18 +12,18 @@ description: "Write and run MDL-based microflow tests with `mxcli test` — anno |---|---| | 33-39 | Reference files | | 40-50 | When to Use This Skill | -| 51-182 | Prerequisites | -| 183-225 | Test File Formats | -| 226-241 | string Operations | -| 242-265 | Running Tests | -| 266-459 | How It Works | -| 460-498 | Check a test file before you run it | -| 499-620 | Writing Good Tests | -| 621-635 | Test File Organization | -| 636-648 | Interpreting Failures | -| 649-670 | CI Integration | -| 671-677 | Related Skills | -| 678-680 | Warm test loop (`mxcli test --local [--watch]`, `--attach`, `run --local --test-endpoint`) | +| 51-184 | Prerequisites | +| 185-227 | Test File Formats | +| 228-243 | string Operations | +| 244-267 | Running Tests | +| 268-461 | How It Works | +| 462-500 | Check a test file before you run it | +| 501-622 | Writing Good Tests | +| 623-637 | Test File Organization | +| 638-650 | Interpreting Failures | +| 651-672 | CI Integration | +| 673-679 | Related Skills | +| 680-682 | Warm test loop (`mxcli test --local [--watch]`, `--attach`, `run --local --test-endpoint`) | # Test Microflows Skill @@ -81,8 +81,10 @@ mxcli test tests/ -p app.mpr --local --db-type hsqldb It is still a scratch database (`_test`), separate from the one `run --local --db-type hsqldb` uses, so a dev loop on the file database keeps its -data and its file lock. Each run starts it **empty** — the previous run's files are -removed before the boot — so seeded data never carries over between runs. +data and its file lock. On either database type each run starts the +`_test` database **empty** — the file database's files are removed, and on +PostgreSQL the app role's objects in that one database are dropped — so committed +or seeded data never carries over between runs. `--db-type` applies only to `--local`; it is refused with `--attach` (which uses the attached app's database) and on the Docker path. diff --git a/cmd/mxcli/docker/localapp.go b/cmd/mxcli/docker/localapp.go index 86c419daa..75e8a6921 100644 --- a/cmd/mxcli/docker/localapp.go +++ b/cmd/mxcli/docker/localapp.go @@ -6,7 +6,9 @@ import ( "fmt" "io" "os" + "os/exec" "path/filepath" + "strings" "time" ) @@ -36,10 +38,12 @@ type LocalAppOptions struct { // EnsureDB provisions the local Postgres + database when missing instead of // only checking reachability. EnsureDB bool - // FreshFileDB removes the built-in file database's data before the boot, so - // the runtime creates it empty. For a caller whose database is its own - // scratch (the test runner), never for one holding the developer's data. - FreshFileDB bool + // FreshDB empties the database before the boot, so the runtime starts on + // none of a previous run's data: the file database's files are removed, and + // a PostgreSQL database is emptied in place (emptyPostgresDatabase). For a + // caller whose database is its own scratch (the test runner), never for one + // holding the developer's data. + FreshDB bool // SkipBuild boots against whatever is already in DeployDir. SkipBuild bool // RuntimeLogPath tees the runtime JVM output and the runtime's own @@ -164,14 +168,15 @@ func (o *LocalAppOptions) applyDefaults() { // prepareLocalAppDatabase makes the configured database usable before the boot: // provisioning it with EnsureDB, otherwise checking that its server answers. // The built-in file database needs neither: the runtime creates its files — -// afresh when FreshFileDB removed the previous ones. +// afresh when FreshDB removed the previous ones. With FreshDB, a PostgreSQL +// database is emptied once it is known to be usable. func prepareLocalAppDatabase(opts *LocalAppOptions, w io.Writer) error { if opts.DB.IsFileBased() { if opts.EnsureDB { return fmt.Errorf("EnsureDB provisions PostgreSQL; the built-in file database " + "needs no provisioning") } - if opts.FreshFileDB { + if opts.FreshDB { return removeFileDatabase(opts.DeployDir, opts.DB.Name, w) } return nil @@ -180,16 +185,58 @@ func prepareLocalAppDatabase(opts *LocalAppOptions, w io.Writer) error { if err := EnsureDatabase(&opts.DB, w); err != nil { return fmt.Errorf("ensuring database: %w", err) } - return nil - } - if err := pingTCP(opts.DB.Host, 3*time.Second); err != nil { + } else if err := pingTCP(opts.DB.Host, 3*time.Second); err != nil { return fmt.Errorf("database not reachable at %s: %w\n"+ " Pass --ensure-db to provision it, or start Postgres and create the %q database (user %q).", opts.DB.Host, err, opts.DB.Name, opts.DB.User) } + if opts.FreshDB { + return emptyPostgresDatabase(opts.DB, w) + } + return nil +} + +// emptyPostgresDatabase removes everything the app role owns inside db, so the +// runtime boots on an empty database and creates its schema afresh. +// +// In place, as the app role, connected to db itself: `DROP OWNED BY +// CURRENT_USER` reaches only objects in the current database, so the database +// stays, and every other database the role owns — the dev loop's, beside the +// test runner's `_test` one — is untouched. Dropping and recreating the +// database instead would need CREATEDB at the point its data is already gone, +// and a superuser path that a machine provisioned by hand may not have. +// Measured on PostgreSQL 15 against a database the runtime had booted on: 88 +// relations to 0, and the role can still create in public afterwards (public +// belongs to pg_database_owner, which DROP OWNED leaves alone). +func emptyPostgresDatabase(db DBConfig, w io.Writer) error { + cmd, err := emptyPostgresDatabaseCommand(db) + if err != nil { + return err + } + if out, err := cmd.CombinedOutput(); err != nil { + return fmt.Errorf("emptying database %q: %w\n%s", db.Name, err, strings.TrimSpace(string(out))) + } + fmt.Fprintf(w, " Emptied database %q; the runtime creates its schema afresh\n", db.Name) return nil } +// emptyPostgresDatabaseCommand builds the psql call emptyPostgresDatabase runs. +// The names are checked like EnsureDatabase checks them, and the password +// reaches psql through PGPASSWORD, never argv. +func emptyPostgresDatabaseCommand(db DBConfig) (*exec.Cmd, error) { + if !pgIdent.MatchString(db.Name) { + return nil, fmt.Errorf("refusing to empty database %q (expected %s)", db.Name, pgIdent) + } + if !pgIdent.MatchString(db.User) { + return nil, fmt.Errorf("refusing to empty database as user %q (expected %s)", db.User, pgIdent) + } + host, port := splitHostPort(db.Host) + cmd := exec.Command("psql", "-X", "-w", "-v", "ON_ERROR_STOP=1", "-h", host, "-p", port, + "-U", db.User, "-d", db.Name, "-c", "DROP OWNED BY CURRENT_USER") + cmd.Env = append(os.Environ(), "PGPASSWORD="+db.Password, "PGCONNECT_TIMEOUT=5") + return cmd, nil +} + // removeFileDatabase deletes the runtime's file database called name, which // lives under /data/database/hsqldb/. // diff --git a/cmd/mxcli/docker/localapp_freshdb_test.go b/cmd/mxcli/docker/localapp_freshdb_test.go index 4a8473154..a3fa8ee97 100644 --- a/cmd/mxcli/docker/localapp_freshdb_test.go +++ b/cmd/mxcli/docker/localapp_freshdb_test.go @@ -6,20 +6,22 @@ import ( "io" "os" "path/filepath" + "slices" + "strings" "testing" ) // `mxcli test --local --db-type hsqldb` booted on the file database the // previous run left, so data an after-startup flow seeded under an earlier // model stayed, an idempotent init skipped seeding, and tests failed on stale -// data (ako/mxcli#1853). FreshFileDB removes the run's database before boot — +// data (ako/mxcli#1853). FreshDB removes the run's database before boot — // and only that one: the dev loop's database beside it is not the run's. -func TestPrepareLocalAppDatabase_FreshFileDBRemovesOnlyItsOwnDatabase(t *testing.T) { +func TestPrepareLocalAppDatabase_FreshDBRemovesOnlyItsOwnDatabase(t *testing.T) { proj := t.TempDir() o := LocalAppOptions{ ProjectPath: filepath.Join(proj, "App.mpr"), DB: DBConfig{Type: RuntimeDatabaseType(DBTypeHSQLDB), Name: "app_test"}, - FreshFileDB: true, + FreshDB: true, } o.applyDefaults() stale := filepath.Join(o.DeployDir, "data", "database", "hsqldb", "app_test", "app_test.script") @@ -44,7 +46,7 @@ func TestPrepareLocalAppDatabase_FreshFileDBRemovesOnlyItsOwnDatabase(t *testing } } -// Without FreshFileDB — `run --local --db-type hsqldb`, the developer's own +// Without FreshDB — `run --local --db-type hsqldb`, the developer's own // data — nothing is removed. Also the control for the test above. func TestPrepareLocalAppDatabase_FileDBKeptByDefault(t *testing.T) { proj := t.TempDir() @@ -64,19 +66,19 @@ func TestPrepareLocalAppDatabase_FileDBKeptByDefault(t *testing.T) { t.Fatalf("prepareLocalAppDatabase: %v", err) } if _, err := os.Stat(f); err != nil { - t.Errorf("the database was removed without FreshFileDB: %v", err) + t.Errorf("the database was removed without FreshDB: %v", err) } } // The name becomes a path under the deployment directory. One that is not a // single path element is refused rather than resolved, so a reset can never // reach above the hsqldb directory. -func TestPrepareLocalAppDatabase_FreshFileDBRefusesAPathName(t *testing.T) { +func TestPrepareLocalAppDatabase_FreshDBRefusesAPathName(t *testing.T) { for _, name := range []string{"", ".", "..", "../app", "a/b"} { o := LocalAppOptions{ ProjectPath: filepath.Join(t.TempDir(), "App.mpr"), DB: DBConfig{Type: RuntimeDatabaseType(DBTypeHSQLDB), Name: name}, - FreshFileDB: true, + FreshDB: true, } o.applyDefaults() o.DB.Name = name // applyDefaults may fill an empty name; test the raw value @@ -85,3 +87,42 @@ func TestPrepareLocalAppDatabase_FreshFileDBRefusesAPathName(t *testing.T) { } } } + +// PostgreSQL's scratch database is emptied in place, as the app role, inside +// that database: `DROP OWNED BY CURRENT_USER` removes every table the runtime +// created there and nothing else — the database itself, and every other +// database the role owns (the dev loop's), are out of its reach. Dropping and +// recreating the database instead would need CREATEDB at the moment the data is +// already gone. Measured on PostgreSQL 15: 88 relations to 0, and the role can +// still create in public afterwards. +func TestEmptyPostgresDatabaseCommand(t *testing.T) { + db := DBConfig{Type: "PostgreSQL", Host: "127.0.0.1:5433", Name: "app_test", User: "mendix", Password: "s3cret"} + cmd, err := emptyPostgresDatabaseCommand(db) + if err != nil { + t.Fatal(err) + } + args := strings.Join(cmd.Args, " ") + for _, want := range []string{"-d app_test", "-U mendix", "-h 127.0.0.1", "-p 5433", "ON_ERROR_STOP=1", "DROP OWNED BY CURRENT_USER"} { + if !strings.Contains(args, want) { + t.Errorf("command %q is missing %q", args, want) + } + } + if strings.Contains(args, "s3cret") { + t.Errorf("the password is on the command line, readable in the process list: %q", args) + } + if !slices.Contains(cmd.Env, "PGPASSWORD=s3cret") { + t.Error("the password does not reach psql through PGPASSWORD") + } +} + +func TestEmptyPostgresDatabaseCommand_RefusesUnsafeNames(t *testing.T) { + for _, db := range []DBConfig{ + {Name: "app_test; drop database app", User: "mendix", Host: "127.0.0.1:5432"}, + {Name: "app_test", User: "x y", Host: "127.0.0.1:5432"}, + {Name: "", User: "mendix", Host: "127.0.0.1:5432"}, + } { + if _, err := emptyPostgresDatabaseCommand(db); err == nil { + t.Errorf("%+v: accepted, want a refusal", db) + } + } +} diff --git a/cmd/mxcli/main.go b/cmd/mxcli/main.go index 99c81d006..11f083c2d 100644 --- a/cmd/mxcli/main.go +++ b/cmd/mxcli/main.go @@ -439,7 +439,7 @@ func init() { testRunCmd.Flags().String("configuration", "", "With --local, which project configuration's constant values to run the tests with (default: the only one, or \"Default\") — the same resolution 'mxcli run --local' uses, so a suite sees the same constants either way") testRunCmd.Flags().StringArray("constant", nil, "With --local, set a constant for THIS RUN only: Module.Name=value (repeatable). Never written to the project. The value is visible in shell history and in `ps` — for a value that must not be, see docs/11-proposals/PROPOSAL_constant_values.md") testRunCmd.Flags().String("mxbuild-path", "", "With --local, the mxbuild to build with, overriding resolution (Studio Pro's bundled mxbuild on macOS/Windows, the cached CDN download on Linux); also settable as MXCLI_MXBUILD_PATH") - testRunCmd.Flags().String("db-type", "", "With --local, the database the scratch app boots on: postgresql (default) or hsqldb (the runtime's built-in file database — no server and no provisioning, so it works without PostgreSQL; each run starts it empty). Same values as 'run --local --db-type'") + testRunCmd.Flags().String("db-type", "", "With --local, the database the scratch app boots on: postgresql (default) or hsqldb (the runtime's built-in file database — no server and no provisioning, so it works without PostgreSQL). Either way each run starts the _test database empty. Same values as 'run --local --db-type'") registerTestPortFlags(testRunCmd) testRunCmd.Flags().BoolP("verbose", "v", false, "Show all runtime log output") testRunCmd.Flags().BoolP("color", "", false, "Use colored output") diff --git a/cmd/mxcli/testrunner/localapp_dbtype_test.go b/cmd/mxcli/testrunner/localapp_dbtype_test.go index 9c9e01057..c73a0e13e 100644 --- a/cmd/mxcli/testrunner/localapp_dbtype_test.go +++ b/cmd/mxcli/testrunner/localapp_dbtype_test.go @@ -92,16 +92,15 @@ func TestResolveTestDBType(t *testing.T) { } } -// A test run's file database is its own scratch, so it starts empty: kept -// between runs, data an earlier run's after-startup seeded made an idempotent -// init skip seeding and the tests fail on it (ako/mxcli#1853). -func TestLocalAppOptions_HSQLDBStartsFresh(t *testing.T) { - got := localAppOptions(RunOptions{ProjectPath: "/tmp/app/App.mpr", DBType: docker.DBTypeHSQLDB}, - "log", nil, io.Discard) - if !got.FreshFileDB { - t.Error("FreshFileDB is false; the run boots on the previous run's data") - } - if pg := localAppOptions(RunOptions{ProjectPath: "/tmp/app/App.mpr"}, "log", nil, io.Discard); pg.FreshFileDB { - t.Error("FreshFileDB set for PostgreSQL, where it means nothing") +// A test run's database is its own scratch, so it starts empty on either +// type: kept between runs, data an earlier run committed or seeded made an +// idempotent init skip seeding and the tests fail on it (ako/mxcli#1853 for the +// file database, the same carry-over measured on PostgreSQL). +func TestLocalAppOptions_TestDatabaseStartsFresh(t *testing.T) { + for _, dbType := range []string{docker.DBTypeHSQLDB, docker.DBTypePostgreSQL, ""} { + got := localAppOptions(RunOptions{ProjectPath: "/tmp/app/App.mpr", DBType: dbType}, "log", nil, io.Discard) + if !got.FreshDB { + t.Errorf("DBType %q: FreshDB is false; the run boots on the previous run's data", dbType) + } } } diff --git a/cmd/mxcli/testrunner/localapp_options.go b/cmd/mxcli/testrunner/localapp_options.go index 62eb546d2..dd654759d 100644 --- a/cmd/mxcli/testrunner/localapp_options.go +++ b/cmd/mxcli/testrunner/localapp_options.go @@ -37,11 +37,11 @@ func localAppOptions(opts RunOptions, logPath string, env []string, w io.Writer) DB: db, // Provisioning is PostgreSQL's; the file database is created by the runtime. EnsureDB: !db.IsFileBased(), - // The file database is this run's scratch, so it starts empty. Kept - // between runs, data a previous run's after-startup seeded under an - // earlier model made an idempotent init skip seeding, and the tests - // failed on stale data (ako/mxcli#1853). - FreshFileDB: db.IsFileBased(), + // The database is this run's scratch, so it starts empty. Kept between + // runs, data a previous run committed or seeded under an earlier model + // made an idempotent init skip seeding, and the tests failed on stale + // data (ako/mxcli#1853) — on PostgreSQL as on the file database. + FreshDB: true, SkipBuild: opts.SkipBuild, Env: env, ConstantOverrides: opts.ConstantOverrides, diff --git a/docs-site/src/tools/running-tests.md b/docs-site/src/tools/running-tests.md index ea3896664..4a903b24c 100644 --- a/docs-site/src/tools/running-tests.md +++ b/docs-site/src/tools/running-tests.md @@ -29,10 +29,14 @@ runtime's built-in file database instead: no server and no provisioning. It takes the same values as `run --local --db-type` (`postgresql`, the default, or `hsqldb`), keeps the `_test` scratch name so it never shares files with a `run --local --db-type hsqldb` dev loop, and is refused with `--attach` and on -the Docker path, which do not boot a database of their own. Each run starts on an -**empty** file database: the previous run's `_test` files are removed -before the boot, so data an after-startup flow seeded under an earlier model -cannot make an idempotent init skip seeding. +the Docker path, which do not boot a database of their own. + +Either way, each `--local` run starts on an **empty** `_test` database: +the file database's files are removed before the boot, and on PostgreSQL +everything the app role owns inside `_test` is dropped (`DROP OWNED BY +CURRENT_USER`, run in that database only — the dev loop's `` database is +untouched). Data a previous run committed, or an after-startup flow seeded under +an earlier model, therefore cannot make an idempotent init skip seeding. ```bash mxcli test tests/ -p app.mpr --local --db-type hsqldb