From 03e340432eb41816dcb251d6e8977876f9fd955d Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 17:10:49 +0000 Subject: [PATCH 01/18] fix(test): a later passing @expect no longer clears an earlier failure (ako/mxcli#1753) Symptom, from the issue: "On the after-startup test runner, a test with more than one `@expect` is reported **PASS** when an earlier assertion fails and a later one passes. `mxcli test` exits 0." Affected: every Docker run and `--local --legacy-runner`; the endpoint runner is not. Cause: writeExpectAssertion logs one verdict line per assertion, MXTEST:PASS: or MXTEST:FAIL::..., and ParseLogResults let every line overwrite the test's status. The last line won, and the PASS case also cleared the failure message. The generated flow returned $AllPassed = false, but nothing reads it. Fix: the parser folds a test's verdict lines with a sticky failure (applyVerdictLine). Once a test has a FAIL or an ERROR, no later PASS or SKIP line clears it. The first failure's message is reported, with "(and N more failure(s) in this test)" when more followed. This matches the endpoint runner, which evaluates an assertion only while $Verdict is PASS and whose skipped-assertions guard never replaces a failure. The skipped-assertions FAIL (#1448), logged after the PASS lines of the assertions that ran, still wins over them: a sticky failure, not "first line wins". A RUN line resets the fold, so a failure cannot outlive its run. The fix is in the parser, so it also covers logs written by runners generated before it. The generator is unchanged: logging one verdict per test would change a flow whose verdicts were measured on 11.13.0, and it would cover nothing more, because parser and generator ship together. Measurement (unit level, the symptom's layer: the parser reads a log). sticky_failure_test.go, run before the fix existed and again with results.go reverted: --- FAIL: TestLaterPassDoesNotClearEarlierFailure FAIL then PASS: status = PASS, want FAIL FAIL then PASS: message = "", want "Expected $r = 1, actual: 2" FAIL then PASS: the suite passed, so `mxcli test` exits 0 --- FAIL: TestFirstFailureMessageIsReported message = "Expected $r = 4, actual: 2", want "Expected $r = 1, actual: 2 (and 2 more failures in this test)" --- FAIL: TestLaterPassDoesNotClearEarlierError ERROR then PASS:test_1: got PASS "", want ERROR "Setup failed: Shop.Seed" TestSkippedAssertionsFailureStillWins and the PASS-then-FAIL and PASS-then-PASS cases pass both before and after; they are the controls. With the RUN reset stubbed out, TestRunLineStartsVerdictAfresh fails. mdl-examples/bug-tests/test-runner-1753-earlier-expect-fails.test.mdl is the runtime reproduction. It was not run against an app for this change. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82sUVQEB3K5W5vh85wM --- ...later-expect-pass-clears-earlier-fail.json | 1 + CHANGELOG.md | 1 + cmd/mxcli/testrunner/results.go | 125 ++++++++-------- cmd/mxcli/testrunner/sticky_failure_test.go | 135 ++++++++++++++++++ docs-site/src/tools/test-annotations.md | 4 + ...-runner-1753-earlier-expect-fails.test.mdl | 55 +++++++ 6 files changed, 258 insertions(+), 63 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-legacy-runner-later-expect-pass-clears-earlier-fail.json create mode 100644 cmd/mxcli/testrunner/sticky_failure_test.go create mode 100644 mdl-examples/bug-tests/test-runner-1753-earlier-expect-fails.test.mdl diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-legacy-runner-later-expect-pass-clears-earlier-fail.json b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-legacy-runner-later-expect-pass-clears-earlier-fail.json new file mode 100644 index 000000000..42148292a --- /dev/null +++ b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-legacy-runner-later-expect-pass-clears-earlier-fail.json @@ -0,0 +1 @@ +{"area": "cmd/mxcli", "date": "2026-10-28", "symptom": "On the after-startup runner (every Docker run, and `--local --legacy-runner`) a test with more than one `@expect` is reported PASS when an earlier assertion fails and a later one passes, and `mxcli test` exits 0. Measured at unit level: log FAIL:test_1:Expected $r = 1, actual: 2 then PASS:test_1 gave status=PASS message=\"\" AllPassed=true. PASS then FAIL reported correctly; the endpoint runner (`--local`, `--attach`, `--watch`) was never affected", "cause": "`writeExpectAssertion` logs a verdict line per assertion (MXTEST:PASS or MXTEST:FAIL), and `ParseLogResults` treated every line as the test's verdict: each case overwrote Status, and the PASS case also cleared Message, so the last line won. The generated flow did compute `$AllPassed = false` and return it — nothing reads that value", "file": "`cmd/mxcli/testrunner/results.go` (`ParseLogResults`, new `applyVerdictLine`)", "insight": "A log protocol that emits several partial verdicts per test needs a monotonic fold, not last-write-wins: FAIL/ERROR is sticky, the first failure's message is reported (the endpoint runner's semantics — its assertions run only while $Verdict is PASS and its skipped-assertions guard never replaces a failure), and further failures are counted onto the message. The plausible wrong turn is 'first verdict line wins': the skipped-assertions guard (#1448) logs its FAIL AFTER the PASS lines of the assertions that did run, so first-wins turns that false green back on — sticky failure, not first line, is the rule. Fixed in the parser rather than the generator: it also corrects logs from runners generated before the fix, and a one-verdict-per-test generator would change a flow shape whose verdicts were measured on 11.13.0 for no additional coverage, since parser and generator ship in one binary. A RUN line resets the fold so a failure cannot outlive the run it was logged in. Second time this parser took a verdict from the wrong line (after #1448's RUN-without-verdict read as the zero status, PASS): when touching `ParseLogResults`, enumerate the line ORDERS the generator can produce per test, not just the line kinds", "refs": ["ako/mxcli#1753", "ako/mxcli#1448"]} diff --git a/CHANGELOG.md b/CHANGELOG.md index 0808e4c63..03eb4926e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed - **A navigation profile's application title and icon can be set and are described** (ako/mxcli#1756, mendixlabs/mxcli#1664) — the title (Studio Pro's *Application title*, `Mendix` in a new project) is what the runtime puts in every browser tab as `Mendix - `, and it had no MDL form: `create or modify navigation Responsive app title 'EWO' …` failed with "extraneous input 'app'", `describe navigation` printed neither it nor the *Application icon*, and `create or modify translations … for en_US` refuses the source language, so every generated app kept "Mendix" in the tab. `create or modify navigation` now takes `app title ''` and `app icon ` (`app icon none` clears it), in any order with the other clauses; `app` and `icon` stay usable as names. The title is written as the project default language's item, the title's other languages kept; the icon is resolved like any image reference, by `check --references` and before exec writes; an omitted clause keeps the stored value; describe prints both. A native profile has neither, so both are refused there. +- **A test whose earlier `@expect` fails is no longer reported PASS on the after-startup runner** (ako/mxcli#1753) — every Docker run and `--local --legacy-runner` logged a verdict line per assertion, and the result was taken from the last one: a failing first assertion followed by a passing second reported PASS, with the failure message erased, and `mxcli test` exited 0. A failure now sticks, and the test reports the first one, plus how many more failed. The endpoint runner (`--local`, `--attach`, `--watch`) was not affected. - **Pages on Mendix 9.24 and 10.x are read and written in the version's own shape** (ako/mxcli#1209) — several page properties are stored differently below a version: a design property's value before 10.2, a data view's page parameter before 10.0, its editability before 10.7, and a text box's placeholder before 10.11. `describe` read only the newer form, so on 9.24 a describe → exec round trip removed every design property and placeholder. Writing any page with a data view or a design property was refused, and `alter styling` on a stored design property reported success and changed nothing. Every write now converts to the project's form, and `describe` reads both. What the older form cannot hold is refused, naming the widget: a Spacing or colour design property, conditional editability, a placeholder with parameters. - **A text box's `KeyboardType` survives describe → exec** (ako/mxcli#1209) — the keyboard a native app opens had no MDL spelling and was written as `Default`, so a rewrite reset it. `textbox t (Attribute: Email, KeyboardType: EmailAddress)` (`Default`, `NumberPad`, `DecimalPad`, `EmailAddress`, `PhonePad`, `URL`); an unknown value is MDL-WIDGET18. - **A button's workflow action survives a rewrite** (ako/mxcli#832) — `describe` printed no action for Show user task page, Show workflow admin page or Call workflow, so re-executing a page turned them into no action (on ako/TestApp's TaskInbox and TaskDashboard, 2 of 6 user-task buttons each). They are now `open user task [with (AssignOnOpen, OpenWhenAssigned)]`, `open workflow with (DefaultPage: M.Page)` and `call workflow M.WF [with (…)]`. `complete task` takes `with (ClosePage, Commit)` instead of always writing both true. `open workflow` without a page is CE1834 and is refused. diff --git a/cmd/mxcli/testrunner/results.go b/cmd/mxcli/testrunner/results.go index 905b232d9..593574229 100644 --- a/cmd/mxcli/testrunner/results.go +++ b/cmd/mxcli/testrunner/results.go @@ -189,6 +189,8 @@ func ParseLogResults(logReader io.Reader, suite *TestSuite, requireAssertions bo // Track which tests we've seen results for resultMap := make(map[string]*TestResult) runTimes := make(map[string]time.Time) + // failures counts each test's FAIL and ERROR lines; see applyVerdictLine. + failures := make(map[string]int) scanner := bufio.NewScanner(logReader) for scanner.Scan() { @@ -233,6 +235,9 @@ func ParseLogResults(logReader io.Reader, suite *TestSuite, requireAssertions bo // verdict did not finish: an error left the runner flow, which // stops it here — a call whose own handler re-raises does exactly // that (ako/mxcli#1448). Its PASS or FAIL line replaces this. + // A RUN line starts the test's verdict afresh, so a failure that + // sticks (applyVerdictLine) cannot outlive the run it belongs to. + delete(failures, id) resultMap[id] = &TestResult{ ID: id, Name: name, @@ -240,77 +245,25 @@ func ParseLogResults(logReader io.Reader, suite *TestSuite, requireAssertions bo Message: "started and reported no verdict: an error left the test runner here, which stopped it (the runtime log has the error)", } - case "PASS": - if r, ok := resultMap[id]; ok { - r.Status = StatusPass - r.Message = "" - if t, ok := runTimes[id]; ok { - r.Duration = time.Since(t) - } - } else { - resultMap[id] = &TestResult{ - ID: id, - Name: id, - Status: StatusPass, - } - } - - case "FAIL": + case "PASS", "FAIL", "ERROR", "SKIP": + // ERROR is a setup that threw: the test never ran, so it is neither a + // pass nor a failure. Only the monolithic runner reports this way — + // the endpoint carries the same outcome back as a SETUP verdict. msg := "" if len(parts) >= 4 { msg = parts[3] } - if r, ok := resultMap[id]; ok { - r.Status = StatusFail - r.Message = msg + r, ok := resultMap[id] + if !ok { + r = &TestResult{ID: id, Name: id} + resultMap[id] = r + } + if action == "PASS" || action == "FAIL" { if t, ok := runTimes[id]; ok { r.Duration = time.Since(t) } - } else { - resultMap[id] = &TestResult{ - ID: id, - Name: id, - Status: StatusFail, - Message: msg, - } - } - - case "ERROR": - // A setup that threw: the test never ran, so it is neither a pass nor - // a failure. Only the monolithic runner reports this way — the - // endpoint carries the same outcome back as a SETUP verdict. - msg := "" - if len(parts) >= 4 { - msg = parts[3] - } - if r, ok := resultMap[id]; ok { - r.Status = StatusError - r.Message = msg - } else { - resultMap[id] = &TestResult{ - ID: id, - Name: id, - Status: StatusError, - Message: msg, - } - } - - case "SKIP": - msg := "" - if len(parts) >= 4 { - msg = parts[3] - } - if r, ok := resultMap[id]; ok { - r.Status = StatusSkip - r.Message = msg - } else { - resultMap[id] = &TestResult{ - ID: id, - Name: id, - Status: StatusSkip, - Message: msg, - } } + applyVerdictLine(r, verdictStatus[action], msg, failures) case "END": result.Duration = time.Since(result.Started) @@ -337,6 +290,13 @@ func ParseLogResults(logReader io.Reader, suite *TestSuite, requireAssertions bo } res.Status = r.Status res.Message = r.Message + if more := failures[tc.ID] - 1; more > 0 { + noun := "failures" + if more == 1 { + noun = "failure" + } + res.Message += fmt.Sprintf(" (and %d more %s in this test)", more, noun) + } res.Duration = r.Duration result.Tests = append(result.Tests, res) } else { @@ -351,6 +311,45 @@ func ParseLogResults(logReader io.Reader, suite *TestSuite, requireAssertions bo return result } +// verdictStatus maps a log protocol verdict action to the status it reports. +var verdictStatus = map[string]TestStatus{ + "PASS": StatusPass, + "FAIL": StatusFail, + "ERROR": StatusError, + "SKIP": StatusSkip, +} + +// applyVerdictLine records one verdict line of the after-startup runner's log +// against a test's result. failures counts each test's FAIL and ERROR lines. +// +// A test can log more than one verdict line: writeExpectAssertion logs PASS or +// FAIL for every @expect on its own, and the skipped-assertions guard logs a +// FAIL after them. A failure is therefore sticky — once a test has a FAIL or an +// ERROR, no later line clears it, and the first failure's message is the one +// reported. When every line simply replaced the last, a test whose first +// assertion failed and whose second held reported PASS with the failure message +// erased, and `mxcli test` exited 0 (ako/mxcli#1753). The rule lives here rather +// than in the generated flow so it also holds for a log written by a runner +// generated before it. +// +// This is the endpoint runner's semantics: there an assertion is evaluated only +// while $Verdict is still PASS, and the skipped-assertions guard does not +// replace an existing failure, so its first failure is what it returns. +// +// The ERROR a RUN line leaves as a placeholder is not a failure line; the +// test's own verdict replaces it. +func applyVerdictLine(r *TestResult, status TestStatus, msg string, failures map[string]int) { + failed := status == StatusFail || status == StatusError + if failed { + failures[r.ID]++ + } + if failures[r.ID] > 1 || (!failed && failures[r.ID] > 0) { + return + } + r.Status = status + r.Message = msg +} + // PrintResults writes a human-readable summary to the writer. func PrintResults(w io.Writer, result *SuiteResult, color bool) { fmt.Fprintf(w, "\n") diff --git a/cmd/mxcli/testrunner/sticky_failure_test.go b/cmd/mxcli/testrunner/sticky_failure_test.go new file mode 100644 index 000000000..993c94a1f --- /dev/null +++ b/cmd/mxcli/testrunner/sticky_failure_test.go @@ -0,0 +1,135 @@ +// SPDX-License-Identifier: Apache-2.0 + +package testrunner + +import ( + "strings" + "testing" +) + +// twoExpectSuite is one test with two @expect lines, the shape the after-startup +// runner reports as two verdict lines: writeExpectAssertion logs PASS or FAIL +// for each assertion on its own. +func twoExpectSuite(t *testing.T) *TestSuite { + t.Helper() + e1, err := ParseExpect("$r = 1") + if err != nil { + t.Fatalf("ParseExpect: %v", err) + } + e2, err := ParseExpect("$r = 2") + if err != nil { + t.Fatalf("ParseExpect: %v", err) + } + tc := TestCase{ID: "test_1", Name: "two expects", MDL: "$r = 2;", Expects: []Expect{e1, e2}} + return &TestSuite{Name: "S", Tests: []TestCase{tc}} +} + +// verdictLog is a runner log for test_1 with the given verdict lines between +// its RUN line and the suite's END. +func verdictLog(lines ...string) string { + var b strings.Builder + b.WriteString("MXTEST: MXTEST:START:S\n") + b.WriteString("MXTEST: MXTEST:RUN:test_1:two expects\n") + for _, l := range lines { + b.WriteString("MXTEST: MXTEST:" + l + "\n") + } + b.WriteString("MXTEST: MXTEST:END:S\n") + return b.String() +} + +// TestLaterPassDoesNotClearEarlierFailure is ako/mxcli#1753: on the after-startup +// runner a test with more than one @expect reported PASS when an earlier +// assertion failed and a later one held, and `mxcli test` exited 0. Each +// assertion logs its own verdict line, and the parser let every line overwrite +// the test's status — the last one won, and a PASS also cleared the failure +// message. The endpoint runner evaluates an assertion only while the verdict is +// still PASS, so there the first failure sticks; the log parser now does the +// same. +func TestLaterPassDoesNotClearEarlierFailure(t *testing.T) { + const want = "Expected $r = 1, actual: 2" + + got := ParseLogResults(strings.NewReader(verdictLog("FAIL:test_1:"+want, "PASS:test_1")), + twoExpectSuite(t), false) + r := got.Tests[0] + if r.Status != StatusFail { + t.Errorf("FAIL then PASS: status = %v, want FAIL", r.Status) + } + if r.Message != want { + t.Errorf("FAIL then PASS: message = %q, want %q", r.Message, want) + } + if got.AllPassed() { + t.Error("FAIL then PASS: the suite passed, so `mxcli test` exits 0") + } + + // Control: the order the old parser already got right. + got = ParseLogResults(strings.NewReader(verdictLog("PASS:test_1", "FAIL:test_1:"+want)), + twoExpectSuite(t), false) + if r := got.Tests[0]; r.Status != StatusFail || r.Message != want { + t.Errorf("control, PASS then FAIL: got %v %q, want FAIL %q", r.Status, r.Message, want) + } + + // Control: every assertion held. + got = ParseLogResults(strings.NewReader(verdictLog("PASS:test_1", "PASS:test_1")), + twoExpectSuite(t), false) + if r := got.Tests[0]; r.Status != StatusPass || r.Message != "" { + t.Errorf("control, PASS then PASS: got %v %q, want PASS", r.Status, r.Message) + } +} + +// TestFirstFailureMessageIsReported pins which message a test that failed more +// than once reports: the first, as the endpoint runner's $Verdict does, with a +// count of the rest so the reader knows the log holds more than the summary. +func TestFirstFailureMessageIsReported(t *testing.T) { + got := ParseLogResults(strings.NewReader(verdictLog( + "FAIL:test_1:Expected $r = 1, actual: 2", + "FAIL:test_1:Expected $r = 3, actual: 2", + "PASS:test_1", + "FAIL:test_1:Expected $r = 4, actual: 2", + )), twoExpectSuite(t), false) + r := got.Tests[0] + if r.Status != StatusFail { + t.Fatalf("status = %v, want FAIL", r.Status) + } + const want = "Expected $r = 1, actual: 2 (and 2 more failures in this test)" + if r.Message != want { + t.Errorf("message = %q, want %q", r.Message, want) + } +} + +// TestSkippedAssertionsFailureStillWins pins the guard writeTestBlock emits after +// the assertions: when a call's own handler resumed the flow past some of them, +// the assertions that did run log PASS and the guard logs a FAIL after them +// (ako/mxcli#1448). That FAIL is the first failure, so it is the verdict. +func TestSkippedAssertionsFailureStillWins(t *testing.T) { + got := ParseLogResults(strings.NewReader(verdictLog( + "PASS:test_1", + "FAIL:test_1:"+assertionsSkippedReason, + )), twoExpectSuite(t), false) + if r := got.Tests[0]; r.Status != StatusFail || r.Message != assertionsSkippedReason { + t.Errorf("got %v %q, want FAIL with the skipped-assertions reason", r.Status, r.Message) + } +} + +// TestLaterPassDoesNotClearEarlierError: a setup that threw logs ERROR, and no +// verdict line after it may turn the test into a pass or a skip either. +func TestLaterPassDoesNotClearEarlierError(t *testing.T) { + const msg = "Setup failed: Shop.Seed" + for _, later := range []string{"PASS:test_1", "SKIP:test_1:skipped"} { + got := ParseLogResults(strings.NewReader(verdictLog("ERROR:test_1:"+msg, later)), + twoExpectSuite(t), false) + if r := got.Tests[0]; r.Status != StatusError || r.Message != msg { + t.Errorf("ERROR then %s: got %v %q, want ERROR %q", later, r.Status, r.Message, msg) + } + } +} + +// TestRunLineStartsVerdictAfresh: a failure sticks for the run it was logged in, +// not for the rest of the log. A RUN line begins a test's verdict, so a log that +// holds the same test twice reports the later run. +func TestRunLineStartsVerdictAfresh(t *testing.T) { + log := verdictLog("FAIL:test_1:Expected $r = 1, actual: 2", "RUN:test_1:two expects", "PASS:test_1", "PASS:test_1") + got := ParseLogResults(strings.NewReader(log), twoExpectSuite(t), false) + if r := got.Tests[0]; r.Status != StatusPass { + t.Errorf("got %v %q, want the second run's PASS", r.Status, r.Message) + } +} diff --git a/docs-site/src/tools/test-annotations.md b/docs-site/src/tools/test-annotations.md index 4cd744b27..65f045448 100644 --- a/docs-site/src/tools/test-annotations.md +++ b/docs-site/src/tools/test-annotations.md @@ -77,6 +77,10 @@ FAIL the board is 81 squares expected length($result) = 81, actual: 27 ``` +A test with several `@expect` lines passes only when every one of them holds, and +it reports the first that does not. The Docker / `--legacy-runner` path evaluates +them all, and adds how many more failed — `(and 1 more failure in this test)`. + `@expect` cannot be combined with `@throws`: a body expected to fail produces no result to assert on, so the combination is refused rather than ignored. diff --git a/mdl-examples/bug-tests/test-runner-1753-earlier-expect-fails.test.mdl b/mdl-examples/bug-tests/test-runner-1753-earlier-expect-fails.test.mdl new file mode 100644 index 000000000..02f7ab684 --- /dev/null +++ b/mdl-examples/bug-tests/test-runner-1753-earlier-expect-fails.test.mdl @@ -0,0 +1,55 @@ +-- ako/mxcli#1753: on the after-startup runner (every Docker run, and --local +-- --legacy-runner) a test with more than one @expect was reported PASS when an +-- earlier assertion failed and a later one held, and `mxcli test` exited 0. +-- Each assertion logs its own PASS or FAIL line, and the log parser let the +-- last line be the test's verdict. A failure now sticks, and the first one is +-- the message reported. +-- +-- Setup: test-runner-1514-setup.mdl (Probe.SUB_Make returns 'a;b'). Then: +-- +-- mxcli test mdl-examples/bug-tests/test-runner-1753-earlier-expect-fails.test.mdl -p app.mpr --local --legacy-runner +-- +-- Expected: test 1 FAIL "Expected $a = 'zzz', actual: a;b" (before the fix: +-- PASS), test 2 FAIL with the same message for $b (the control — the order the +-- parser always got right), test 3 FAIL "Expected $c = 'zzz', actual: a;b (and +-- 1 more failure in this test)", test 4 PASS. The endpoint runner (--local +-- alone) evaluates an assertion only while the test is still passing, so it +-- reported test 1 correctly before the fix too. +-- +-- The log parser's verdicts are pinned by cmd/mxcli/testrunner/ +-- sticky_failure_test.go; this file is the runtime reproduction and was not run +-- against an app for the fix. Each test uses its own variable so the file does +-- not depend on the runner renaming a shared one. +mdl 1; +/** + * @test 1 earlier assertion fails, later one holds + * @expect $a = 'zzz' + * @expect $a = 'a;b' + */ +$a = call microflow Probe.SUB_Make(); +/ + +/** + * @test 2 earlier assertion holds, later one fails + * @expect $b = 'a;b' + * @expect $b = 'zzz' + */ +$b = call microflow Probe.SUB_Make(); +/ + +/** + * @test 3 two assertions fail, one holds + * @expect $c = 'zzz' + * @expect $c = 'a;b' + * @expect $c = 'yyy' + */ +$c = call microflow Probe.SUB_Make(); +/ + +/** + * @test 4 every assertion holds + * @expect $d = 'a;b' + * @expect length($d) = 3 + */ +$d = call microflow Probe.SUB_Make(); +/ From 819d3ceb2d56a7fd294f2f60bbc0342f21a3c80e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 17:15:45 +0000 Subject: [PATCH 02/18] fix(test): rename a lower-case `call`/`create` variable per test on the after-startup runner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Symptom, from ako/mxcli#1753's sibling note: "on this runner, `assignPattern` matches only upper-case `CALL|CREATE`. So mdl 1's `$r = call …` is not renamed per test, and two tests that both use `$r` fail injection (MDL063 / CE0111, measured)." Affected: every Docker run and `--local --legacy-runner`. The endpoint runner gives each test its own flow and does not rename. Cause: the legacy runner compiles every test into one microflow, so it gives each test's variables a `_N` suffix. assignPattern decides which variables are renamed, and it matched only `CALL|CREATE` in upper case. MDL keywords are case-insensitive, and mdl 1 and `fmt` write them in lower case, so `$r = call microflow …` stayed `$r` in every test. Fix: assignPattern matches `call|create` in any case, followed by a word boundary so `$r = callback` does not count. No other pattern in cmd/mxcli/testrunner makes the upper-case assumption: the rest lower-case their input or use EqualFold. TestThrowsCatchesEveryCallKind and TestVerdictCatchesEveryCallKind matched the legacy output against the unrenamed lower-case statement, so they passed only because of the defect. Their legacy assertions now expect the renamed statement (legacyStmt). The 1448 bug-test's note that the legacy runner "needs a distinct variable name per test" is updated. Measurement. rename_keyword_case_test.go with the old pattern (the upper-case control subtest passes): --- FAIL: TestLegacyRunnerRenamesLowerCaseAssignments/call_microflow,_mdl_1 test 1: no "$r_1 = call microflow" — the assignment was not renamed an assertion still reads the unrenamed $r MDL063: '$r' is created twice in this microflow — first by call microflow, then by call microflow. ... mxbuild rejects this with CE0111 "Duplicate variable name" (and the same for mdl 0, call java action, create object, create list, mixed case) --- FAIL: TestAssignPatternNeedsAWholeKeyword control: $r = call microflow was not picked up: map[] I also swept every non-.fail `.test.mdl` in mdl-examples/ (16 files) through GenerateTestRunner and executor.ValidateProgram. Before the fix, 10 files drew MDL063, among them the 1448, 1514 and 923 bug-tests and doctype-tests/microflow-spec (86). After it, 2 do. Those 2 come from `retrieve $Brands` and from `retrieve $Request` / `$Request = head(…)`. They are a separate gap: assignPattern has never matched `retrieve`, `declare`, list operations, `send rest request` or `execute database query` in any case. This change does not address it. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82sUVQEB3K5W5vh85wM --- ...-lower-case-call-variable-not-renamed.json | 1 + CHANGELOG.md | 1 + cmd/mxcli/testrunner/generator.go | 10 +- .../testrunner/on_error_coverage_test.go | 13 ++- .../testrunner/rename_keyword_case_test.go | 94 +++++++++++++++++++ ...est-runner-1448-own-error-handler.test.mdl | 7 +- 6 files changed, 120 insertions(+), 6 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-legacy-runner-lower-case-call-variable-not-renamed.json create mode 100644 cmd/mxcli/testrunner/rename_keyword_case_test.go diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-legacy-runner-lower-case-call-variable-not-renamed.json b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-legacy-runner-lower-case-call-variable-not-renamed.json new file mode 100644 index 000000000..cd1b37502 --- /dev/null +++ b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-legacy-runner-lower-case-call-variable-not-renamed.json @@ -0,0 +1 @@ +{"area": "cmd/mxcli", "date": "2026-10-28", "symptom": "On the after-startup runner (Docker, `--local --legacy-runner`) two tests in one file that both assign the same variable with a lower-case keyword — `$r = call microflow …`, mdl 1's and `fmt`'s spelling — fail injection with MDL063 (CE0111 \"Duplicate variable name\" from mxbuild); the same file runs under the endpoint runner, which gives each test its own flow", "cause": "The legacy runner compiles every test into one microflow and renames each test's variables with a `_N` suffix, but which variables to rename was decided by `assignPattern`, which matched only upper-case `CALL|CREATE` — the spelling of the runner's own early examples. A lower-case assignment was left unrenamed and declared once per test", "file": "`cmd/mxcli/testrunner/generator.go` (`assignPattern`, now `(?mi)…(?:call|create)\\b`)", "insight": "The cheapest detector was a sweep, not a reading: every `.test.mdl` in the repo through `GenerateTestRunner` and `executor.ValidateProgram` — 10 of the 16 in mdl-examples/ drew MDL063 before the fix, 2 after. Those 2 (`retrieve $Brands`, `retrieve $Request` + `$Request = head(…)`) are the remaining, separate gap: `assignPattern` never matched `retrieve $x`, `declare $x`, list operations (`$x = filter(…)`), `send rest request` or `execute database query` in ANY case, so a variable those define in two tests still collides; the durable fix is to take the declared names from the parsed body (`visitor.FlowStatements`, as `attachOnError` already does), not a regex. Two existing tests (`TestThrowsCatchesEveryCallKind`, `TestVerdictCatchesEveryCallKind`) matched the legacy output against the unrenamed lower-case statement and passed only because of the defect — a test that asserts a generator's text verbatim can encode the bug it should catch. The `\\b` matters once the match is case-insensitive (`$r = callback`). Found alongside ako/mxcli#1753, whose bug-test therefore uses a distinct variable per test", "refs": ["ako/mxcli#1753"], "rules": ["MDL063"], "ce": ["CE0111"]} diff --git a/CHANGELOG.md b/CHANGELOG.md index 03eb4926e..093bf72ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - **A navigation profile's application title and icon can be set and are described** (ako/mxcli#1756, mendixlabs/mxcli#1664) — the title (Studio Pro's *Application title*, `Mendix` in a new project) is what the runtime puts in every browser tab as `Mendix - `, and it had no MDL form: `create or modify navigation Responsive app title 'EWO' …` failed with "extraneous input 'app'", `describe navigation` printed neither it nor the *Application icon*, and `create or modify translations … for en_US` refuses the source language, so every generated app kept "Mendix" in the tab. `create or modify navigation` now takes `app title ''` and `app icon ` (`app icon none` clears it), in any order with the other clauses; `app` and `icon` stay usable as names. The title is written as the project default language's item, the title's other languages kept; the icon is resolved like any image reference, by `check --references` and before exec writes; an omitted clause keeps the stored value; describe prints both. A native profile has neither, so both are refused there. - **A test whose earlier `@expect` fails is no longer reported PASS on the after-startup runner** (ako/mxcli#1753) — every Docker run and `--local --legacy-runner` logged a verdict line per assertion, and the result was taken from the last one: a failing first assertion followed by a passing second reported PASS, with the failure message erased, and `mxcli test` exited 0. A failure now sticks, and the test reports the first one, plus how many more failed. The endpoint runner (`--local`, `--attach`, `--watch`) was not affected. +- **Two tests that assign the same variable with a lower-case `call` or `create` run on the after-startup runner** (found with ako/mxcli#1753) — that runner compiles every test into one microflow and renames each test's variables, but it recognised only an upper-case `CALL` or `CREATE`. `$r = call microflow …`, the spelling mdl 1 and `fmt` write, was left as it was, so two tests assigning `$r` declared it twice and injection failed with MDL063 (CE0111 from mxbuild). 8 of the 16 test files in `mdl-examples/` hit this under Docker or `--legacy-runner`. A variable first assigned by `retrieve`, `declare` or a list operation is still not renamed there. - **Pages on Mendix 9.24 and 10.x are read and written in the version's own shape** (ako/mxcli#1209) — several page properties are stored differently below a version: a design property's value before 10.2, a data view's page parameter before 10.0, its editability before 10.7, and a text box's placeholder before 10.11. `describe` read only the newer form, so on 9.24 a describe → exec round trip removed every design property and placeholder. Writing any page with a data view or a design property was refused, and `alter styling` on a stored design property reported success and changed nothing. Every write now converts to the project's form, and `describe` reads both. What the older form cannot hold is refused, naming the widget: a Spacing or colour design property, conditional editability, a placeholder with parameters. - **A text box's `KeyboardType` survives describe → exec** (ako/mxcli#1209) — the keyboard a native app opens had no MDL spelling and was written as `Default`, so a rewrite reset it. `textbox t (Attribute: Email, KeyboardType: EmailAddress)` (`Default`, `NumberPad`, `DecimalPad`, `EmailAddress`, `PhonePad`, `URL`); an unknown value is MDL-WIDGET18. - **A button's workflow action survives a rewrite** (ako/mxcli#832) — `describe` printed no action for Show user task page, Show workflow admin page or Call workflow, so re-executing a page turned them into no action (on ako/TestApp's TaskInbox and TaskDashboard, 2 of 6 user-task buttons each). They are now `open user task [with (AssignOnOpen, OpenWhenAssigned)]`, `open workflow with (DefaultPage: M.Page)` and `call workflow M.WF [with (…)]`. `complete task` takes `with (ClosePage, Commit)` instead of always writing both true. `open workflow` without a page is CE1834 and is refused. diff --git a/cmd/mxcli/testrunner/generator.go b/cmd/mxcli/testrunner/generator.go index 674b81b3d..0e347a7f9 100644 --- a/cmd/mxcli/testrunner/generator.go +++ b/cmd/mxcli/testrunner/generator.go @@ -273,9 +273,15 @@ func writeExpectAssertion(b *strings.Builder, testID string, exp Expect, evaluat // varPattern matches $VariableName in MDL ($ followed by word characters). var varPattern = regexp.MustCompile(`\$([A-Za-z_][A-Za-z0-9_]*)`) -// assignPattern matches "$var = CALL" or "$var = CREATE" at the start of any +// assignPattern matches "$var = call …" or "$var = create …" at the start of any // statement line inside the test body. -var assignPattern = regexp.MustCompile(`(?m)^\s*\$([A-Za-z_][A-Za-z0-9_]*)\s*=\s*(?:CALL|CREATE)`) +// +// The keywords match in any case. MDL keywords are case-insensitive, and mdl 1 +// and `fmt` write them in lower case; matching only `CALL|CREATE` left +// `$r = call microflow …` unrenamed, so two tests that both assigned `$r` declared +// it twice in the one runner flow and injection failed with MDL063 (CE0111). +// The `\b` keeps a value that merely starts with those letters from matching. +var assignPattern = regexp.MustCompile(`(?mi)^\s*\$([A-Za-z_][A-Za-z0-9_]*)\s*=\s*(?:call|create)\b`) // extractVariableNames finds all user-defined variable names in the MDL body. // Returns the set of variable names (without $ prefix). diff --git a/cmd/mxcli/testrunner/on_error_coverage_test.go b/cmd/mxcli/testrunner/on_error_coverage_test.go index d190814ac..669a89858 100644 --- a/cmd/mxcli/testrunner/on_error_coverage_test.go +++ b/cmd/mxcli/testrunner/on_error_coverage_test.go @@ -36,7 +36,7 @@ func TestThrowsCatchesEveryCallKind(t *testing.T) { t.Run(c.kind, func(t *testing.T) { tc := parseOne(t, "/**\n * @test it raises\n * @throws 'Only http and https csv urls are supported'\n */\n"+c.stmt+"\n/\n") assertHandled(t, "endpoint runner", GenerateTestFlows(&TestSuite{Name: "s", Tests: []TestCase{tc}}), c.stmt) - assertHandled(t, "legacy runner", GenerateTestRunner(&TestSuite{Name: "s", Tests: []TestCase{tc}}), c.stmt) + assertHandled(t, "legacy runner", GenerateTestRunner(&TestSuite{Name: "s", Tests: []TestCase{tc}}), legacyStmt(c.stmt)) }) } } @@ -53,7 +53,7 @@ func TestVerdictCatchesEveryCallKind(t *testing.T) { if !strings.Contains(mdl, verdictFailPrefix+"exception during execution: ") { t.Errorf("no FAIL verdict in the handler:\n%s", mdl) } - assertHandled(t, "legacy runner", GenerateTestRunner(&TestSuite{Name: "s", Tests: []TestCase{tc}}), c.stmt) + assertHandled(t, "legacy runner", GenerateTestRunner(&TestSuite{Name: "s", Tests: []TestCase{tc}}), legacyStmt(c.stmt)) }) } } @@ -78,3 +78,12 @@ func assertHandled(t *testing.T, runner, mdl, stmt string) { t.Errorf("%s: generated MDL does not parse: %v\n%s", runner, errs[0], mdl) } } + +// legacyStmt is stmt as the legacy runner writes it for the suite's first test: +// that runner compiles every test into one flow, so it renames the variable a +// call assigns with the test's suffix. These statements used to be matched +// unrenamed, which held only because a lower-case `call` was not renamed +// (TestLegacyRunnerRenamesLowerCaseAssignments). +func legacyStmt(stmt string) string { + return renameVariables(stmt, extractVariableNames(stmt), "_1") +} diff --git a/cmd/mxcli/testrunner/rename_keyword_case_test.go b/cmd/mxcli/testrunner/rename_keyword_case_test.go new file mode 100644 index 000000000..ef6384724 --- /dev/null +++ b/cmd/mxcli/testrunner/rename_keyword_case_test.go @@ -0,0 +1,94 @@ +// SPDX-License-Identifier: Apache-2.0 + +package testrunner + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/executor" + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +// TestLegacyRunnerRenamesLowerCaseAssignments: the after-startup runner compiles +// every test into one microflow, so a variable two tests both assign has to be +// renamed per test. Which variables are renamed was decided by a pattern that +// matched only upper-case `CALL` and `CREATE` — the spelling the runner's own +// examples used — so `$r = call microflow …`, how mdl 1 and `fmt` spell it, was +// not renamed. Two tests that both assign `$r` then declare it twice, and +// injection fails with MDL063 (CE0111 "Duplicate variable name" from mxbuild). +// Found alongside ako/mxcli#1753. +func TestLegacyRunnerRenamesLowerCaseAssignments(t *testing.T) { + cases := []struct { + name, header, body, renamed string + }{ + {"call microflow, mdl 1", "mdl 1;\n", + "$r = call microflow Probe.SUB_Make();", "$r_%d = call microflow"}, + {"call microflow, mdl 0", "", + "$r = call microflow Probe.SUB_Make();", "$r_%d = call microflow"}, + {"call java action", "mdl 1;\n", + "$r = call java action Probe.JA_Upper(Text = 'a');", "$r_%d = call java action"}, + {"create object", "mdl 1;\n", + "$r = create Probe.Item (Name = 'a');", "$r_%d = create Probe.Item"}, + {"create list", "mdl 1;\n", + "$r = create list of Probe.Item;", "$r_%d = create list of"}, + {"mixed case", "mdl 1;\n", + "$r = Call Microflow Probe.SUB_Make();", "$r_%d = Call Microflow"}, + // Control: the spelling the pattern always matched. + {"upper case (control)", "", + "$r = CALL MICROFLOW Probe.SUB_Make();", "$r_%d = CALL MICROFLOW"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + block := func(n string) string { + return "/**\n * @test " + n + "\n * @expect $r != empty\n */\n" + c.body + "\n/\n" + } + tests, err := parseMDLTests(c.header+block("one")+"\n"+block("two"), "rename.test.mdl") + if err != nil { + t.Fatalf("parse: %v", err) + } + src := GenerateTestRunner(&TestSuite{Name: "s", Tests: tests}) + + for i, want := range []string{ + strings.Replace(c.renamed, "%d", "1", 1), + strings.Replace(c.renamed, "%d", "2", 1), + } { + if !strings.Contains(src, want) { + t.Errorf("test %d: no %q — the assignment was not renamed", i+1, want) + } + } + if strings.Contains(src, "($r != empty)") { + t.Errorf("an assertion still reads the unrenamed $r") + } + + prog, errs := visitor.Build(src) + if len(errs) > 0 { + t.Fatalf("generated runner does not parse: %v\n%s", errs[0], src) + } + for _, v := range executor.ValidateProgram(prog, "") { + if v.RuleID == "MDL063" { + t.Errorf("%s: %s", v.RuleID, v.Message) + } + } + if t.Failed() { + t.Logf("generated runner:\n%s", src) + } + }) + } +} + +// TestAssignPatternNeedsAWholeKeyword: matching case-insensitively must not turn +// an assignment whose value merely starts with the letters of a keyword into one. +func TestAssignPatternNeedsAWholeKeyword(t *testing.T) { + for _, body := range []string{ + "$r = callback;", + "$r = createdDate;", + } { + if names := extractVariableNames(body); len(names) != 0 { + t.Errorf("%q: renamed %v, want nothing", body, names) + } + } + if names := extractVariableNames("$r = call microflow M.F();"); !names["r"] { + t.Errorf("control: $r = call microflow was not picked up: %v", names) + } +} diff --git a/mdl-examples/bug-tests/test-runner-1448-own-error-handler.test.mdl b/mdl-examples/bug-tests/test-runner-1448-own-error-handler.test.mdl index 7bf7545c7..1987183ff 100644 --- a/mdl-examples/bug-tests/test-runner-1448-own-error-handler.test.mdl +++ b/mdl-examples/bug-tests/test-runner-1448-own-error-handler.test.mdl @@ -31,8 +31,11 @@ -- -- Under --legacy-runner a test whose error leaves the runner (05, 06, 11) ends -- the whole run; 05 was reported PASS there (a RUN line with no verdict read as --- the zero status) and is now an ERROR. That runner also needs a distinct --- variable name per test. +-- the zero status) and is now an ERROR. That runner also needed a distinct +-- variable name per test: it renamed a variable per test only after an +-- upper-case `CALL`, so this file's `$r` and `$Result` drew 14 MDL063 +-- duplicate-variable errors there. It renames one after a lower-case `call` too +-- since the sibling fix to ako/mxcli#1753. mdl 1; /** * @test 01 plain call that throws From 68e0d381ea3c624fb46b061ce00b24d234827658 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 17:50:32 +0000 Subject: [PATCH 03/18] fix(security): keep a module role's description when create or modify omits it Symptom (ako/mxcli#1218): a bare `create or modify module role M.R;` reported "Modified module role: M.R" and erased the role's stored description. `describe` -> `exec` was unaffected because describe prints the description; grants were untouched. Cause: CreateModuleRoleStmt.Description was a plain string, so "no description clause" and `description ''` were the same value, and the OR MODIFY branch of execCreateModuleRole passed it to AddModuleRole, which overwrites the existing role in place. Fix: Description is a *string, nil when the statement has no clause, as CreateUserRoleStmt's properties have been since ako/mxcli#707. OR MODIFY writes the stored description when it is nil, so the write is elided and the run reports "Unchanged"; `description ''` clears it. Adopting an auto-provisioned role still drops its marker description. Docs (skill, docs-site reference, `mxcli syntax`) state the rule. Measurement: - Unit test (mdl/executor/issue1218_module_role_description_test.go) fails before the fix, and with the fix forced back, with the reported symptom: description written = "", want "Read the published OData services. Nothing else." / Modified module role: Shop.ApiUser. - Real app, Mendix 11.13.0: role created with a description; bare statement with the fixed build -> "Unchanged module role", describe keeps the description, describe -> exec "Unchanged"; with the pre-fix build -> "Modified module role" and the description is empty. `description ''` with the fixed build clears it. mx check: 0 errors in every state, the same as the blank app, fault included. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82sUVQEB3K5W5vh85wM --- ...e-create-or-modify-erases-description.json | 1 + .../skills/mendix/manage-security/SKILL.md | 12 ++- CHANGELOG.md | 1 + cmd/mxcli/syntax/features_security.go | 2 +- .../reference/security/create-module-role.md | 12 ++- .../1218-module-role-description-kept.mdl | 34 +++++++ mdl/ast/ast_security.go | 15 ++- mdl/executor/cmd_security_write.go | 16 +++- .../issue1218_module_role_description_test.go | 94 +++++++++++++++++++ .../issue707_describe_roundtrip_test.go | 4 +- .../module_role_create_statement_test.go | 2 +- mdl/visitor/visitor_security.go | 5 +- .../visitor_security_or_modify_test.go | 4 +- mdl/visitor/visitor_security_test.go | 25 ++++- 14 files changed, 203 insertions(+), 24 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-10-28-module-role-bare-create-or-modify-erases-description.json create mode 100644 mdl-examples/bug-tests/1218-module-role-description-kept.mdl create mode 100644 mdl/executor/issue1218_module_role_description_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-28-module-role-bare-create-or-modify-erases-description.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-28-module-role-bare-create-or-modify-erases-description.json new file mode 100644 index 000000000..cae865287 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-28-module-role-bare-create-or-modify-erases-description.json @@ -0,0 +1 @@ +{"area":"mdl/executor","date":"2026-10-28","symptom":"A bare `create or modify module role M.R;` (no description clause) reported `Modified module role: M.R` and erased the role's stored Description; `describe` → `exec` was unaffected because describe prints the description, and grants (on the entity, keyed by role name) were untouched.","cause":"CreateModuleRoleStmt.Description was a plain string, so 'no clause' and `description ''` were the same value, and execCreateModuleRole's OR MODIFY branch passed it to Backend.AddModuleRole, which overwrites the existing role in place.","file":"mdl/ast/ast_security.go (Description *string), mdl/visitor/visitor_security.go, mdl/executor/cmd_security_write.go (execCreateModuleRole)","fix":"Description is a *string, nil when unstated, as CreateUserRoleStmt's properties have been since ako/mxcli#707; OR MODIFY writes the stored description when it is nil, so the write is elided and the run reports Unchanged. `description ''` clears it. Adopting an auto-provisioned role still drops its marker description.","insight":"A neighbouring statement already had the right shape: the user-role code even said 'the rest keep their stored values, as a module role's missing description does', which was false — a comment asserting a sibling's behaviour is a claim to test, not evidence. The tell for this class is an optional clause parsed into a value type: grep the AST for optional clauses stored as string/bool without a *T or a ...Set flag, then follow each to an OR MODIFY branch. mx check is 0 errors with the fault (measured, 11.13.0), and describe → exec cannot show it because describe restates the value; only a hand-written minimal statement does.","refs":["ako/mxcli#1218"],"test":"mdl/executor/issue1218_module_role_description_test.go, mdl/visitor/visitor_security_test.go"} diff --git a/.claude/skills/mendix/manage-security/SKILL.md b/.claude/skills/mendix/manage-security/SKILL.md index 9f294a433..e75335f11 100644 --- a/.claude/skills/mendix/manage-security/SKILL.md +++ b/.claude/skills/mendix/manage-security/SKILL.md @@ -13,11 +13,11 @@ description: "Configure Mendix security in MDL — module and user roles, access | 27-35 | When to Use This Skill | | 36-43 | Security Concepts | | 44-120 | The System-module ceiling — decide the role model around this FIRST | -| 121-558 | Syntax Reference | -| 559-573 | Starlark Lint Rule APIs | -| 574-626 | Common Workflow: Setting Up Module Security | -| 627-636 | Common Mistakes | -| 637-646 | Validation | +| 121-560 | Syntax Reference | +| 561-575 | Starlark Lint Rule APIs | +| 576-628 | Common Workflow: Setting Up Module Security | +| 629-638 | Common Mistakes | +| 639-648 | Validation | # Security Management Skill @@ -205,7 +205,9 @@ create module role MyModule.Viewer description 'Read-only access'; -- `or modify` updates an existing role's description instead of failing, so the -- whole security script stays re-runnable rather than needing a run-once file. +-- Without `description` it keeps the stored one; `description ''` clears it. create or modify module role MyModule.ApiUser description 'API consumer'; +create or modify module role MyModule.Admin; -- ensures it exists, keeps its description -- Remove a module role (`if exists` makes it a no-op when the role is gone) drop module role MyModule.Viewer; diff --git a/CHANGELOG.md b/CHANGELOG.md index 093bf72ec..a947aaac4 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/). - **`check -p --references` reports CE1406 for a retrieve, create or create list of a Hidden entity of a Protected module** (ako/mxcli#1699, mendixlabs/mxcli#1652) — a flow in an app module that ran `retrieve $l from AddOn.Internal`, `$o = create AddOn.Internal` or `$l = create list of AddOn.Internal` on a Hidden entity of a Protected (add-on) module passed `check` and `exec`, and mxbuild refused it: `[CE1406] "Retrieve object(s) activity 'Retrieve list of Internal from database' is not allowed to use entity 'AddOn.Internal' because app modules cannot reference hidden elements in add-on modules."` (likewise "Create object activity", "Create list activity"). The check judged an entity only as a parameter or return type; it now judges the entity these three activities name, in microflows and nanoflows, and `exec` refuses the script before writing it. Measured on Mendix 11.13.0: each shape refused against a Hidden entity, 0 errors against a Usable one and inside the add-on; an association retrieve that reaches a Hidden entity over a Usable association names only the association and is not refused. - **A pluggable widget's text-template parameter across an association builds, and describes so it re-executes** (mendixlabs/mxcli#1494) — `fieldset fs (legend: '{1}', legendParams: ({1} = M.Parent_Child/Label))` passed `check` and `exec` and failed the build with `[CE1365] "Attribute 'M.Child.Label' is not an attribute of entity 'M.Parent'."`: the parameter was written with its attribute but without the association hop, so mxbuild read the attribute against the widget's own entity. The same writer serves an object-list item's template (an Accordion group's `headerText`) and a Data grid 2 column's dynamic text, and all three failed; on Mendix 11.12.4 they now build with 0 errors. `describe page` printed such a parameter as the bare attribute (`{1} = Label`), which re-executed into CE1613 "The selected attribute 'M.Parent.Label' no longer exists"; it now prints `M.Parent_Child/Label`. The companion report, an expression parameter (`{1} = if 1 = 1 then 'A' else 'B'`) stored as an attribute name (mendixlabs/mxcli#1495), was fixed in v0.25.0 (#823) and is now pinned by a test. - **`alter entity … add event handler` adds a second handler for the same event** (mendixlabs/mxcli#1540) — `add event handler on before commit call M.Second($currentObject)` failed with "event handler already exists for Before Commit" when the entity already had one, though Studio Pro allows several handlers for one event and `create or modify entity` with two `on before commit` clauses stores both (measured on 11.12.4: two Before Commit handlers are 0 errors, a void-returning one is CE7247, so the check does look at them). A handler is now identified by its moment, event and microflow: another microflow adds a second handler, the same microflow again is still a duplicate. The same key had made `add event handler if not exists … call M.New` skip — reporting "already exists" — whenever any handler for the event existed, and `drop event handler on before commit` remove whichever handler was stored first. `drop event handler` now takes `call M.Mf` to say which one; without it, a drop that matches several handlers is refused and names them. +- **A bare `create or modify module role M.R;` keeps the role's description** (ako/mxcli#1218) — the "make sure this role exists" statement reported `Modified module role: M.R` and erased the stored description: a statement without `description` was written as an empty one. An omitted `description` now keeps what is stored, as an omitted user role property already did, and the run reports `Unchanged`; `description ''` clears it. Grants were never affected, and `describe` → `exec` was not, since `describe` prints the description. Measured on Mendix 11.13.0: `mx check` is 0 errors with and without the fault, so only the stored description showed it. ## [0.28.0] - 2026-10-23 diff --git a/cmd/mxcli/syntax/features_security.go b/cmd/mxcli/syntax/features_security.go index 83372b349..698f5a7d4 100644 --- a/cmd/mxcli/syntax/features_security.go +++ b/cmd/mxcli/syntax/features_security.go @@ -22,7 +22,7 @@ func init() { "module role", "create role", "drop role", }, Syntax: "CREATE [OR MODIFY] MODULE ROLE . [DESCRIPTION ''];\nDROP MODULE ROLE .;", - Example: "mdl 1;\nCREATE MODULE ROLE Shop.Admin DESCRIPTION 'Full access';\n-- OR MODIFY makes a security script re-runnable:\nCREATE OR MODIFY MODULE ROLE Shop.User DESCRIPTION 'Read-only access';", + Example: "mdl 1;\nCREATE MODULE ROLE Shop.Admin DESCRIPTION 'Full access';\n-- OR MODIFY makes a security script re-runnable:\nCREATE OR MODIFY MODULE ROLE Shop.User DESCRIPTION 'Read-only access';\n-- Without DESCRIPTION it keeps the stored one; DESCRIPTION '' clears it:\nCREATE OR MODIFY MODULE ROLE Shop.Admin;", SeeAlso: []string{"security.user-role", "security.entity-access"}, }) diff --git a/docs-site/src/reference/security/create-module-role.md b/docs-site/src/reference/security/create-module-role.md index 13ed8b972..6a073d25c 100644 --- a/docs-site/src/reference/security/create-module-role.md +++ b/docs-site/src/reference/security/create-module-role.md @@ -3,7 +3,7 @@ ## Synopsis ```sql -CREATE MODULE ROLE module.RoleName [ DESCRIPTION 'text' ] +CREATE [ OR MODIFY ] MODULE ROLE module.RoleName [ DESCRIPTION 'text' ] ``` ## Description @@ -12,7 +12,7 @@ Creates a new module role within a module. Module roles are the foundation of Me Each module role belongs to exactly one module. The role name must be unique within the module. By convention, common role names include `User`, `Admin`, `Viewer`, and `Manager`. -If the module does not exist, the statement returns an error. +If the module does not exist, the statement returns an error. If the role exists, `CREATE MODULE ROLE` returns an error; `CREATE OR MODIFY` updates it instead, so a security script can be re-run. Without `DESCRIPTION`, `CREATE OR MODIFY` keeps the role's stored description; `DESCRIPTION ''` clears it. ## Parameters @@ -20,7 +20,7 @@ If the module does not exist, the statement returns an error. : Qualified name consisting of the module name and the new role name, separated by a dot. The module must already exist. `DESCRIPTION 'text'` -: Optional human-readable description of the role's purpose. +: Optional human-readable description of the role's purpose. Left out, a new role has none and an existing role keeps its own. ## Examples @@ -45,6 +45,12 @@ CREATE MODULE ROLE HR.Employee DESCRIPTION 'Self-service access'; CREATE MODULE ROLE HR.Viewer DESCRIPTION 'Read-only access'; ``` +Make sure a role exists, keeping whatever description it has: + +```sql +CREATE OR MODIFY MODULE ROLE HR.Manager; +``` + ## See Also [CREATE USER ROLE](create-user-role.md), [GRANT](grant.md), [REVOKE](revoke.md) diff --git a/mdl-examples/bug-tests/1218-module-role-description-kept.mdl b/mdl-examples/bug-tests/1218-module-role-description-kept.mdl new file mode 100644 index 000000000..417838328 --- /dev/null +++ b/mdl-examples/bug-tests/1218-module-role-description-kept.mdl @@ -0,0 +1,34 @@ +mdl 1; +-- ako/mxcli#1218: a bare `create or modify module role M.R;` silently erased +-- the role's description. +-- +-- Reported: +-- $ mxcli exec probe.mdl -p Formula1Backend.mpr +-- Modified module role: Formula1Backend.ApiUser +-- and `show module roles` had the role's Description empty afterwards. The +-- executor handed the statement's empty description to the backend, which +-- overwrites; an omitted DESCRIPTION clause was read as "set it to ''". +-- +-- An omitted clause now keeps what is stored, as an omitted user role property +-- already did (ako/mxcli#707). `description ''` is the spelling that clears it. +-- +-- mxcli exec 1218-module-role-description-kept.mdl -p app.mpr +-- mxcli -p app.mpr -c "show module roles in Zz1218" +-- +-- After the run Zz1218.ApiUser has the description from statement 1, and +-- Zz1218.Cleared has none. Re-running the script reports `Unchanged` for the +-- bare statement and `Modified` only where it states a different description. +-- Measured on Mendix 11.13.0: `mx check` is 0 errors with the fault and +-- without it, so only the stored description shows the difference. +-- The unit test is mdl/executor/issue1218_module_role_description_test.go. + +create or modify module Zz1218; + +create or modify module role Zz1218.ApiUser description 'Read the published OData services. Nothing else.'; + +-- The "make sure this role exists" idiom: keeps the description above. +create or modify module role Zz1218.ApiUser; + +-- Stating an empty description is how a script clears one. +create or modify module role Zz1218.Cleared description 'Temporary'; +create or modify module role Zz1218.Cleared description ''; diff --git a/mdl/ast/ast_security.go b/mdl/ast/ast_security.go index 18c3d38fc..80e6f0328 100644 --- a/mdl/ast/ast_security.go +++ b/mdl/ast/ast_security.go @@ -10,13 +10,26 @@ package ast type CreateModuleRoleStmt struct { CreateGuard // `create … if not exists` (ako/mxcli#731) Name QualifiedName - Description string + // Description is nil when the statement has no DESCRIPTION clause: a new + // role then has none, and `create or modify` keeps the stored one + // (ako/mxcli#1218), as CreateUserRoleStmt's properties do. `description ''` + // is the stated empty description, which clears it. + Description *string // CreateOrModify makes the statement idempotent: an existing role has its // description updated instead of the statement failing, so a security script // can be re-run. CreateOrModify bool } +// DescriptionText is the description the statement states, or "" when it +// states none. +func (s *CreateModuleRoleStmt) DescriptionText() string { + if s.Description == nil { + return "" + } + return *s.Description +} + func (s *CreateModuleRoleStmt) isStatement() {} // DropModuleRoleStmt represents: DROP MODULE ROLE [IF EXISTS] Module.RoleName diff --git a/mdl/executor/cmd_security_write.go b/mdl/executor/cmd_security_write.go index 808f4354a..3b8eccd8a 100644 --- a/mdl/executor/cmd_security_write.go +++ b/mdl/executor/cmd_security_write.go @@ -44,7 +44,9 @@ func execCreateModuleRole(ctx *ExecContext, s *ast.CreateModuleRoleStmt) error { if mr.Description == autoDocumentRoleDescription { oldQualified := s.Name.Module + "." + mr.Name newQualified := s.Name.Module + "." + s.Name.Name - if err := ctx.Backend.AddModuleRole(ms.ID, s.Name.Name, s.Description); err != nil { + // Adopting the role takes it over: the marker description is not + // the author's documentation, so an unstated description clears it. + if err := ctx.Backend.AddModuleRole(ms.ID, s.Name.Name, s.DescriptionText()); err != nil { return mdlerrors.NewBackend("create module role", err) } // If the casing actually changed, propagate the rename across every @@ -64,8 +66,14 @@ func execCreateModuleRole(ctx *ExecContext, s *ast.CreateModuleRoleStmt) error { if s.CreateOrModify { // Re-running a security script must not fail on a role that is // already there. AddModuleRole overwrites, so this also adopts a new - // description and the caller's casing. - if err := ctx.Backend.AddModuleRole(ms.ID, s.Name.Name, s.Description); err != nil { + // description and the caller's casing. A statement without a + // description clause keeps the stored one: passing "" erased it, + // reported only as "Modified" (ako/mxcli#1218). + description := mr.Description + if s.Description != nil { + description = *s.Description + } + if err := ctx.Backend.AddModuleRole(ms.ID, s.Name.Name, description); err != nil { return mdlerrors.NewBackend("modify module role", err) } if !ctx.Quiet { @@ -76,7 +84,7 @@ func execCreateModuleRole(ctx *ExecContext, s *ast.CreateModuleRoleStmt) error { return mdlerrors.NewAlreadyExists("module role", s.Name.Module+"."+s.Name.Name) } - if err := ctx.Backend.AddModuleRole(ms.ID, s.Name.Name, s.Description); err != nil { + if err := ctx.Backend.AddModuleRole(ms.ID, s.Name.Name, s.DescriptionText()); err != nil { return mdlerrors.NewBackend("create module role", err) } diff --git a/mdl/executor/issue1218_module_role_description_test.go b/mdl/executor/issue1218_module_role_description_test.go new file mode 100644 index 000000000..767bb64d0 --- /dev/null +++ b/mdl/executor/issue1218_module_role_description_test.go @@ -0,0 +1,94 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend/mock" + "github.com/mendixlabs/mxcli/mdl/visitor" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/security" +) + +// ako/mxcli#1218: "A bare `create or modify module role M.R;` silently erases +// the role's description". The statement reported "Modified module role" and +// the role's Description was gone: the executor handed the statement's empty +// description to AddModuleRole, which overwrites. An omitted clause keeps what +// is stored, as an omitted user role property already did (ako/mxcli#707); a +// description clause with an empty string is how a script clears it. +// +// The statements are parsed with the real visitor, so the test covers the +// whole chain from "the statement does not say" to the value written. +func TestIssue1218_CreateOrModifyModuleRole_KeepsUnstatedDescription(t *testing.T) { + const stored = "Read the published OData services. Nothing else." + cases := []struct { + name string + src string + stored *security.ModuleRole // nil: the role does not exist yet + want string // the description AddModuleRole writes + }{ + {"bare statement keeps the stored description", + "create or modify module role Shop.ApiUser;", + &security.ModuleRole{Name: "ApiUser", Description: stored}, stored}, + {"a stated description replaces it", + "create or modify module role Shop.ApiUser description 'Calls the API';", + &security.ModuleRole{Name: "ApiUser", Description: stored}, "Calls the API"}, + {"an empty description clears it", + "create or modify module role Shop.ApiUser description '';", + &security.ModuleRole{Name: "ApiUser", Description: stored}, ""}, + {"a casing change keeps the stored description", + "create or modify module role Shop.apiUser;", + &security.ModuleRole{Name: "ApiUser", Description: stored}, stored}, + {"or replace reads as or modify and keeps it too", + "mdl 1;\ncreate or replace module role Shop.ApiUser;", + &security.ModuleRole{Name: "ApiUser", Description: stored}, stored}, + // Adopting a role mxcli auto-provisioned takes it over: its marker + // description is not the author's documentation, so it is not kept. + {"adopting an auto-provisioned role drops its marker", + "create module role Shop.ApiUser;", + &security.ModuleRole{Name: "ApiUser", Description: autoDocumentRoleDescription}, ""}, + {"a new role without a description has none", + "create or modify module role Shop.ApiUser;", + nil, ""}, + {"a new role takes the stated description", + "create module role Shop.ApiUser description 'Calls the API';", + nil, "Calls the API"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + mod := &model.Module{BaseElement: model.BaseElement{ID: "mod-shop"}, Name: "Shop"} + ms := &security.ModuleSecurity{BaseElement: model.BaseElement{ID: "ms-shop"}, ContainerID: mod.ID} + if tc.stored != nil { + ms.ModuleRoles = []*security.ModuleRole{tc.stored} + } + var written []string + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListModulesFunc: func() ([]*model.Module, error) { return []*model.Module{mod}, nil }, + GetModuleSecurityFunc: func(model.ID) (*security.ModuleSecurity, error) { return ms, nil }, + AddModuleRoleFunc: func(_ model.ID, _ string, description string) error { + written = append(written, description) + return nil + }, + } + ctx, buf := newMockCtx(t, withBackend(mb), withHierarchy(mkHierarchy(mod))) + + prog, errs := visitor.Build(tc.src) + if len(errs) > 0 { + t.Fatalf("parse: %v", errs) + } + stmt := findStmt[*ast.CreateModuleRoleStmt](t, prog, tc.src) + if err := execCreateModuleRole(ctx, stmt); err != nil { + t.Fatalf("exec: %v\n%s", err, buf.String()) + } + if len(written) != 1 { + t.Fatalf("AddModuleRole called %d time(s), want 1\n%s", len(written), buf.String()) + } + if written[0] != tc.want { + t.Errorf("description written = %q, want %q\n%s", written[0], tc.want, buf.String()) + } + }) + } +} diff --git a/mdl/executor/issue707_describe_roundtrip_test.go b/mdl/executor/issue707_describe_roundtrip_test.go index 923c7868a..296dcd9fb 100644 --- a/mdl/executor/issue707_describe_roundtrip_test.go +++ b/mdl/executor/issue707_describe_roundtrip_test.go @@ -114,8 +114,8 @@ func TestIssue707_ModuleRoleDescriptionReparses(t *testing.T) { out := buf.String() stmt := findStmt[*ast.CreateModuleRoleStmt](t, reparse(t, out), out) - if stmt.Description != "Can't delete orders" { - t.Errorf("description: stored %q, re-exec stores %q\n%s", "Can't delete orders", stmt.Description, out) + if stmt.DescriptionText() != "Can't delete orders" { + t.Errorf("description: stored %q, re-exec stores %q\n%s", "Can't delete orders", stmt.DescriptionText(), out) } } diff --git a/mdl/visitor/module_role_create_statement_test.go b/mdl/visitor/module_role_create_statement_test.go index abdc78eab..6aaaf4296 100644 --- a/mdl/visitor/module_role_create_statement_test.go +++ b/mdl/visitor/module_role_create_statement_test.go @@ -35,7 +35,7 @@ func TestCreateModuleRole_IsACreateStatementKind(t *testing.T) { if !ok { t.Fatalf("%s: got %T", tc.src, prog.Statements[0]) } - if s.Name.String() != "M.Admin" || s.CreateOrModify != tc.orModify || s.Description != tc.desc { + if s.Name.String() != "M.Admin" || s.CreateOrModify != tc.orModify || s.DescriptionText() != tc.desc { t.Errorf("%s: got %+v", tc.src, s) } if got := len(prog.Deprecations) > 0; got != tc.deprecated { diff --git a/mdl/visitor/visitor_security.go b/mdl/visitor/visitor_security.go index 0defad6da..3f00edd35 100644 --- a/mdl/visitor/visitor_security.go +++ b/mdl/visitor/visitor_security.go @@ -27,7 +27,10 @@ func (b *Builder) ExitCreateModuleRoleStatement(ctx *parser.CreateModuleRoleStat } if ctx.DESCRIPTION() != nil { if sl := ctx.STRING_LITERAL(); sl != nil { - stmt.Description = unquoteStringLit(sl) + // A pointer, so `description ''` (clear it) and no clause at all + // (keep what is stored) stay different statements (ako/mxcli#1218). + d := unquoteStringLit(sl) + stmt.Description = &d } } b.statements = append(b.statements, stmt) diff --git a/mdl/visitor/visitor_security_or_modify_test.go b/mdl/visitor/visitor_security_or_modify_test.go index 22521e404..f38cbed14 100644 --- a/mdl/visitor/visitor_security_or_modify_test.go +++ b/mdl/visitor/visitor_security_or_modify_test.go @@ -42,8 +42,8 @@ func TestCreateModuleRole_OrModify(t *testing.T) { if got.Name.Name != tc.wantName { t.Errorf("Name = %q, want %q", got.Name.Name, tc.wantName) } - if got.Description != tc.desc { - t.Errorf("Description = %q, want %q", got.Description, tc.desc) + if got.DescriptionText() != tc.desc { + t.Errorf("Description = %q, want %q", got.DescriptionText(), tc.desc) } }) } diff --git a/mdl/visitor/visitor_security_test.go b/mdl/visitor/visitor_security_test.go index 440717b87..00e783c1a 100644 --- a/mdl/visitor/visitor_security_test.go +++ b/mdl/visitor/visitor_security_test.go @@ -28,8 +28,25 @@ func TestCreateModuleRole(t *testing.T) { if stmt.Name.Module != "MyModule" || stmt.Name.Name != "Editor" { t.Errorf("Expected MyModule.Editor, got %s.%s", stmt.Name.Module, stmt.Name.Name) } - if stmt.Description != "Can edit content" { - t.Errorf("Expected description 'Can edit content', got %q", stmt.Description) + if stmt.DescriptionText() != "Can edit content" { + t.Errorf("Expected description 'Can edit content', got %q", stmt.DescriptionText()) + } +} + +// A description clause with an empty string states an empty description, which +// clears a stored one under `create or modify`; no clause states none, which +// keeps it (ako/mxcli#1218). +func TestCreateModuleRole_EmptyDescriptionIsStated(t *testing.T) { + prog, errs := Build(`create or modify module role MyModule.Viewer description '';`) + if len(errs) > 0 { + t.Fatalf("parse: %v", errs) + } + stmt, ok := prog.Statements[0].(*ast.CreateModuleRoleStmt) + if !ok { + t.Fatalf("Expected CreateModuleRoleStmt, got %T", prog.Statements[0]) + } + if stmt.Description == nil || *stmt.Description != "" { + t.Errorf("Description = %v, want a stated empty string", stmt.Description) } } @@ -46,8 +63,8 @@ func TestCreateModuleRole_NoDescription(t *testing.T) { if !ok { t.Fatalf("Expected CreateModuleRoleStmt, got %T", prog.Statements[0]) } - if stmt.Description != "" { - t.Errorf("Expected empty description, got %q", stmt.Description) + if stmt.Description != nil { + t.Errorf("Expected no description clause, got %q", *stmt.Description) } } From 3c17e99992bbbdc659b9c27227f22acf0b5cfd21 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 17:28:53 +0000 Subject: [PATCH 04/18] fix(modelsdk): declare AggregateFunction on Mendix 9, floor Reduce's fold at 10.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Symptom (ako/mxcli#1163, 02-microflow-examples on 9.24.30): every count/sum/average/minimum/maximum in a microflow was refused with "this Mendix 9.24.30 project cannot store Microflows$AggregateAction.AggregateFunction (Mendix 10.0.0+)". Cause: not an older storage shape. The vendored generated version data gave aggregateFunction the row of reduceReturnDataType ({Introduced 10.0.0, Required}) and had no row for reduceReturnDataType, so the property floor refused the key every aggregate carries and let the fold's return type through unrefused. Evidence: mendixmodelsdk 4.117.0 has aggregateFunction with no version (its default even changes at 9.24.0) and reduceReturnDataType introduced 10.0.0. Studio Pro 9.24.0 stores AggregateFunction on 10 of 10 aggregates in Workflow Commons 2.6.0. A floor-off build's aggregate survives `mx convert` 9.24.30 with every key. Fix: correct the Microflows$AggregateAction row by hand (gen cannot be regenerated here) and regenerate the copy in modelsdk/version. Measured: bug-test 1163-aggregate-on-mendix9.mdl on a fresh 9.24.30 app: exec writes all five aggregates, mx check 0 errors, mx convert strips nothing. Before, the first was refused. Control: the new tests fail on the unfixed table with the reported refusal. 02-microflow-examples on 9.24 now stops at the expression aggregate (`sum … of …`, UseExpression and Expression, Mendix 10.0+), which is correct. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82sUVQEB3K5W5vh85wM --- ...ggregate-function-floored-at-mendix10.json | 1 + CHANGELOG.md | 1 + .../src/appendixes/version-compatibility.md | 5 +- .../bug-tests/1163-aggregate-on-mendix9.mdl | 36 ++++++++ modelsdk/gen/microflows/version.go | 8 +- .../mpr/writer_version_shape_1163_test.go | 83 +++++++++++++++++++ modelsdk/version/metamodel_versions_gen.go | 8 +- 7 files changed, 138 insertions(+), 4 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/modelsdk/2026-10-28-aggregate-function-floored-at-mendix10.json create mode 100644 mdl-examples/bug-tests/1163-aggregate-on-mendix9.mdl create mode 100644 modelsdk/mpr/writer_version_shape_1163_test.go diff --git a/.claude/skills/fix-issue/findings/modelsdk/2026-10-28-aggregate-function-floored-at-mendix10.json b/.claude/skills/fix-issue/findings/modelsdk/2026-10-28-aggregate-function-floored-at-mendix10.json new file mode 100644 index 000000000..bac85dc0a --- /dev/null +++ b/.claude/skills/fix-issue/findings/modelsdk/2026-10-28-aggregate-function-floored-at-mendix10.json @@ -0,0 +1 @@ +{"area": "modelsdk", "date": "2026-10-28", "symptom": "On Mendix 9.x every count/sum/average/minimum/maximum in a microflow was refused: `this Mendix 9.24.30 project cannot store Microflows$AggregateAction.AggregateFunction (Mendix 10.0.0+)` (02-microflow-examples on 9.24)", "cause": "The vendored generated version data (modelsdk/gen/microflows/version.go) gave aggregateFunction the row of reduceReturnDataType ({Introduced 10.0.0, Required}) and had no row for reduceReturnDataType, so the floor refused the one key every aggregate has and let the fold's return type through unrefused", "file": "`modelsdk/gen/microflows/version.go` (Microflows$AggregateAction, fixed by hand), `modelsdk/version/metamodel_versions_gen.go` (go generate ./modelsdk/version)", "insight": "**A refusal can be the floor's data being wrong, not the writer's shape.** The prompt to look for an 'older shape' was a guess; evidence settled it in two steps: mendixmodelsdk 4.117.0 has `aggregateFunction: {}` (no version) and even sets its default differently either side of 9.24.0, and Studio Pro-authored 9.24.0 bytes (Workflow Commons 2.6.0 from the Marketplace, `mxcli marketplace download 117066 --version 2.6.0`, project.mpr inside the .mpk) store `AggregateFunction: \"Count\"` on 10 of 10 aggregates. A one-off script comparing every SDK StructureVersionInfo `introduced` with modelsdk/gen/*/version.go found only 4 mismatches in the whole table (this one; CustomWidgets$WidgetReturnType.type 9.20 vs none; Forms$GroupBox.caption 8.10 vs none; Workflows$CallMicroflowTask.boundaryEvents), so the generator's misattribution is rare, not systematic. **The cheapest 'which keys does version X declare' oracle is a floor-off build + `mx convert -p` on that version**: build mxcli with StripUndeclaredProperties stubbed, exec, dump units, convert, dump again, diff key sets per $Type — 9.24's convert kept all of AggregateAction's keys. Measured: 5 aggregates on a fresh 9.24.30 app → exec ok, mx check 0 errors, mx convert strips nothing (before: refused). Tests: TestNormalize_AggregateFunctionIsDeclaredOnMendix9, TestNormalize_ReduceFoldIsRefusedBelowMendix10; mdl-examples/bug-tests/1163-aggregate-on-mendix9.mdl", "refs": ["#1163"]} diff --git a/CHANGELOG.md b/CHANGELOG.md index a947aaac4..d0905b71c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - **A navigation profile's application title and icon can be set and are described** (ako/mxcli#1756, mendixlabs/mxcli#1664) — the title (Studio Pro's *Application title*, `Mendix` in a new project) is what the runtime puts in every browser tab as `Mendix - `, and it had no MDL form: `create or modify navigation Responsive app title 'EWO' …` failed with "extraneous input 'app'", `describe navigation` printed neither it nor the *Application icon*, and `create or modify translations … for en_US` refuses the source language, so every generated app kept "Mendix" in the tab. `create or modify navigation` now takes `app title ''` and `app icon ` (`app icon none` clears it), in any order with the other clauses; `app` and `icon` stay usable as names. The title is written as the project default language's item, the title's other languages kept; the icon is resolved like any image reference, by `check --references` and before exec writes; an omitted clause keeps the stored value; describe prints both. A native profile has neither, so both are refused there. - **A test whose earlier `@expect` fails is no longer reported PASS on the after-startup runner** (ako/mxcli#1753) — every Docker run and `--local --legacy-runner` logged a verdict line per assertion, and the result was taken from the last one: a failing first assertion followed by a passing second reported PASS, with the failure message erased, and `mxcli test` exited 0. A failure now sticks, and the test reports the first one, plus how many more failed. The endpoint runner (`--local`, `--attach`, `--watch`) was not affected. - **Two tests that assign the same variable with a lower-case `call` or `create` run on the after-startup runner** (found with ako/mxcli#1753) — that runner compiles every test into one microflow and renames each test's variables, but it recognised only an upper-case `CALL` or `CREATE`. `$r = call microflow …`, the spelling mdl 1 and `fmt` write, was left as it was, so two tests assigning `$r` declared it twice and injection failed with MDL063 (CE0111 from mxbuild). 8 of the 16 test files in `mdl-examples/` hit this under Docker or `--legacy-runner`. A variable first assigned by `retrieve`, `declare` or a list operation is still not renamed there. +- **Aggregates in microflows are written on Mendix 9** (ako/mxcli#1163) — every `count`, `sum`, `average`, `minimum` and `maximum` on a 9.x project was refused with "cannot store Microflows$AggregateAction.AggregateFunction (Mendix 10.0.0+)". The version data had the aggregate's function introduced in 10.0, which belongs to the fold `reduce` added. Studio Pro 9.24 stores it on every aggregate. An aggregate over an expression, `reduce`, `all` and `any` are Mendix 10.0 and are still refused below it, now naming the fold. - **Pages on Mendix 9.24 and 10.x are read and written in the version's own shape** (ako/mxcli#1209) — several page properties are stored differently below a version: a design property's value before 10.2, a data view's page parameter before 10.0, its editability before 10.7, and a text box's placeholder before 10.11. `describe` read only the newer form, so on 9.24 a describe → exec round trip removed every design property and placeholder. Writing any page with a data view or a design property was refused, and `alter styling` on a stored design property reported success and changed nothing. Every write now converts to the project's form, and `describe` reads both. What the older form cannot hold is refused, naming the widget: a Spacing or colour design property, conditional editability, a placeholder with parameters. - **A text box's `KeyboardType` survives describe → exec** (ako/mxcli#1209) — the keyboard a native app opens had no MDL spelling and was written as `Default`, so a rewrite reset it. `textbox t (Attribute: Email, KeyboardType: EmailAddress)` (`Default`, `NumberPad`, `DecimalPad`, `EmailAddress`, `PhonePad`, `URL`); an unknown value is MDL-WIDGET18. - **A button's workflow action survives a rewrite** (ako/mxcli#832) — `describe` printed no action for Show user task page, Show workflow admin page or Call workflow, so re-executing a page turned them into no action (on ako/TestApp's TaskInbox and TaskDashboard, 2 of 6 user-task buttons each). They are now `open user task [with (AssignOnOpen, OpenWhenAssigned)]`, `open workflow with (DefaultPage: M.Page)` and `call workflow M.WF [with (…)]`. `complete task` takes `with (ClosePage, Commit)` instead of always writing both true. `open workflow` without a page is CE1834 and is refused. diff --git a/docs-site/src/appendixes/version-compatibility.md b/docs-site/src/appendixes/version-compatibility.md index 67099ac20..05ee85520 100644 --- a/docs-site/src/appendixes/version-compatibility.md +++ b/docs-site/src/appendixes/version-compatibility.md @@ -186,11 +186,12 @@ Some page properties kept their meaning and changed how they are stored. mxcli b | A text box's, text area's or date picker's placeholder | 10.11 | `Placeholder` text, instead of a `PlaceholderTemplate` | | Any data source's `ForceFullObjects` | 10.8 | Not stored | -What an older shape cannot hold is refused, naming the widget, rather than dropped: +What an older shape cannot hold is refused, naming the element, rather than dropped: - a Spacing, colour-picker or toggle-button-group design property below 10.2; - conditional data view editability below 10.7; - a placeholder with parameters below 10.11; -- a data view fed by another widget below 10.0. +- a data view fed by another widget below 10.0; +- an aggregate over an expression (`sum $L of …`), `reduce`, `all` or `any` below 10.0. The plain `count`, `sum`, `average`, `minimum` and `maximum` are written on every version ([ako/mxcli#1163](https://github.com/ako/mxcli/issues/1163)). Before this, `describe` on 9.24 printed none of these properties, so a describe → exec round trip removed them. Writing a data view on 9.24 was refused outright. diff --git a/mdl-examples/bug-tests/1163-aggregate-on-mendix9.mdl b/mdl-examples/bug-tests/1163-aggregate-on-mendix9.mdl new file mode 100644 index 000000000..d13e79e3a --- /dev/null +++ b/mdl-examples/bug-tests/1163-aggregate-on-mendix9.mdl @@ -0,0 +1,36 @@ +mdl 1; +-- ako/mxcli#1163: on Mendix 9.x every count/sum/average/minimum/maximum in a +-- microflow was refused: +-- +-- this Mendix 9.24.30 project cannot store +-- Microflows$AggregateAction.AggregateFunction (Mendix 10.0.0+) +-- +-- The property floor came from the vendored generated version data, which had +-- given AggregateFunction the row of ReduceReturnDataType (the fold Reduce +-- added in 10.0) and left that one out. Studio Pro 9.24.0 stores +-- AggregateFunction on every aggregate (Workflow Commons 2.6.0), `mx convert` +-- 9.24.30 keeps it, and mendixmodelsdk gives it no version. +-- +-- Run it on a fresh 9.24.30 app (`mxcli new … --version 9.24.30.51827`): exec +-- writes all five, and `mxcli docker check` reports the blank app's error +-- count. An aggregate over an expression (`sum $L of …`) and `reduce`, `all` +-- and `any` are Mendix 10.0 and are still refused below it. + +create module Probe1163Agg; + +create persistent entity Probe1163Agg.Line ( + Amount: Decimal +); + +create microflow Probe1163Agg.Totals ( + $Lines: list of Probe1163Agg.Line +) +returns decimal as $Sum +begin + $Count = count $Lines; + $Sum = sum $Lines by Amount; + $Average = average $Lines by Amount; + $Min = minimum $Lines by Amount; + $Max = maximum $Lines by Amount; + return $Sum; +end; diff --git a/modelsdk/gen/microflows/version.go b/modelsdk/gen/microflows/version.go index 01eea8f03..dcd1c8162 100644 --- a/modelsdk/gen/microflows/version.go +++ b/modelsdk/gen/microflows/version.go @@ -45,9 +45,15 @@ var VersionInfos = map[string]version.TypeVersionInfo{ }, "Microflows$AggregateAction": { Properties: map[string]version.PropertyVersionInfo{ - "aggregateFunction": {Introduced: "10.0.0", Required: true}, + // Fixed by hand (ako/mxcli#1163): the vendored generator gave + // aggregateFunction the row of reduceReturnDataType, which it then + // left out. mendixmodelsdk 4.117.0 has aggregateFunction with no + // version and reduceReturnDataType introduced 10.0.0 and required; + // Studio Pro 9.24 stores AggregateFunction (Workflow Commons 2.6.0). + "aggregateFunction": {}, "expression": {Introduced: "10.0.0"}, "reduceInitialValueExpression": {Introduced: "10.0.0"}, + "reduceReturnDataType": {Introduced: "10.0.0", Required: true}, "useExpression": {Introduced: "10.0.0"}, }, }, diff --git a/modelsdk/mpr/writer_version_shape_1163_test.go b/modelsdk/mpr/writer_version_shape_1163_test.go new file mode 100644 index 000000000..d62ab14de --- /dev/null +++ b/modelsdk/mpr/writer_version_shape_1163_test.go @@ -0,0 +1,83 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mpr + +import ( + "strings" + "testing" + + "go.mongodb.org/mongo-driver/v2/bson" + + "github.com/mendixlabs/mxcli/modelsdk/mpr/version" +) + +// ako/mxcli#1163: what the 9.24 doctype run found mxcli writing that Mendix 9 +// stores differently, or not at all. Each case is the document as mxcli builds +// it, normalized for the project's version the way every unit write is. + +// aggregateMicroflow is a microflow holding one Aggregate list activity as the +// writer builds it: Count/Sum/… carry no fold; Reduce, All and Any carry +// ReduceInitialValueExpression and ReduceReturnDataType. +func aggregateMicroflow(function string) bson.D { + action := bson.D{{Key: "$ID", Value: bid()}, {Key: "$Type", Value: "Microflows$AggregateAction"}, + {Key: "AggregateFunction", Value: function}, + {Key: "AggregateVariableName", Value: "Items"}, + {Key: "Attribute", Value: "M.Item.Price"}, + {Key: "ErrorHandlingType", Value: "Rollback"}} + switch function { + case "Reduce", "All", "Any": + initial := "" + if function == "Reduce" { + initial = "0" + } + action = append(action, + bson.E{Key: "ReduceInitialValueExpression", Value: initial}, + bson.E{Key: "ReduceReturnDataType", Value: bson.D{{Key: "$ID", Value: bid()}, {Key: "$Type", Value: "DataTypes$BooleanType"}}}) + } + action = append(action, bson.E{Key: "VariableName", Value: "Total"}) + activity := bson.D{{Key: "$ID", Value: bid()}, {Key: "$Type", Value: "Microflows$ActionActivity"}, {Key: "Action", Value: action}} + return bson.D{{Key: "$ID", Value: bid()}, {Key: "$Type", Value: "Microflows$Microflow"}, {Key: "Name", Value: "Agg"}, + {Key: "ObjectCollection", Value: bson.D{{Key: "$ID", Value: bid()}, {Key: "$Type", Value: "Microflows$MicroflowObjectCollection"}, + {Key: "Objects", Value: bson.A{int32(3), activity}}}}} +} + +// Microflows$AggregateAction.AggregateFunction is declared on every version +// mxcli writes. The generated version data floored it at 10.0.0, so every +// count/sum/average/minimum/maximum on 9.x was refused ("cannot store +// Microflows$AggregateAction.AggregateFunction (Mendix 10.0.0+)"). Studio Pro +// 9.24.0 stores it (Workflow Commons 2.6.0: 10 of 10 aggregates, `Count`), `mx +// convert` 9.24.30 keeps it, and mendixmodelsdk gives it no version at all; the +// 10.0.0 floor belongs to ReduceReturnDataType, the fold Reduce added. +func TestNormalize_AggregateFunctionIsDeclaredOnMendix9(t *testing.T) { + for _, v := range []*version.ProjectVersion{pv(9, 0, 5), pv(9, 24, 30), pv(10, 0, 0), pv(11, 13, 0)} { + d, err := normalizeShape(t, v, aggregateMicroflow("Sum")) + if err != nil { + t.Errorf("%d.%d: a sum aggregate is refused: %v", v.MajorVersion, v.MinorVersion, err) + continue + } + if f, _ := get(find(d, "Microflows$AggregateAction"), "AggregateFunction"); f != "Sum" { + t.Errorf("%d.%d: AggregateFunction = %v, want Sum", v.MajorVersion, v.MinorVersion, f) + } + } +} + +// The fold is what Mendix 10.0 added (mendixmodelsdk: ReduceReturnDataType +// introduced 10.0.0). `all` carries an empty initial value, so the fold's +// return type is the one key that says it: on 9.24 it is refused, naming it, +// rather than written into a document Studio Pro 9 cannot resolve. Before, the +// generated data had no row for it and it passed through. +func TestNormalize_ReduceFoldIsRefusedBelowMendix10(t *testing.T) { + for _, fn := range []string{"Reduce", "All"} { + if _, err := normalizeShape(t, pv(9, 24, 30), aggregateMicroflow(fn)); err == nil || + !strings.Contains(err.Error(), "Microflows$AggregateAction.ReduceReturnDataType (Mendix 10.0.0+)") { + t.Errorf("%s on 9.24 was not refused naming its fold's return type: %v", fn, err) + } + d, err := normalizeShape(t, pv(10, 0, 0), aggregateMicroflow(fn)) + if err != nil { + t.Fatalf("%s on 10.0 is refused: %v", fn, err) + } + if _, ok := get(find(d, "Microflows$AggregateAction"), "ReduceReturnDataType"); !ok { + t.Errorf("%s on 10.0 lost ReduceReturnDataType", fn) + } + } +} diff --git a/modelsdk/version/metamodel_versions_gen.go b/modelsdk/version/metamodel_versions_gen.go index d5e02fa57..37591339c 100644 --- a/modelsdk/version/metamodel_versions_gen.go +++ b/modelsdk/version/metamodel_versions_gen.go @@ -1241,9 +1241,15 @@ var metamodelVersions = []map[string]TypeVersionInfo{ }, "Microflows$AggregateAction": { Properties: map[string]PropertyVersionInfo{ - "aggregateFunction": {Introduced: "10.0.0", Required: true}, + // Fixed by hand (ako/mxcli#1163): the vendored generator gave + // aggregateFunction the row of reduceReturnDataType, which it then + // left out. mendixmodelsdk 4.117.0 has aggregateFunction with no + // version and reduceReturnDataType introduced 10.0.0 and required; + // Studio Pro 9.24 stores AggregateFunction (Workflow Commons 2.6.0). + "aggregateFunction": {}, "expression": {Introduced: "10.0.0"}, "reduceInitialValueExpression": {Introduced: "10.0.0"}, + "reduceReturnDataType": {Introduced: "10.0.0", Required: true}, "useExpression": {Introduced: "10.0.0"}, }, }, From 890899d8b5635c9136d733ecd8222d5360ebf0ed Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 17:41:27 +0000 Subject: [PATCH 05/18] fix(canon): write no workflow PersistentId below Mendix 10.21 Symptom (ako/mxcli#1163, 25-rename on 9.24.30): every workflow was refused with "this Mendix 9.24.30 project cannot store Workflows$Workflow.PersistentId (Mendix 10.21.0+)". The same held on 10.0-10.20. Cause: the workflow writer, and the ALTER WORKFLOW mutator for the activities it inserts, mint a fresh PersistentId on the workflow and on every activity, outcome and boundary event. mendixmodelsdk introduces PersistentId in 10.21.0 on all four (Workflow, and the abstract WorkflowActivity, Outcome and BoundaryEvent). The floor refused the workflow's own. It never saw the others, which are declared on supertypes it does not look up. `mx convert` 9.24.30 strips the PersistentId from all 6 element types a floor-off build wrote. Fix: canon.LowerToProjectShape drops PersistentId from every Workflows$ element below 10.21. It runs on every unit write before the floor, so CREATE, CREATE OR MODIFY and wfmutator's raw inserts are all covered. The ID is dropped rather than refused because the writer mints it; no script states it, and Studio Pro 9.24 stores none. Measured: bug-test 1163-workflow-on-mendix9.mdl on a fresh 9.24.30 app: exec ok, mx check 0 errors, `mx convert -p` strips no key. 25-rename on 9.24: FAIL (the refusal) -> PASS. Controls: 25-rename and 24b-workflow-mendix10 PASS on 10.24.24 and 11.13.0. The unit test fails on the unfixed code with the reported refusal. Not fixed: a parallel split writes Workflows$EndOfParallelSplitPathActivity, a 10.21 type, and 9.24 then does not load the project. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82sUVQEB3K5W5vh85wM --- ...-28-workflow-persistentid-below-10-21.json | 1 + CHANGELOG.md | 1 + .../src/appendixes/version-compatibility.md | 5 +- .../bug-tests/1163-workflow-on-mendix9.mdl | 55 +++++++++++++ modelsdk/canon/versionshape.go | 27 +++++- .../mpr/writer_version_shape_1163_test.go | 82 +++++++++++++++++++ 6 files changed, 167 insertions(+), 4 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/modelsdk/2026-10-28-workflow-persistentid-below-10-21.json create mode 100644 mdl-examples/bug-tests/1163-workflow-on-mendix9.mdl diff --git a/.claude/skills/fix-issue/findings/modelsdk/2026-10-28-workflow-persistentid-below-10-21.json b/.claude/skills/fix-issue/findings/modelsdk/2026-10-28-workflow-persistentid-below-10-21.json new file mode 100644 index 000000000..52ce2c428 --- /dev/null +++ b/.claude/skills/fix-issue/findings/modelsdk/2026-10-28-workflow-persistentid-below-10-21.json @@ -0,0 +1 @@ +{"area": "modelsdk", "date": "2026-10-28", "symptom": "On Mendix 9.x and 10.0–10.20 every workflow was refused: `this Mendix 9.24.30 project cannot store Workflows$Workflow.PersistentId (Mendix 10.21.0+)` (25-rename on 9.24)", "cause": "The workflow writer (and the ALTER WORKFLOW mutator, for the activities it inserts) mints a fresh PersistentId on the workflow and on every activity, outcome and boundary event. PersistentId is 10.21.0 on all four (Workflow, and the abstract WorkflowActivity, Outcome, BoundaryEvent). The floor refused the workflow's own and never saw the rest, because it does not look up abstract supertypes", "file": "`modelsdk/canon/versionshape.go` (LowerToProjectShape: dropPersistentID on every Workflows$ element below 10.21)", "insight": "**An identity the writer mints is not a value the statement said**, so below its version it is dropped, not refused (unlike ForceFullObjects, which a script can set). Lower it in canon.LowerToProjectShape rather than encodeWorkflow: wfmutator's raw inserts (mutator.go: PersistentId on inserted activities, outcomes, boundary events) never pass encodeWorkflow, but every write passes normalizePropertySetsFor. **Test with a non-zero GUID**: a zero GUID is an empty value the floor silently drops, so the first draft of the test passed on the unfixed code. **The floor-off build + `mx convert -p` 9.24 key diff** showed the whole picture at once: PersistentId on 6 element types, plus the empty keys the floor already handles. The same run found the next gap: a `parallel split` writes Workflows$EndOfParallelSplitPathActivity (a 10.21 type) and 9.24 does not load the project (TypeCacheUnknownTypeException) — not fixed here. Measured: bug-test on a fresh 9.24.30 app → exec ok, mx check 0 errors, mx convert strips no key (adds WorkflowOnStateChangeEvent/UsertaskOnStateChangeEvent null). 25-rename 9.24 FAIL→PASS; 25-rename and 24b on 10.24.24 and 11.13 PASS. Tests: TestNormalize_WorkflowPersistentIDsBelowMendix10_21; mdl-examples/bug-tests/1163-workflow-on-mendix9.mdl", "refs": ["#1163"]} diff --git a/CHANGELOG.md b/CHANGELOG.md index d0905b71c..5a03ede06 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - **A navigation profile's application title and icon can be set and are described** (ako/mxcli#1756, mendixlabs/mxcli#1664) — the title (Studio Pro's *Application title*, `Mendix` in a new project) is what the runtime puts in every browser tab as `Mendix - `, and it had no MDL form: `create or modify navigation Responsive app title 'EWO' …` failed with "extraneous input 'app'", `describe navigation` printed neither it nor the *Application icon*, and `create or modify translations … for en_US` refuses the source language, so every generated app kept "Mendix" in the tab. `create or modify navigation` now takes `app title ''` and `app icon ` (`app icon none` clears it), in any order with the other clauses; `app` and `icon` stay usable as names. The title is written as the project default language's item, the title's other languages kept; the icon is resolved like any image reference, by `check --references` and before exec writes; an omitted clause keeps the stored value; describe prints both. A native profile has neither, so both are refused there. - **A test whose earlier `@expect` fails is no longer reported PASS on the after-startup runner** (ako/mxcli#1753) — every Docker run and `--local --legacy-runner` logged a verdict line per assertion, and the result was taken from the last one: a failing first assertion followed by a passing second reported PASS, with the failure message erased, and `mxcli test` exited 0. A failure now sticks, and the test reports the first one, plus how many more failed. The endpoint runner (`--local`, `--attach`, `--watch`) was not affected. - **Two tests that assign the same variable with a lower-case `call` or `create` run on the after-startup runner** (found with ako/mxcli#1753) — that runner compiles every test into one microflow and renames each test's variables, but it recognised only an upper-case `CALL` or `CREATE`. `$r = call microflow …`, the spelling mdl 1 and `fmt` write, was left as it was, so two tests assigning `$r` declared it twice and injection failed with MDL063 (CE0111 from mxbuild). 8 of the 16 test files in `mdl-examples/` hit this under Docker or `--legacy-runner`. A variable first assigned by `retrieve`, `declare` or a list operation is still not renamed there. +- **Workflows are written on Mendix 9 and on 10.0–10.20** (ako/mxcli#1163) — every workflow was refused with "cannot store Workflows$Workflow.PersistentId (Mendix 10.21.0+)". Workflows exist from 9.0, but the persistent ID on a workflow, its activities, outcomes and boundary events arrived in 10.21. The activities' and outcomes' IDs were never refused, and `mx convert` 9.24 stripped them. mxcli mints these IDs itself, so below 10.21 it no longer writes them, for `create`, `create or modify` and the activities `alter workflow` inserts. A `parallel split` is still unwritable below 10.21: its paths end in an activity type that version does not have. - **Aggregates in microflows are written on Mendix 9** (ako/mxcli#1163) — every `count`, `sum`, `average`, `minimum` and `maximum` on a 9.x project was refused with "cannot store Microflows$AggregateAction.AggregateFunction (Mendix 10.0.0+)". The version data had the aggregate's function introduced in 10.0, which belongs to the fold `reduce` added. Studio Pro 9.24 stores it on every aggregate. An aggregate over an expression, `reduce`, `all` and `any` are Mendix 10.0 and are still refused below it, now naming the fold. - **Pages on Mendix 9.24 and 10.x are read and written in the version's own shape** (ako/mxcli#1209) — several page properties are stored differently below a version: a design property's value before 10.2, a data view's page parameter before 10.0, its editability before 10.7, and a text box's placeholder before 10.11. `describe` read only the newer form, so on 9.24 a describe → exec round trip removed every design property and placeholder. Writing any page with a data view or a design property was refused, and `alter styling` on a stored design property reported success and changed nothing. Every write now converts to the project's form, and `describe` reads both. What the older form cannot hold is refused, naming the widget: a Spacing or colour design property, conditional editability, a placeholder with parameters. - **A text box's `KeyboardType` survives describe → exec** (ako/mxcli#1209) — the keyboard a native app opens had no MDL spelling and was written as `Default`, so a rewrite reset it. `textbox t (Attribute: Email, KeyboardType: EmailAddress)` (`Default`, `NumberPad`, `DecimalPad`, `EmailAddress`, `PhonePad`, `URL`); an unknown value is MDL-WIDGET18. diff --git a/docs-site/src/appendixes/version-compatibility.md b/docs-site/src/appendixes/version-compatibility.md index 05ee85520..3f773a76f 100644 --- a/docs-site/src/appendixes/version-compatibility.md +++ b/docs-site/src/appendixes/version-compatibility.md @@ -174,9 +174,9 @@ What *is* version-specific is the optional/default-value half: `PageParameter.Is Atlas UI v3 (Mendix 11.0+) introduced new design properties like "Card style" and "Disable row wrap" for containers. These don't exist in the Atlas v2 theme bundled with 10.x, causing CE6083. -### Page Properties Stored in an Older Shape (below 10.2, 10.7, 10.11) +### Properties Stored in an Older Shape -Some page properties kept their meaning and changed how they are stored. mxcli builds the current shape and, on every write, rewrites it into the shape the project's version stores. `describe` reads both shapes ([ako/mxcli#1209](https://github.com/ako/mxcli/issues/1209)). The older shapes were measured on Studio Pro-authored 9.24.30 pages: +Some properties kept their meaning and changed how they are stored. mxcli builds the current shape and, on every write, rewrites it into the shape the project's version stores. `describe` reads both shapes ([ako/mxcli#1209](https://github.com/ako/mxcli/issues/1209), [ako/mxcli#1163](https://github.com/ako/mxcli/issues/1163)). The older shapes were measured on Studio Pro-authored 9.24 documents and with `mx convert` 9.24.30: | Property | Below | Stored as | |---|---|---| @@ -185,6 +185,7 @@ Some page properties kept their meaning and changed how they are stored. mxcli b | A data view's editability | 10.7 | `Editable: true\|false`, instead of `Editability` | | A text box's, text area's or date picker's placeholder | 10.11 | `Placeholder` text, instead of a `PlaceholderTemplate` | | Any data source's `ForceFullObjects` | 10.8 | Not stored | +| The `PersistentId` of a workflow, activity, outcome or boundary event | 10.21 | Not stored | What an older shape cannot hold is refused, naming the element, rather than dropped: - a Spacing, colour-picker or toggle-button-group design property below 10.2; diff --git a/mdl-examples/bug-tests/1163-workflow-on-mendix9.mdl b/mdl-examples/bug-tests/1163-workflow-on-mendix9.mdl new file mode 100644 index 000000000..ecea673ad --- /dev/null +++ b/mdl-examples/bug-tests/1163-workflow-on-mendix9.mdl @@ -0,0 +1,55 @@ +mdl 1; +-- ako/mxcli#1163: on Mendix 9.x and on 10.0–10.20 every workflow was refused: +-- +-- this Mendix 9.24.30 project cannot store +-- Workflows$Workflow.PersistentId (Mendix 10.21.0+) +-- +-- Workflows exist from 9.0; PersistentId arrived in 10.21.0, on the workflow +-- and on every activity, outcome and boundary event (mendixmodelsdk). The +-- activities' and outcomes' were declared on abstract supertypes the property +-- floor does not look up, so they were never refused, and `mx convert` 9.24.30 +-- strips every one. A PersistentId is minted by the writer, never stated by a +-- script, so below 10.21 none is written: for CREATE, CREATE OR MODIFY and +-- the activities ALTER WORKFLOW inserts on its own. +-- +-- Run it on a fresh 9.24.30 app (`mxcli new … --version 9.24.30.51827`): exec, +-- then `mxcli docker check` reports the blank app's error count, and `mx +-- convert -p` strips no PersistentId. +-- +-- Not covered: a `parallel split`. Its paths end in a +-- Workflows$EndOfParallelSplitPathActivity, a type Mendix introduced in 10.21, +-- so below 10.21 the project does not load (TypeCacheUnknownTypeException). + +create module Probe1163Wf; + +create persistent entity Probe1163Wf.Request ( + Title: String(100), + Amount: Integer +); + +create microflow Probe1163Wf.ACT_Validate ($WorkflowContext: Probe1163Wf.Request) +begin + log info node 'Probe1163Wf' 'validate'; +end; + +create microflow Probe1163Wf.ACT_Notify ($WorkflowContext: Probe1163Wf.Request) +begin + log info node 'Probe1163Wf' 'notify'; +end; + +create workflow Probe1163Wf.WF_Request + parameter $WorkflowContext: Probe1163Wf.Request + display 'Request' +begin + call microflow Probe1163Wf.ACT_Validate; + decision $WorkflowContext/Amount > 1000 + outcomes + true -> { call microflow Probe1163Wf.ACT_Notify; } + false -> { end workflow; } + ; +end workflow; + +-- ALTER serializes the inserted activity on its own, outside CREATE's encoder. +alter workflow Probe1163Wf.WF_Request { + insert after ACT_Validate { call microflow Probe1163Wf.ACT_Notify; } +}; diff --git a/modelsdk/canon/versionshape.go b/modelsdk/canon/versionshape.go index fb1efebde..31be06cae 100644 --- a/modelsdk/canon/versionshape.go +++ b/modelsdk/canon/versionshape.go @@ -4,6 +4,7 @@ package canon import ( "fmt" + "strings" "go.mongodb.org/mongo-driver/v2/bson" @@ -37,6 +38,10 @@ import ( // CE6050 "The selected entity must be configured with a page parameter". // - ForceFullObjects, declared on the abstract Forms$DataSource from 10.8.0: // every data source carried it, and the floor never saw it. +// - Workflows$ PersistentId, from 10.21.0 on the workflow and on the abstract +// WorkflowActivity, Outcome and BoundaryEvent (ako/mxcli#1163). Workflows +// exist from 9.0; `mx convert` 9.24.30 strips every one. The workflow's +// own was refused; the others slipped past the floor. // // It runs on every unit write, before the floor, so no writer needs to know // these shapes. What the older shape cannot hold — a design property value @@ -53,8 +58,10 @@ func LowerToProjectShape(contents []byte, pv *version.Version) ([]byte, error) { textPlaceholder: !PropertyDeclared("Forms$AttributeWidgetWithPlaceholder", "PlaceholderTemplate", *pv), namedParameter: !PropertyDeclared("Forms$EntityPathSource", "SourceVariable", *pv), noFullObjects: !PropertyDeclared("Forms$DataSource", "ForceFullObjects", *pv), + noPersistentID: !PropertyDeclared("Workflows$Workflow", "PersistentId", *pv), } - if !l.flatDesignProps && !l.boolEditable && !l.textPlaceholder && !l.namedParameter && !l.noFullObjects { + if !l.flatDesignProps && !l.boolEditable && !l.textPlaceholder && !l.namedParameter && !l.noFullObjects && + !l.noPersistentID { return contents, nil } var doc bson.D @@ -74,7 +81,7 @@ func LowerToProjectShape(contents []byte, pv *version.Version) ([]byte, error) { type shapeLowering struct { pv version.Version flatDesignProps, boolEditable, textPlaceholder bool - namedParameter, noFullObjects bool + namedParameter, noFullObjects, noPersistentID bool changed bool } @@ -112,6 +119,9 @@ func (l *shapeLowering) walk(v any) (any, error) { return nil, err } } + if l.noPersistentID && strings.HasPrefix(typ, "Workflows$") { + x = l.dropPersistentID(x) + } for i := range x { if x[i].Value, err = l.walk(x[i].Value); err != nil { return nil, err @@ -261,6 +271,19 @@ func (l *shapeLowering) dropForceFullObjects(d bson.D, typ string) (bson.D, erro return append(append(bson.D{}, d[:i]...), d[i+1:]...), nil } +// dropPersistentID removes a workflow element's PersistentId below 10.21, where +// none declares it. Whatever it holds is dropped, not refused: a PersistentId is +// minted by the writer (a fresh GUID on every build), never stated by a script, +// and a workflow Studio Pro 9.24 stores has none. +func (l *shapeLowering) dropPersistentID(d bson.D) bson.D { + i := shapeIndex(d, "PersistentId") + if i < 0 { + return d + } + l.changed = true + return append(append(bson.D{}, d[:i]...), d[i+1:]...) +} + // listItems returns the elements of a stored list, without its type marker. func listItems(v any) bson.A { a, _ := v.(bson.A) diff --git a/modelsdk/mpr/writer_version_shape_1163_test.go b/modelsdk/mpr/writer_version_shape_1163_test.go index d62ab14de..f3ae39ac5 100644 --- a/modelsdk/mpr/writer_version_shape_1163_test.go +++ b/modelsdk/mpr/writer_version_shape_1163_test.go @@ -81,3 +81,85 @@ func TestNormalize_ReduceFoldIsRefusedBelowMendix10(t *testing.T) { } } } + +// pid is a fresh PersistentId: a non-zero GUID, as the writer mints one (a zero +// GUID is an empty value, which the floor would drop rather than refuse). +func pid() bson.Binary { + pidSeq++ + return bson.Binary{Subtype: 0, Data: []byte{pidSeq, 0x11, 2, 3, 4, 5, 0x46, 7, 0x88, 9, 10, 11, 12, 13, 14, 15}} +} + +var pidSeq byte + +// workflowWithPersistentIDs is a workflow as mxcli builds it: a fresh +// PersistentId on the workflow, on every activity and on every outcome. +func workflowWithPersistentIDs() bson.D { + outcome := func(value bool) bson.D { + return bson.D{{Key: "$ID", Value: bid()}, {Key: "$Type", Value: "Workflows$BooleanConditionOutcome"}, + {Key: "Flow", Value: bson.D{{Key: "$ID", Value: bid()}, {Key: "$Type", Value: "Workflows$Flow"}, {Key: "Activities", Value: bson.A{int32(3)}}}}, + {Key: "PersistentId", Value: pid()}, {Key: "Value", Value: value}} + } + split := bson.D{{Key: "$ID", Value: bid()}, {Key: "$Type", Value: "Workflows$ExclusiveSplitActivity"}, + {Key: "Caption", Value: "Active?"}, {Key: "Expression", Value: "$WorkflowContext/Active"}, {Key: "Name", Value: "split"}, + {Key: "Outcomes", Value: bson.A{int32(3), outcome(true), outcome(false)}}, + {Key: "PersistentId", Value: pid()}} + call := bson.D{{Key: "$ID", Value: bid()}, {Key: "$Type", Value: "Workflows$CallMicroflowTask"}, + {Key: "BoundaryEvents", Value: bson.A{int32(2)}}, {Key: "Caption", Value: "Validate"}, + {Key: "Microflow", Value: "M.Validate"}, {Key: "Name", Value: "call"}, {Key: "PersistentId", Value: pid()}} + return bson.D{{Key: "$ID", Value: bid()}, {Key: "$Type", Value: "Workflows$Workflow"}, + {Key: "Flow", Value: bson.D{{Key: "$ID", Value: bid()}, {Key: "$Type", Value: "Workflows$Flow"}, + {Key: "Activities", Value: bson.A{int32(3), call, split}}}}, + {Key: "Name", Value: "WF"}, + {Key: "PersistentId", Value: pid()}, + {Key: "Title", Value: "WF"}} +} + +// persistentIDHolders returns the $Type of every element in v carrying a +// PersistentId. +func persistentIDHolders(v any) []string { + var out []string + switch x := v.(type) { + case bson.D: + typ, _ := get(x, "$Type") + for _, e := range x { + if e.Key == "PersistentId" { + out = append(out, typ.(string)) + } + out = append(out, persistentIDHolders(e.Value)...) + } + case bson.A: + for _, el := range x { + out = append(out, persistentIDHolders(el)...) + } + } + return out +} + +// Workflows exist from 9.0, PersistentId from 10.21.0 — on the workflow and on +// WorkflowActivity, Outcome and BoundaryEvent (mendixmodelsdk). Below that a +// workflow was refused ("cannot store Workflows$Workflow.PersistentId (Mendix +// 10.21.0+)"), and the activities' and outcomes' were never seen by the floor, +// which does not look up the abstract supertypes that declare them: `mx +// convert` 9.24.30 strips all of them. A PersistentId is minted by the writer, +// never stated by a script, so below 10.21 it is not written. +func TestNormalize_WorkflowPersistentIDsBelowMendix10_21(t *testing.T) { + for _, tt := range []struct { + v *version.ProjectVersion + want int + }{ + {pv(9, 24, 30), 0}, + {pv(10, 20, 0), 0}, + {pv(10, 21, 0), 5}, + {pv(10, 24, 24), 5}, + {pv(11, 13, 0), 5}, // control + } { + d, err := normalizeShape(t, tt.v, workflowWithPersistentIDs()) + if err != nil { + t.Errorf("%d.%d: a workflow is refused: %v", tt.v.MajorVersion, tt.v.MinorVersion, err) + continue + } + if got := persistentIDHolders(d); len(got) != tt.want { + t.Errorf("%d.%d: PersistentId on %d elements %v, want %d", tt.v.MajorVersion, tt.v.MinorVersion, len(got), got, tt.want) + } + } +} From 238f9dec58b9216420657979126599722c986069 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 17:43:08 +0000 Subject: [PATCH 06/18] fix(canon): complete a workflow's state-change handlers on Mendix 9.24 Symptom (found while measuring ako/mxcli#1163): with PersistentId lowered, `mx convert -p` 9.24.30 strips nothing from an mxcli-written workflow but adds WorkflowOnStateChangeEvent: null and UsertaskOnStateChangeEvent: null. Studio Pro's next save then differs in property names, which Mendix's merge engine refuses to compare (#1373). Cause: the two defaults (mendixmodelsdk: introduced 9.12.0, removed 11.0.0) were measured on 10.24.24 only, and an entry is added only from the lowest version it was measured on. Fix: lower their measured floor to 9.24.30, where `mx convert` adds both with the same null value. Measured: bug-test 1163-workflow-on-mendix9.mdl on a fresh 9.24.30 app: mx check 0 errors, and `mx convert -p` now neither strips nor adds a key on the workflow. Before, it added these two. The test fails on the previous floor (present false on 9.24). The 11.13 case stays absent as the control. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82sUVQEB3K5W5vh85wM --- CHANGELOG.md | 1 + .../canon/studiopro_property_defaults.json | 4 +-- .../mpr/writer_version_shape_1163_test.go | 26 +++++++++++++++++++ 3 files changed, 29 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a03ede06..bcd5486ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - **A navigation profile's application title and icon can be set and are described** (ako/mxcli#1756, mendixlabs/mxcli#1664) — the title (Studio Pro's *Application title*, `Mendix` in a new project) is what the runtime puts in every browser tab as `Mendix - `, and it had no MDL form: `create or modify navigation Responsive app title 'EWO' …` failed with "extraneous input 'app'", `describe navigation` printed neither it nor the *Application icon*, and `create or modify translations … for en_US` refuses the source language, so every generated app kept "Mendix" in the tab. `create or modify navigation` now takes `app title ''` and `app icon ` (`app icon none` clears it), in any order with the other clauses; `app` and `icon` stay usable as names. The title is written as the project default language's item, the title's other languages kept; the icon is resolved like any image reference, by `check --references` and before exec writes; an omitted clause keeps the stored value; describe prints both. A native profile has neither, so both are refused there. - **A test whose earlier `@expect` fails is no longer reported PASS on the after-startup runner** (ako/mxcli#1753) — every Docker run and `--local --legacy-runner` logged a verdict line per assertion, and the result was taken from the last one: a failing first assertion followed by a passing second reported PASS, with the failure message erased, and `mxcli test` exited 0. A failure now sticks, and the test reports the first one, plus how many more failed. The endpoint runner (`--local`, `--attach`, `--watch`) was not affected. - **Two tests that assign the same variable with a lower-case `call` or `create` run on the after-startup runner** (found with ako/mxcli#1753) — that runner compiles every test into one microflow and renames each test's variables, but it recognised only an upper-case `CALL` or `CREATE`. `$r = call microflow …`, the spelling mdl 1 and `fmt` write, was left as it was, so two tests assigning `$r` declared it twice and injection failed with MDL063 (CE0111 from mxbuild). 8 of the 16 test files in `mdl-examples/` hit this under Docker or `--legacy-runner`. A variable first assigned by `retrieve`, `declare` or a list operation is still not renamed there. +- **A workflow written to Mendix 9.24 has Studio Pro's property set** (ako/mxcli#1163) — its two state-change event handlers (`WorkflowOnStateChangeEvent`, `UsertaskOnStateChangeEvent`, removed in 11.0) were completed only from 10.24, where they were measured. `mx convert` 9.24.30 adds both as well, and Mendix's merge engine refuses to compare documents whose property names differ (#1373). - **Workflows are written on Mendix 9 and on 10.0–10.20** (ako/mxcli#1163) — every workflow was refused with "cannot store Workflows$Workflow.PersistentId (Mendix 10.21.0+)". Workflows exist from 9.0, but the persistent ID on a workflow, its activities, outcomes and boundary events arrived in 10.21. The activities' and outcomes' IDs were never refused, and `mx convert` 9.24 stripped them. mxcli mints these IDs itself, so below 10.21 it no longer writes them, for `create`, `create or modify` and the activities `alter workflow` inserts. A `parallel split` is still unwritable below 10.21: its paths end in an activity type that version does not have. - **Aggregates in microflows are written on Mendix 9** (ako/mxcli#1163) — every `count`, `sum`, `average`, `minimum` and `maximum` on a 9.x project was refused with "cannot store Microflows$AggregateAction.AggregateFunction (Mendix 10.0.0+)". The version data had the aggregate's function introduced in 10.0, which belongs to the fold `reduce` added. Studio Pro 9.24 stores it on every aggregate. An aggregate over an expression, `reduce`, `all` and `any` are Mendix 10.0 and are still refused below it, now naming the fold. - **Pages on Mendix 9.24 and 10.x are read and written in the version's own shape** (ako/mxcli#1209) — several page properties are stored differently below a version: a design property's value before 10.2, a data view's page parameter before 10.0, its editability before 10.7, and a text box's placeholder before 10.11. `describe` read only the newer form, so on 9.24 a describe → exec round trip removed every design property and placeholder. Writing any page with a data view or a design property was refused, and `alter styling` on a stored design property reported success and changed nothing. Every write now converts to the project's form, and `describe` reads both. What the older form cannot hold is refused, naming the widget: a Spacing or colour design property, conditional editability, a placeholder with parameters. diff --git a/modelsdk/canon/studiopro_property_defaults.json b/modelsdk/canon/studiopro_property_defaults.json index 5ceeade35..f05785e61 100644 --- a/modelsdk/canon/studiopro_property_defaults.json +++ b/modelsdk/canon/studiopro_property_defaults.json @@ -109,6 +109,6 @@ {"type":"Workflows$InterruptingTimerBoundaryEvent","key":"Annotation","since":"11.6.8","seen":"11.15.0","value":{"v":null}}, {"type":"Workflows$NonInterruptingTimerBoundaryEvent","key":"Annotation","since":"11.6.8","seen":"11.15.0","value":{"v":null}}, {"type":"Workflows$NonInterruptingTimerEventSubProcessStartActivity","key":"Recurrence","since":"11.15.0","seen":"11.15.0","value":{"v":null}}, - {"type":"Workflows$Workflow","key":"UsertaskOnStateChangeEvent","since":"10.24.24","until":"10.25.0","seen":"10.24.24","value":{"v":null}}, - {"type":"Workflows$Workflow","key":"WorkflowOnStateChangeEvent","since":"10.24.24","until":"10.25.0","seen":"10.24.24","value":{"v":null}} + {"type":"Workflows$Workflow","key":"UsertaskOnStateChangeEvent","since":"9.24.30","until":"10.25.0","seen":"10.24.24","value":{"v":null}}, + {"type":"Workflows$Workflow","key":"WorkflowOnStateChangeEvent","since":"9.24.30","until":"10.25.0","seen":"10.24.24","value":{"v":null}} ] diff --git a/modelsdk/mpr/writer_version_shape_1163_test.go b/modelsdk/mpr/writer_version_shape_1163_test.go index f3ae39ac5..a916b70ea 100644 --- a/modelsdk/mpr/writer_version_shape_1163_test.go +++ b/modelsdk/mpr/writer_version_shape_1163_test.go @@ -163,3 +163,29 @@ func TestNormalize_WorkflowPersistentIDsBelowMendix10_21(t *testing.T) { } } } + +// Studio Pro's property set for a workflow on 9.24 includes the two state-change +// event handlers Mendix removed in 11.0: `mx convert` 9.24.30 adds both as null +// to a workflow mxcli wrote, as 10.24.24 does — a property-name difference +// Mendix's merge engine refuses to compare (#1373). +func TestNormalize_WorkflowStateChangeEventsOnMendix9(t *testing.T) { + for _, tt := range []struct { + v *version.ProjectVersion + want bool + }{ + {pv(9, 24, 30), true}, + {pv(10, 24, 24), true}, + {pv(11, 13, 0), false}, // removed in 11.0 + } { + d, err := normalizeShape(t, tt.v, workflowWithPersistentIDs()) + if err != nil { + t.Fatal(err) + } + wf := find(d, "Workflows$Workflow") + for _, k := range []string{"WorkflowOnStateChangeEvent", "UsertaskOnStateChangeEvent"} { + if v, ok := get(wf, k); ok != tt.want || v != nil { + t.Errorf("%d.%d: %s present %v (%v), want present %v and null", tt.v.MajorVersion, tt.v.MinorVersion, k, ok, v, tt.want) + } + } + } +} From 2a76e0ee49dfa3e585f5d3212aa51301055468ab Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 17:51:21 +0000 Subject: [PATCH 07/18] fix(canon): write a constant override flat below Mendix 10.9 Symptom (ako/mxcli#1163, 14-project-settings on 9.24.30): `alter settings constant @M.C value '...' in configuration 'Default'` was refused with "this Mendix 9.24.30 project cannot store Settings$ConstantValue.SharedOrPrivateValue (Mendix 10.9.0+)". The same held on 10.0-10.8. Cause: the settings overlay writes a new override as a Settings$SharedValue nested under SharedOrPrivateValue. From 10.9 that is the only shape Mendix reads. Below 10.9 an override is {$ID, $Type, ConstantId, Value} with a flat string (mendixmodelsdk: Value deleted and SharedOrPrivateValue introduced in 10.9.0). With the floor off, `mx convert` 9.24.30 turns the nested form into Value: "" and the override silently becomes empty. Fix: canon.LowerToProjectShape replaces SharedOrPrivateValue with the flat Value in its place, or fills the empty flat sibling the overlay leaves. A Settings$PrivateValue has no flat form, so it is refused, naming the constant. The reader already preferred a flat Value; a backend test now pins reading and updating a 9.x flat override. Measured: bug-test 1163-constant-override-on-mendix9.mdl on a fresh 9.24.30 app: describe settings shows 'broker:9092', mx check 0 errors, and mx convert keeps the value and changes no key in the settings unit. 14-project-settings on 9.24: FAIL (the refusal) -> PASS. Controls: 10.24.24 and 11.13.0 PASS. The unit tests fail on the unfixed code with the reported refusal. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82sUVQEB3K5W5vh85wM --- ...-28-constant-override-flat-below-10-9.json | 1 + CHANGELOG.md | 1 + .../src/appendixes/version-compatibility.md | 2 + .../1163-constant-override-on-mendix9.mdl | 30 ++++++ .../settings_mendix9_constant_value_test.go | 101 ++++++++++++++++++ mdl/settingsoverlay/settingsoverlay.go | 8 +- modelsdk/canon/versionshape.go | 38 ++++++- .../mpr/writer_version_shape_1163_test.go | 71 ++++++++++++ 8 files changed, 248 insertions(+), 4 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/modelsdk/2026-10-28-constant-override-flat-below-10-9.json create mode 100644 mdl-examples/bug-tests/1163-constant-override-on-mendix9.mdl create mode 100644 mdl/backend/modelsdk/settings_mendix9_constant_value_test.go diff --git a/.claude/skills/fix-issue/findings/modelsdk/2026-10-28-constant-override-flat-below-10-9.json b/.claude/skills/fix-issue/findings/modelsdk/2026-10-28-constant-override-flat-below-10-9.json new file mode 100644 index 000000000..27ca2e65a --- /dev/null +++ b/.claude/skills/fix-issue/findings/modelsdk/2026-10-28-constant-override-flat-below-10-9.json @@ -0,0 +1 @@ +{"area": "modelsdk", "date": "2026-10-28", "symptom": "On Mendix 9.x and 10.0–10.8 a new constant override was refused: `alter settings constant @M.C value '…' in configuration 'Default'` → `this Mendix 9.24.30 project cannot store Settings$ConstantValue.SharedOrPrivateValue (Mendix 10.9.0+)` (14-project-settings on 9.24)", "cause": "settingsoverlay writes a NEW override in the nested shape (Settings$SharedValue under SharedOrPrivateValue), which is the only shape 10.9+ reads; below 10.9 the override is `{$ID, $Type, ConstantId, Value}` with a flat string. The overlay already kept an existing flat override flat, so only a new one failed", "file": "`modelsdk/canon/versionshape.go` (LowerToProjectShape: lowerConstantValue), `mdl/settingsoverlay/settingsoverlay.go` (comment)", "insight": "**`mx convert` on the older version is a silent-data-loss detector, not just a key oracle**: given the nested form, 9.24.30 kept the override but emptied it (`Value: \"\"`) and still reported success — an unlowered write that got past the floor would have shipped an empty override with mx check at 0 errors. Measure the VALUE after convert, not only the key set. A private value (Settings$PrivateValue, workstation-local) has no flat form and is refused naming the constant. The reader (settings_read.go constantValueOf) already preferred the flat Value, so no reader change was needed — TestProjectSettings_Mendix9FlatConstantValue pins it. Measured: bug-test on a fresh 9.24.30 app → describe settings prints 'broker:9092', mx check 0 errors, mx convert keeps the value and changes no key in the settings unit. 14-project-settings 9.24 FAIL→PASS; 10.24.24 and 11.13 PASS. Tests: TestNormalize_ConstantValueIsFlatBelowMendix10_9, TestNormalize_RefusesAPrivateConstantValueBelowMendix10_9; mdl-examples/bug-tests/1163-constant-override-on-mendix9.mdl", "refs": ["#1163", "#801"]} diff --git a/CHANGELOG.md b/CHANGELOG.md index bcd5486ec..5437a9653 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - **A navigation profile's application title and icon can be set and are described** (ako/mxcli#1756, mendixlabs/mxcli#1664) — the title (Studio Pro's *Application title*, `Mendix` in a new project) is what the runtime puts in every browser tab as `Mendix - `, and it had no MDL form: `create or modify navigation Responsive app title 'EWO' …` failed with "extraneous input 'app'", `describe navigation` printed neither it nor the *Application icon*, and `create or modify translations … for en_US` refuses the source language, so every generated app kept "Mendix" in the tab. `create or modify navigation` now takes `app title ''` and `app icon ` (`app icon none` clears it), in any order with the other clauses; `app` and `icon` stay usable as names. The title is written as the project default language's item, the title's other languages kept; the icon is resolved like any image reference, by `check --references` and before exec writes; an omitted clause keeps the stored value; describe prints both. A native profile has neither, so both are refused there. - **A test whose earlier `@expect` fails is no longer reported PASS on the after-startup runner** (ako/mxcli#1753) — every Docker run and `--local --legacy-runner` logged a verdict line per assertion, and the result was taken from the last one: a failing first assertion followed by a passing second reported PASS, with the failure message erased, and `mxcli test` exited 0. A failure now sticks, and the test reports the first one, plus how many more failed. The endpoint runner (`--local`, `--attach`, `--watch`) was not affected. - **Two tests that assign the same variable with a lower-case `call` or `create` run on the after-startup runner** (found with ako/mxcli#1753) — that runner compiles every test into one microflow and renames each test's variables, but it recognised only an upper-case `CALL` or `CREATE`. `$r = call microflow …`, the spelling mdl 1 and `fmt` write, was left as it was, so two tests assigning `$r` declared it twice and injection failed with MDL063 (CE0111 from mxbuild). 8 of the 16 test files in `mdl-examples/` hit this under Docker or `--legacy-runner`. A variable first assigned by `retrieve`, `declare` or a list operation is still not renamed there. +- **A constant override in a configuration is written on Mendix 9 and on 10.0–10.8** (ako/mxcli#1163) — `alter settings constant @M.C value '…' in configuration 'Default'` was refused with "cannot store Settings$ConstantValue.SharedOrPrivateValue (Mendix 10.9.0+)". Before 10.9 an override's value is a flat `Value` string; Studio Pro 9.24 does not know the nested form and empties the override on load. A new override is now written flat there, and an existing one is updated in its own shape. A private value has no form before 10.9 and is refused, naming the constant. - **A workflow written to Mendix 9.24 has Studio Pro's property set** (ako/mxcli#1163) — its two state-change event handlers (`WorkflowOnStateChangeEvent`, `UsertaskOnStateChangeEvent`, removed in 11.0) were completed only from 10.24, where they were measured. `mx convert` 9.24.30 adds both as well, and Mendix's merge engine refuses to compare documents whose property names differ (#1373). - **Workflows are written on Mendix 9 and on 10.0–10.20** (ako/mxcli#1163) — every workflow was refused with "cannot store Workflows$Workflow.PersistentId (Mendix 10.21.0+)". Workflows exist from 9.0, but the persistent ID on a workflow, its activities, outcomes and boundary events arrived in 10.21. The activities' and outcomes' IDs were never refused, and `mx convert` 9.24 stripped them. mxcli mints these IDs itself, so below 10.21 it no longer writes them, for `create`, `create or modify` and the activities `alter workflow` inserts. A `parallel split` is still unwritable below 10.21: its paths end in an activity type that version does not have. - **Aggregates in microflows are written on Mendix 9** (ako/mxcli#1163) — every `count`, `sum`, `average`, `minimum` and `maximum` on a 9.x project was refused with "cannot store Microflows$AggregateAction.AggregateFunction (Mendix 10.0.0+)". The version data had the aggregate's function introduced in 10.0, which belongs to the fold `reduce` added. Studio Pro 9.24 stores it on every aggregate. An aggregate over an expression, `reduce`, `all` and `any` are Mendix 10.0 and are still refused below it, now naming the fold. diff --git a/docs-site/src/appendixes/version-compatibility.md b/docs-site/src/appendixes/version-compatibility.md index 3f773a76f..6256f085e 100644 --- a/docs-site/src/appendixes/version-compatibility.md +++ b/docs-site/src/appendixes/version-compatibility.md @@ -186,12 +186,14 @@ Some properties kept their meaning and changed how they are stored. mxcli builds | A text box's, text area's or date picker's placeholder | 10.11 | `Placeholder` text, instead of a `PlaceholderTemplate` | | Any data source's `ForceFullObjects` | 10.8 | Not stored | | The `PersistentId` of a workflow, activity, outcome or boundary event | 10.21 | Not stored | +| A constant's value in a configuration (`alter settings constant … in configuration`) | 10.9 | A flat `Value` string on the override, instead of a `Settings$SharedValue` under `SharedOrPrivateValue` | What an older shape cannot hold is refused, naming the element, rather than dropped: - a Spacing, colour-picker or toggle-button-group design property below 10.2; - conditional data view editability below 10.7; - a placeholder with parameters below 10.11; - a data view fed by another widget below 10.0; +- a private constant value in a configuration below 10.9; - an aggregate over an expression (`sum $L of …`), `reduce`, `all` or `any` below 10.0. The plain `count`, `sum`, `average`, `minimum` and `maximum` are written on every version ([ako/mxcli#1163](https://github.com/ako/mxcli/issues/1163)). Before this, `describe` on 9.24 printed none of these properties, so a describe → exec round trip removed them. Writing a data view on 9.24 was refused outright. diff --git a/mdl-examples/bug-tests/1163-constant-override-on-mendix9.mdl b/mdl-examples/bug-tests/1163-constant-override-on-mendix9.mdl new file mode 100644 index 000000000..a0d8db719 --- /dev/null +++ b/mdl-examples/bug-tests/1163-constant-override-on-mendix9.mdl @@ -0,0 +1,30 @@ +mdl 1; +-- ako/mxcli#1163: on Mendix 9.x and on 10.0–10.8 a new constant override in a +-- configuration was refused: +-- +-- this Mendix 9.24.30 project cannot store +-- Settings$ConstantValue.SharedOrPrivateValue (Mendix 10.9.0+) +-- +-- Before 10.9 an override's value is a flat Value string on the +-- Settings$ConstantValue; from 10.9 it is a Settings$SharedValue nested under +-- SharedOrPrivateValue (mendixmodelsdk). `mx convert` 9.24.30 turns the nested +-- form into `{$ID, $Type, ConstantId, Value: ""}`: it does not know it, and the +-- override silently becomes empty. mxcli builds the nested form and lowers it +-- on the write. A private value (stored on the developer's workstation) has no +-- flat form and is refused below 10.9. +-- +-- Run it on a fresh 9.24.30 app (`mxcli new … --version 9.24.30.51827`): exec, +-- then `describe settings` shows the override as 'broker:9092', and `mxcli +-- docker check` reports the blank app's error count. + +create module Probe1163Const; + +create constant Probe1163Const.ServerUrl ( Type: string, DefaultValue: 'localhost:9092' ); + +-- A new override: built nested, stored flat. +alter settings constant @Probe1163Const.ServerUrl value 'kafka:9092' + in configuration 'Default'; + +-- An existing flat override is updated in place. +alter settings constant @Probe1163Const.ServerUrl value 'broker:9092' + in configuration 'Default'; diff --git a/mdl/backend/modelsdk/settings_mendix9_constant_value_test.go b/mdl/backend/modelsdk/settings_mendix9_constant_value_test.go new file mode 100644 index 000000000..b574c4221 --- /dev/null +++ b/mdl/backend/modelsdk/settings_mendix9_constant_value_test.go @@ -0,0 +1,101 @@ +// SPDX-License-Identifier: Apache-2.0 + +package modelsdkbackend + +import ( + "testing" + + "go.mongodb.org/mongo-driver/v2/bson" + + "github.com/mendixlabs/mxcli/mdl/bsonutil" + "github.com/mendixlabs/mxcli/mdl/settingsoverlay" + "github.com/mendixlabs/mxcli/modelsdk/bsoncompat" + mmpr "github.com/mendixlabs/mxcli/modelsdk/mpr" +) + +// seedFlatConstantValue gives the fixture's Default configuration one constant +// override in the shape Mendix stores below 10.9 (ako/mxcli#1163; `mx convert` +// 9.24.30): `{$ID, $Type, ConstantId, Value}`, the value a flat string. +func seedFlatConstantValue(t *testing.T, proj, constantID, value string) { + t.Helper() + r, err := mmpr.OpenWithOptions(proj, mmpr.OpenOptions{ReadOnly: false}) + if err != nil { + t.Fatalf("open fixture: %v", err) + } + defer r.Close() + refs, err := r.ListUnitsByType("Settings$ProjectSettings") + if err != nil || len(refs) != 1 { + t.Fatalf("ListUnitsByType(Settings$ProjectSettings) = %v, %v", refs, err) + } + raw, err := r.GetRawUnitBytes(refs[0].ID) + if err != nil { + t.Fatalf("GetRawUnitBytes: %v", err) + } + var doc bson.M + if err := bsoncompat.UnmarshalDocM(raw, &doc); err != nil { + t.Fatalf("unmarshal: %v", err) + } + seeded := false + for _, part := range settingsoverlay.ArrayElements(doc["Settings"]) { + if part["$Type"] != "Settings$ConfigurationSettings" { + continue + } + cfgs := settingsoverlay.ArrayElements(part["Configurations"]) + if len(cfgs) == 0 { + t.Fatalf("fixture has no configurations to seed") + } + cfgs[0]["ConstantValues"] = bson.A{int32(3), bson.D{ + {Key: "$ID", Value: bsonutil.NewIDBsonBinary()}, + {Key: "$Type", Value: "Settings$ConstantValue"}, + {Key: "ConstantId", Value: constantID}, + {Key: "Value", Value: value}, + }} + seeded = true + } + if !seeded { + t.Fatalf("fixture has no Settings$ConfigurationSettings part") + } + contents, err := bson.Marshal(bsonutil.OrderStorageValue(doc)) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if err := mmpr.NewWriterWithReader(r).UpdateRawUnit(refs[0].ID, contents); err != nil { + t.Fatalf("UpdateRawUnit: %v", err) + } +} + +// A Mendix 9 constant override is read from its flat Value, and updating it +// keeps that shape rather than introducing the nested one 9.x does not have. +func TestProjectSettings_Mendix9FlatConstantValue(t *testing.T) { + proj := copyFixture(t) + const id = "MyFirstModule.ServerUrl" + seedFlatConstantValue(t, proj, id, "kafka:9092") + + b := New() + if err := b.Connect(proj); err != nil { + t.Fatalf("connect: %v", err) + } + ps, err := b.GetProjectSettings() + if err != nil { + t.Fatalf("GetProjectSettings: %v", err) + } + cvs := ps.Configuration.Configurations[0].ConstantValues + if len(cvs) != 1 || cvs[0].ConstantId != id || cvs[0].Value != "kafka:9092" { + t.Fatalf("flat override read as %+v, want %s = kafka:9092", cvs, id) + } + + cvs[0].Value = "broker:9092" + if err := b.UpdateProjectSettings(ps); err != nil { + t.Fatalf("UpdateProjectSettings: %v", err) + } + if err := b.Disconnect(); err != nil { + t.Fatalf("disconnect: %v", err) + } + after := settingsoverlay.ArrayElements(readConfiguration(t, proj)["ConstantValues"]) + if len(after) != 1 || after[0]["Value"] != "broker:9092" { + t.Fatalf("flat override after update = %#v, want Value broker:9092", after) + } + if _, nested := after[0]["SharedOrPrivateValue"]; nested { + t.Errorf("updating a flat override added the nested shape: %#v", after[0]) + } +} diff --git a/mdl/settingsoverlay/settingsoverlay.go b/mdl/settingsoverlay/settingsoverlay.go index a11bfa375..1434bf917 100644 --- a/mdl/settingsoverlay/settingsoverlay.go +++ b/mdl/settingsoverlay/settingsoverlay.go @@ -343,9 +343,11 @@ func SetWebUISettings(ws *model.WebUISettings, raw map[string]any) map[string]an // ConstantValues rebuilds a configuration's ConstantValues list, updating each // override in the slot it is already stored in so its value shape survives. -// Studio Pro and mxbuild only read the nested SharedOrPrivateValue; a flat "Value" -// is a legacy shape mxcli's reader tolerates but the platform ignores, so writing -// every override flat made all constant overrides look empty in Studio Pro. +// From Mendix 10.9 Studio Pro and mxbuild only read the nested +// SharedOrPrivateValue, so writing every override flat made all constant +// overrides look empty in Studio Pro. Below 10.9 the flat "Value" is the only +// shape: a new override is still built nested here, and the write lowers it +// (canon.LowerToProjectShape, ako/mxcli#1163). func ConstantValues(cvs []*model.ConstantValue, raw any) bson.A { rawValues := ArrayElements(raw) byConstant := make(map[string]map[string]any, len(rawValues)) diff --git a/modelsdk/canon/versionshape.go b/modelsdk/canon/versionshape.go index 31be06cae..399afe049 100644 --- a/modelsdk/canon/versionshape.go +++ b/modelsdk/canon/versionshape.go @@ -38,6 +38,10 @@ import ( // CE6050 "The selected entity must be configured with a page parameter". // - ForceFullObjects, declared on the abstract Forms$DataSource from 10.8.0: // every data source carried it, and the floor never saw it. +// - Settings$ConstantValue: a Settings$SharedValue under SharedOrPrivateValue +// from 10.9.0; before, the value as a flat Value string, `{$ID, $Type, +// ConstantId, Value}` (`mx convert` 9.24.30, which empties the nested form +// it does not know). // - Workflows$ PersistentId, from 10.21.0 on the workflow and on the abstract // WorkflowActivity, Outcome and BoundaryEvent (ako/mxcli#1163). Workflows // exist from 9.0; `mx convert` 9.24.30 strips every one. The workflow's @@ -59,9 +63,10 @@ func LowerToProjectShape(contents []byte, pv *version.Version) ([]byte, error) { namedParameter: !PropertyDeclared("Forms$EntityPathSource", "SourceVariable", *pv), noFullObjects: !PropertyDeclared("Forms$DataSource", "ForceFullObjects", *pv), noPersistentID: !PropertyDeclared("Workflows$Workflow", "PersistentId", *pv), + flatConstant: !PropertyDeclared("Settings$ConstantValue", "SharedOrPrivateValue", *pv), } if !l.flatDesignProps && !l.boolEditable && !l.textPlaceholder && !l.namedParameter && !l.noFullObjects && - !l.noPersistentID { + !l.noPersistentID && !l.flatConstant { return contents, nil } var doc bson.D @@ -82,6 +87,7 @@ type shapeLowering struct { pv version.Version flatDesignProps, boolEditable, textPlaceholder bool namedParameter, noFullObjects, noPersistentID bool + flatConstant bool changed bool } @@ -113,6 +119,10 @@ func (l *shapeLowering) walk(v any) (any, error) { if err = l.lowerPlaceholder(x, typ); err != nil { return nil, err } + case l.flatConstant && typ == "Settings$ConstantValue": + if x, err = l.lowerConstantValue(x); err != nil { + return nil, err + } } if l.noFullObjects { if x, err = l.dropForceFullObjects(x, typ); err != nil { @@ -271,6 +281,32 @@ func (l *shapeLowering) dropForceFullObjects(d bson.D, typ string) (bson.D, erro return append(append(bson.D{}, d[:i]...), d[i+1:]...), nil } +// lowerConstantValue replaces a constant override's nested SharedOrPrivateValue +// with the flat Value older versions store, in its place. A flat Value already +// beside it (the overlay clears one to "" when it writes the nested form) takes +// the nested value. A private value — kept on the developer's workstation, which +// the flat form cannot say — is refused. +func (l *shapeLowering) lowerConstantValue(d bson.D) (bson.D, error) { + i := shapeIndex(d, "SharedOrPrivateValue") + if i < 0 { + return d, nil + } + inner, _ := d[i].Value.(bson.D) + if t, _ := shapeString(inner, "$Type"); t == "Settings$PrivateValue" { + constant, _ := shapeString(d, "ConstantId") + return nil, fmt.Errorf("constant %q has a private value in this configuration, which Mendix introduced in 10.9 "+ + "(this project is %s): a constant value there is shared, stored in the model", constant, l.pv) + } + value, _ := shapeString(inner, "Value") + l.changed = true + if j := shapeIndex(d, "Value"); j >= 0 { + d[j].Value = value + return append(append(bson.D{}, d[:i]...), d[i+1:]...), nil + } + d[i] = bson.E{Key: "Value", Value: value} + return d, nil +} + // dropPersistentID removes a workflow element's PersistentId below 10.21, where // none declares it. Whatever it holds is dropped, not refused: a PersistentId is // minted by the writer (a fresh GUID on every build), never stated by a script, diff --git a/modelsdk/mpr/writer_version_shape_1163_test.go b/modelsdk/mpr/writer_version_shape_1163_test.go index a916b70ea..eb00c2189 100644 --- a/modelsdk/mpr/writer_version_shape_1163_test.go +++ b/modelsdk/mpr/writer_version_shape_1163_test.go @@ -164,6 +164,77 @@ func TestNormalize_WorkflowPersistentIDsBelowMendix10_21(t *testing.T) { } } +// settingsWithConstantValue is the project settings unit as `alter settings +// constant … in configuration` writes a new override: the value nested in a +// Settings$SharedValue (or a Settings$PrivateValue, which holds none). flat is +// the empty flat Value the overlay leaves beside a nested one, "-" for none. +func settingsWithConstantValue(valueType, value, flat string) bson.D { + inner := bson.D{{Key: "$ID", Value: bid()}, {Key: "$Type", Value: valueType}} + if valueType == "Settings$SharedValue" { + inner = append(inner, bson.E{Key: "Value", Value: value}) + } + cv := bson.D{{Key: "$ID", Value: bid()}, {Key: "$Type", Value: "Settings$ConstantValue"}, + {Key: "ConstantId", Value: "M.ServerUrl"}, {Key: "SharedOrPrivateValue", Value: inner}} + if flat != "-" { + cv = append(cv, bson.E{Key: "Value", Value: flat}) + } + config := bson.D{{Key: "$ID", Value: bid()}, {Key: "$Type", Value: "Settings$ServerConfiguration"}, + {Key: "ConstantValues", Value: bson.A{int32(3), cv}}, {Key: "Name", Value: "Default"}} + part := bson.D{{Key: "$ID", Value: bid()}, {Key: "$Type", Value: "Settings$ConfigurationSettings"}, + {Key: "Configurations", Value: bson.A{int32(3), config}}} + return bson.D{{Key: "$ID", Value: bid()}, {Key: "$Type", Value: "Settings$ProjectSettings"}, + {Key: "Settings", Value: bson.A{int32(2), part}}} +} + +// A configuration's constant override is a flat Value string below 10.9 and a +// Settings$SharedValue under SharedOrPrivateValue from it (mendixmodelsdk: +// Value deleted, SharedOrPrivateValue introduced 10.9.0). `mx convert` 9.24.30 +// turns the nested form into `{$ID, $Type, ConstantId, Value: ""}` — it does +// not know the nested value, so the override silently became empty, and mxcli +// refused it ("cannot store Settings$ConstantValue.SharedOrPrivateValue +// (Mendix 10.9.0+)"). +func TestNormalize_ConstantValueIsFlatBelowMendix10_9(t *testing.T) { + for _, tt := range []struct { + v *version.ProjectVersion + flat bool + }{ + {pv(9, 24, 30), true}, + {pv(10, 8, 0), true}, + {pv(10, 9, 0), false}, + {pv(11, 13, 0), false}, // control + } { + for _, flat := range []string{"-", ""} { + d, err := normalizeShape(t, tt.v, settingsWithConstantValue("Settings$SharedValue", "kafka:9092", flat)) + if err != nil { + t.Errorf("%d.%d: a constant override is refused: %v", tt.v.MajorVersion, tt.v.MinorVersion, err) + continue + } + cv := find(d, "Settings$ConstantValue") + if !tt.flat { + if _, ok := get(cv, "SharedOrPrivateValue"); !ok { + t.Errorf("%d.%d: SharedOrPrivateValue was lowered: %v", tt.v.MajorVersion, tt.v.MinorVersion, keysOf(cv)) + } + continue + } + if got := strings.Join(keysOf(cv), ","); got != "$ID,$Type,ConstantId,Value" { + t.Errorf("%d.%d: constant value keys = %s, want $ID,$Type,ConstantId,Value", tt.v.MajorVersion, tt.v.MinorVersion, got) + } + if v, _ := get(cv, "Value"); v != "kafka:9092" { + t.Errorf("%d.%d: Value = %v, want kafka:9092", tt.v.MajorVersion, tt.v.MinorVersion, v) + } + } + } +} + +// A private value lives on the developer's workstation, which a flat Value +// cannot say: refused, naming the constant, not written as an empty override. +func TestNormalize_RefusesAPrivateConstantValueBelowMendix10_9(t *testing.T) { + _, err := normalizeShape(t, pv(9, 24, 30), settingsWithConstantValue("Settings$PrivateValue", "", "-")) + if err == nil || !strings.Contains(err.Error(), "M.ServerUrl") { + t.Errorf("a private constant value on 9.24 was not refused naming the constant: %v", err) + } +} + // Studio Pro's property set for a workflow on 9.24 includes the two state-change // event handlers Mendix removed in 11.0: `mx convert` 9.24.30 adds both as null // to a workflow mxcli wrote, as 10.24.24 does — a property-name difference From 2d87eb921682afaef074d62cd9e9fce3b93fff3a Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 17:58:53 +0000 Subject: [PATCH 08/18] fix(canon): leave a $currentObject flow argument out below Mendix 10.10 Symptom (ako/mxcli#1163, 29-datagrid DG12 on 9.24.30): a page passing `$currentObject` to a microflow or nanoflow was refused with "this Mendix 9.24.30 project cannot store Forms$MicroflowParameterMapping.Expression (Mendix 10.10.0+)". The same held on 10.0-10.9. Cause: bindParameterMappingValue writes such an argument as Expression text with Variable null. Expression is 10.10.0 on both mapping types. Before 10.10 a mapping holds only Variable, a Forms$PageVariable that is required there. Measured with the floor off: 9.24 does not load the page at all ("An error occurred when trying to set the 'Variable' property of a Microflow parameter mapping ... Value cannot be null"). Studio Pro 9 stores no mapping for an argument that is the enclosing object; it infers it. The 9.24.30 starter app and Workflow Commons 2.6.0 (9.24.0) have 82 flow calls taking 1-2 parameters and no mapping on any of them. Fix: below 10.10, canon.LowerToProjectShape leaves a $currentObject (or empty) argument out of every ParameterMappings list. An argument bound through Variable is kept; the floor drops its empty Expression. Any other expression has no form there, so it is refused, naming the parameter. Measured: bug-test 1163-flow-arguments-on-mendix9.mdl on a fresh 9.24.30 app: exec ok, mx check 0 errors, mx convert changes no key on the page, and describe -> exec is Unchanged. 29-datagrid on 9.24 now stops at DG15 (page variables, 10.17, a correct refusal); the DG2 NullReferenceException in mx check is the separate CE0463 gap. Controls: 29-datagrid PASS on 10.24.24 and 11.13.0. The unit tests fail on the unfixed code with the reported refusal. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82sUVQEB3K5W5vh85wM --- ...-flow-argument-expression-below-10-10.json | 1 + CHANGELOG.md | 1 + .../src/appendixes/version-compatibility.md | 2 + .../1163-flow-arguments-on-mendix9.mdl | 50 +++++++ modelsdk/canon/versionshape.go | 62 ++++++++- .../mpr/writer_version_shape_1163_test.go | 127 ++++++++++++++++++ 6 files changed, 241 insertions(+), 2 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/modelsdk/2026-10-28-flow-argument-expression-below-10-10.json create mode 100644 mdl-examples/bug-tests/1163-flow-arguments-on-mendix9.mdl diff --git a/.claude/skills/fix-issue/findings/modelsdk/2026-10-28-flow-argument-expression-below-10-10.json b/.claude/skills/fix-issue/findings/modelsdk/2026-10-28-flow-argument-expression-below-10-10.json new file mode 100644 index 000000000..78c6b027a --- /dev/null +++ b/.claude/skills/fix-issue/findings/modelsdk/2026-10-28-flow-argument-expression-below-10-10.json @@ -0,0 +1 @@ +{"area": "modelsdk", "date": "2026-10-28", "symptom": "On Mendix 9.x and 10.0–10.9 a page passing `$currentObject` to a microflow or nanoflow was refused: `this Mendix 9.24.30 project cannot store Forms$MicroflowParameterMapping.Expression (Mendix 10.10.0+)` (29-datagrid DG12 on 9.24)", "cause": "bindParameterMappingValue writes an argument that is not a page/snippet parameter or widget as Expression text with Variable null. Expression is 10.10.0; before it a mapping holds only Variable (a Forms$PageVariable, required before 10.10), and Studio Pro 9 does not store an argument that is the enclosing object at all — it infers it", "file": "`modelsdk/canon/versionshape.go` (LowerToProjectShape: lowerFlowArguments on any ParameterMappings list)", "insight": "**The floor's refusal hid a worse failure**: with the floor off, 9.24 does not load the project at all — `An error occurred when trying to set the 'Variable' property of a Microflow parameter mapping … ArgumentNullException` — so dropping only Expression would have shipped an unloadable page. **'Older shape' can be 'nothing stored'**: the 9.24 starter app and Workflow Commons 2.6.0 (9.24.0, from the Marketplace) have 82 flow calls taking 1–2 parameters and NOT ONE mapping (count calls against the called flow's parameters; an empty list alone proves nothing). So below 10.10 `$currentObject` (or an empty argument) is left out, a Variable-bound one keeps its Variable (the floor drops the empty Expression), and any other expression is refused naming the parameter. Readers needed nothing: describe already prints a call with no mappings as `call microflow M.F`, and re-executing that on 9.24 is Unchanged. Seen in passing: mxcli writes a populated Forms$CallNanoflowClientAction.ParameterMappings list with marker 3 (no RegisterListMarker for NanoflowParameterMapping), Studio Pro 2. Measured: bug-test on a fresh 9.24.30 app → exec ok, mx check 0 errors, mx convert changes no key; describe → exec Unchanged. 29-datagrid on 9.24 now stops at DG15 page variables (10.17, correct); 10.24.24 and 11.13 PASS. Tests: TestNormalize_FlowArgumentsBelowMendix10_10, TestNormalize_RefusesAnExpressionArgumentBelowMendix10_10; mdl-examples/bug-tests/1163-flow-arguments-on-mendix9.mdl", "refs": ["#1163", "#1140"], "ce": ["CE1571"]} diff --git a/CHANGELOG.md b/CHANGELOG.md index 5437a9653..2c79bb216 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - **A navigation profile's application title and icon can be set and are described** (ako/mxcli#1756, mendixlabs/mxcli#1664) — the title (Studio Pro's *Application title*, `Mendix` in a new project) is what the runtime puts in every browser tab as `Mendix - `, and it had no MDL form: `create or modify navigation Responsive app title 'EWO' …` failed with "extraneous input 'app'", `describe navigation` printed neither it nor the *Application icon*, and `create or modify translations … for en_US` refuses the source language, so every generated app kept "Mendix" in the tab. `create or modify navigation` now takes `app title ''` and `app icon ` (`app icon none` clears it), in any order with the other clauses; `app` and `icon` stay usable as names. The title is written as the project default language's item, the title's other languages kept; the icon is resolved like any image reference, by `check --references` and before exec writes; an omitted clause keeps the stored value; describe prints both. A native profile has neither, so both are refused there. - **A test whose earlier `@expect` fails is no longer reported PASS on the after-startup runner** (ako/mxcli#1753) — every Docker run and `--local --legacy-runner` logged a verdict line per assertion, and the result was taken from the last one: a failing first assertion followed by a passing second reported PASS, with the failure message erased, and `mxcli test` exited 0. A failure now sticks, and the test reports the first one, plus how many more failed. The endpoint runner (`--local`, `--attach`, `--watch`) was not affected. - **Two tests that assign the same variable with a lower-case `call` or `create` run on the after-startup runner** (found with ako/mxcli#1753) — that runner compiles every test into one microflow and renames each test's variables, but it recognised only an upper-case `CALL` or `CREATE`. `$r = call microflow …`, the spelling mdl 1 and `fmt` write, was left as it was, so two tests assigning `$r` declared it twice and injection failed with MDL063 (CE0111 from mxbuild). 8 of the 16 test files in `mdl-examples/` hit this under Docker or `--legacy-runner`. A variable first assigned by `retrieve`, `declare` or a list operation is still not renamed there. +- **A page passing `$currentObject` to a microflow or nanoflow is written on Mendix 9 and on 10.0–10.9** (ako/mxcli#1163) — `call microflow M.F(P = $currentObject)` on a button or a data grid column was refused with "cannot store Forms$MicroflowParameterMapping.Expression (Mendix 10.10.0+)". An argument held as an expression arrived in 10.10. Before it, Studio Pro stores no argument for the enclosing object and infers it. Without the refusal, 9.24 could not even load the page, because the mapping had no `Variable`. Below 10.10 that argument is now left out, and a page parameter argument is stored as before. Any other expression argument is refused, naming the parameter. - **A constant override in a configuration is written on Mendix 9 and on 10.0–10.8** (ako/mxcli#1163) — `alter settings constant @M.C value '…' in configuration 'Default'` was refused with "cannot store Settings$ConstantValue.SharedOrPrivateValue (Mendix 10.9.0+)". Before 10.9 an override's value is a flat `Value` string; Studio Pro 9.24 does not know the nested form and empties the override on load. A new override is now written flat there, and an existing one is updated in its own shape. A private value has no form before 10.9 and is refused, naming the constant. - **A workflow written to Mendix 9.24 has Studio Pro's property set** (ako/mxcli#1163) — its two state-change event handlers (`WorkflowOnStateChangeEvent`, `UsertaskOnStateChangeEvent`, removed in 11.0) were completed only from 10.24, where they were measured. `mx convert` 9.24.30 adds both as well, and Mendix's merge engine refuses to compare documents whose property names differ (#1373). - **Workflows are written on Mendix 9 and on 10.0–10.20** (ako/mxcli#1163) — every workflow was refused with "cannot store Workflows$Workflow.PersistentId (Mendix 10.21.0+)". Workflows exist from 9.0, but the persistent ID on a workflow, its activities, outcomes and boundary events arrived in 10.21. The activities' and outcomes' IDs were never refused, and `mx convert` 9.24 stripped them. mxcli mints these IDs itself, so below 10.21 it no longer writes them, for `create`, `create or modify` and the activities `alter workflow` inserts. A `parallel split` is still unwritable below 10.21: its paths end in an activity type that version does not have. diff --git a/docs-site/src/appendixes/version-compatibility.md b/docs-site/src/appendixes/version-compatibility.md index 6256f085e..006e0596c 100644 --- a/docs-site/src/appendixes/version-compatibility.md +++ b/docs-site/src/appendixes/version-compatibility.md @@ -187,6 +187,7 @@ Some properties kept their meaning and changed how they are stored. mxcli builds | Any data source's `ForceFullObjects` | 10.8 | Not stored | | The `PersistentId` of a workflow, activity, outcome or boundary event | 10.21 | Not stored | | A constant's value in a configuration (`alter settings constant … in configuration`) | 10.9 | A flat `Value` string on the override, instead of a `Settings$SharedValue` under `SharedOrPrivateValue` | +| A page's flow call argument that is the enclosing object (`call microflow M.F(P = $currentObject)`, `call nanoflow …`) | 10.10 | Not stored: Studio Pro infers it. A page parameter, snippet parameter or widget argument is stored as before, as a `Variable` | What an older shape cannot hold is refused, naming the element, rather than dropped: - a Spacing, colour-picker or toggle-button-group design property below 10.2; @@ -194,6 +195,7 @@ What an older shape cannot hold is refused, naming the element, rather than drop - a placeholder with parameters below 10.11; - a data view fed by another widget below 10.0; - a private constant value in a configuration below 10.9; +- a page's flow call argument that is any other expression (a literal, an attribute path) below 10.10; - an aggregate over an expression (`sum $L of …`), `reduce`, `all` or `any` below 10.0. The plain `count`, `sum`, `average`, `minimum` and `maximum` are written on every version ([ako/mxcli#1163](https://github.com/ako/mxcli/issues/1163)). Before this, `describe` on 9.24 printed none of these properties, so a describe → exec round trip removed them. Writing a data view on 9.24 was refused outright. diff --git a/mdl-examples/bug-tests/1163-flow-arguments-on-mendix9.mdl b/mdl-examples/bug-tests/1163-flow-arguments-on-mendix9.mdl new file mode 100644 index 000000000..8934125f3 --- /dev/null +++ b/mdl-examples/bug-tests/1163-flow-arguments-on-mendix9.mdl @@ -0,0 +1,50 @@ +mdl 1; +-- ako/mxcli#1163: on Mendix 9.x and on 10.0–10.9 a page passing +-- `$currentObject` to a microflow or nanoflow was refused: +-- +-- this Mendix 9.24.30 project cannot store +-- Forms$MicroflowParameterMapping.Expression (Mendix 10.10.0+) +-- +-- An argument stored as Expression text arrived in 10.10. Before it, a +-- mapping holds only a Variable (a Forms$PageVariable, required: a null one +-- makes 9.24 fail to load the page). An argument that is the enclosing object +-- is not stored at all, because Studio Pro 9.24 infers it: Workflow Commons +-- 2.6.0 (9.24.0) has 77 flow calls taking parameters and no mapping on any. +-- Below 10.10 mxcli therefore leaves `$currentObject` out and keeps a page +-- parameter as its Variable. Any other expression (a literal, an attribute +-- path) has no form there and is refused, naming the parameter. +-- +-- Run it on a fresh 9.24.30 app (`mxcli new … --version 9.24.30.51827`): exec, +-- then `mxcli docker check` reports the blank app's error count, and `mx +-- convert -p` changes no key on the page. + +create module Probe1163Args; + +create persistent entity Probe1163Args.Item ( + Name: String(100) +); + +create microflow Probe1163Args.ACT_Item ( $Item: Probe1163Args.Item ) +begin + log info node 'Probe1163Args' 'item'; +end; + +create nanoflow Probe1163Args.NF_Item ( $Item: Probe1163Args.Item ) +begin + log info node 'Probe1163Args' 'item'; +end; + +create page Probe1163Args.Item_Edit ( + Title: 'Item', + Layout: Atlas_Core.Atlas_Default, + Params: ( $Item: Probe1163Args.Item ) +) { + dataview dv (DataSource: $Item) { + actionbutton btnCurrent (Caption: 'Current', + Action: call microflow Probe1163Args.ACT_Item(Item = $currentObject)) + actionbutton btnNanoflow (Caption: 'Nanoflow', + Action: call nanoflow Probe1163Args.NF_Item(Item = $currentObject)) + } + actionbutton btnParam (Caption: 'Param', + Action: call microflow Probe1163Args.ACT_Item(Item = $Item)) +}; diff --git a/modelsdk/canon/versionshape.go b/modelsdk/canon/versionshape.go index 399afe049..0ee201248 100644 --- a/modelsdk/canon/versionshape.go +++ b/modelsdk/canon/versionshape.go @@ -42,6 +42,13 @@ import ( // from 10.9.0; before, the value as a flat Value string, `{$ID, $Type, // ConstantId, Value}` (`mx convert` 9.24.30, which empties the nested form // it does not know). +// - Forms$MicroflowParameterMapping / NanoflowParameterMapping: an argument +// as Expression text from 10.10.0; before, only a Variable (a required +// Forms$PageVariable), and an argument that is the enclosing object is not +// stored at all — Studio Pro 9.24 infers it (Workflow Commons 2.6.0: 77 +// flow calls taking parameters, no mapping on any). mxcli writes that +// argument as Expression `$currentObject` with Variable null, which 9.24 +// cannot even load. // - Workflows$ PersistentId, from 10.21.0 on the workflow and on the abstract // WorkflowActivity, Outcome and BoundaryEvent (ako/mxcli#1163). Workflows // exist from 9.0; `mx convert` 9.24.30 strips every one. The workflow's @@ -64,9 +71,10 @@ func LowerToProjectShape(contents []byte, pv *version.Version) ([]byte, error) { noFullObjects: !PropertyDeclared("Forms$DataSource", "ForceFullObjects", *pv), noPersistentID: !PropertyDeclared("Workflows$Workflow", "PersistentId", *pv), flatConstant: !PropertyDeclared("Settings$ConstantValue", "SharedOrPrivateValue", *pv), + variableArgs: !PropertyDeclared("Forms$MicroflowParameterMapping", "Expression", *pv), } if !l.flatDesignProps && !l.boolEditable && !l.textPlaceholder && !l.namedParameter && !l.noFullObjects && - !l.noPersistentID && !l.flatConstant { + !l.noPersistentID && !l.flatConstant && !l.variableArgs { return contents, nil } var doc bson.D @@ -87,7 +95,7 @@ type shapeLowering struct { pv version.Version flatDesignProps, boolEditable, textPlaceholder bool namedParameter, noFullObjects, noPersistentID bool - flatConstant bool + flatConstant, variableArgs bool changed bool } @@ -132,6 +140,11 @@ func (l *shapeLowering) walk(v any) (any, error) { if l.noPersistentID && strings.HasPrefix(typ, "Workflows$") { x = l.dropPersistentID(x) } + if l.variableArgs { + if err = l.lowerFlowArguments(x); err != nil { + return nil, err + } + } for i := range x { if x[i].Value, err = l.walk(x[i].Value); err != nil { return nil, err @@ -307,6 +320,51 @@ func (l *shapeLowering) lowerConstantValue(d bson.D) (bson.D, error) { return d, nil } +var flowParameterMappings = map[string]bool{ + "Forms$MicroflowParameterMapping": true, "Forms$NanoflowParameterMapping": true, +} + +// lowerFlowArguments rewrites the flow call arguments in d's ParameterMappings +// list, if it has one, into what a version before 10.10 stores: an argument +// bound through Variable keeps it (its empty Expression is the floor's to +// drop); an argument that is the enclosing object (`$currentObject`) — or one +// that binds nothing — is left out, which is how Studio Pro 9.24 stores it. +// Any other expression has no Variable form and is refused, naming the +// parameter. +func (l *shapeLowering) lowerFlowArguments(d bson.D) error { + i := shapeIndex(d, "ParameterMappings") + if i < 0 { + return nil + } + list, _ := d[i].Value.(bson.A) + out := bson.A{} + dropped := false + for _, el := range list { + m, ok := el.(bson.D) + typ, _ := shapeString(m, "$Type") + if !ok || !flowParameterMappings[typ] { + out = append(out, el) + continue + } + if v, _ := shapeValue(m, "Variable"); v != nil { + out = append(out, el) + continue + } + expr, _ := shapeString(m, "Expression") + if e := strings.TrimSpace(expr); e != "" && !strings.EqualFold(e, "$currentObject") { + param, _ := shapeString(m, "Parameter") + return fmt.Errorf("the argument for %s is the expression %s, which Mendix introduced in 10.10 (this project is %s): "+ + "an argument there is the enclosing object, or a page parameter, snippet parameter or widget", param, expr, l.pv) + } + dropped = true + } + if dropped { + d[i].Value = out + l.changed = true + } + return nil +} + // dropPersistentID removes a workflow element's PersistentId below 10.21, where // none declares it. Whatever it holds is dropped, not refused: a PersistentId is // minted by the writer (a fresh GUID on every build), never stated by a script, diff --git a/modelsdk/mpr/writer_version_shape_1163_test.go b/modelsdk/mpr/writer_version_shape_1163_test.go index eb00c2189..3d33cfcb7 100644 --- a/modelsdk/mpr/writer_version_shape_1163_test.go +++ b/modelsdk/mpr/writer_version_shape_1163_test.go @@ -235,6 +235,133 @@ func TestNormalize_RefusesAPrivateConstantValueBelowMendix10_9(t *testing.T) { } } +// flowMapping is one argument of a page's flow call as mxcli builds it +// (bindParameterMappingValue): a page-variable reference under Variable with an +// empty Expression, anything else as Expression text with Variable null. +func flowMapping(mappingType, param, expression, pageParam string) bson.D { + var variable any + if pageParam != "" { + variable = bson.D{{Key: "$ID", Value: bid()}, {Key: "$Type", Value: "Forms$PageVariable"}, + {Key: "PageParameter", Value: pageParam}, {Key: "SnippetParameter", Value: ""}, + {Key: "UseAllPages", Value: false}, {Key: "Widget", Value: ""}} + } + return bson.D{{Key: "$ID", Value: bid()}, {Key: "$Type", Value: mappingType}, + {Key: "Expression", Value: expression}, {Key: "Parameter", Value: param}, {Key: "Variable", Value: variable}} +} + +// pageWithFlowCalls is a page with a microflow button and a nanoflow button, +// each passing the given mappings. +func pageWithFlowCalls(microflowMappings, nanoflowMappings []bson.D) bson.D { + list := func(ms []bson.D) bson.A { + a := bson.A{int32(2)} + for _, m := range ms { + a = append(a, m) + } + return a + } + mfButton := bson.D{{Key: "$ID", Value: bid()}, {Key: "$Type", Value: "Forms$ActionButton"}, {Key: "Name", Value: "btnMf"}, + {Key: "Action", Value: bson.D{{Key: "$ID", Value: bid()}, {Key: "$Type", Value: "Forms$MicroflowAction"}, + {Key: "MicroflowSettings", Value: bson.D{{Key: "$ID", Value: bid()}, {Key: "$Type", Value: "Forms$MicroflowSettings"}, + {Key: "Microflow", Value: "M.ACT"}, {Key: "ParameterMappings", Value: list(microflowMappings)}}}}}} + nfButton := bson.D{{Key: "$ID", Value: bid()}, {Key: "$Type", Value: "Forms$ActionButton"}, {Key: "Name", Value: "btnNf"}, + {Key: "Action", Value: bson.D{{Key: "$ID", Value: bid()}, {Key: "$Type", Value: "Forms$CallNanoflowClientAction"}, + {Key: "Nanoflow", Value: "M.NF"}, {Key: "ParameterMappings", Value: list(nanoflowMappings)}}}} + return bson.D{{Key: "$ID", Value: bid()}, {Key: "$Type", Value: "Forms$Page"}, {Key: "Name", Value: "P"}, + {Key: "Widgets", Value: bson.A{int32(2), mfButton, nfButton}}} +} + +// mappingsOf returns the mappings stored under the first $Type holder in d. +func mappingsOf(t *testing.T, d bson.D, holder string) []bson.D { + t.Helper() + list, _ := get(find(d, holder), "ParameterMappings") + var out []bson.D + for _, el := range listItems(list) { + out = append(out, el.(bson.D)) + } + return out +} + +func listItems(v any) bson.A { + a, _ := v.(bson.A) + if len(a) > 0 { + if _, marker := a[0].(int32); marker { + return a[1:] + } + } + return a +} + +// A flow argument on a page is a Variable (a Forms$PageVariable, required) below +// 10.10, and Expression text only from 10.10 (mendixmodelsdk). mxcli writes +// `$currentObject` as Expression with Variable null: the floor refused it on +// 9.24 ("cannot store Forms$MicroflowParameterMapping.Expression (Mendix +// 10.10.0+)"), and without the floor 9.24 does not load the page at all ("An +// error occurred when trying to set the 'Variable' property of a Microflow +// parameter mapping … Value cannot be null"). Studio Pro 9.24 stores no mapping +// for an argument that is the enclosing object — it infers it: Workflow +// Commons 2.6.0 (9.24.0), 75 microflow and 2 nanoflow calls taking parameters, +// none with a mapping; the 9.24.30 starter app's ShowPasswordForm(Account) +// button likewise. So below 10.10 that mapping is not written; a page +// parameter keeps its Variable and loses only the empty Expression. +func TestNormalize_FlowArgumentsBelowMendix10_10(t *testing.T) { + build := func() bson.D { + return pageWithFlowCalls( + []bson.D{flowMapping("Forms$MicroflowParameterMapping", "M.ACT.Item", "$currentObject", ""), + flowMapping("Forms$MicroflowParameterMapping", "M.ACT.Order", "", "Order")}, + []bson.D{flowMapping("Forms$NanoflowParameterMapping", "M.NF.Item", "$currentObject", "")}) + } + for _, tt := range []struct { + v *version.ProjectVersion + lowered bool + mfCount int + nfCount int + mfParams string + }{ + {pv(9, 24, 30), true, 1, 0, "M.ACT.Order"}, + {pv(10, 9, 0), true, 1, 0, "M.ACT.Order"}, + {pv(10, 10, 0), false, 2, 1, "M.ACT.Item,M.ACT.Order"}, + {pv(11, 13, 0), false, 2, 1, "M.ACT.Item,M.ACT.Order"}, // control + } { + d, err := normalizeShape(t, tt.v, build()) + if err != nil { + t.Errorf("%d.%d: the page is refused: %v", tt.v.MajorVersion, tt.v.MinorVersion, err) + continue + } + mf := mappingsOf(t, d, "Forms$MicroflowSettings") + nf := mappingsOf(t, d, "Forms$CallNanoflowClientAction") + var params []string + for _, m := range mf { + p, _ := get(m, "Parameter") + params = append(params, p.(string)) + if _, ok := get(m, "Expression"); ok == tt.lowered { + t.Errorf("%d.%d: mapping %v keeps Expression %v", tt.v.MajorVersion, tt.v.MinorVersion, p, ok) + } + if v, _ := get(m, "Variable"); tt.lowered && v == nil { + t.Errorf("%d.%d: mapping %v has no Variable, which 9.x cannot load", tt.v.MajorVersion, tt.v.MinorVersion, p) + } + } + if len(mf) != tt.mfCount || len(nf) != tt.nfCount || strings.Join(params, ",") != tt.mfParams { + t.Errorf("%d.%d: %d microflow mappings (%s) and %d nanoflow mappings, want %d (%s) and %d", + tt.v.MajorVersion, tt.v.MinorVersion, len(mf), strings.Join(params, ","), len(nf), tt.mfCount, tt.mfParams, tt.nfCount) + } + } +} + +// Any other expression — a literal, an attribute path — has no Variable form: +// refused below 10.10, naming the parameter, rather than dropped. +func TestNormalize_RefusesAnExpressionArgumentBelowMendix10_10(t *testing.T) { + for _, m := range []bson.D{ + flowMapping("Forms$MicroflowParameterMapping", "M.ACT.Flag", "true", ""), + flowMapping("Forms$NanoflowParameterMapping", "M.NF.Name", "$Order/Name", ""), + } { + _, err := normalizeShape(t, pv(9, 24, 30), pageWithFlowCalls([]bson.D{m}, nil)) + p, _ := get(m, "Parameter") + if err == nil || !strings.Contains(err.Error(), p.(string)) || !strings.Contains(err.Error(), "10.10") { + t.Errorf("an expression argument for %v on 9.24 was not refused naming it: %v", p, err) + } + } +} + // Studio Pro's property set for a workflow on 9.24 includes the two state-change // event handlers Mendix removed in 11.0: `mx convert` 9.24.30 adds both as null // to a workflow mxcli wrote, as 10.24.24 does — a property-name difference From 16c1c7fee3ec9dec7b4c22ff2d715da397b1f196 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 18:07:21 +0000 Subject: [PATCH 09/18] fix(executor): gate create published odata service at Mendix 10.4 Symptom (ako/mxcli#1163, 10-odata and 18-folder on 9.24.30): a published OData service was refused at write time with "this Mendix 9.24.30 project cannot store ODataPublish$PublishedODataService2.IncludeMetadataByDefault (Mendix 10.8.0+)". By then the script's earlier statements were already written. Cause: not the property. mxcli writes the service as ODataPublish$PublishedODataService2, a document type mendixmodelsdk introduces in 10.4.0. Before 10.4 a published OData service is a Rest$PublishedODataService, which is deleted in 10.4.0: a different document with its own children, and mxcli does not write it. The generated version data has property floors only, so the floor tripped over the first versioned key. Measured with the floor off: mx check 9.24.30 cannot load the project ("TypeCacheUnknownTypeException: The type cache does not contain a type with qualified name ODataPublish$PublishedODataService2"). Lowering the property would have made an unloadable app. Fix: a version gate. `create [or modify] published odata service` needs integration.published_odata_service, 10.4.0. `check -p` and exec's pre-flight report it as MDL-VERSION01 before anything is written, with a hint naming the 9.x document. Writing Rest$PublishedODataService for 9.x is not attempted here. Measured: bug-test 1163-published-odata-below-10-4.mdl with `check -p` on a fresh 9.24.30 app reports MDL-VERSION01. 10-odata and 18-folder on 9.24 now stop at the gate. Controls: both PASS on 10.24.24 and 11.13.0. The new gate cases fail without the gate (0 violations). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82sUVQEB3K5W5vh85wM --- ...28-published-odata-is-a-10-4-document.json | 1 + CHANGELOG.md | 1 + .../src/appendixes/version-compatibility.md | 2 + .../1163-published-odata-below-10-4.mdl | 42 +++++++++++++++++++ mdl/executor/version_gates.go | 11 ++++- mdl/executor/version_gates_test.go | 11 ++++- sdk/versions/mendix-10.yaml | 11 +++++ sdk/versions/mendix-11.yaml | 4 ++ 8 files changed, 79 insertions(+), 4 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-10-28-published-odata-is-a-10-4-document.json create mode 100644 mdl-examples/bug-tests/1163-published-odata-below-10-4.mdl diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-28-published-odata-is-a-10-4-document.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-28-published-odata-is-a-10-4-document.json new file mode 100644 index 000000000..87ed75f19 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-28-published-odata-is-a-10-4-document.json @@ -0,0 +1 @@ +{"area": "mdl/executor", "date": "2026-10-28", "symptom": "On Mendix 9.x `create published odata service` was refused at write time with `this Mendix 9.24.30 project cannot store ODataPublish$PublishedODataService2.IncludeMetadataByDefault (Mendix 10.8.0+)` (10-odata and 18-folder on 9.24), after the earlier statements of the script were already written", "cause": "mxcli writes a published OData service as ODataPublish$PublishedODataService2, a document type introduced in 10.4.0; before it the service is Rest$PublishedODataService (deleted 10.4.0), a different document with its own children. The generated version data has no type-level floor, so the property floor could only trip over the first versioned key and named the wrong thing", "file": "`mdl/executor/version_gates.go` (CreateODataServiceStmt gate), `sdk/versions/mendix-10.yaml` / `mendix-11.yaml` (integration.published_odata_service 10.4.0)", "insight": "**Before lowering a refused property, check the type it sits on exists in that version.** Lowering IncludeMetadataByDefault was the obvious fix and would have made it worse: with the floor off, mx check 9.24.30 stops loading the project at all (`TypeCacheUnknownTypeException: The type cache does not contain a type with qualified name ODataPublish$PublishedODataService2`). mendixmodelsdk says it directly: `PublishedODataService2.versionInfo = {introduced: \"10.4.0\"}`, `rest.PublishedODataService` `deleted: \"10.4.0\"`. The generated tables carry property floors only, so a whole-type floor has to be a version gate (check-time, MDL-VERSION01, before anything is written) — the same gap lets Workflows$EndOfParallelSplitPathActivity (10.21) through on 9.x. Measured: 10-odata and 18-folder on 9.24 now fail at the gate (no write); on 10.24.24 and 11.13 both PASS. Tests: TestCheckVersionGates_ReportsEveryHandlerGate (published odata on 9.24.30 and 10.3), TestCheckVersionGates_UngatedFormsPass (10.4 passes); mdl-examples/bug-tests/1163-published-odata-below-10-4.mdl", "refs": ["#1163", "#1434"]} diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c79bb216..8982c988e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - **A navigation profile's application title and icon can be set and are described** (ako/mxcli#1756, mendixlabs/mxcli#1664) — the title (Studio Pro's *Application title*, `Mendix` in a new project) is what the runtime puts in every browser tab as `Mendix - `, and it had no MDL form: `create or modify navigation Responsive app title 'EWO' …` failed with "extraneous input 'app'", `describe navigation` printed neither it nor the *Application icon*, and `create or modify translations … for en_US` refuses the source language, so every generated app kept "Mendix" in the tab. `create or modify navigation` now takes `app title ''` and `app icon ` (`app icon none` clears it), in any order with the other clauses; `app` and `icon` stay usable as names. The title is written as the project default language's item, the title's other languages kept; the icon is resolved like any image reference, by `check --references` and before exec writes; an omitted clause keeps the stored value; describe prints both. A native profile has neither, so both are refused there. - **A test whose earlier `@expect` fails is no longer reported PASS on the after-startup runner** (ako/mxcli#1753) — every Docker run and `--local --legacy-runner` logged a verdict line per assertion, and the result was taken from the last one: a failing first assertion followed by a passing second reported PASS, with the failure message erased, and `mxcli test` exited 0. A failure now sticks, and the test reports the first one, plus how many more failed. The endpoint runner (`--local`, `--attach`, `--watch`) was not affected. - **Two tests that assign the same variable with a lower-case `call` or `create` run on the after-startup runner** (found with ako/mxcli#1753) — that runner compiles every test into one microflow and renames each test's variables, but it recognised only an upper-case `CALL` or `CREATE`. `$r = call microflow …`, the spelling mdl 1 and `fmt` write, was left as it was, so two tests assigning `$r` declared it twice and injection failed with MDL063 (CE0111 from mxbuild). 8 of the 16 test files in `mdl-examples/` hit this under Docker or `--legacy-runner`. A variable first assigned by `retrieve`, `declare` or a list operation is still not renamed there. +- **`create published odata service` on Mendix 9 and 10.0–10.3 is refused up front, naming the real reason** (ako/mxcli#1163) — it was refused at write time for "ODataPublish$PublishedODataService2.IncludeMetadataByDefault (Mendix 10.8.0+)". That property was not the problem: the whole document mxcli writes is a 10.4 type, and Mendix 9.24 cannot load a project that holds one. Before 10.4 a published OData service is a different document that mxcli does not write. `check -p` and `exec` now report MDL-VERSION01 before writing anything. - **A page passing `$currentObject` to a microflow or nanoflow is written on Mendix 9 and on 10.0–10.9** (ako/mxcli#1163) — `call microflow M.F(P = $currentObject)` on a button or a data grid column was refused with "cannot store Forms$MicroflowParameterMapping.Expression (Mendix 10.10.0+)". An argument held as an expression arrived in 10.10. Before it, Studio Pro stores no argument for the enclosing object and infers it. Without the refusal, 9.24 could not even load the page, because the mapping had no `Variable`. Below 10.10 that argument is now left out, and a page parameter argument is stored as before. Any other expression argument is refused, naming the parameter. - **A constant override in a configuration is written on Mendix 9 and on 10.0–10.8** (ako/mxcli#1163) — `alter settings constant @M.C value '…' in configuration 'Default'` was refused with "cannot store Settings$ConstantValue.SharedOrPrivateValue (Mendix 10.9.0+)". Before 10.9 an override's value is a flat `Value` string; Studio Pro 9.24 does not know the nested form and empties the override on load. A new override is now written flat there, and an existing one is updated in its own shape. A private value has no form before 10.9 and is refused, naming the constant. - **A workflow written to Mendix 9.24 has Studio Pro's property set** (ako/mxcli#1163) — its two state-change event handlers (`WorkflowOnStateChangeEvent`, `UsertaskOnStateChangeEvent`, removed in 11.0) were completed only from 10.24, where they were measured. `mx convert` 9.24.30 adds both as well, and Mendix's merge engine refuses to compare documents whose property names differ (#1373). diff --git a/docs-site/src/appendixes/version-compatibility.md b/docs-site/src/appendixes/version-compatibility.md index 006e0596c..e6b4cd22a 100644 --- a/docs-site/src/appendixes/version-compatibility.md +++ b/docs-site/src/appendixes/version-compatibility.md @@ -200,6 +200,8 @@ What an older shape cannot hold is refused, naming the element, rather than drop Before this, `describe` on 9.24 printed none of these properties, so a describe → exec round trip removed them. Writing a data view on 9.24 was refused outright. +A published OData service is not an older shape of the same document. Before 10.4 it is a different document (`Rest$PublishedODataService`, not `ODataPublish$PublishedODataService2`), which mxcli does not write. `create published odata service` is refused below 10.4 with MDL-VERSION01, by `check -p` and by `exec` before anything is written. Mendix 9.24 cannot load a project that holds the 10.4 document. + ### Widget Type Definitions Pluggable widget PropertyTypes change between Mendix versions. A widget template extracted from 11.6 may have more or fewer properties than the widget installed in a 10.24 project. The [widget augmentation system](../internals/widget-templates.md) handles this by reconciling templates against the project's `.mpk` files. diff --git a/mdl-examples/bug-tests/1163-published-odata-below-10-4.mdl b/mdl-examples/bug-tests/1163-published-odata-below-10-4.mdl new file mode 100644 index 000000000..d42e11fea --- /dev/null +++ b/mdl-examples/bug-tests/1163-published-odata-below-10-4.mdl @@ -0,0 +1,42 @@ +mdl 1; +-- ako/mxcli#1163: on Mendix 9.x a published OData service was refused with +-- a message naming the wrong thing: +-- +-- this Mendix 9.24.30 project cannot store +-- ODataPublish$PublishedODataService2.IncludeMetadataByDefault (Mendix 10.8.0+) +-- +-- That property is not the problem. The whole document mxcli writes, +-- ODataPublish$PublishedODataService2, was introduced in 10.4.0 +-- (mendixmodelsdk). Before 10.4 a published OData service is a +-- Rest$PublishedODataService, a different document that mxcli does not +-- write. With the floor off, mx check 9.24.30 cannot load the project +-- ("TypeCacheUnknownTypeException: ... ODataPublish$PublishedODataService2"), +-- so lowering the property would have produced an unloadable app. +-- +-- `mxcli check -p <9.24 app>` now reports MDL-VERSION01 "create +-- published odata service" before anything is written, and exec refuses the +-- same way. On 10.4 and later the service is written as before. + +create module Probe1163OData; + +create persistent entity Probe1163OData.Customer ( + Name: String(100), + Email: String(200) unique +); + +create published odata service Probe1163OData.CustomerAPI ( + Path: 'odata/customers/', + Version: '1.0.0', + ODataVersion: OData4, + Namespace: 'Probe1163OData.Customers', + ServiceName: 'Customer Service' +) +{ + publish entity Probe1163OData.Customer as 'Customers' ( + ReadMode: source + ) + expose ( + Email as 'customerId' (KEY, Filterable, Sortable), + Name + ); +}; diff --git a/mdl/executor/version_gates.go b/mdl/executor/version_gates.go index 8d6fe0ae5..33514acad 100644 --- a/mdl/executor/version_gates.go +++ b/mdl/executor/version_gates.go @@ -53,11 +53,18 @@ func statementVersionGates(stmt ast.Statement) []versionGate { return []versionGate{{"integration", "published_rest_grant_revoke", "grant access on published rest service", "upgrade your project to 10.0+"}} case *ast.CreateODataServiceStmt: + // The document mxcli writes, ODataPublish$PublishedODataService2, is + // 10.4+; before, a published OData service is a Rest$PublishedODataService, + // a different document mxcli does not write (ako/mxcli#1163). + gates := []versionGate{{"integration", "published_odata_service", "create published odata service", + "Before Mendix 10.4 a published OData service is a different document " + + "(Rest$PublishedODataService), which mxcli does not write. Publish it in Studio Pro, or upgrade the project."}} if s.SupportsGraphQL { - return []versionGate{{"integration", "odata_graphql", "SupportsGraphQL on a published OData service", + gates = append(gates, versionGate{"integration", "odata_graphql", "SupportsGraphQL on a published OData service", "Publishing a service as GraphQL as well arrived in Studio Pro 10.14. " + - "Remove SupportsGraphQL to publish OData only."}} + "Remove SupportsGraphQL to publish OData only."}) } + return gates case *ast.CreateMessageDefinitionStmt: return []versionGate{messageDefinitionDocumentGate("create message definition")} case *ast.AlterSettingsStmt: diff --git a/mdl/executor/version_gates_test.go b/mdl/executor/version_gates_test.go index 18b555be7..7b320b2e0 100644 --- a/mdl/executor/version_gates_test.go +++ b/mdl/executor/version_gates_test.go @@ -32,6 +32,10 @@ func TestCheckVersionGates_ReportsEveryHandlerGate(t *testing.T) { {"alter published rest", &ast.AlterPublishedRestServiceStmt{}, [3]int{9, 24, 0}, "alter published rest service"}, {"grant published rest", &ast.GrantPublishedRestServiceAccessStmt{}, [3]int{9, 24, 0}, "grant access on published rest service"}, {"odata graphql", &ast.CreateODataServiceStmt{SupportsGraphQL: true}, [3]int{10, 13, 0}, "SupportsGraphQL"}, + // ako/mxcli#1163: the document mxcli writes is 10.4+; 9.24.30 does not + // load a project holding one (TypeCacheUnknownTypeException). + {"published odata", &ast.CreateODataServiceStmt{}, [3]int{9, 24, 30}, "create published odata service"}, + {"published odata 10.3", &ast.CreateODataServiceStmt{}, [3]int{10, 3, 0}, "create published odata service"}, {"message definition document", &ast.CreateMessageDefinitionStmt{}, [3]int{11, 14, 0}, "create message definition"}, {"workflow group", &ast.AlterSettingsStmt{Section: "WORKFLOWS", AddGroup: true, GroupName: "G"}, [3]int{11, 1, 0}, "add group"}, {"view entity", &ast.CreateViewEntityStmt{}, [3]int{10, 17, 0}, "create view entity"}, @@ -104,14 +108,13 @@ func TestCheckVersionGates_ReportsEveryHandlerGate(t *testing.T) { // Statements with no gate, or a gated statement without the gated clause, are // not reported — `synchronize all` on 9.0, a page without parameters, OData -// without GraphQL. +// without GraphQL from 10.4. func TestCheckVersionGates_UngatedFormsPass(t *testing.T) { prog := &ast.Program{Statements: []ast.Statement{ &ast.CreateModuleStmt{Name: "M"}, &ast.CreateMicroflowStmt{}, &ast.CreateNanoflowStmt{Body: []ast.MicroflowStatement{&ast.SynchronizeStmt{SyncType: "All"}}}, &ast.CreatePageStmtV3{}, - &ast.CreateODataServiceStmt{}, &ast.AlterSettingsStmt{Section: "MODEL"}, // No EXPORT LEVEL clause: the stored level is carried (#1498). &ast.CreateEntityStmt{Attributes: []ast.Attribute{{Name: "A"}}}, @@ -122,6 +125,10 @@ func TestCheckVersionGates_UngatedFormsPass(t *testing.T) { if vs := versionGateViolations(versionCtx(t, 9, 0, 0), prog); len(vs) != 0 { t.Errorf("ungated forms reported on 9.0: %+v", vs) } + odata := &ast.Program{Statements: []ast.Statement{&ast.CreateModuleStmt{Name: "M"}, &ast.CreateODataServiceStmt{}}} + if vs := versionGateViolations(versionCtx(t, 10, 4, 0), odata); len(vs) != 0 { + t.Errorf("a published OData service without GraphQL reported on 10.4: %+v", vs) + } } // One source of truth: a handler that called checkFeature with its own feature diff --git a/sdk/versions/mendix-10.yaml b/sdk/versions/mendix-10.yaml index adebf0d35..207443cab 100644 --- a/sdk/versions/mendix-10.yaml +++ b/sdk/versions/mendix-10.yaml @@ -167,6 +167,17 @@ features: mdl: "CREATE BUSINESS EVENT SERVICE Module.Name ..." odata_client: min_version: "10.0.0" + published_odata_service: + # mxcli writes a published OData service as ODataPublish$PublishedODataService2, + # introduced in 10.4.0 (mendixmodelsdk); before, it is a + # Rest$PublishedODataService, a different document with its own children, + # which mxcli does not write. Measured: mx check 9.24.30 cannot load a + # project holding a PublishedODataService2 — + # "TypeCacheUnknownTypeException: ... ODataPublish$PublishedODataService2" + # (ako/mxcli#1163). Below this the property floor used to refuse + # IncludeMetadataByDefault instead, which named the wrong thing. + min_version: "10.4.0" + mdl: "CREATE PUBLISHED ODATA SERVICE Module.Name (...) { PUBLISH ENTITY ... }" odata_graphql: # A published OData service can also answer GraphQL — one boolean, same # resources, an extra endpoint. Introduced in Studio Pro 10.14 as an diff --git a/sdk/versions/mendix-11.yaml b/sdk/versions/mendix-11.yaml index 687a18b3b..02a08812e 100644 --- a/sdk/versions/mendix-11.yaml +++ b/sdk/versions/mendix-11.yaml @@ -179,6 +179,10 @@ features: mdl: "CREATE BUSINESS EVENT SERVICE Module.Name ..." odata_client: min_version: "10.0.0" + published_odata_service: + # ODataPublish$PublishedODataService2 is 10.4+; see mendix-10.yaml. + min_version: "10.4.0" + mdl: "CREATE PUBLISHED ODATA SERVICE Module.Name (...) { PUBLISH ENTITY ... }" odata_graphql: # A published OData service can also answer GraphQL — one boolean, same # resources, an extra endpoint. Introduced in Studio Pro 10.14 as an From 1b62f211cf6c1fc993fe8539afa911206ea980db Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 18:07:44 +0000 Subject: [PATCH 10/18] docs(wiki): a version refusal is not always an older shape The 9.24 doctype run for ako/mxcli#1163 split five refusals three ways: a wrong floor in the generated data (AggregateFunction), a document type the version lacks (PublishedODataService2, where lowering would have produced an unloadable project), and an older shape that stores nothing (a flow argument that is the enclosing object). Record what settled each one: the SDK's version data, Studio Pro-authored bytes from a Marketplace .mpk, and `mx convert -p` on the older version against a floor-off build, comparing values as well as keys. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82sUVQEB3K5W5vh85wM --- .../bug-patterns/the-platform-ci-does-not-run.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/docs-wiki/bug-patterns/the-platform-ci-does-not-run.md b/docs-wiki/bug-patterns/the-platform-ci-does-not-run.md index 1b1116a8d..8e4c471e1 100644 --- a/docs-wiki/bug-patterns/the-platform-ci-does-not-run.md +++ b/docs-wiki/bug-patterns/the-platform-ci-does-not-run.md @@ -91,6 +91,18 @@ every unit write passes (`canon.LowerToProjectShape`, before the floor), and to read both shapes. A starter app is the cheapest source of the older shape's real bytes: `mxcli new --version 9.24.30.51827` authored by Studio Pro. +A refusal is not always an older shape, though. In the next 9.24 run (#1163), one +of five refusals was the floor's own data being wrong: an aggregate's function had +been given the row of the fold `reduce` added in 10.0. One was a document type the +version does not have: a published OData service before 10.4. That needs a gate, +because lowering its property would have produced a project that does not load. +For one more, the older shape was "nothing stored": Studio Pro 9 infers a flow +argument that is the enclosing object. Three things settled each one: the SDK's +version data, Studio Pro-authored bytes (a Marketplace `.mpk` holds a whole +`project.mpr` of its Mendix version), and `mx convert -p` on the older version +run against a floor-off build. Convert strips undeclared keys and empties values +it does not understand, so compare values as well as key sets. + **mxcli sometimes generates the environment it then fails in.** The non-root devcontainer that could not provision PostgreSQL was scaffolded by `mxcli new` — the same code emits that base image, installs postgresql, and runs as `vscode`. From 714fdf42f8e3185240604654d8caaa2906689240 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 19:49:15 +0000 Subject: [PATCH 11/18] fix(test): git doctor tests no longer read the developer's git config `git remote get-url` applies url..insteadOf, and doctorEnv left the system, global and GIT_CONFIG_COUNT/KEY/VALUE configuration in place. In a sandbox that rewrites git@github.com: to https://github.com/ through the environment, the SSH remote the tests add already read back as HTTPS, and TestGitDoctor_SSHRemoteRewrittenByFix and TestGitDoctor_SSHURLFormRewrittenByFix failed `make test` on main (#1670) while CI was green. A ~/.gitconfig rewriting https to git@ fails the same two tests the other way. doctorEnv now sets GIT_CONFIG_GLOBAL to the null device, GIT_CONFIG_NOSYSTEM=1 and GIT_CONFIG_COUNT=0. Control: before the fix both tests fail under each rewrite separately (environment, and HOME with an https->git@ insteadOf); with an empty HOME and no environment rewrite they pass. After it, both rewrites pass. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82sUVQEB3K5W5vh85wM --- ...026-10-28-git-doctor-tests-read-ambient-insteadof.json | 1 + cmd/mxcli/gitdoctor_test.go | 8 ++++++++ 2 files changed, 9 insertions(+) create mode 100644 .claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-git-doctor-tests-read-ambient-insteadof.json diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-git-doctor-tests-read-ambient-insteadof.json b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-git-doctor-tests-read-ambient-insteadof.json new file mode 100644 index 000000000..85b1854a1 --- /dev/null +++ b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-git-doctor-tests-read-ambient-insteadof.json @@ -0,0 +1 @@ +{"area":"cmd/mxcli","date":"2026-10-28","symptom":"`make test` failed on main right after #1670 (git doctor) merged, in a hosted sandbox only: TestGitDoctor_SSHRemoteRewrittenByFix and TestGitDoctor_SSHURLFormRewrittenByFix reported `remote url = \"https://github.com/org/app.git\\n\", want https://github.com/org/app`; CI was green","cause":"`git remote get-url` applies url..insteadOf, and the tests ran git under the developer's own configuration. The sandbox sets GIT_CONFIG_COUNT/KEY/VALUE rewriting git@github.com: and ssh://git@github.com/ to https://github.com/, so the SSH remote the test added already read back as HTTPS and --fix never touched it; a ~/.gitconfig rewriting https://github.com/ to git@github.com: (the usual developer setup) fails the same two tests the other way, `git@github.com:org/app`","file":"`cmd/mxcli/gitdoctor_test.go` (`doctorEnv`: GIT_CONFIG_GLOBAL=os.DevNull, GIT_CONFIG_NOSYSTEM=1, GIT_CONFIG_COUNT=0)","insight":"A test that shells out to git inherits three configuration sources besides the repository's — system, global, and the GIT_CONFIG_COUNT environment — and isolating HOME or the identity variables covers none of the third. Reproduce each source separately, because they fail in opposite directions: env rewrite on this host, then `env -u GIT_CONFIG_* HOME=git@ insteadOf>`, with an empty HOME as the passing control. Both failed before the fix and pass after. Not fixed, and a product question rather than a test one: with a global https->git@ rewrite, `git doctor` reads the effective URL, so it reports SSH and `--fix` sets an HTTPS URL that the rewrite turns straight back into SSH, every run","refs":["ako/mxcli#1670"]} diff --git a/cmd/mxcli/gitdoctor_test.go b/cmd/mxcli/gitdoctor_test.go index c0e0393f4..fb2cf56c1 100644 --- a/cmd/mxcli/gitdoctor_test.go +++ b/cmd/mxcli/gitdoctor_test.go @@ -23,6 +23,14 @@ import ( func doctorEnv(t *testing.T, dir string) { t.Helper() requireGit(t) + // The repository under test is the only git configuration these tests may + // see. `git remote get-url` applies url..insteadOf, so a rewrite in + // ~/.gitconfig (https -> git@, common on developer machines) or in the + // GIT_CONFIG_COUNT/KEY/VALUE environment (git@ -> https, set by hosted + // sandboxes) changed the URL the remote tests read back. + t.Setenv("GIT_CONFIG_GLOBAL", os.DevNull) + t.Setenv("GIT_CONFIG_NOSYSTEM", "1") + t.Setenv("GIT_CONFIG_COUNT", "0") t.Setenv("GIT_CEILING_DIRECTORIES", filepath.Dir(dir)) t.Setenv("GIT_AUTHOR_NAME", "t") t.Setenv("GIT_AUTHOR_EMAIL", "t@example.com") From b927395afb92cac7ae014b7e918d7c8eab2bce6c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 19:29:12 +0000 Subject: [PATCH 12/18] fix(make): fmt-check uses the toolchain's gofmt, not the one on PATH go.mod's toolchain line makes `go` (and `make fmt`) switch to go1.27.3, but fmt-check ran a bare `gofmt`, which is whatever Go is installed. With go1.24.7 installed, `make lint` failed main on a file CI accepts (mdl/backend/mfmutator/splice_handler_test.go) and passed a file CI then rejected (ako/mxcli#1759). GOFMT defaults to $(go env GOROOT)/bin/gofmt. Control: `make fmt-check GOFMT=/usr/local/go/bin/gofmt` still fails on splice_handler_test.go; the default passes. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82sUVQEB3K5W5vh85wM --- Makefile | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 12f80861e..4af1eae41 100644 --- a/Makefile +++ b/Makefile @@ -470,13 +470,21 @@ lint-go: fmt-check vet # Tracked files only: the ANTLR parser under mdl/grammar/parser is generated at # build time and deliberately not committed. The empty-list guard is a positive # control -- a check that inspected nothing must fail loudly rather than pass. +# +# The gofmt is the toolchain's, not the first one on PATH. go.mod's `toolchain` +# line makes `go` (and so `make fmt`) switch to that release, but a bare `gofmt` +# is whatever Go is installed, and gofmt's output changes between releases. With +# Go 1.24.7 installed and the toolchain at 1.27.3, the bare one failed main on a +# file CI accepts (mdl/backend/mfmutator/splice_handler_test.go) and passed a +# file CI then rejected (ako/mxcli#1759). +GOFMT ?= $(shell go env GOROOT)/bin/gofmt fmt-check: @files=$$(git ls-files '*.go' | grep -v '/parser/'); \ if [ -z "$$files" ]; then \ echo "fmt-check: found no Go files to check -- the file list is wrong"; \ exit 1; \ fi; \ - unformatted=$$(gofmt -l $$files); \ + unformatted=$$($(GOFMT) -l $$files); \ if [ -n "$$unformatted" ]; then \ echo "Not gofmt-clean (run 'make fmt'):"; \ echo "$$unformatted"; \ From e1d6cc90b6e44e87027040be9f37c25d0fbf9b77 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 19:29:12 +0000 Subject: [PATCH 13/18] build(make): add ci-local, the build-and-test job run before pushing `make build test lint` covers less than CI's build-and-test job gates on: check-mdl, check-findings, check-skill-index, check-wiki-pages and the rest ran only in CI unless someone remembered them. ci-local runs the job's steps in its order (check-skill-mdl standing for its three skill/docs MDL steps), without govulncheck, which the job installs @latest over the network, and without the integration jobs, which need mxbuild. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82sUVQEB3K5W5vh85wM --- Makefile | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 4af1eae41..f64a174fa 100644 --- a/Makefile +++ b/Makefile @@ -8,6 +8,7 @@ # make test-integration - Run integration tests (requires mx/mxbuild) # make test-mdl - Run MDL integration tests (requires Docker) # make lint - Lint all code (Go + TypeScript) +# make ci-local - Run what CI's build-and-test job runs, before pushing # make lint-go - Lint Go code (fmt + vet) # make lint-ts - Lint TypeScript code (tsc --noEmit) # make grammar - Regenerate ANTLR parser @@ -35,7 +36,7 @@ GO_BUILD_FLAGS = -trimpath # Clean version for VS Code extension (must be valid semver: major.minor.patch) VSCE_VERSION = $(shell echo "$(VERSION)" | sed 's/^v//; s/-.*//' | grep -E '^[0-9]+\.[0-9]+\.[0-9]+$$' || echo "0.0.0") -.PHONY: build build-debug size release clean test test-mdl check-mdl check-skill-mdl check-conformance conformance-shrink gen-migration-reference check-migration-reference check-skill-pack-js check-findings check-wiki-pages digest-status check-tunnel-deps check-test-timeouts check-widget-versions test-integration test-integration-executor test-integration-roundtrip test-integration-parity test-integration-upgrade test-integration-other grammar completions sync-skills sync-skill-packs sync-commands sync-mods sync-lint-rules sync-changelog sync-all docs documentation docs-site docs-serve vscode-ext vscode-install source-tree sbom sbom-report lint lint-go lint-ts fmt fmt-check vet skill-index check-skill-index +.PHONY: build build-debug size release clean test ci-local test-mdl check-mdl check-skill-mdl check-conformance conformance-shrink gen-migration-reference check-migration-reference check-skill-pack-js check-findings check-wiki-pages digest-status check-tunnel-deps check-test-timeouts check-widget-versions test-integration test-integration-executor test-integration-roundtrip test-integration-parity test-integration-upgrade test-integration-other grammar completions sync-skills sync-skill-packs sync-commands sync-mods sync-lint-rules sync-changelog sync-all docs documentation docs-site docs-serve vscode-ext vscode-install source-tree sbom sbom-report lint lint-go lint-ts fmt fmt-check vet skill-index check-skill-index # Helper: copy file only if content differs (avoids mtime updates that invalidate go build cache) # Usage: $(call copy-if-changed,src,dst) @@ -453,6 +454,20 @@ 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 +# `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. +# +# 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 + @echo "ci-local passed (build-and-test, without govulncheck)" + # Lint Go code # # Depends on fmt-check, which VERIFIES, not on fmt, which rewrites. `go fmt ./...` From 76d34ab8c5821ab6dece7b0e099c6e54a7b46cb1 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 19:49:31 +0000 Subject: [PATCH 14/18] docs(skills): add the steward skill, the loop from PR opened to merged Holds this repository's conventions for driving a PR Claude opened to green: the user reviews before the PR is opened and it is merged on green by default (merge commit, expectedHeadSha, branch restarted from main afterwards); `make ci-local` before every push; the macOS tunnel-seam job that is cancelled before it gets a runner, and so gets one re-run; the conflicts this repo keeps hitting (CHANGELOG, generated indexes and tables); and how to treat the AI PR Review comment's Critical / Moderate / Minor sections. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82sUVQEB3K5W5vh85wM --- .claude/skills/steward/SKILL.md | 121 ++++++++++++++++++++++++++++++++ 1 file changed, 121 insertions(+) create mode 100644 .claude/skills/steward/SKILL.md diff --git a/.claude/skills/steward/SKILL.md b/.claude/skills/steward/SKILL.md new file mode 100644 index 000000000..76b305efe --- /dev/null +++ b/.claude/skills/steward/SKILL.md @@ -0,0 +1,121 @@ +--- +name: steward +description: Drive an mxcli pull request Claude opened to green and merge it. Covers when to open it, the local gate to pass before every push (make ci-local), which CI failure is a known runner flake, how to resolve the conflicts this repo keeps hitting, how to read the AI PR Review comment, and the merge-on-green default. Read before acting on a CI, review or conflict event on such a PR. +--- + +## Find it fast + +`sed -n ',p' SKILL.md` one section rather than reading the file; +`./mxcli syntax ` answers *how do I write this* for a fraction of either. + +| lines | section | +|---|---| +| 27-54 | Merge on green is the default | +| 55-72 | Before every push: `make ci-local` | +| 73-87 | Reading a red CI job | +| 88-103 | Conflicts this repo keeps hitting | +| 104-121 | The AI PR Review comment | + + +# Steward: Driving a PR to Merged + +This is a **contributor** skill (dev-only); it is not synced to user projects. +It holds this repository's conventions for the loop after a PR is opened. How +to make the change itself is `/mxcli-dev:fix-issue` and the CLAUDE.md working +rules; this file starts where they stop. + +## Merge on green is the default + +The user reviews a change **in the session, before the PR exists**. So: + +- Open a PR only after the user has seen the change summarized in the session + and asked for it. Never open one to get a review. +- Once it is open, merge it without asking again as soon as all of these hold + on the **current head**: + - every job of `Build, Test & Lint` (`.github/workflows/push-test.yml`) is + green, `integration-passed` included. A push to the PR branch runs that + workflow twice on the same commit, for the push and the pull_request + event; a job that passed in one run and was cancelled before it got a + runner in the other counts as green; + - GitHub reports no merge conflict; + - no human review thread or changes-requested review is open; + - no Critical item from the AI PR Review comment is unanswered (see below). +- Merge with a **merge commit** (not squash, not rebase), passing the head SHA + you checked as `expectedHeadSha`, so a push that lands in between is not + merged unseen. +- After the merge, unsubscribe from the PR, cancel its pending check-in, and + restart the working branch from the new main: + `git fetch origin main && git checkout -B origin/main`, then push. + The next PR starts from there; never stack new work on merged history. + +Ask instead of merging when a human reviewer has commented or requested +changes, or when the PR is someone else's. Never merge a red head, whatever +the reason it is red. + +## Before every push: `make ci-local` + +`make build test lint` is a subset of the gate CI applies. `make ci-local` +runs what the `build-and-test` job runs, in its order, minus `govulncheck`. +Run it before the first push and before every push that changes code, and +push only on a clean exit. It takes a while; run it in the background. + +- **Formatting:** `make fmt` is the fixer. `fmt-check` uses the toolchain's + gofmt (`go env GOROOT`), because the gofmt on `PATH` can be an older Go than + the `toolchain` line in `go.mod`, and its verdict differs from CI's. +- **Integration suites** need mxbuild, so `ci-local` does not run them. When + the change touches a package with integration tests, run that suite: + `make test-integration-`. +- **When the job gains a step,** add it to `ci-local` in the same PR. +- **A failure in code the change does not touch:** run the same target on + `origin/main`. If it fails there too, it is main's: fix it in its own PR + (it can go first), and never skip or disable the test to get past it. + +## Reading a red CI job + +The workflow is the source of truth for what runs; these are the jobs whose +failures need reading differently. + +- **`build-and-test`:** reproduce with `make ci-local`. +- **`integration ()`:** reproduce with `make test-integration-`. +- **`tunnel-seam-cross-platform (macos-latest | windows-latest)`:** the one + known runner flake. The hosted macOS queue sometimes cancels the job before + it gets a runner (the job shows runner id 0 and no step ran). That is a job + that died before any test body ran: re-run the failed jobs **once**. A test + failure in this job is real: it runs the tunnel seam's own tests on that + platform (ADR-0009). +- **`AI PR Review`** is always green: it posts a comment, it does not gate. + +## Conflicts this repo keeps hitting + +While the PR is open, merge `origin/main` into its head. Do not rebase or +force-push it. + +| File | Resolution | +|---|---| +| `CHANGELOG.md` `[Unreleased]` | Keep **both** sides: every entry is one line, so a conflict is two fixes landing at once. Afterwards check for a stray blank line or a duplicated entry. | +| A `SKILL.md` section index | Keep either side, then `make skill-index`. | +| `cmd/mxcli/skills/` | Never resolve by hand: `make sync-skills` regenerates it from `.claude/skills/mendix/`. | +| `docs-site/src/language/versions.md` tables | `make gen-migration-reference`. | +| `mdl/conformance/allowlist.txt` | Take the higher count on each conflicting line, then `make conformance-shrink`, which lowers every count to what the gate finds and never raises one. | +| Bug findings | They do not conflict: each finding is its own new file. | + +Run `make ci-local` on the merge result before pushing it. + +## The AI PR Review comment + +`ai-review.yml` posts **one PR comment** headed `## AI Code Review`, not review +threads. It has sections Critical Issues, Moderate Issues, Minor Issues, What +Looks Good and Recommendation. On a large PR the model sees only a selected +part of the diff, so a claim that something is missing may only mean that file +was outside the selection. + +| Section | How to treat it | +|---|---| +| Critical | Blocking. Verify each item against the code: fix and push it, or answer it with the path you traced and why it does not hold. | +| Moderate | A claim to verify, not an order. Fix it when a real caller reaches the failure and the fix is in proportion to it; otherwise say why not. | +| Minor | Optional. Never start a push for one. Carry the plainly correct ones into a push that already touches those files. | + +Answer it with **one** PR comment that gives the outcome for each Critical and +Moderate item. Sometimes nothing is posted at all: the workflow skips posting +when its API key is missing or the API call fails, and still reports success. +A missing comment is neither an approval nor a blocker. From 982c4b1f55cba180317ea9e9ad5101291aa40892 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 21:55:19 +0000 Subject: [PATCH 15/18] fix(git): doctor judges notes only once the remote's notes are fetched `git clone` never fetches refs/notes/*, so on a fresh clone or a CI checkout `git doctor` reported every commit as "without a note" and exited 1, while the same report said `[x] origin has refs/notes/mx_metadata`. `--fix` then wrote computed notes over commits the remote already annotates, and the push it recommends was rejected as non-fast-forward. Anyone who forced that push would overwrite the real notes on the remote. The remote check already reads the remote's notes commit with ls-remote; it is now passed on. If the local ref is not, or does not contain, that commit, the commits are not judged. The finding is the unfetched ref, given with the command that fetches it. `--fix` fetches it as a fast-forward only, so a note that exists only locally is never dropped, and stops if that fetch is refused. Test: TestGitDoctor_UnfetchedNotesAreNotMissing (bare remote, pushed note, fresh clone). Before the fix it fails with the reported symptom: "[ ] 9450425668 without a note" next to "[x] origin has refs/notes/mx_metadata", and --fix replaced the remote's note on HEAD. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82tNcDLefAskVSzcKPm --- ...judges-notes-against-an-unfetched-ref.json | 1 + CHANGELOG.md | 2 +- cmd/mxcli/cmd_git_doctor.go | 11 +++-- cmd/mxcli/gitdoctor.go | 46 ++++++++++++++++++- cmd/mxcli/gitdoctor_test.go | 42 +++++++++++++++++ docs-site/src/tools/outside-studio-pro.md | 11 +++++ 6 files changed, 107 insertions(+), 6 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-git-doctor-judges-notes-against-an-unfetched-ref.json diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-git-doctor-judges-notes-against-an-unfetched-ref.json b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-git-doctor-judges-notes-against-an-unfetched-ref.json new file mode 100644 index 000000000..0e730ec6a --- /dev/null +++ b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-git-doctor-judges-notes-against-an-unfetched-ref.json @@ -0,0 +1 @@ +{"area": "cmd/mxcli", "date": "2026-10-28", "symptom": "`mxcli git doctor` on a fresh clone (or any CI checkout) reports every commit as `[ ] without a note` and exits 1, while the same report says `[x] origin has refs/notes/mx_metadata`; `--fix` then writes computed notes over commits the remote already annotates, and the `git push origin refs/notes/mx_metadata` it tells you to run is rejected as non-fast-forward", "cause": "doctorNotes judged each commit against the LOCAL refs/notes/mx_metadata only, and nothing fetched it: git clone and a plain git fetch never fetch refs/notes/*. The remote check had already run `git ls-remote refs/notes/mx_metadata` and printed the tip, but that result was not passed on, so an empty local ref read as 'no commit has a note'", "file": "`cmd/mxcli/gitdoctor.go` (`doctorRun` passes the remote's notes commit to `doctorNotes`; `hasNotesCommit`: the local ref is, or descends from, that commit); test `TestGitDoctor_UnfetchedNotesAreNotMissing` in `cmd/mxcli/gitdoctor_test.go`", "insight": "**Every existing notes test built its repository with `git init`, never `git clone`**, so the local notes ref was always the only one there was; the defect lives exactly in the gap between a repository that writes notes and one that receives them. A fixture with a bare remote, a pushed note and a fresh clone reproduced it on the first run. **Do not judge, and do not write, against a ref that is behind the remote**: Studio Pro's own fetch force-replaces the local notes ref with the remote's, so the remote's notes are the ones that count; a note written over a remote-annotated commit is either refused on push or thrown away on Studio Pro's next fetch. **Fetch as a fast-forward only** (`git fetch refs/notes/mx_metadata:refs/notes/mx_metadata`, no `+`): it never discards a note that exists only locally, which a force-fetch would; when the local ref has diverged the fetch is refused and the doctor says so instead of writing. Report mode stays read-only: it names the unfetched ref and the command rather than fetching", "refs": ["ako/mxcli#1670", "mendixlabs/mxcli#1669"]} diff --git a/CHANGELOG.md b/CHANGELOG.md index 12a35f81c..e97d561d1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added - **`mxcli git commit -m … -p app.mpr` — commit the way Studio Pro does** (mendixlabs/mxcli#1669) — Studio Pro's *Commit* is more than `git commit`: it keeps the ContentsHash index in step, stages exactly the files a Mendix repository ships, and attaches the mx_metadata note. The new command is that sequence in one line: mismatched hashes are repaired from the files on disk first (`MISSING` and `ORPHAN` entries have no automatic repair and refuse the commit), everything is staged and the files Studio Pro never commits (`project-settings.user.json`, `*.mpr.lock`, `*.mpr.bak`, `.mendix-cache/`, `deployment/`, `theme-cache/`, `.mxcli/`) are unstaged again, `git commit` runs with the caller's message and identity, and the mx_metadata note is computed from the new commit and attached, so Studio Pro can version-check the revision and show what changed. `--push` pushes the branch and the notes ref together (`git push` alone loses the note the next time Studio Pro fetches); `--dry-run` stages, reports and unstages without committing — and without repairing anything, hash drift included, because a dry run writes nothing. The commit is refused while Studio Pro has the project open (`.mpr.lock`) — honouring the same `MXCLI_ALLOW_STUDIO_PRO_OPEN=1` override as every write — and during an unresolved merge, which `git add -A` would otherwise commit with its conflict markers. Docs: "Working Outside Studio Pro" › *One commit, all the steps*. -- **`mxcli git doctor [-p app.mpr] [--fix]` — audit a repository for everything a plain-git history misses, and repair it** (mendixlabs/mxcli#1669) — a repository committed only with git drifts from what Studio Pro expects in four ways, each invisible on its own: the local git config Studio Pro writes is absent (`core.autocrlf=False` keeps a checkout from rewriting line endings inside `.mxunit` files on Windows; `mendix.commits-since-gc` and `mendix.lineEndingResetDone` are its own bookkeeping), the remote is SSH which Studio Pro cannot use (HTTPS only), commits have no note under `refs/notes/mx_metadata` (history shows `(unknown)`, the version check fails), and a malformed note Studio Pro cannot read at all. `doctor` reports all four in one read-only pass and exits 1 while an issue remains, so it can gate CI. `--fix` sets the config keys, rewrites SSH remote URLs (`git@host:org/app.git`, `ssh://…`) to HTTPS, and backfills notes: computed from the model with `-p` (the same engine `git note` uses), or a template note with a `--version` ModelerVersion where the model at a commit cannot be read (MPR v1 history, a revision without the `.mpr`); with neither, the newest valid note in history supplies the version. Notes Studio Pro wrote are never touched, with or without `--fix`; `--fix` does not push. Docs: "Working Outside Studio Pro" › *Auditing a repository*. +- **`mxcli git doctor [-p app.mpr] [--fix]` — audit a repository for everything a plain-git history misses, and repair it** (mendixlabs/mxcli#1669) — a repository committed only with git drifts from what Studio Pro expects in four ways, each invisible on its own: the local git config Studio Pro writes is absent (`core.autocrlf=False` keeps a checkout from rewriting line endings inside `.mxunit` files on Windows; `mendix.commits-since-gc` and `mendix.lineEndingResetDone` are its own bookkeeping), the remote is SSH which Studio Pro cannot use (HTTPS only), commits have no note under `refs/notes/mx_metadata` (history shows `(unknown)`, the version check fails), and a malformed note Studio Pro cannot read at all. `doctor` reports all four in one read-only pass and exits 1 while an issue remains, so it can gate CI. `--fix` sets the config keys, rewrites SSH remote URLs (`git@host:org/app.git`, `ssh://…`) to HTTPS, and backfills notes: computed from the model with `-p` (the same engine `git note` uses), or a template note with a `--version` ModelerVersion where the model at a commit cannot be read (MPR v1 history, a revision without the `.mpr`); with neither, the newest valid note in history supplies the version. Notes Studio Pro wrote are never touched, with or without `--fix`; `--fix` does not push. A clone whose notes ref lacks the remote's notes — `git clone` never fetches `refs/notes/*`, so every fresh clone and CI checkout — is reported as exactly that, with the fetch command, rather than as a history of unannotated commits; `--fix` fetches the notes first, as a fast-forward only. Docs: "Working Outside Studio Pro" › *Auditing a repository*. ### Fixed diff --git a/cmd/mxcli/cmd_git_doctor.go b/cmd/mxcli/cmd_git_doctor.go index 1e17d23f3..486c56323 100644 --- a/cmd/mxcli/cmd_git_doctor.go +++ b/cmd/mxcli/cmd_git_doctor.go @@ -29,16 +29,21 @@ Checked: (git@host:org/app.git or ssh://…) is rewritten to HTTPS by --fix. Other non-HTTPS URLs are reported without being changed. 3. The remote's refs/notes/mx_metadata. Absent or unreachable is a hint: - push the notes ref together with the branch. + push the notes ref together with the branch. When the remote has notes + the local ref does not hold (git clone never fetches refs/notes/*), the + commits are not judged: that is the finding, and --fix fetches the ref, + as a fast-forward only. 4. Every commit's mx_metadata note. Missing notes make Studio Pro show "(unknown)"; a malformed note it cannot read at all. --fix computes real notes from the model (with -p), and falls back to a template note with a --version ModelerVersion where the model at a commit cannot be read. Notes Studio Pro wrote are never touched. -Exits 1 while an issue remains, so the audit can gate CI. --fix does not push: -push the branch and the notes ref together yourself. +Exits 1 while an issue remains, so the audit can gate CI; fetch the notes +first there. --fix does not push: push the branch and the notes ref together +yourself. + git fetch refs/notes/mx_metadata:refs/notes/mx_metadata git push refs/notes/mx_metadata`, Example: ` mxcli git doctor mxcli git doctor -p app.mpr --fix diff --git a/cmd/mxcli/gitdoctor.go b/cmd/mxcli/gitdoctor.go index 48f045728..d01dfbaca 100644 --- a/cmd/mxcli/gitdoctor.go +++ b/cmd/mxcli/gitdoctor.go @@ -76,6 +76,7 @@ func doctorRun(root, mprPath, remoteOverride, version string, fix bool, out io.W fmt.Fprintln(out, "Remote") remote := detectDoctorRemote(root, remoteOverride) + remoteNotes := "" // the remote's notes commit, when it has one and could be reached switch { case remote == "": fmt.Fprintln(out, " [i] no remote configured; skipping remote checks") @@ -112,12 +113,15 @@ func doctorRun(root, mprPath, remoteOverride, version string, fix bool, out io.W fmt.Fprintf(out, " [i] could not reach %s to check %s\n", remote, mxMetadataRef) case strings.Contains(ls, mxMetadataRef): fmt.Fprintf(out, " [x] %s has %s\n", remote, mxMetadataRef) + if f := strings.Fields(ls); len(f) > 0 { + remoteNotes = f[0] + } default: fmt.Fprintf(out, " [i] %s has no %s yet; push it together with the branch\n", remote, mxMetadataRef) } } - fn, fx, err := doctorNotes(root, mprPath, version, fix, out) + fn, fx, err := doctorNotes(root, mprPath, version, remote, remoteNotes, fix, out) if err != nil { return err } @@ -143,12 +147,35 @@ func doctorRun(root, mprPath, remoteOverride, version string, fix bool, out io.W // doctorNotes reports (and with fix, repairs) missing and malformed notes over // the whole current history. It returns how many it found and how many it fixed. -func doctorNotes(root, mprPath, version string, fix bool, out io.Writer) (found, fixed int, err error) { +// +// remoteNotes is the remote's notes commit ("" when it has none or could not +// be reached). Those are the notes Studio Pro reads: its fetch force-replaces +// the local ref with them. A local ref that does not contain that commit — a +// fresh clone, since git clone never fetches refs/notes/* — cannot be judged: +// every commit the remote annotates would look unannotated, and a note written +// over it could not be pushed (non-fast-forward). So the commits are not +// judged until the ref is fetched; --fix fetches it, as a fast-forward only, +// which never drops a note that exists only locally. +func doctorNotes(root, mprPath, version, remote, remoteNotes string, fix bool, out io.Writer) (found, fixed int, err error) { fmt.Fprintln(out, "mx_metadata notes") if _, err := gitOut(root, "rev-parse", "--verify", "--quiet", "HEAD"); err != nil { fmt.Fprintln(out, " [i] no commits yet") return 0, 0, nil } + if remoteNotes != "" && !hasNotesCommit(root, remoteNotes) { + found++ + fmt.Fprintf(out, " [ ] %s from %s is not fetched; the notes cannot be judged without it\n", mxMetadataRef, remote) + if !fix { + fmt.Fprintf(out, " git fetch %s %s:%s\n", remote, mxMetadataRef, mxMetadataRef) + return found, 0, nil + } + if _, err := gitOut(root, "fetch", "--quiet", remote, mxMetadataRef+":"+mxMetadataRef); err != nil { + fmt.Fprintf(out, " could not fetch it as a fast-forward (%v); if the local ref has notes the remote lacks, push or reconcile them first\n", err) + return found, 0, nil + } + fixed++ + fmt.Fprintf(out, " fetched from %s\n", remote) + } revs := strings.Fields(gitOutOr(root, "rev-list", "--reverse", "HEAD")) noted := map[string]bool{} for _, line := range strings.Split(strings.TrimRight(gitOutOr(root, "notes", "--ref="+mxMetadataRef, "list"), "\n"), "\n") { @@ -316,6 +343,21 @@ func sshToHTTPS(url string) (host, p string, ok bool) { return "", "", false } +// hasNotesCommit reports whether the local notes ref is, or contains, the +// notes commit c. False when the ref is absent or c is not in its history +// (including when c's object was never fetched). +func hasNotesCommit(root, c string) bool { + local, err := gitOut(root, "rev-parse", "--verify", "--quiet", mxMetadataRef) + if err != nil { + return false + } + if strings.TrimSpace(local) == c { + return true + } + _, err = gitOut(root, "merge-base", "--is-ancestor", c, strings.TrimSpace(local)) + return err == nil +} + func gitNoteText(root, c string) string { out, err := gitOut(root, "notes", "--ref="+mxMetadataRef, "show", c) if err != nil { diff --git a/cmd/mxcli/gitdoctor_test.go b/cmd/mxcli/gitdoctor_test.go index fb2cf56c1..be8a948d4 100644 --- a/cmd/mxcli/gitdoctor_test.go +++ b/cmd/mxcli/gitdoctor_test.go @@ -218,6 +218,48 @@ func TestGitDoctor_FixKeepsRealNotes(t *testing.T) { } } +// A fresh clone has no refs/notes/* (git clone never fetches them), while the +// remote carries the notes Studio Pro wrote. Judged against the empty local +// ref, every commit looked unannotated, and --fix wrote notes over the +// remote's that it then refused to accept (non-fast-forward). +func TestGitDoctor_UnfetchedNotesAreNotMissing(t *testing.T) { + src, _ := doctorNoteRepo(t) + doctorEnv(t, src) + gitIn(t, src, "notes", "--ref="+mxMetadataRef, "add", "-f", "-m", realStudioProNote, "HEAD") + remote := t.TempDir() + gitIn(t, remote, "init", "-q", "--bare") + gitIn(t, src, "remote", "add", "origin", remote) + gitIn(t, src, "push", "-q", "origin", "feature", mxMetadataRef) + + clone := filepath.Join(t.TempDir(), "clone") + gitIn(t, filepath.Dir(clone), "clone", "-q", "-b", "feature", remote, clone) + for _, kv := range mxConfigKeys { + gitIn(t, clone, "config", "--local", kv[0], kv[1]) + } + mprPath := filepath.Join(clone, "App.mpr") + head := strings.TrimSpace(gitOutOr(clone, "rev-parse", "HEAD")) + + var out bytes.Buffer + err := doctorRun(clone, mprPath, "", "", false, &out) + if strings.Contains(out.String(), shortCommit(head)+" without a note") { + t.Errorf("HEAD has a note on the remote; an unfetched notes ref must not report it missing:\n%s", out.String()) + } + if !errors.Is(err, errDoctorIssuesRemain) || !strings.Contains(out.String(), "git fetch origin "+mxMetadataRef) { + t.Errorf("the unfetched notes ref must be the finding, with the command that fetches it: %v\n%s", err, out.String()) + } + + out.Reset() + if err := doctorRun(clone, mprPath, "", "", true, &out); err != nil { + t.Fatalf("--fix must fetch the notes and annotate the rest: %v\n%s", err, out.String()) + } + if got := strings.TrimSpace(noteOn(t, clone, "HEAD")); got != realStudioProNote { + t.Fatalf("--fix replaced the remote's note on HEAD:\n got %s\nwant %s\n%s", got, realStudioProNote, out.String()) + } + if _, err := gitOut(clone, "push", "origin", "feature", mxMetadataRef); err != nil { + t.Errorf("the notes --fix leaves must push as a fast-forward: %v\n%s", err, out.String()) + } +} + func TestGitDoctor_TemplateWithoutProjectUsesVersionFlag(t *testing.T) { dir := t.TempDir() doctorEnv(t, dir) diff --git a/docs-site/src/tools/outside-studio-pro.md b/docs-site/src/tools/outside-studio-pro.md index 9cf80d53a..6c38a75b9 100644 --- a/docs-site/src/tools/outside-studio-pro.md +++ b/docs-site/src/tools/outside-studio-pro.md @@ -227,11 +227,22 @@ mxcli git doctor --fix --version 10.21.0 | local git config: `core.autocrlf=False` (a checkout must not rewrite line endings inside `.mxunit` files), `mendix.commits-since-gc`, `mendix.lineEndingResetDone` | sets them | | the remote URL: Studio Pro can only use HTTPS | rewrites SSH (`git@host:org/app.git`, `ssh://…`) to HTTPS; other non-HTTPS URLs are reported unchanged | | the remote's `refs/notes/mx_metadata` | reported only — push the notes ref together with the branch | +| the local `refs/notes/mx_metadata` holds the remote's notes: `git clone` and a plain `git fetch` never fetch them, so a fresh clone or a CI checkout has none | fetches them, as a fast-forward only; refused when the local ref has notes the remote does not | | each commit on the checked-out branch and its mx_metadata note: missing notes make Studio Pro show `(unknown)`; a malformed note it cannot read at all | computes real notes from the model (`-p`), falling back to a template note with a `--version` ModelerVersion where the model at a commit cannot be read (MPR v1 history, a revision without the `.mpr`); when no version is known the newest valid note in history supplies it | Notes Studio Pro wrote are never touched, with or without `--fix`. `--fix` does not push. +The commits are judged only once the local notes ref holds the remote's. Until +then the report names the unfetched ref and the command that fetches it, rather +than calling every commit the remote annotates unannotated. In CI, fetch the +notes before the audit: + +```bash +git fetch origin refs/notes/mx_metadata:refs/notes/mx_metadata +mxcli git doctor -p app.mpr +``` + ## Commit metadata: `mxcli git note` Studio Pro attaches a git note under `refs/notes/mx_metadata` to every commit From 16eac218fc56856b84c12c28f4fa364790e2b459 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 21:57:11 +0000 Subject: [PATCH 16/18] fix(git): doctor judges the configured remote URL, not the insteadOf rewrite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `git doctor` read the remote with `git remote get-url`, which applies url..insteadOf. With a global https -> git@ rule (a common developer setup), an HTTPS remote read as SSH and the doctor exited 1. `--fix` then set the configured URL to HTTPS on every run, which the rule rewrote straight back, and dropped the URL's `.git` suffix along the way. ako/mxcli#1765 isolated the tests from this and left the product question open; this answers it. The check now reads `git config --get remote..url`, the value --fix actually writes. An SSH remote is still found and rewritten when a rule makes it read as HTTPS. When the effective URL differs, the report gives it as an [i] line naming the insteadOf rule, which --fix leaves alone. It is not counted as an issue: the rule applies to every git command on the machine, and changing the remote cannot undo it. Tests: TestGitDoctor_InsteadOfRewriteIsNotAnSSHRemote and TestGitDoctor_ConfiguredSSHRemoteFoundBehindRewrite. Both use a rule in the test repository's own config, on a host nobody rewrites. Before the fix: "origin: git@git.example.com:org/app.git — SSH; Studio Pro needs HTTPS", --fix changed the configured URL to https://git.example.com/org/app, and the configured SSH remote was missed. The two existing SSH tests also pass now in an environment that rewrites github.com URLs. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82tNcDLefAskVSzcKPm --- ...es-the-insteadof-rewritten-remote-url.json | 1 + CHANGELOG.md | 2 +- cmd/mxcli/cmd_git_doctor.go | 8 +-- cmd/mxcli/gitdoctor.go | 9 +++- cmd/mxcli/gitdoctor_test.go | 50 +++++++++++++++++++ docs-site/src/tools/outside-studio-pro.md | 2 +- 6 files changed, 66 insertions(+), 6 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-git-doctor-judges-the-insteadof-rewritten-remote-url.json diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-git-doctor-judges-the-insteadof-rewritten-remote-url.json b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-git-doctor-judges-the-insteadof-rewritten-remote-url.json new file mode 100644 index 000000000..608387c55 --- /dev/null +++ b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-git-doctor-judges-the-insteadof-rewritten-remote-url.json @@ -0,0 +1 @@ +{"area": "cmd/mxcli", "date": "2026-10-28", "symptom": "With a global `url.git@github.com:.insteadOf https://github.com/` rule (a common developer setup), `mxcli git doctor` reports an HTTPS remote as `origin: git@github.com:org/app.git — SSH; Studio Pro needs HTTPS`, exits 1, and `--fix` \"rewrites\" the configured URL to HTTPS on every run (dropping its `.git` suffix) without changing anything git uses", "cause": "The remote check read `git remote get-url`, which applies url..insteadOf, and --fix set `remote..url`, which the rule rewrites again on the next read, so the check could never pass. The same read made the two SSH tests depend on the developer's and the sandbox's git config (ako/mxcli#1765 isolated the tests; the product question stayed open)", "file": "`cmd/mxcli/gitdoctor.go` (`doctorRun`: judge `git config --get remote..url`; report a differing `git remote get-url` as an [i] insteadOf line); tests `TestGitDoctor_InsteadOfRewriteIsNotAnSSHRemote`, `TestGitDoctor_ConfiguredSSHRemoteFoundBehindRewrite` in `cmd/mxcli/gitdoctor_test.go`", "insight": "**A check and its fix must read and write the same thing.** `remote get-url` is the effective URL, `remote set-url` writes the configured one; with a rewrite in between, a fix can never make its own check pass, and a loop like that is the tell. Judge what you can change (the configured URL) and report what you cannot (the rule, which applies to every git command on the machine). **Put the rule in the test repository's own config and use a host nobody rewrites** (git.example.com): the test then shows the behaviour without inheriting the developer's or the CI sandbox's rewrites of github.com, which point in opposite directions. Set GIT_SSH_COMMAND=false when the effective URL is SSH, or ls-remote reaches for the network", "refs": ["ako/mxcli#1670", "ako/mxcli#1765"]} diff --git a/CHANGELOG.md b/CHANGELOG.md index e97d561d1..d13bae0fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added - **`mxcli git commit -m … -p app.mpr` — commit the way Studio Pro does** (mendixlabs/mxcli#1669) — Studio Pro's *Commit* is more than `git commit`: it keeps the ContentsHash index in step, stages exactly the files a Mendix repository ships, and attaches the mx_metadata note. The new command is that sequence in one line: mismatched hashes are repaired from the files on disk first (`MISSING` and `ORPHAN` entries have no automatic repair and refuse the commit), everything is staged and the files Studio Pro never commits (`project-settings.user.json`, `*.mpr.lock`, `*.mpr.bak`, `.mendix-cache/`, `deployment/`, `theme-cache/`, `.mxcli/`) are unstaged again, `git commit` runs with the caller's message and identity, and the mx_metadata note is computed from the new commit and attached, so Studio Pro can version-check the revision and show what changed. `--push` pushes the branch and the notes ref together (`git push` alone loses the note the next time Studio Pro fetches); `--dry-run` stages, reports and unstages without committing — and without repairing anything, hash drift included, because a dry run writes nothing. The commit is refused while Studio Pro has the project open (`.mpr.lock`) — honouring the same `MXCLI_ALLOW_STUDIO_PRO_OPEN=1` override as every write — and during an unresolved merge, which `git add -A` would otherwise commit with its conflict markers. Docs: "Working Outside Studio Pro" › *One commit, all the steps*. -- **`mxcli git doctor [-p app.mpr] [--fix]` — audit a repository for everything a plain-git history misses, and repair it** (mendixlabs/mxcli#1669) — a repository committed only with git drifts from what Studio Pro expects in four ways, each invisible on its own: the local git config Studio Pro writes is absent (`core.autocrlf=False` keeps a checkout from rewriting line endings inside `.mxunit` files on Windows; `mendix.commits-since-gc` and `mendix.lineEndingResetDone` are its own bookkeeping), the remote is SSH which Studio Pro cannot use (HTTPS only), commits have no note under `refs/notes/mx_metadata` (history shows `(unknown)`, the version check fails), and a malformed note Studio Pro cannot read at all. `doctor` reports all four in one read-only pass and exits 1 while an issue remains, so it can gate CI. `--fix` sets the config keys, rewrites SSH remote URLs (`git@host:org/app.git`, `ssh://…`) to HTTPS, and backfills notes: computed from the model with `-p` (the same engine `git note` uses), or a template note with a `--version` ModelerVersion where the model at a commit cannot be read (MPR v1 history, a revision without the `.mpr`); with neither, the newest valid note in history supplies the version. Notes Studio Pro wrote are never touched, with or without `--fix`; `--fix` does not push. A clone whose notes ref lacks the remote's notes — `git clone` never fetches `refs/notes/*`, so every fresh clone and CI checkout — is reported as exactly that, with the fetch command, rather than as a history of unannotated commits; `--fix` fetches the notes first, as a fast-forward only. Docs: "Working Outside Studio Pro" › *Auditing a repository*. +- **`mxcli git doctor [-p app.mpr] [--fix]` — audit a repository for everything a plain-git history misses, and repair it** (mendixlabs/mxcli#1669) — a repository committed only with git drifts from what Studio Pro expects in four ways, each invisible on its own: the local git config Studio Pro writes is absent (`core.autocrlf=False` keeps a checkout from rewriting line endings inside `.mxunit` files on Windows; `mendix.commits-since-gc` and `mendix.lineEndingResetDone` are its own bookkeeping), the remote is SSH which Studio Pro cannot use (HTTPS only), commits have no note under `refs/notes/mx_metadata` (history shows `(unknown)`, the version check fails), and a malformed note Studio Pro cannot read at all. `doctor` reports all four in one read-only pass and exits 1 while an issue remains, so it can gate CI. `--fix` sets the config keys, rewrites SSH remote URLs (`git@host:org/app.git`, `ssh://…`) to HTTPS — judged as configured, so a `url..insteadOf` rule in the user's git config (https → `git@` is a common one) is reported rather than "repaired" on every run — and backfills notes: computed from the model with `-p` (the same engine `git note` uses), or a template note with a `--version` ModelerVersion where the model at a commit cannot be read (MPR v1 history, a revision without the `.mpr`); with neither, the newest valid note in history supplies the version. Notes Studio Pro wrote are never touched, with or without `--fix`; `--fix` does not push. A clone whose notes ref lacks the remote's notes — `git clone` never fetches `refs/notes/*`, so every fresh clone and CI checkout — is reported as exactly that, with the fetch command, rather than as a history of unannotated commits; `--fix` fetches the notes first, as a fast-forward only. Docs: "Working Outside Studio Pro" › *Auditing a repository*. ### Fixed diff --git a/cmd/mxcli/cmd_git_doctor.go b/cmd/mxcli/cmd_git_doctor.go index 486c56323..06fb78f18 100644 --- a/cmd/mxcli/cmd_git_doctor.go +++ b/cmd/mxcli/cmd_git_doctor.go @@ -25,9 +25,11 @@ Checked: 1. Local git config: core.autocrlf=False (a checkout must not rewrite line endings inside .mxunit files), mendix.commits-since-gc and mendix.lineEndingResetDone (Studio Pro's own bookkeeping). --fix sets them. - 2. The remote URL. Studio Pro can only use HTTPS; an SSH remote - (git@host:org/app.git or ssh://…) is rewritten to HTTPS by --fix. Other - non-HTTPS URLs are reported without being changed. + 2. The remote URL, as configured. Studio Pro can only use HTTPS; an SSH + remote (git@host:org/app.git or ssh://…) is rewritten to HTTPS by --fix. + Other non-HTTPS URLs are reported without being changed. A + url..insteadOf rule in your git config that makes git use another + URL is reported too; --fix leaves it alone. 3. The remote's refs/notes/mx_metadata. Absent or unreachable is a hint: push the notes ref together with the branch. When the remote has notes the local ref does not hold (git clone never fetches refs/notes/*), the diff --git a/cmd/mxcli/gitdoctor.go b/cmd/mxcli/gitdoctor.go index d01dfbaca..0d106e9f6 100644 --- a/cmd/mxcli/gitdoctor.go +++ b/cmd/mxcli/gitdoctor.go @@ -81,7 +81,11 @@ func doctorRun(root, mprPath, remoteOverride, version string, fix bool, out io.W case remote == "": fmt.Fprintln(out, " [i] no remote configured; skipping remote checks") default: - urlRaw, urlErr := gitOut(root, "remote", "get-url", remote) + // The configured URL, not `git remote get-url`: that one applies + // url..insteadOf, so a rewrite in the user's git config (https -> + // git@ is a common one) made an HTTPS remote read as SSH, and --fix set + // the configured URL on every run without changing what git uses. + urlRaw, urlErr := gitOut(root, "config", "--get", "remote."+remote+".url") if urlErr != nil { found++ fmt.Fprintf(out, " [ ] remote %s does not exist (git remote -v lists the names)\n", remote) @@ -106,6 +110,9 @@ func doctorRun(root, mprPath, remoteOverride, version string, fix bool, out io.W } else { fmt.Fprintf(out, " [i] %s: %s (not HTTPS; Studio Pro can only use HTTPS remotes)\n", remote, url) } + if eff, err := gitOut(root, "remote", "get-url", remote); err == nil && strings.TrimSpace(eff) != url { + fmt.Fprintf(out, " [i] a url..insteadOf rule in your git config makes git use %s instead; --fix leaves the rule alone\n", strings.TrimSpace(eff)) + } // Absent or unreachable notes ref is a hint, not an issue. ls, err := gitOut(root, "ls-remote", remote, mxMetadataRef) switch { diff --git a/cmd/mxcli/gitdoctor_test.go b/cmd/mxcli/gitdoctor_test.go index be8a948d4..1a3b49e3e 100644 --- a/cmd/mxcli/gitdoctor_test.go +++ b/cmd/mxcli/gitdoctor_test.go @@ -143,6 +143,56 @@ func TestGitDoctor_SSHURLFormRewrittenByFix(t *testing.T) { } } +// A url..insteadOf rule rewrites the URL every git command uses, not the +// URL the repository is configured with. The doctor judged the rewritten one: +// with the common https -> git@ rule in ~/.gitconfig an HTTPS remote read as +// SSH, and --fix "repaired" the configured URL on every run. +func TestGitDoctor_InsteadOfRewriteIsNotAnSSHRemote(t *testing.T) { + dir := t.TempDir() + doctorEnv(t, dir) + t.Setenv("GIT_SSH_COMMAND", "false") // the rewritten URL is SSH: never reach the network + gitIn(t, dir, "init", "-q", "-b", "main") + for _, kv := range mxConfigKeys { + gitIn(t, dir, "config", "--local", kv[0], kv[1]) + } + gitIn(t, dir, "config", "--local", "url.git@git.example.com:.insteadOf", "https://git.example.com/") + gitIn(t, dir, "remote", "add", "origin", "https://git.example.com/org/app.git") + + var out bytes.Buffer + if err := doctorRun(dir, "", "", "", false, &out); err != nil { + t.Errorf("a configured HTTPS remote is not an issue, whatever a rewrite makes of it: %v\n%s", err, out.String()) + } + if !strings.Contains(out.String(), "insteadOf") { + t.Errorf("the report must explain the rewrite:\n%s", out.String()) + } + + out.Reset() + if err := doctorRun(dir, "", "", "", true, &out); err != nil { + t.Errorf("--fix: %v\n%s", err, out.String()) + } + if got := configLocal(t, dir, "remote.origin.url"); got != "https://git.example.com/org/app.git" { + t.Errorf("--fix changed the configured URL to %q:\n%s", got, out.String()) + } +} + +// The configured URL is what --fix rewrites, so an SSH remote is still found +// when a rule rewrites it to HTTPS (as hosted sandboxes do). +func TestGitDoctor_ConfiguredSSHRemoteFoundBehindRewrite(t *testing.T) { + dir := t.TempDir() + doctorEnv(t, dir) + gitIn(t, dir, "init", "-q", "-b", "main") + gitIn(t, dir, "config", "--local", "url.https://git.example.com/.insteadOf", "git@git.example.com:") + gitIn(t, dir, "remote", "add", "origin", "git@git.example.com:org/app.git") + + var out bytes.Buffer + if err := doctorRun(dir, "", "", "", true, &out); err != nil { + t.Fatalf("--fix: %v\n%s", err, out.String()) + } + if got := configLocal(t, dir, "remote.origin.url"); got != "https://git.example.com/org/app" { + t.Errorf("configured URL = %q, want https://git.example.com/org/app\n%s", got, out.String()) + } +} + // --- doctor: mx_metadata notes ---------------------------------------------- // doctorNoteRepo makes a two-commit repository holding an MPR v2 project: diff --git a/docs-site/src/tools/outside-studio-pro.md b/docs-site/src/tools/outside-studio-pro.md index 6c38a75b9..e48b61ff9 100644 --- a/docs-site/src/tools/outside-studio-pro.md +++ b/docs-site/src/tools/outside-studio-pro.md @@ -225,7 +225,7 @@ mxcli git doctor --fix --version 10.21.0 | Check | `--fix` | |-------|---------| | local git config: `core.autocrlf=False` (a checkout must not rewrite line endings inside `.mxunit` files), `mendix.commits-since-gc`, `mendix.lineEndingResetDone` | sets them | -| the remote URL: Studio Pro can only use HTTPS | rewrites SSH (`git@host:org/app.git`, `ssh://…`) to HTTPS; other non-HTTPS URLs are reported unchanged | +| the remote URL as configured: Studio Pro can only use HTTPS | rewrites SSH (`git@host:org/app.git`, `ssh://…`) to HTTPS; other non-HTTPS URLs are reported unchanged. A `url..insteadOf` rule in your git config that makes git use another URL is reported, and left alone | | the remote's `refs/notes/mx_metadata` | reported only — push the notes ref together with the branch | | the local `refs/notes/mx_metadata` holds the remote's notes: `git clone` and a plain `git fetch` never fetch them, so a fresh clone or a CI checkout has none | fetches them, as a fast-forward only; refused when the local ref has notes the remote does not | | each commit on the checked-out branch and its mx_metadata note: missing notes make Studio Pro show `(unknown)`; a malformed note it cannot read at all | computes real notes from the model (`-p`), falling back to a template note with a `--version` ModelerVersion where the model at a commit cannot be read (MPR v1 history, a revision without the `.mpr`); when no version is known the newest valid note in history supplies it | From 0081f56929058027a42a970c45c0ed9328d890ab Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 21:59:07 +0000 Subject: [PATCH 17/18] fix(git): commit --dry-run leaves what the user staged as it was `mxcli git commit --dry-run` staged everything in the real index to report it, then undid that with `git reset -q`. A reset goes back to HEAD, not to the state before the run, so anything the user had staged was unstaged. The docs promise that a dry run writes nothing. A dry run now stages into a temporary copy of the index (GIT_INDEX_FILE), so the real index is never touched: staged changes, stat data and intent-to-add entries all stay as they were. A real commit still uses the real index, as before. `gitOutInEnv` is the git helper with extra environment. Test: TestGitCommit_DryRunKeepsTheUsersStaging. Before the fix: `staged after --dry-run = "", want exactly what the user had staged (staged.txt)`. The existing dry-run test started from an empty index, where the bug and the correct behaviour look the same. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82tNcDLefAskVSzcKPm --- ...it-dry-run-unstages-the-users-staging.json | 1 + CHANGELOG.md | 2 +- cmd/mxcli/cmd_git_commit.go | 70 ++++++++++++++++--- cmd/mxcli/gitdoctor_test.go | 28 ++++++++ cmd/mxcli/gitnote.go | 8 ++- docs-site/src/tools/outside-studio-pro.md | 3 +- 6 files changed, 99 insertions(+), 13 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-git-commit-dry-run-unstages-the-users-staging.json diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-git-commit-dry-run-unstages-the-users-staging.json b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-git-commit-dry-run-unstages-the-users-staging.json new file mode 100644 index 000000000..80af6632b --- /dev/null +++ b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-git-commit-dry-run-unstages-the-users-staging.json @@ -0,0 +1 @@ +{"area": "cmd/mxcli", "date": "2026-10-28", "symptom": "`mxcli git commit -m … -p app.mpr --dry-run` unstages everything the user had staged: a file added with `git add` before the dry run is no longer staged afterwards (`git diff --cached` is empty), although the command committed nothing and documents that a dry run writes nothing", "cause": "The dry run staged everything in the real index (`git add -A`) to report it, then undid that with a plain `git reset -q`, which resets the index to HEAD rather than to what it was before the run", "file": "`cmd/mxcli/cmd_git_commit.go` (`commitRun`: a dry run stages into a temporary copy of the index through GIT_INDEX_FILE, `indexCopy`); `cmd/mxcli/gitnote.go` (`gitOutInEnv`); test `TestGitCommit_DryRunKeepsTheUsersStaging` in `cmd/mxcli/gitdoctor_test.go`", "insight": "**The existing dry-run test started from an empty index**, so 'nothing staged afterwards' was both the correct result and the bug's result; a test of an undo has to start from a state the undo can get wrong. **Do not undo, isolate**: staging into a GIT_INDEX_FILE copy leaves the real index untouched byte for byte (stat data and intent-to-add entries included), which no reset or write-tree/read-tree round trip quite does. An empty file is not a valid index, so with no index yet the temporary path is left free for git to create", "refs": ["ako/mxcli#1670", "mendixlabs/mxcli#1669"]} diff --git a/CHANGELOG.md b/CHANGELOG.md index d13bae0fb..22b0ac578 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added -- **`mxcli git commit -m … -p app.mpr` — commit the way Studio Pro does** (mendixlabs/mxcli#1669) — Studio Pro's *Commit* is more than `git commit`: it keeps the ContentsHash index in step, stages exactly the files a Mendix repository ships, and attaches the mx_metadata note. The new command is that sequence in one line: mismatched hashes are repaired from the files on disk first (`MISSING` and `ORPHAN` entries have no automatic repair and refuse the commit), everything is staged and the files Studio Pro never commits (`project-settings.user.json`, `*.mpr.lock`, `*.mpr.bak`, `.mendix-cache/`, `deployment/`, `theme-cache/`, `.mxcli/`) are unstaged again, `git commit` runs with the caller's message and identity, and the mx_metadata note is computed from the new commit and attached, so Studio Pro can version-check the revision and show what changed. `--push` pushes the branch and the notes ref together (`git push` alone loses the note the next time Studio Pro fetches); `--dry-run` stages, reports and unstages without committing — and without repairing anything, hash drift included, because a dry run writes nothing. The commit is refused while Studio Pro has the project open (`.mpr.lock`) — honouring the same `MXCLI_ALLOW_STUDIO_PRO_OPEN=1` override as every write — and during an unresolved merge, which `git add -A` would otherwise commit with its conflict markers. Docs: "Working Outside Studio Pro" › *One commit, all the steps*. +- **`mxcli git commit -m … -p app.mpr` — commit the way Studio Pro does** (mendixlabs/mxcli#1669) — Studio Pro's *Commit* is more than `git commit`: it keeps the ContentsHash index in step, stages exactly the files a Mendix repository ships, and attaches the mx_metadata note. The new command is that sequence in one line: mismatched hashes are repaired from the files on disk first (`MISSING` and `ORPHAN` entries have no automatic repair and refuse the commit), everything is staged and the files Studio Pro never commits (`project-settings.user.json`, `*.mpr.lock`, `*.mpr.bak`, `.mendix-cache/`, `deployment/`, `theme-cache/`, `.mxcli/`) are unstaged again, `git commit` runs with the caller's message and identity, and the mx_metadata note is computed from the new commit and attached, so Studio Pro can version-check the revision and show what changed. `--push` pushes the branch and the notes ref together (`git push` alone loses the note the next time Studio Pro fetches); `--dry-run` reports what it would commit without committing — and without repairing anything, hash drift included, because a dry run writes nothing: it stages into a copy of the index, so what the user had staged is left as it was. The commit is refused while Studio Pro has the project open (`.mpr.lock`) — honouring the same `MXCLI_ALLOW_STUDIO_PRO_OPEN=1` override as every write — and during an unresolved merge, which `git add -A` would otherwise commit with its conflict markers. Docs: "Working Outside Studio Pro" › *One commit, all the steps*. - **`mxcli git doctor [-p app.mpr] [--fix]` — audit a repository for everything a plain-git history misses, and repair it** (mendixlabs/mxcli#1669) — a repository committed only with git drifts from what Studio Pro expects in four ways, each invisible on its own: the local git config Studio Pro writes is absent (`core.autocrlf=False` keeps a checkout from rewriting line endings inside `.mxunit` files on Windows; `mendix.commits-since-gc` and `mendix.lineEndingResetDone` are its own bookkeeping), the remote is SSH which Studio Pro cannot use (HTTPS only), commits have no note under `refs/notes/mx_metadata` (history shows `(unknown)`, the version check fails), and a malformed note Studio Pro cannot read at all. `doctor` reports all four in one read-only pass and exits 1 while an issue remains, so it can gate CI. `--fix` sets the config keys, rewrites SSH remote URLs (`git@host:org/app.git`, `ssh://…`) to HTTPS — judged as configured, so a `url..insteadOf` rule in the user's git config (https → `git@` is a common one) is reported rather than "repaired" on every run — and backfills notes: computed from the model with `-p` (the same engine `git note` uses), or a template note with a `--version` ModelerVersion where the model at a commit cannot be read (MPR v1 history, a revision without the `.mpr`); with neither, the newest valid note in history supplies the version. Notes Studio Pro wrote are never touched, with or without `--fix`; `--fix` does not push. A clone whose notes ref lacks the remote's notes — `git clone` never fetches `refs/notes/*`, so every fresh clone and CI checkout — is reported as exactly that, with the fetch command, rather than as a history of unannotated commits; `--fix` fetches the notes first, as a fast-forward only. Docs: "Working Outside Studio Pro" › *Auditing a repository*. ### Fixed diff --git a/cmd/mxcli/cmd_git_commit.go b/cmd/mxcli/cmd_git_commit.go index cbdb52cbe..3db953617 100644 --- a/cmd/mxcli/cmd_git_commit.go +++ b/cmd/mxcli/cmd_git_commit.go @@ -60,7 +60,7 @@ func init() { gitCommitCmd.Flags().StringP("project", "p", "", "path to the Mendix project (.mpr)") _ = gitCommitCmd.MarkFlagRequired("project") gitCommitCmd.Flags().Bool("push", false, "push the branch and the notes ref to their remote afterwards") - gitCommitCmd.Flags().Bool("dry-run", false, "stage and report, then unstage; commit nothing") + gitCommitCmd.Flags().Bool("dry-run", false, "report what would be committed; commit nothing and leave your staging as it is") gitCmd.AddCommand(gitCommitCmd) } @@ -115,12 +115,25 @@ func commitRun(mprPath, message string, push, dryRun bool, out io.Writer) error } } - // 2. Stage everything, then unstage what Studio Pro never commits. - if _, err := gitOutIn(root, nil, "add", "-A"); err != nil { + // 2. Stage everything, then unstage what Studio Pro never commits. A dry + // run stages into a copy of the index: resetting the real one afterwards + // unstaged whatever the user had staged before running it. + var env []string + if dryRun { + idx, cleanup, err := indexCopy(root) + if err != nil { + return fmt.Errorf("copy the index for the dry run: %w", err) + } + defer cleanup() + env = []string{"GIT_INDEX_FILE=" + idx} + } + git := func(args ...string) (string, error) { return gitOutInEnv(root, env, nil, args...) } + if _, err := git("add", "-A"); err != nil { return fmt.Errorf("stage changes: %w", err) } // -z: paths come back NUL-separated, so spaces and quotes stay inside one path. - staged := nulFields(gitOutOr(root, "diff", "--cached", "--name-only", "-z")) + cached, _ := git("diff", "--cached", "--name-only", "-z") + staged := nulFields(cached) var junk []string for _, f := range staged { if isJunkPath(f) { @@ -128,8 +141,7 @@ func commitRun(mprPath, message string, push, dryRun bool, out io.Writer) error } } if len(junk) > 0 { - args := append([]string{"reset", "-q", "--"}, junk...) - if _, err := gitOutIn(root, nil, args...); err != nil { + if _, err := git(append([]string{"reset", "-q", "--"}, junk...)...); err != nil { return fmt.Errorf("unstage files Studio Pro never commits: %w", err) } fmt.Fprintf(out, "Left out %d file(s) Studio Pro never commits (project-settings.user.json, *.mpr.lock, caches).\n", len(junk)) @@ -151,10 +163,7 @@ func commitRun(mprPath, message string, push, dryRun bool, out io.Writer) error } if dryRun { - if _, err := gitOutIn(root, nil, "reset", "-q"); err != nil { - return fmt.Errorf("unstage: %w", err) - } - fmt.Fprintln(out, "Dry run: staged nothing permanently, committed nothing.") + fmt.Fprintln(out, "Dry run: committed nothing; what you had staged is unchanged.") return nil } @@ -185,6 +194,47 @@ func commitRun(mprPath, message string, push, dryRun bool, out io.Writer) error return nil } +// indexCopy copies the repository's index to a temporary file, for a +// GIT_INDEX_FILE that a dry run can stage into without touching the real one. +// With no index yet, the path is left free for git to create it. +func indexCopy(root string) (string, func(), error) { + p, err := gitOut(root, "rev-parse", "--git-path", "index") + if err != nil { + return "", nil, err + } + src := strings.TrimSpace(p) + if !filepath.IsAbs(src) { + src = filepath.Join(root, src) + } + f, err := os.CreateTemp("", "mxcli-dry-run-index-*") + if err != nil { + return "", nil, err + } + tmp := f.Name() + cleanup := func() { _ = os.Remove(tmp) } + data, err := os.ReadFile(src) + switch { + case errors.Is(err, os.ErrNotExist): + f.Close() + cleanup() // an empty file is not a valid index + return tmp, cleanup, nil + case err != nil: + f.Close() + cleanup() + return "", nil, err + } + if _, err := f.Write(data); err != nil { + f.Close() + cleanup() + return "", nil, err + } + if err := f.Close(); err != nil { + cleanup() + return "", nil, err + } + return tmp, cleanup, nil +} + // nulFields splits git's -z output (NUL-separated, no quoting) into paths. func nulFields(s string) []string { var out []string diff --git a/cmd/mxcli/gitdoctor_test.go b/cmd/mxcli/gitdoctor_test.go index 1a3b49e3e..edb2b38ec 100644 --- a/cmd/mxcli/gitdoctor_test.go +++ b/cmd/mxcli/gitdoctor_test.go @@ -458,6 +458,34 @@ func TestGitCommit_RefusedWhileStudioProOpen(t *testing.T) { } } +// --dry-run staged everything to report it and then reset the whole index, so +// whatever the user had staged before running it was unstaged. +func TestGitCommit_DryRunKeepsTheUsersStaging(t *testing.T) { + dir, mprPath := doctorNoteRepo(t) + doctorEnv(t, dir) + for _, name := range []string{"staged.txt", "unstaged.txt"} { + if err := os.WriteFile(filepath.Join(dir, name), []byte(name), 0o644); err != nil { + t.Fatal(err) + } + } + gitIn(t, dir, "add", "staged.txt") + + var out bytes.Buffer + if err := commitRun(mprPath, "dry", false, true, &out); err != nil { + t.Fatalf("dry run failed: %v\n%s", err, out.String()) + } + if !strings.Contains(out.String(), "unstaged.txt") { + t.Errorf("the dry run must still report every file it would commit:\n%s", out.String()) + } + staged, err := gitOut(dir, "diff", "--cached", "--name-only") + if err != nil { + t.Fatal(err) + } + if got := strings.TrimSpace(staged); got != "staged.txt" { + t.Errorf("staged after --dry-run = %q, want exactly what the user had staged (staged.txt)", got) + } +} + func TestGitCommit_DryRunCommitsNothing(t *testing.T) { dir, mprPath := doctorNoteRepo(t) doctorEnv(t, dir) diff --git a/cmd/mxcli/gitnote.go b/cmd/mxcli/gitnote.go index 24a5934d6..b9e55e1bd 100644 --- a/cmd/mxcli/gitnote.go +++ b/cmd/mxcli/gitnote.go @@ -366,8 +366,14 @@ func gitOut(dir string, args ...string) (string, error) { } func gitOutIn(dir string, stdin []byte, args ...string) (string, error) { + return gitOutInEnv(dir, nil, stdin, args...) +} + +// gitOutInEnv is gitOutIn with extra environment entries, such as the +// GIT_INDEX_FILE a dry run stages into. +func gitOutInEnv(dir string, env []string, stdin []byte, args ...string) (string, error) { cmd := exec.Command("git", append([]string{"-C", dir}, args...)...) - cmd.Env = append(os.Environ(), "GIT_TERMINAL_PROMPT=0", "GIT_PAGER=cat") + cmd.Env = append(append(os.Environ(), "GIT_TERMINAL_PROMPT=0", "GIT_PAGER=cat"), env...) if stdin != nil { cmd.Stdin = bytes.NewReader(stdin) } diff --git a/docs-site/src/tools/outside-studio-pro.md b/docs-site/src/tools/outside-studio-pro.md index e48b61ff9..2da3867e1 100644 --- a/docs-site/src/tools/outside-studio-pro.md +++ b/docs-site/src/tools/outside-studio-pro.md @@ -208,7 +208,8 @@ The commit is refused while Studio Pro has the project open (the `.mpr.lock` beside it), and during an unresolved merge — `git add -A` would otherwise commit the conflict markers. `--dry-run` verifies and reports (including ContentsHash drift, which it does not repair — a dry run writes nothing), -stages, reports what it would commit, and unstages again without committing. +and reports what it would commit without committing. It stages into a copy of +the index, so whatever you had staged is left exactly as it was. ## Auditing a repository: `mxcli git doctor` From e438d6cbdd6fcc7b3f262230107c25c176c6e5a6 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 22:01:11 +0000 Subject: [PATCH 18/18] fix(git): commit leaves out only the project's own build folders `mxcli git commit` dropped any path with a segment named `.mendix-cache`, `deployment`, `theme-cache` or `.mxcli`, at any depth. So `resources/deployment/endpoints.xml` was silently left out of the commit. The only trace was a count in "Left out 1 file(s) Studio Pro never commits". Those folders are the project's own: Studio Pro, mxbuild and mxcli create them directly beside the .mpr. isJunkPath now matches them there only. When the project sits in a subfolder of the repository, that subfolder is the anchor, taken from gitNoteRepo.MPRPath. The lock, backup and user-settings file names still match at any level. Tests: TestGitCommit_NestedFolderNamedLikeJunkIsCommitted and TestGitCommit_JunkAnchoredAtProjectInSubfolder. Before the fix both fail: "resources/deployment/endpoints.xml is not the project's own folder and must be committed", and the same for javascriptsource/theme-cache/, app/resources/deployment/ and tools/deployment/. The project's own deployment/ and theme-cache/ stay out, as the existing spaced-path test also checks. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82tNcDLefAskVSzcKPm --- ...sted-folders-named-like-build-folders.json | 1 + CHANGELOG.md | 2 +- cmd/mxcli/cmd_git_commit.go | 37 ++++++--- cmd/mxcli/gitdoctor_test.go | 79 +++++++++++++++++++ docs-site/src/tools/outside-studio-pro.md | 6 +- 5 files changed, 112 insertions(+), 13 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-git-commit-drops-nested-folders-named-like-build-folders.json diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-git-commit-drops-nested-folders-named-like-build-folders.json b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-git-commit-drops-nested-folders-named-like-build-folders.json new file mode 100644 index 000000000..b6fbea8c4 --- /dev/null +++ b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-28-git-commit-drops-nested-folders-named-like-build-folders.json @@ -0,0 +1 @@ +{"area": "cmd/mxcli", "date": "2026-10-28", "symptom": "`mxcli git commit` silently leaves a user file out of the commit when any folder on its path is named like a build folder: `resources/deployment/endpoints.xml` (or `javascriptsource/theme-cache/x.js`) stays untracked after the commit, with only `Left out 1 file(s) Studio Pro never commits (project-settings.user.json, *.mpr.lock, caches)` to show for it", "cause": "isJunkPath matched `.mendix-cache`, `deployment`, `theme-cache` and `.mxcli` on every path segment, so any folder of that name at any depth was dropped. Those are the project's own folders, the ones Studio Pro, mxbuild and mxcli create directly beside the .mpr; the file-name rules (lock, backup, user settings) are the ones that rightly match at any level", "file": "`cmd/mxcli/cmd_git_commit.go` (`isJunkPath(rel, projDir)`, projDir from `path.Dir(gitNoteRepo.MPRPath)`, which already resolves the project folder inside the repository, symlinks included); tests `TestGitCommit_NestedFolderNamedLikeJunkIsCommitted`, `TestGitCommit_JunkAnchoredAtProjectInSubfolder` in `cmd/mxcli/gitdoctor_test.go`", "insight": "**Anchor a path rule where the thing it names is created**: the build and cache folders only ever appear beside the .mpr, and a rule that matches the name at every depth also catches user folders that share it. **The report that something was left out did not name it**, which is what made the drop silent; a nested-folder fixture next to the root one (the existing test only had root `deployment/`) is the control that tells the two apart. Test the project-in-a-subfolder layout too: the anchor is the project's folder, not the repository root, and the same names outside the project are not its own", "refs": ["ako/mxcli#1670", "mendixlabs/mxcli#1669"]} diff --git a/CHANGELOG.md b/CHANGELOG.md index 22b0ac578..3ff88359a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added -- **`mxcli git commit -m … -p app.mpr` — commit the way Studio Pro does** (mendixlabs/mxcli#1669) — Studio Pro's *Commit* is more than `git commit`: it keeps the ContentsHash index in step, stages exactly the files a Mendix repository ships, and attaches the mx_metadata note. The new command is that sequence in one line: mismatched hashes are repaired from the files on disk first (`MISSING` and `ORPHAN` entries have no automatic repair and refuse the commit), everything is staged and the files Studio Pro never commits (`project-settings.user.json`, `*.mpr.lock`, `*.mpr.bak`, `.mendix-cache/`, `deployment/`, `theme-cache/`, `.mxcli/`) are unstaged again, `git commit` runs with the caller's message and identity, and the mx_metadata note is computed from the new commit and attached, so Studio Pro can version-check the revision and show what changed. `--push` pushes the branch and the notes ref together (`git push` alone loses the note the next time Studio Pro fetches); `--dry-run` reports what it would commit without committing — and without repairing anything, hash drift included, because a dry run writes nothing: it stages into a copy of the index, so what the user had staged is left as it was. The commit is refused while Studio Pro has the project open (`.mpr.lock`) — honouring the same `MXCLI_ALLOW_STUDIO_PRO_OPEN=1` override as every write — and during an unresolved merge, which `git add -A` would otherwise commit with its conflict markers. Docs: "Working Outside Studio Pro" › *One commit, all the steps*. +- **`mxcli git commit -m … -p app.mpr` — commit the way Studio Pro does** (mendixlabs/mxcli#1669) — Studio Pro's *Commit* is more than `git commit`: it keeps the ContentsHash index in step, stages exactly the files a Mendix repository ships, and attaches the mx_metadata note. The new command is that sequence in one line: mismatched hashes are repaired from the files on disk first (`MISSING` and `ORPHAN` entries have no automatic repair and refuse the commit), everything is staged and the files Studio Pro never commits (`project-settings.user.json`, `*.mpr.lock`, `*.mpr.bak`, and the project's own `.mendix-cache/`, `deployment/`, `theme-cache/`, `.mxcli/` beside the `.mpr` — a folder of the same name further down, such as `resources/deployment/`, is committed) are unstaged again, `git commit` runs with the caller's message and identity, and the mx_metadata note is computed from the new commit and attached, so Studio Pro can version-check the revision and show what changed. `--push` pushes the branch and the notes ref together (`git push` alone loses the note the next time Studio Pro fetches); `--dry-run` reports what it would commit without committing — and without repairing anything, hash drift included, because a dry run writes nothing: it stages into a copy of the index, so what the user had staged is left as it was. The commit is refused while Studio Pro has the project open (`.mpr.lock`) — honouring the same `MXCLI_ALLOW_STUDIO_PRO_OPEN=1` override as every write — and during an unresolved merge, which `git add -A` would otherwise commit with its conflict markers. Docs: "Working Outside Studio Pro" › *One commit, all the steps*. - **`mxcli git doctor [-p app.mpr] [--fix]` — audit a repository for everything a plain-git history misses, and repair it** (mendixlabs/mxcli#1669) — a repository committed only with git drifts from what Studio Pro expects in four ways, each invisible on its own: the local git config Studio Pro writes is absent (`core.autocrlf=False` keeps a checkout from rewriting line endings inside `.mxunit` files on Windows; `mendix.commits-since-gc` and `mendix.lineEndingResetDone` are its own bookkeeping), the remote is SSH which Studio Pro cannot use (HTTPS only), commits have no note under `refs/notes/mx_metadata` (history shows `(unknown)`, the version check fails), and a malformed note Studio Pro cannot read at all. `doctor` reports all four in one read-only pass and exits 1 while an issue remains, so it can gate CI. `--fix` sets the config keys, rewrites SSH remote URLs (`git@host:org/app.git`, `ssh://…`) to HTTPS — judged as configured, so a `url..insteadOf` rule in the user's git config (https → `git@` is a common one) is reported rather than "repaired" on every run — and backfills notes: computed from the model with `-p` (the same engine `git note` uses), or a template note with a `--version` ModelerVersion where the model at a commit cannot be read (MPR v1 history, a revision without the `.mpr`); with neither, the newest valid note in history supplies the version. Notes Studio Pro wrote are never touched, with or without `--fix`; `--fix` does not push. A clone whose notes ref lacks the remote's notes — `git clone` never fetches `refs/notes/*`, so every fresh clone and CI checkout — is reported as exactly that, with the fetch command, rather than as a history of unannotated commits; `--fix` fetches the notes first, as a fast-forward only. Docs: "Working Outside Studio Pro" › *Auditing a repository*. ### Fixed diff --git a/cmd/mxcli/cmd_git_commit.go b/cmd/mxcli/cmd_git_commit.go index 3db953617..3942c276c 100644 --- a/cmd/mxcli/cmd_git_commit.go +++ b/cmd/mxcli/cmd_git_commit.go @@ -32,8 +32,9 @@ var gitCommitCmd = &cobra.Command{ rewritten from the file, which is the source of truth; MISSING or ORPHAN entries have no automatic repair and refuse the commit; 2. everything is staged, then the files Studio Pro never commits are - unstaged again: project-settings.user.json, *.mpr.lock, *.mpr.bak, - .mendix-cache/, deployment/, theme-cache/, .mxcli/; + unstaged again: project-settings.user.json, *.mpr.lock, *.mpr.bak, and + the project's own .mendix-cache/, deployment/, theme-cache/ and .mxcli/ + beside the .mpr (a folder of the same name further down is committed); 3. git commit runs with your message and your git identity; 4. the mx_metadata note is computed from the new commit and attached, so Studio Pro can version-check it and show what changed. @@ -134,9 +135,10 @@ func commitRun(mprPath, message string, push, dryRun bool, out io.Writer) error // -z: paths come back NUL-separated, so spaces and quotes stay inside one path. cached, _ := git("diff", "--cached", "--name-only", "-z") staged := nulFields(cached) + projDir := path.Dir(repo.MPRPath) // the project's folder in the repository; "." at its root var junk []string for _, f := range staged { - if isJunkPath(f) { + if isJunkPath(f, projDir) { junk = append(junk, f) } } @@ -247,9 +249,14 @@ func nulFields(s string) []string { } // isJunkPath reports whether a repo-relative path is one of the files Studio -// Pro never commits. Matched on any path level and without case, the way -// Studio Pro matches its own lock file (whose .gitignore entry is lowercase). -func isJunkPath(rel string) bool { +// Pro never commits. The files match by name at any level and without case, +// the way Studio Pro matches its own lock file (whose .gitignore entry is +// lowercase). The folders are the project's own, the ones Studio Pro, mxbuild +// and mxcli create directly in projDir (the project's folder relative to the +// repository root, "." at the root): matched at any depth, a user folder of +// the same name, resources/deployment/ say, was silently left out of the +// commit. +func isJunkPath(rel, projDir string) bool { f := filepath.ToSlash(rel) base := path.Base(f) switch { @@ -258,11 +265,21 @@ func isJunkPath(rel string) bool { case strings.HasSuffix(strings.ToLower(base), ".mpr.lock"), strings.HasSuffix(strings.ToLower(base), ".mpr.bak"): return true } - for _, seg := range strings.Split(f, "/") { - switch strings.ToLower(seg) { - case ".mendix-cache", "deployment", "theme-cache", ".mxcli": - return true + inProject := f + if projDir != "." && projDir != "" { + prefix := projDir + "/" + if len(f) <= len(prefix) || !strings.EqualFold(f[:len(prefix)], prefix) { + return false } + inProject = f[len(prefix):] + } + folder, rest, inFolder := strings.Cut(inProject, "/") + if !inFolder || rest == "" { + return false + } + switch strings.ToLower(folder) { + case ".mendix-cache", "deployment", "theme-cache", ".mxcli": + return true } return false } diff --git a/cmd/mxcli/gitdoctor_test.go b/cmd/mxcli/gitdoctor_test.go index edb2b38ec..5457a1b31 100644 --- a/cmd/mxcli/gitdoctor_test.go +++ b/cmd/mxcli/gitdoctor_test.go @@ -592,6 +592,85 @@ func TestGitCommit_SpacedPathsStagedWholeAndJunkLeft(t *testing.T) { } } +// writeFiles creates each repo-relative path with its parent folders. +func writeFiles(t *testing.T, dir string, paths ...string) { + t.Helper() + for _, p := range paths { + full := filepath.Join(dir, filepath.FromSlash(p)) + if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(full, []byte(p), 0o644); err != nil { + t.Fatal(err) + } + } +} + +// committedSet is the set of paths HEAD changed. +func committedSet(t *testing.T, dir string) map[string]bool { + t.Helper() + set := map[string]bool{} + for _, f := range strings.Split(strings.TrimSpace(committedFiles(t, dir)), "\n") { + set[f] = true + } + return set +} + +// The never-commit folders are the project's own, beside the .mpr, where +// Studio Pro's .gitignore anchors them (/deployment/). Matched at any depth, a +// user folder of the same name was silently left out of the commit. +func TestGitCommit_NestedFolderNamedLikeJunkIsCommitted(t *testing.T) { + dir, mprPath := doctorNoteRepo(t) + doctorEnv(t, dir) + writeFiles(t, dir, "resources/deployment/endpoints.xml", "javascriptsource/theme-cache/x.js", "deployment/run.log", "theme-cache/a.css") + + var out bytes.Buffer + if err := commitRun(mprPath, "nested", false, false, &out); err != nil { + t.Fatalf("commitRun: %v\n%s", err, out.String()) + } + files := committedSet(t, dir) + for _, keep := range []string{"resources/deployment/endpoints.xml", "javascriptsource/theme-cache/x.js"} { + if !files[keep] { + t.Errorf("%s is not the project's own folder and must be committed:\n%s", keep, out.String()) + } + } + for _, junk := range []string{"deployment/run.log", "theme-cache/a.css"} { + if files[junk] { + t.Errorf("%s is the project's own and must be left out", junk) + } + } +} + +// With the project in a subfolder of the repository, its folders are under +// that subfolder; the same names elsewhere in the repository are not its own. +func TestGitCommit_JunkAnchoredAtProjectInSubfolder(t *testing.T) { + root := t.TempDir() + doctorEnv(t, root) + gitIn(t, root, "init", "-q", "-b", "feature") + app := filepath.Join(root, "app") + if err := os.MkdirAll(app, 0o755); err != nil { + t.Fatal(err) + } + writeProject(t, app, "11.13.0", baseUnits(t)) + gitIn(t, root, "add", "-A") + gitIn(t, root, "commit", "-q", "-m", "base") + writeFiles(t, root, "app/deployment/run.log", "app/resources/deployment/endpoints.xml", "tools/deployment/deploy.sh") + + var out bytes.Buffer + if err := commitRun(filepath.Join(app, "App.mpr"), "sub", false, false, &out); err != nil { + t.Fatalf("commitRun: %v\n%s", err, out.String()) + } + files := committedSet(t, root) + if files["app/deployment/run.log"] { + t.Error("app/deployment/ is the project's own and must be left out") + } + for _, keep := range []string{"app/resources/deployment/endpoints.xml", "tools/deployment/deploy.sh"} { + if !files[keep] { + t.Errorf("%s must be committed:\n%s", keep, out.String()) + } + } +} + // An unresolved merge must refuse the commit: `git add -A` would otherwise // stage the conflict markers and commit them. func TestGitCommit_RefusedDuringUnresolvedMerge(t *testing.T) { diff --git a/docs-site/src/tools/outside-studio-pro.md b/docs-site/src/tools/outside-studio-pro.md index 2da3867e1..8219bcf62 100644 --- a/docs-site/src/tools/outside-studio-pro.md +++ b/docs-site/src/tools/outside-studio-pro.md @@ -196,8 +196,10 @@ In order it: 1. repairs ContentsHash mismatches from the files on disk (`MISSING` and `ORPHAN` entries have no automatic repair and refuse the commit); 2. stages everything, then unstages the files Studio Pro never commits: - `project-settings.user.json`, `*.mpr.lock`, `*.mpr.bak`, `.mendix-cache/`, - `deployment/`, `theme-cache/`, `.mxcli/`; + `project-settings.user.json`, `*.mpr.lock`, `*.mpr.bak`, and the project's + own `.mendix-cache/`, `deployment/`, `theme-cache/` and `.mxcli/`, the + folders Studio Pro, mxbuild and mxcli create beside the `.mpr`. A folder of + the same name further down, such as `resources/deployment/`, is committed; 3. runs `git commit` with your message and your git identity; 4. computes the mx_metadata note from the new commit and attaches it, and prints the `git push origin refs/notes/mx_metadata` command —