From b987327c4c6b4fdaa28f203f3a1dca38c137e359 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 23:46:03 +0000 Subject: [PATCH 01/29] fix(run-local): verify the web client bundle after the boot, not before it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every page in a locally-run app rendered as a black screen, intermittently and at the same mxcli version. `run --local` bundles the browser client at step 5b and it succeeds; then the runtime boot runs Gradle `clean-custom-classes compile package`, whose package pass repopulates deployment/web and takes dist/ with it. The bundle was deleted 51 seconds after the same command wrote it. It only bites when Gradle has work to do — a new Java action, a full recompile — which is why an app boots fine for weeks and then stops with nothing changed. Nothing reported it. `mxcli check` passes, the build succeeds, the runtime log is quiet, `curl /` returns 200 with a valid HTML shell, and every OData service answers. Only a browser sees it. A pre-condition established before a step that rewrites the same directory is not a post-condition, so the bundle is now verified after the boot: - WebClientBundled / EnsureWebClientBundle re-bundle only when it is gone, and say so. When Gradle had nothing to do this is a stat. - Re-ordering the bundle to after the boot instead was rejected: it would leave the app reachable-but-blank for ~30s on every cold start. - A failed re-bundle warns and leaves the runtime up — the app's services still work, only the browser is broken — and names the blank page. `mxcli test --local` boots the same way and destroys the bundle too, which is why a test run between a boot and a browser looked like a rendering bug. Tests are headless, so that path reports the loss and the remedy instead of spending ~30s on a loop whose point is two seconds. Both controls run: with the guard never firing the wipe test fails, and with it always firing the survivor test fails. mxcli-formula1 §35 --- .claude/skills/fix-issue.md | 1 + .claude/skills/mendix/run-local.md | 18 +++ cmd/mxcli/docker/localapp.go | 6 + cmd/mxcli/docker/runlocal.go | 12 ++ cmd/mxcli/docker/webclient.go | 78 +++++++++++- cmd/mxcli/docker/webclient_bundle_test.go | 144 ++++++++++++++++++++++ 6 files changed, 256 insertions(+), 3 deletions(-) create mode 100644 cmd/mxcli/docker/webclient_bundle_test.go diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index cb6eddabe..3d90b651d 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -424,3 +424,4 @@ extracting `OffsetExpression`/`LimitExpression`. | `create odata client` with credentials given as constants (`HttpUsername: '@Module.ApiUser'`) still gets HTTP 401 and an empty client, after the fix that made literal credentials work. Sharpened by the same release making a constant `ServiceUrl` mandatory — the shape the tool insists on is the shape whose credentials it will not read | `resolveCredential` trusted the visitor's isLiteral flag. `'@Module.ApiUser'` **is** a STRING_LITERAL, so the flag said "literal" and the previous code sent the fifteen characters `@Module.ApiUser` as the username — and the unresolved-credential note did not fire either, because as far as the code knew nothing was unresolved | `mdl/executor/cmd_odata.go` (`resolveCredential`, `constantReference`, `designTimeConstants`) | **A syntactic classification is not a semantic one.** The visitor can say "this was a quoted string"; only the executor can say "this quoted string names a constant". Any flag of the form isLiteral needs the consumer to ask what the literal *contains* before treating it as a value. **The fix is to resolve, not to refuse**: a constant's design-time default is exactly what Studio Pro sends on the same fetch, so reading it is the value rather than a workaround — and mxcli already has the project open. Three spellings must all work (`'v'`, `@M.C`, `'@M.C'`); a dotted literal like a password containing a dot must not be mistaken for a reference. Tests `cmd_odata_metadata_auth_test.go`. mxcli-formula1 #23 follow-up | | An app themed dark still shows light-mode drop shadows under the datagrid's filter-operator popover and dropdown filter lists | The generated widget layer re-pointed `.column-selectors` but not the four rules in `_datagrid-filters.scss` that bake the same two-layer shadow. Each already takes its *background* from `--bg-color-secondary`, so Atlas re-colours the panel and leaves the shadow — which is why it reads as a partial fix rather than an untouched widget | `cmd/mxcli/theme/assets/*/files/theme/web/_mxcli-widgets.scss` | **Read the selectors out of the shipped `themesource/`, not the bug report** — the fourth here is `.dropdown-container .dropdown-list`, nested under a parent the report did not name. **Verify in the compiled CSS, never the source** (the §33 rule): apply the theme to a real project, run `mxbuild --target=deploy --java-home=… --java-exe-path=…`, then grep `theme-cache/web/theme.compiled.css` and check the *line number* — the fix must land after the widget module's own rule (30794 vs 27765 here) or the cascade eats it. A theme-cache file older than the SCSS you just wrote is a stale compile, and mtime is the cheapest way to catch it. mxcli-formula1 #33 / suggested issue 4 | | A `MOVE` cannot be confirmed and a module's layout cannot be reviewed: `SHOW STRUCTURE` groups by document type at every depth and never names a folder, `DESCRIBE` answers for one document at a time, so checking where things ended up means opening the `.mpr` as SQLite | The read side of folders was simply never built. `MOVE`/`DROP FOLDER` write containment; nothing read it back | New `mdl/executor/cmd_list_folders.go` + grammar (`FOLDERS` lexer token, `showOrList FOLDERS (IN …)?` in `MDLCatalog.g4`, `FOLDERS` added to the `keyword` rule so it stays usable as an identifier), `mdl/ast/ast_query.go`, `mdl/visitor/visitor_query.go`, `mdl/executor/executor_query.go` | **A layout listing must show what is *not* there**: empty folders (`[0]`) and documents still at the module root, or it cannot be diffed against an intended layout — that is the whole use. Documents are indexed by `ContainerID` across every list call the backend offers, each best-effort, so a backend that cannot answer one kind yields a listing missing that kind rather than no listing. **Do not stub the hierarchy in the test** — `mkHierarchy` populates `moduleNames` but not `folderNames`, so `BuildFolderPath` returns `""` and every folder silently collapses into the module root, which looks exactly like the bug. Build it from the mock's `ListModules`/`ListUnits`/`ListFolders`, as `getHierarchy` does. Tests `cmd_list_folders_test.go`, example in `18-folder-examples.mdl`. mxcli-formula1 issue #2 | +| Every page in a locally-run app is a black screen. `mxcli check` passes, the build reports success, the runtime log is quiet, `curl /` returns 200 with a valid HTML shell, and every OData service answers — only a browser sees it. It also comes and goes between runs of the same command at the same version | `run --local` bundles the browser client at step 5b, then the runtime boot runs Gradle `clean-custom-classes compile package`, whose package pass repopulates `deployment/web` and takes `dist/` with it — deleting the bundle 51 seconds after the same command wrote it. It only bites when Gradle has work to do (a new Java action, a full recompile), which is why the app boots fine for weeks and then stops | `cmd/mxcli/docker/webclient.go` (`WebClientBundled`, `EnsureWebClientBundle`, `ReportLostWebClientBundle`), `cmd/mxcli/docker/runlocal.go` (step 6b, after the boot), `cmd/mxcli/docker/localapp.go` (headless boots warn instead of paying ~30s) | **A pre-condition established before a step that rewrites the same directory is not a post-condition** — verify after, not before. The guard is a `stat` when the bundle survived, which is what makes it affordable at every boot; re-ordering the bundle to after the boot instead would leave the app reachable-but-blank for ~30s on every cold start. **`curl /` returning 200 is not evidence the app renders**: the shell is served by the runtime, the client by a file the shell references. The one-line check is `curl -o /dev/null -w '%{http_code}' /dist/index.js`. **Do not silently pay for a repair on a path that does not need it** — `test --local` destroys the bundle too, but tests are headless, so it prints the loss and the remedy rather than spending 30s on a two-second loop. Tests `webclient_bundle_test.go`; both controls run (guard never fires → the wipe test fails; guard always fires → the survivor test fails). mxcli-formula1 §35 | diff --git a/.claude/skills/mendix/run-local.md b/.claude/skills/mendix/run-local.md index 81e8f4eb6..48917d3d1 100644 --- a/.claude/skills/mendix/run-local.md +++ b/.claude/skills/mendix/run-local.md @@ -209,6 +209,24 @@ Playwright + the devcontainer's Chromium). skips the bundle and just hot-reloads. It uses `CHOKIDAR_USEPOLLING` because inotify is silent on container filesystems. - Without `--watch`, a single one-shot bundle (~7 s) runs before boot. +- **The bundle is re-checked after the boot**, because bundling before it is not + enough: the runtime's boot runs Gradle `clean-custom-classes compile package`, + and when Gradle has work to do (a new Java action, a full recompile) its package + pass repopulates `deployment/web` and deletes `dist/` — the bundle written + seconds earlier by the same command. If that happened, `run --local` says + `re-bundling` and rebuilds it. When Gradle had nothing to do the check is a + `stat` and costs nothing. + +**If you ever see a black page:** that is this failure, and nothing else reports +it — `mxcli check` passes, the build succeeds, the runtime log is quiet, `curl /` +returns **200** with a valid HTML shell, and the OData services all answer. Only a +browser sees it. Confirm with `curl -o /dev/null -w '%{http_code}' /dist/index.js`; +a 404 there is the whole diagnosis. + +`mxcli test --local` boots the same way and destroys the bundle too. Tests are +headless so it is not rebuilt for them (that would cost ~30 s on a loop whose point +is two seconds) — the run prints a note instead, and a subsequent `run --local` +restores it. ## Pixel-perfect page loop diff --git a/cmd/mxcli/docker/localapp.go b/cmd/mxcli/docker/localapp.go index d68874178..8842a9c4b 100644 --- a/cmd/mxcli/docker/localapp.go +++ b/cmd/mxcli/docker/localapp.go @@ -151,6 +151,11 @@ func StartLocalApp(opts LocalAppOptions) (*LocalApp, error) { app := &LocalApp{Version: version, RuntimeLogPath: opts.RuntimeLogPath} + // Whether a browser bundle exists *before* this boot. The boot's Gradle + // packaging removes it, and this app is booted headless (tests), so the loss + // is only noticed later by whoever opens a browser (mxcli-formula1 §35). + hadWebClient := WebClientBundled(opts.DeployDir) + // 4. Build, unless the caller is reusing an existing deployment. if !opts.SkipBuild { fmt.Fprintln(w, "Building project (mxbuild --serve)...") @@ -189,6 +194,7 @@ func StartLocalApp(opts LocalAppOptions) (*LocalApp, error) { return nil, err } app.Runtime = rt + ReportLostWebClientBundle(opts.DeployDir, hadWebClient, w) return app, nil } diff --git a/cmd/mxcli/docker/runlocal.go b/cmd/mxcli/docker/runlocal.go index 96023d7a2..a8725dc66 100644 --- a/cmd/mxcli/docker/runlocal.go +++ b/cmd/mxcli/docker/runlocal.go @@ -740,6 +740,18 @@ func RunLocal(opts LocalRunOptions) error { } defer rt.Stop() + // 6b. The boot's Gradle `package` pass repopulates deployment/web, and when + // Gradle had work to do it takes dist/ with it — deleting the bundle step 5b + // wrote seconds ago. Verify after the boot, because before it proves nothing + // (mxcli-formula1 §35). Costs a stat when the bundle survived. + if _, err := EnsureWebClientBundle(WebClientOptions{ + DeployDir: opts.DeployDir, MxBuildPath: mxbuildPath, Stdout: w, + }); err != nil { + // The app is up and its services answer; only the browser is broken. Say so + // and keep running rather than tearing down a working runtime. + fmt.Fprintf(stderr, "Warning: %v\n", err) + } + if opts.OnReady != nil { opts.OnReady(LocalAppInfo{ AppPort: opts.AppPort, diff --git a/cmd/mxcli/docker/webclient.go b/cmd/mxcli/docker/webclient.go index 0d56cd035..364ffc314 100644 --- a/cmd/mxcli/docker/webclient.go +++ b/cmd/mxcli/docker/webclient.go @@ -138,10 +138,82 @@ func BuildWebClient(opts WebClientOptions) error { return fmt.Errorf("web client build timed out after %s", timeout) } - dist := filepath.Join(webDir, "dist", "index.js") - if _, err := os.Stat(dist); err != nil { - return fmt.Errorf("web client build reported success but %s is missing:\n%s", dist, log.String()) + if !WebClientBundled(opts.DeployDir) { + return fmt.Errorf("web client build reported success but %s is missing:\n%s", + webClientBundlePath(opts.DeployDir), log.String()) } fmt.Fprintf(w, " Web client bundled in %s\n", time.Since(start).Round(time.Millisecond)) return nil } + +// webClientBundlePath is the one file whose absence is the black screen: the +// shell loads, paints the theme's background, and never starts the client. +func webClientBundlePath(deployDir string) string { + return filepath.Join(deployDir, "web", "dist", "index.js") +} + +// WebClientBundled reports whether the deployment currently has a browser +// bundle to serve. +func WebClientBundled(deployDir string) bool { + fi, err := os.Stat(webClientBundlePath(deployDir)) + return err == nil && !fi.IsDir() && fi.Size() > 0 +} + +// EnsureWebClientBundle re-bundles when the bundle is missing, and reports +// whether it had to. +// +// Bundling before the runtime boots is not enough. The boot runs Gradle +// `clean-custom-classes compile package`, and when Gradle has work to do — a new +// Java action, a full recompile — its package pass repopulates deployment/web +// and takes dist/ with it, deleting the bundle written seconds earlier by a +// previous step of the same command. Nothing reports this: `mxcli check` passes, +// the build succeeds, the runtime logs nothing, `curl /` returns 200 with a valid +// HTML shell, and every OData service answers. Only a browser sees the black +// screen, which is how it survives restarts (mxcli-formula1 §35). +// +// So the bundle is verified *after* the boot rather than trusted from before it. +// When Gradle had nothing to do the check is a stat and costs nothing, which is +// why this is a guard rather than a reordering — the bundle still exists before +// the boot for the common case where the app is reachable immediately. +func EnsureWebClientBundle(opts WebClientOptions) (bool, error) { + return ensureWebClientBundle(opts.DeployDir, opts.Stdout, func() error { + return BuildWebClient(opts) + }) +} + +// ReportLostWebClientBundle says so when a boot destroyed a bundle that existed +// before it, and reports whether it did. +// +// This is the second way into §35: `mxcli test --local` boots the same way and +// its Gradle package pass wipes the bundle too, so a test run between a `run +// --local` and a browser leaves the app serving a black screen even though +// nothing was rebuilt. Tests are headless and do not need the bundle, and +// re-bundling would cost ~30s on a loop whose whole point is two seconds — so +// this warns with the remedy instead of paying for it uninvited. +func ReportLostWebClientBundle(deployDir string, hadBundle bool, w io.Writer) bool { + if w == nil || !hadBundle || WebClientBundled(deployDir) { + return false + } + fmt.Fprintf(w, "Note: this boot's packaging step removed the browser bundle at %s.\n"+ + " The app will render a blank page until it is rebuilt — re-run 'mxcli run --local' to restore it.\n", + webClientBundlePath(deployDir)) + return true +} + +// ensureWebClientBundle holds the decision, separated from the node invocation +// so it can be tested without mxbuild's tooling. +func ensureWebClientBundle(deployDir string, w io.Writer, bundle func() error) (bool, error) { + if w == nil { + w = io.Discard + } + if WebClientBundled(deployDir) { + return false, nil + } + fmt.Fprintln(w, "Web client bundle was removed by the boot's packaging step; re-bundling...") + if err := bundle(); err != nil { + return true, fmt.Errorf("re-bundling web client after boot: %w\n"+ + " The app is running but will render a blank page until %s exists.", + err, webClientBundlePath(deployDir)) + } + return true, nil +} diff --git a/cmd/mxcli/docker/webclient_bundle_test.go b/cmd/mxcli/docker/webclient_bundle_test.go new file mode 100644 index 000000000..d2c47d1ec --- /dev/null +++ b/cmd/mxcli/docker/webclient_bundle_test.go @@ -0,0 +1,144 @@ +// SPDX-License-Identifier: Apache-2.0 + +package docker + +import ( + "bytes" + "errors" + "os" + "path/filepath" + "strings" + "testing" +) + +// writeBundle creates a deployment whose web client is bundled. +func writeBundle(t *testing.T, deployDir, content string) { + t.Helper() + dist := filepath.Join(deployDir, "web", "dist") + if err := os.MkdirAll(dist, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dist, "index.js"), []byte(content), 0o644); err != nil { + t.Fatal(err) + } +} + +// mxcli-formula1 §35: the boot's Gradle package pass repopulates deployment/web +// and removes dist/, deleting the bundle a previous step of the same command +// wrote. The app then serves a 200 HTML shell and a black screen — invisible to +// check, build, the runtime log and curl. +func TestEnsureWebClientBundle_RebundlesAfterThePackagingWipe(t *testing.T) { + deployDir := t.TempDir() + writeBundle(t, deployDir, "// bundled at 15:15:31") + + // What the Gradle package pass does at 15:16:22. + if err := os.RemoveAll(filepath.Join(deployDir, "web", "dist")); err != nil { + t.Fatal(err) + } + + var out bytes.Buffer + called := 0 + rebuilt, err := ensureWebClientBundle(deployDir, &out, func() error { + called++ + writeBundle(t, deployDir, "// re-bundled after boot") + return nil + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !rebuilt || called != 1 { + t.Fatalf("the wipe was not repaired: rebuilt=%v calls=%d", rebuilt, called) + } + if !WebClientBundled(deployDir) { + t.Error("the app would still serve a blank page") + } + // Silence is what made this survive several restarts, so the repair is stated. + if !strings.Contains(out.String(), "re-bundling") { + t.Errorf("the re-bundle was not reported:\n%s", out.String()) + } +} + +// A bundle that survived the boot must not be rebuilt — the guard costs a stat +// in the common case, which is what makes it affordable at every boot. +func TestEnsureWebClientBundle_LeavesASurvivingBundleAlone(t *testing.T) { + deployDir := t.TempDir() + writeBundle(t, deployDir, "// bundled, and Gradle had nothing to do") + + var out bytes.Buffer + called := 0 + rebuilt, err := ensureWebClientBundle(deployDir, &out, func() error { + called++ + return nil + }) + if err != nil || rebuilt || called != 0 { + t.Fatalf("re-bundled a bundle that was already there: rebuilt=%v calls=%d err=%v", rebuilt, called, err) + } + if out.Len() != 0 { + t.Errorf("the quiet path should stay quiet, got:\n%s", out.String()) + } +} + +// A failed re-bundle must name the consequence. The runtime is up and its +// services answer, so "the app is running" is not the whole truth. +func TestEnsureWebClientBundle_FailureNamesTheBlankPage(t *testing.T) { + deployDir := t.TempDir() + + _, err := ensureWebClientBundle(deployDir, nil, func() error { + return errors.New("rollup exploded") + }) + if err == nil { + t.Fatal("expected an error") + } + if !strings.Contains(err.Error(), "blank page") || !strings.Contains(err.Error(), "rollup exploded") { + t.Errorf("error should carry both the cause and the symptom, got: %v", err) + } +} + +// An empty dist/index.js is the same blank page as a missing one — a truncated +// write must not read as "bundled". +func TestWebClientBundled_RejectsAnEmptyBundle(t *testing.T) { + deployDir := t.TempDir() + writeBundle(t, deployDir, "") + if WebClientBundled(deployDir) { + t.Error("an empty bundle should not count as bundled") + } + + // And a directory called index.js is not a bundle either. + other := t.TempDir() + if err := os.MkdirAll(filepath.Join(other, "web", "dist", "index.js"), 0o755); err != nil { + t.Fatal(err) + } + if WebClientBundled(other) { + t.Error("a directory named index.js should not count as bundled") + } +} + +// The second way in: a headless boot (mxcli test --local) destroys a bundle a +// previous `run --local` left behind. Tests do not need it, so the loss is +// reported rather than paid for — but it must not be silent, which is what let +// a test run between a boot and a browser look like a rendering bug. +func TestReportLostWebClientBundle(t *testing.T) { + deployDir := t.TempDir() + var out bytes.Buffer + + // Destroyed by this boot: say so, and name the remedy. + if !ReportLostWebClientBundle(deployDir, true, &out) { + t.Error("a destroyed bundle should be reported") + } + if !strings.Contains(out.String(), "blank page") || !strings.Contains(out.String(), "mxcli run --local") { + t.Errorf("the note should carry the symptom and the fix:\n%s", out.String()) + } + + // Never there to begin with: not this boot's doing, so nothing to say. + out.Reset() + if ReportLostWebClientBundle(deployDir, false, &out) || out.Len() > 0 { + t.Errorf("an absent bundle this boot did not destroy should be silent:\n%s", out.String()) + } + + // Survived: silent. + out.Reset() + writeBundle(t, deployDir, "// survived") + if ReportLostWebClientBundle(deployDir, true, &out) || out.Len() > 0 { + t.Errorf("a surviving bundle should be silent:\n%s", out.String()) + } +} From 4551a4ea1dfe1b35c36f6919aafc4ffb091e161d Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 23:50:18 +0000 Subject: [PATCH 02/29] fix(describe): put back the page-parameter mapping mxcli stores implicitly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A grid drill-down written as linkbutton btnWeekend (Caption: 'Weekend', Action: SHOW_PAGE Mod.Race_Weekend(Race: $currentObject)) described back as `show_page Mod.Race_Weekend`, with no argument. The mapping was never missing from the model. mxcli writes a page action's ParameterMappings as an empty array on purpose: Studio Pro infers the current row object from the enclosing widget, and an explicit mapping whose Argument is "$currentObject" is rejected as CE0115 (#296). That decision stands. What was missing is its other half — DESCRIBE read only explicit mappings, so the implicit argument had nowhere to come from. The writer's comment claimed DESCRIBE recovered it; it did not, and now it does. The cost is not cosmetic. DESCRIBE is what you reach for once you have stopped trusting the model, so its output arrives already looking like a conclusion: the mapping was dropped, that is why the page gets an empty object. It is a plausible, wrong answer at the worst possible moment, and it cost three debugging cycles replacing a button that was correct. Recovery reads the target page's own declared parameters, since that is where the information lives. Both ends are guarded: an explicit mapping still wins, and an unresolvable page yields no arguments rather than invented ones. Control: with recovery removed the test reproduces the reported output verbatim. mxcli-formula1 §39 --- .claude/skills/fix-issue.md | 1 + mdl/executor/cmd_pages_describe_output.go | 68 +++++++++- .../cmd_pages_describe_pageparams_test.go | 126 ++++++++++++++++++ sdk/mpr/writer_widgets_action.go | 8 +- 4 files changed, 199 insertions(+), 4 deletions(-) create mode 100644 mdl/executor/cmd_pages_describe_pageparams_test.go diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index 3d90b651d..f4fc96a8b 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -425,3 +425,4 @@ extracting `OffsetExpression`/`LimitExpression`. | An app themed dark still shows light-mode drop shadows under the datagrid's filter-operator popover and dropdown filter lists | The generated widget layer re-pointed `.column-selectors` but not the four rules in `_datagrid-filters.scss` that bake the same two-layer shadow. Each already takes its *background* from `--bg-color-secondary`, so Atlas re-colours the panel and leaves the shadow — which is why it reads as a partial fix rather than an untouched widget | `cmd/mxcli/theme/assets/*/files/theme/web/_mxcli-widgets.scss` | **Read the selectors out of the shipped `themesource/`, not the bug report** — the fourth here is `.dropdown-container .dropdown-list`, nested under a parent the report did not name. **Verify in the compiled CSS, never the source** (the §33 rule): apply the theme to a real project, run `mxbuild --target=deploy --java-home=… --java-exe-path=…`, then grep `theme-cache/web/theme.compiled.css` and check the *line number* — the fix must land after the widget module's own rule (30794 vs 27765 here) or the cascade eats it. A theme-cache file older than the SCSS you just wrote is a stale compile, and mtime is the cheapest way to catch it. mxcli-formula1 #33 / suggested issue 4 | | A `MOVE` cannot be confirmed and a module's layout cannot be reviewed: `SHOW STRUCTURE` groups by document type at every depth and never names a folder, `DESCRIBE` answers for one document at a time, so checking where things ended up means opening the `.mpr` as SQLite | The read side of folders was simply never built. `MOVE`/`DROP FOLDER` write containment; nothing read it back | New `mdl/executor/cmd_list_folders.go` + grammar (`FOLDERS` lexer token, `showOrList FOLDERS (IN …)?` in `MDLCatalog.g4`, `FOLDERS` added to the `keyword` rule so it stays usable as an identifier), `mdl/ast/ast_query.go`, `mdl/visitor/visitor_query.go`, `mdl/executor/executor_query.go` | **A layout listing must show what is *not* there**: empty folders (`[0]`) and documents still at the module root, or it cannot be diffed against an intended layout — that is the whole use. Documents are indexed by `ContainerID` across every list call the backend offers, each best-effort, so a backend that cannot answer one kind yields a listing missing that kind rather than no listing. **Do not stub the hierarchy in the test** — `mkHierarchy` populates `moduleNames` but not `folderNames`, so `BuildFolderPath` returns `""` and every folder silently collapses into the module root, which looks exactly like the bug. Build it from the mock's `ListModules`/`ListUnits`/`ListFolders`, as `getHierarchy` does. Tests `cmd_list_folders_test.go`, example in `18-folder-examples.mdl`. mxcli-formula1 issue #2 | | Every page in a locally-run app is a black screen. `mxcli check` passes, the build reports success, the runtime log is quiet, `curl /` returns 200 with a valid HTML shell, and every OData service answers — only a browser sees it. It also comes and goes between runs of the same command at the same version | `run --local` bundles the browser client at step 5b, then the runtime boot runs Gradle `clean-custom-classes compile package`, whose package pass repopulates `deployment/web` and takes `dist/` with it — deleting the bundle 51 seconds after the same command wrote it. It only bites when Gradle has work to do (a new Java action, a full recompile), which is why the app boots fine for weeks and then stops | `cmd/mxcli/docker/webclient.go` (`WebClientBundled`, `EnsureWebClientBundle`, `ReportLostWebClientBundle`), `cmd/mxcli/docker/runlocal.go` (step 6b, after the boot), `cmd/mxcli/docker/localapp.go` (headless boots warn instead of paying ~30s) | **A pre-condition established before a step that rewrites the same directory is not a post-condition** — verify after, not before. The guard is a `stat` when the bundle survived, which is what makes it affordable at every boot; re-ordering the bundle to after the boot instead would leave the app reachable-but-blank for ~30s on every cold start. **`curl /` returning 200 is not evidence the app renders**: the shell is served by the runtime, the client by a file the shell references. The one-line check is `curl -o /dev/null -w '%{http_code}' /dist/index.js`. **Do not silently pay for a repair on a path that does not need it** — `test --local` destroys the bundle too, but tests are headless, so it prints the loss and the remedy rather than spending 30s on a two-second loop. Tests `webclient_bundle_test.go`; both controls run (guard never fires → the wipe test fails; guard always fires → the survivor test fails). mxcli-formula1 §35 | +| `DESCRIBE PAGE` reports a drill-down button as `linkbutton b (Caption: 'Weekend', Action: show_page Mod.Page)` — the `(Race: $currentObject)` argument is gone. The model is fine (`mx check` is clean, and an unmapped required page parameter is a consistency error, so it could not have built), but the description reads as a diagnosis mid-hunt and costs cycles fixing a button that was correct | mxcli deliberately writes `ParameterMappings` as an empty array for a page action, because Studio Pro infers the row object from the enclosing widget and rejects an explicit `$currentObject` Argument as CE0115 (#296). That decision was right; its other half was missing. `renderClientActionMDL` read only explicit mappings, and the writer's comment *asserted* DESCRIBE recovered the implicit one — it never did | `mdl/executor/cmd_pages_describe_output.go` (`pageActionParameters`, `targetPageParameterNames`), stale comment corrected in `sdk/mpr/writer_widgets_action.go` | **When a writer stores something implicitly, the reader owes it an explicit reconstruction — and a comment claiming the reader already does is worth nothing until a test says so.** Recover from the *target page's* declared parameters, not from the action, since that is where the information actually lives. Guard both ends: an explicit mapping still wins (recovery fills a gap, it does not override Studio Pro), and an unresolvable page yields no arguments rather than invented ones — a description that omits an argument is recoverable, one that names a parameter that does not exist is not. **A lossy DESCRIBE is costliest exactly when it is most used**, because DESCRIBE is what you reach for once you have stopped trusting the model. Tests `cmd_pages_describe_pageparams_test.go`; the control (explicit-only) reproduces the reported output verbatim. mxcli-formula1 §39 | diff --git a/mdl/executor/cmd_pages_describe_output.go b/mdl/executor/cmd_pages_describe_output.go index 563f5882b..3cbff7d17 100644 --- a/mdl/executor/cmd_pages_describe_output.go +++ b/mdl/executor/cmd_pages_describe_output.go @@ -1077,7 +1077,7 @@ func renderClientActionMDL(ctx *ExecContext, action map[string]any) string { if formSettings, ok := action["FormSettings"].(map[string]any); ok { if pageName, ok := formSettings["Form"].(string); ok && pageName != "" { result := "show_page " + pageName - params := extractPageParameters(ctx, formSettings) + params := pageActionParameters(ctx, formSettings, pageName) if params != "" { result += "(" + params + ")" } @@ -1087,7 +1087,7 @@ func renderClientActionMDL(ctx *ExecContext, action map[string]any) string { if pageSettings, ok := action["PageSettings"].(map[string]any); ok { if pageName, ok := pageSettings["Form"].(string); ok && pageName != "" { result := "show_page " + pageName - params := extractPageParameters(ctx, pageSettings) + params := pageActionParameters(ctx, pageSettings, pageName) if params != "" { result += "(" + params + ")" } @@ -1159,6 +1159,70 @@ func getPageQualifiedName(ctx *ExecContext, pageID model.ID) string { return "" } +// pageActionParameters renders a SHOW_PAGE action's argument list, recovering the +// implicit mapping when the model stores none. +// +// A page action written by mxcli deliberately stores ParameterMappings as an +// empty array: Studio Pro infers the current row object from the enclosing +// widget, and an explicit mapping whose Argument is "$currentObject" is rejected +// as CE0115 "parameters do not match" (issue #296). Nothing was wrong with that +// decision — what was missing is its other half. DESCRIBE read only the explicit +// mappings, so `SHOW_PAGE P(Race: $currentObject)` came back as `show_page P`, +// and the description read as a diagnosis: the mapping was dropped, that is why +// the page gets an empty object. It was not dropped, and Mendix could not have +// built the page if it were — an unmapped required page parameter is a +// consistency error. Three debugging cycles were spent replacing a button that +// was correct (mxcli-formula1 §39). +// +// DESCRIBE is what you reach for once you have stopped trusting the model, so a +// lossy DESCRIBE is costliest exactly when it is most used. +func pageActionParameters(ctx *ExecContext, settings map[string]any, pageName string) string { + if explicit := extractPageParameters(ctx, settings); explicit != "" { + return explicit + } + // No stored mapping: the target page's own parameters are the mapping, each + // bound to the row object the enclosing widget supplies. + var params []string + for _, name := range targetPageParameterNames(ctx, pageName) { + params = append(params, mdlIdent(name)+": $currentObject") + } + return strings.Join(params, ", ") +} + +// targetPageParameterNames returns the parameter names declared by a page, by +// qualified name. Returns nil when the page cannot be resolved — a description +// that omits an argument is better than one that invents a name. +func targetPageParameterNames(ctx *ExecContext, qualifiedName string) []string { + module, name, ok := strings.Cut(qualifiedName, ".") + if !ok || module == "" || name == "" { + return nil + } + allPages, err := ctx.Backend.ListPages() + if err != nil { + return nil + } + h, err := getHierarchy(ctx) + if err != nil { + return nil + } + for _, p := range allPages { + if p == nil || !strings.EqualFold(p.Name, name) { + continue + } + if !strings.EqualFold(h.GetModuleName(h.FindModuleID(p.ContainerID)), module) { + continue + } + var names []string + for _, param := range p.Parameters { + if param != nil && param.Name != "" { + names = append(names, param.Name) + } + } + return names + } + return nil +} + // extractPageParameters extracts page parameter mappings from a FormSettings/PageSettings object. // Returns formatted string like "Product: $currentObject" or empty string if no params. func extractPageParameters(ctx *ExecContext, settings map[string]any) string { diff --git a/mdl/executor/cmd_pages_describe_pageparams_test.go b/mdl/executor/cmd_pages_describe_pageparams_test.go new file mode 100644 index 000000000..e9c38459d --- /dev/null +++ b/mdl/executor/cmd_pages_describe_pageparams_test.go @@ -0,0 +1,126 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/backend/mock" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/pages" +) + +// pageParamFixture builds a module with a target page that declares one +// parameter — the drill-down page a grid's link button opens. +func pageParamFixture(t *testing.T, params ...string) *ExecContext { + t.Helper() + mod := mkModule("Formula1Frontend") + + target := &pages.Page{ + BaseElement: model.BaseElement{ID: "pg-weekend"}, + ContainerID: mod.ID, + Name: "Race_Weekend", + } + for i, p := range params { + target.Parameters = append(target.Parameters, &pages.PageParameter{ + BaseElement: model.BaseElement{ID: model.ID("pp-" + p)}, + ContainerID: target.ID, + Name: p, + IsRequired: i == 0, + }) + } + + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListModulesFunc: func() ([]*model.Module, error) { return []*model.Module{mod}, nil }, + ListPagesFunc: func() ([]*pages.Page, error) { return []*pages.Page{target}, nil }, + } + ctx, _ := newMockCtx(t, withBackend(mb)) + return ctx +} + +// showPageAction is the BSON mxcli writes for SHOW_PAGE: ParameterMappings is a +// deliberately empty array, because Studio Pro infers the row object from the +// enclosing widget and rejects an explicit "$currentObject" Argument as CE0115. +func showPageAction(page string, mappings []any) map[string]any { + settings := map[string]any{ + "$Type": "Forms$FormSettings", + "Form": page, + } + if mappings != nil { + settings["ParameterMappings"] = mappings + } + return map[string]any{ + "$Type": "Forms$FormAction", + "FormSettings": settings, + } +} + +// mxcli-formula1 §39: `SHOW_PAGE P(Race: $currentObject)` described back as +// `show_page P`. The mapping was never in the model — mxcli stores it implicitly +// on purpose — but DESCRIBE had no compensating recovery, so its output read as +// a diagnosis ("the mapping was dropped, that is why the page gets an empty +// object") in the middle of a hunt for an unrelated bug. Three cycles were spent +// replacing a button that was correct. +func TestRenderShowPageAction_RecoversTheImplicitParameter(t *testing.T) { + ctx := pageParamFixture(t, "Race") + + got := renderClientActionMDL(ctx, showPageAction("Formula1Frontend.Race_Weekend", nil)) + want := "show_page Formula1Frontend.Race_Weekend(Race: $currentObject)" + if got != want { + t.Errorf("DESCRIBE lost the page parameter:\n got: %s\nwant: %s", got, want) + } +} + +// A page with several parameters gets all of them, in declaration order. +func TestRenderShowPageAction_RecoversEveryParameter(t *testing.T) { + ctx := pageParamFixture(t, "Race", "Season") + + got := renderClientActionMDL(ctx, showPageAction("Formula1Frontend.Race_Weekend", nil)) + want := "show_page Formula1Frontend.Race_Weekend(Race: $currentObject, Season: $currentObject)" + if got != want { + t.Errorf("got: %s\nwant: %s", got, want) + } +} + +// A page that takes no parameters must not grow an argument list. +func TestRenderShowPageAction_NoParametersStaysBare(t *testing.T) { + ctx := pageParamFixture(t) + + got := renderClientActionMDL(ctx, showPageAction("Formula1Frontend.Race_Weekend", nil)) + if want := "show_page Formula1Frontend.Race_Weekend"; got != want { + t.Errorf("got: %s\nwant: %s", got, want) + } +} + +// An unresolvable page yields no arguments rather than invented ones: a +// description that omits an argument is recoverable, one that names a parameter +// that does not exist is not. +func TestRenderShowPageAction_UnknownPageInventsNothing(t *testing.T) { + ctx := pageParamFixture(t, "Race") + + got := renderClientActionMDL(ctx, showPageAction("OtherModule.Gone", nil)) + if want := "show_page OtherModule.Gone"; got != want { + t.Errorf("got: %s\nwant: %s", got, want) + } +} + +// An explicit mapping in the model still wins — recovery fills a gap, it does +// not override what Studio Pro actually stored. +func TestRenderShowPageAction_ExplicitMappingWins(t *testing.T) { + ctx := pageParamFixture(t, "Race") + + mappings := []any{ + int32(3), + map[string]any{ + "$Type": "Forms$PageParameterMapping", + "Parameter": "Formula1Frontend.Race_Weekend.Race", + "Argument": "$SelectedRace", + }, + } + got := renderClientActionMDL(ctx, showPageAction("Formula1Frontend.Race_Weekend", mappings)) + want := "show_page Formula1Frontend.Race_Weekend(Race: $SelectedRace)" + if got != want { + t.Errorf("an explicit mapping was overwritten:\n got: %s\nwant: %s", got, want) + } +} diff --git a/sdk/mpr/writer_widgets_action.go b/sdk/mpr/writer_widgets_action.go index 4fadebab8..c7ec8942d 100644 --- a/sdk/mpr/writer_widgets_action.go +++ b/sdk/mpr/writer_widgets_action.go @@ -86,8 +86,12 @@ func serializeClientAction(action pages.ClientAction) bson.D { // "$currentObject" makes Studio Pro report CE0115 "parameters do not match" — a // widget's current-row object is represented by an inferred WidgetValue, not an // Argument expression (issue #296; re-confirmed against mxbuild 11.12.1 for - // FINDINGS #56 — DESCRIBE recovers the implicit $currentObject instead, see - // renderClientActionMDL). + // FINDINGS #56). + // + // The other half of this decision lives in DESCRIBE, which must put the + // argument back from the target page's own parameters — see + // pageActionParameters. It did not, for a long time, and this comment + // asserted that it did (mxcli-formula1 §39). formSettings := bson.D{ {Key: "$ID", Value: idToBsonBinary(generateUUID())}, {Key: "$Type", Value: "Forms$FormSettings"}, From 28ce82130dbcb63b663699f6fdd3f20148feaa3f Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 23:54:54 +0000 Subject: [PATCH 03/29] fix(odata): stop deleting a published service's role grants on modify MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `create or modify odata service` silently revoked the service's access, so the next build failed with "At least one allowed role must be selected for the published OData service to be accessible." Grants are made by a separate statement and cannot be re-stated in the create script, so nothing in the script could put them back — only a manual re-grant, until the next modify. serializePublishedODataService never wrote AllowedModuleRoles. The document is serialized wholesale and written with updateUnit, so a field the serializer omits is not left alone: it is deleted. A wholesale re-serialization makes the writer's field list a data-retention policy, and this one was missing an entry. The array uses storage marker 1 (BY_NAME references) — the same shape the working GRANT path writes via makeMendixStringArray, rather than a marker reasoned out from an unrelated type. An earlier pass looked for this loss at model level, found the grants present and correctly carried through the modify branch, and recorded it as "reported but does not reproduce". That conclusion was wrong: the loss only exists after the round trip to BSON, which a model-level check cannot see. The carry-through guard added then was a no-op; it is kept for a caller that clears the slice, and its comment now says which layer actually held the bug. Control: with the field omitted again the test reproduces the empty-grants document. mxcli-formula1 §26 --- .claude/skills/fix-issue.md | 1 + mdl/executor/cmd_odata.go | 13 ++++--- sdk/mpr/writer_odata.go | 17 +++++++++ sdk/mpr/writer_odata_test.go | 68 ++++++++++++++++++++++++++++++++++++ 4 files changed, 94 insertions(+), 5 deletions(-) diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index f4fc96a8b..dbe856ecf 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -426,3 +426,4 @@ extracting `OffsetExpression`/`LimitExpression`. | A `MOVE` cannot be confirmed and a module's layout cannot be reviewed: `SHOW STRUCTURE` groups by document type at every depth and never names a folder, `DESCRIBE` answers for one document at a time, so checking where things ended up means opening the `.mpr` as SQLite | The read side of folders was simply never built. `MOVE`/`DROP FOLDER` write containment; nothing read it back | New `mdl/executor/cmd_list_folders.go` + grammar (`FOLDERS` lexer token, `showOrList FOLDERS (IN …)?` in `MDLCatalog.g4`, `FOLDERS` added to the `keyword` rule so it stays usable as an identifier), `mdl/ast/ast_query.go`, `mdl/visitor/visitor_query.go`, `mdl/executor/executor_query.go` | **A layout listing must show what is *not* there**: empty folders (`[0]`) and documents still at the module root, or it cannot be diffed against an intended layout — that is the whole use. Documents are indexed by `ContainerID` across every list call the backend offers, each best-effort, so a backend that cannot answer one kind yields a listing missing that kind rather than no listing. **Do not stub the hierarchy in the test** — `mkHierarchy` populates `moduleNames` but not `folderNames`, so `BuildFolderPath` returns `""` and every folder silently collapses into the module root, which looks exactly like the bug. Build it from the mock's `ListModules`/`ListUnits`/`ListFolders`, as `getHierarchy` does. Tests `cmd_list_folders_test.go`, example in `18-folder-examples.mdl`. mxcli-formula1 issue #2 | | Every page in a locally-run app is a black screen. `mxcli check` passes, the build reports success, the runtime log is quiet, `curl /` returns 200 with a valid HTML shell, and every OData service answers — only a browser sees it. It also comes and goes between runs of the same command at the same version | `run --local` bundles the browser client at step 5b, then the runtime boot runs Gradle `clean-custom-classes compile package`, whose package pass repopulates `deployment/web` and takes `dist/` with it — deleting the bundle 51 seconds after the same command wrote it. It only bites when Gradle has work to do (a new Java action, a full recompile), which is why the app boots fine for weeks and then stops | `cmd/mxcli/docker/webclient.go` (`WebClientBundled`, `EnsureWebClientBundle`, `ReportLostWebClientBundle`), `cmd/mxcli/docker/runlocal.go` (step 6b, after the boot), `cmd/mxcli/docker/localapp.go` (headless boots warn instead of paying ~30s) | **A pre-condition established before a step that rewrites the same directory is not a post-condition** — verify after, not before. The guard is a `stat` when the bundle survived, which is what makes it affordable at every boot; re-ordering the bundle to after the boot instead would leave the app reachable-but-blank for ~30s on every cold start. **`curl /` returning 200 is not evidence the app renders**: the shell is served by the runtime, the client by a file the shell references. The one-line check is `curl -o /dev/null -w '%{http_code}' /dist/index.js`. **Do not silently pay for a repair on a path that does not need it** — `test --local` destroys the bundle too, but tests are headless, so it prints the loss and the remedy rather than spending 30s on a two-second loop. Tests `webclient_bundle_test.go`; both controls run (guard never fires → the wipe test fails; guard always fires → the survivor test fails). mxcli-formula1 §35 | | `DESCRIBE PAGE` reports a drill-down button as `linkbutton b (Caption: 'Weekend', Action: show_page Mod.Page)` — the `(Race: $currentObject)` argument is gone. The model is fine (`mx check` is clean, and an unmapped required page parameter is a consistency error, so it could not have built), but the description reads as a diagnosis mid-hunt and costs cycles fixing a button that was correct | mxcli deliberately writes `ParameterMappings` as an empty array for a page action, because Studio Pro infers the row object from the enclosing widget and rejects an explicit `$currentObject` Argument as CE0115 (#296). That decision was right; its other half was missing. `renderClientActionMDL` read only explicit mappings, and the writer's comment *asserted* DESCRIBE recovered the implicit one — it never did | `mdl/executor/cmd_pages_describe_output.go` (`pageActionParameters`, `targetPageParameterNames`), stale comment corrected in `sdk/mpr/writer_widgets_action.go` | **When a writer stores something implicitly, the reader owes it an explicit reconstruction — and a comment claiming the reader already does is worth nothing until a test says so.** Recover from the *target page's* declared parameters, not from the action, since that is where the information actually lives. Guard both ends: an explicit mapping still wins (recovery fills a gap, it does not override Studio Pro), and an unresolvable page yields no arguments rather than invented ones — a description that omits an argument is recoverable, one that names a parameter that does not exist is not. **A lossy DESCRIBE is costliest exactly when it is most used**, because DESCRIBE is what you reach for once you have stopped trusting the model. Tests `cmd_pages_describe_pageparams_test.go`; the control (explicit-only) reproduces the reported output verbatim. mxcli-formula1 §39 | +| `create or modify odata service` silently revokes the service's access; the next build fails with "At least one allowed role must be selected for the published OData service to be accessible." Re-granting fixes it until the next modify | `serializePublishedODataService` never wrote `AllowedModuleRoles`. The document is serialized wholesale and written with `updateUnit`, so a field the serializer omits is not left alone — it is deleted. The grants were read correctly and carried through the executor, then dropped one layer down | `sdk/mpr/writer_odata.go` (`AllowedModuleRoles`, marker 1 / BY_NAME, matching the working `GRANT` path's `makeMendixStringArray`); stale executor comment corrected in `mdl/executor/cmd_odata.go` | **A wholesale re-serialization deletes every field it does not write, so the writer's field list is a data-retention policy.** Audit it against the parser, not against the struct — the round trip is the contract. **Where you look decides what you conclude**: an earlier pass looked for this loss at model level, found the value present and carried, and recorded "reported but does not reproduce" — the loss only exists after the BSON round trip, so a model-level check could never have seen it. When a report says a value disappears, reproduce at the persistence boundary before disbelieving it. For the marker, copy the shape from the code path that already works (`GRANT` writes marker 1 and builds fine) instead of reasoning from an unrelated type. Tests `writer_odata_test.go`; the control (field omitted again) reproduces the empty-grants document. mxcli-formula1 §26 | diff --git a/mdl/executor/cmd_odata.go b/mdl/executor/cmd_odata.go index bbdf8034b..dd82768bb 100644 --- a/mdl/executor/cmd_odata.go +++ b/mdl/executor/cmd_odata.go @@ -1397,11 +1397,14 @@ func createODataService(ctx *ExecContext, stmt *ast.CreateODataServiceStmt) erro // with "At least one allowed role must be selected for the // published OData service to be accessible." // - // A guard, not a fix for an observed defect: the loss was - // reported (mxcli-formula1 #26) but did not reproduce on - // 11.12.1 — grants survived a modify on both the current and - // the previous build. Kept because the invariant is real and - // the cost is a slice copy; if it never fires, nothing is lost. + // The reported loss (mxcli-formula1 §26) was real, and this + // carry-through was never what fixed it: the grants were read + // and carried correctly, then dropped one layer down, because + // serializePublishedODataService did not write the field at + // all. Looking for the loss at model level and concluding "does + // not reproduce" was the mistake — the round trip to BSON is + // where a wholesale re-serialization deletes what it omits. + // Kept as a guard for a caller that clears the slice. if len(svc.AllowedModuleRoles) == 0 && len(existingRoles) > 0 { svc.AllowedModuleRoles = existingRoles } diff --git a/sdk/mpr/writer_odata.go b/sdk/mpr/writer_odata.go index e79557311..2dc5ff74c 100644 --- a/sdk/mpr/writer_odata.go +++ b/sdk/mpr/writer_odata.go @@ -238,6 +238,22 @@ func (w *Writer) serializePublishedODataService(svc *model.PublishedODataService authTypes = append(authTypes, at) } + // AllowedModuleRoles: BY_NAME references, storage marker 1 — the same array + // shape the working GRANT path writes (makeMendixStringArray). + // + // This document is serialized wholesale and written with updateUnit, so a + // field the serializer omits is not left alone: it is deleted. Omitting it + // silently revoked a service's access on every `create or modify`, and the + // next build failed with "At least one allowed role must be selected for the + // published OData service to be accessible." Grants are made by a separate + // statement (`grant access on odata service …`) and cannot be re-stated in + // the create script, so nothing in the script could put them back + // (mxcli-formula1 §26). + allowedRoles := bson.A{int32(1)} + for _, name := range svc.AllowedModuleRoles { + allowedRoles = append(allowedRoles, name) + } + // Serialize entity types and build ID map for entity set pointers. // Issue #595: key by qualified entity name (et.Entity), not ExposedName. // PublishedEntitySet.EntityTypeName holds the qualified name, so keying @@ -289,6 +305,7 @@ func (w *Writer) serializePublishedODataService(svc *model.PublishedODataService {Key: "PublishAssociations", Value: svc.PublishAssociations}, {Key: "UseGeneralization", Value: svc.UseGeneralization}, {Key: "AuthenticationMicroflow", Value: svc.AuthMicroflow}, + {Key: "AllowedModuleRoles", Value: allowedRoles}, {Key: "AuthenticationTypes", Value: authTypes}, {Key: "EntityTypes", Value: entityTypes}, {Key: "EntitySets", Value: entitySets}, diff --git a/sdk/mpr/writer_odata_test.go b/sdk/mpr/writer_odata_test.go index fdc446a7e..f2d530258 100644 --- a/sdk/mpr/writer_odata_test.go +++ b/sdk/mpr/writer_odata_test.go @@ -389,3 +389,71 @@ func assertField(t *testing.T, m map[string]any, key, expected string) { t.Errorf("field %q: expected %q, got %q", key, expected, s) } } + +// mxcli-formula1 §26: `create or modify odata service` silently revoked the +// service's access, and the next build failed with "At least one allowed role +// must be selected for the published OData service to be accessible." +// +// The grants were read correctly and carried through the executor — and then +// dropped here. This document is serialized wholesale and written with +// updateUnit, so a field the serializer omits is not left alone, it is deleted. +// Because grants are made by a separate statement (`grant access on odata +// service …`) and cannot be re-stated in the create script, nothing in the +// script could put them back. +func TestSerializePublishedODataService_KeepsAllowedModuleRoles(t *testing.T) { + w := &Writer{} + svc := &model.PublishedODataService{ + BaseElement: model.BaseElement{ID: "svc-roles"}, + Name: "CustomerAPI", + ServiceName: "CustomerAPI", + AllowedModuleRoles: []string{"MyModule.User", "MyModule.Admin"}, + } + + data, err := w.serializePublishedODataService(svc) + if err != nil { + t.Fatalf("serialize failed: %v", err) + } + var raw map[string]any + if err := bson.Unmarshal(data, &raw); err != nil { + t.Fatalf("unmarshal failed: %v", err) + } + + // Storage marker 1 (BY_NAME references) — the same array shape the working + // GRANT path writes via makeMendixStringArray, and extractBsonArray only + // strips markers 2 and 3, so the marker is still element 0 here. + roles := extractBsonArray(raw["AllowedModuleRoles"]) + if len(roles) != 3 { + t.Fatalf("AllowedModuleRoles: expected marker + 2 grants, got %v — the service is now inaccessible and the build fails", roles) + } + if m, _ := roles[0].(int32); m != 1 { + t.Errorf("storage marker = %v, want 1 (BY_NAME)", roles[0]) + } + for i, want := range []string{"MyModule.User", "MyModule.Admin"} { + if got, _ := roles[i+1].(string); got != want { + t.Errorf("role %d = %q, want %q", i, got, want) + } + } +} + +// A service with no grants must still carry the field, as an empty versioned +// array — the absence of the key and an empty list are different documents. +func TestSerializePublishedODataService_EmptyRolesStillWritesTheField(t *testing.T) { + w := &Writer{} + data, err := w.serializePublishedODataService(&model.PublishedODataService{ + BaseElement: model.BaseElement{ID: "svc-noroles"}, + Name: "Bare", + }) + if err != nil { + t.Fatalf("serialize failed: %v", err) + } + var raw map[string]any + if err := bson.Unmarshal(data, &raw); err != nil { + t.Fatalf("unmarshal failed: %v", err) + } + if _, present := raw["AllowedModuleRoles"]; !present { + t.Error("AllowedModuleRoles must be present even when empty") + } + if arr := extractBsonArray(raw["AllowedModuleRoles"]); len(arr) != 1 { + t.Errorf("expected the bare marker and no grants, got %v", arr) + } +} From 01ef224ffe5b1db12f2f30ab80461bba058dd91a Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 23:58:20 +0000 Subject: [PATCH 04/29] fix(init): keep .ai-context/skills/ in step with the binary that serves them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The skills are embedded in mxcli and written exactly once, by `mxcli init`, so upgrading the binary did nothing to them: a project initialised on Monday still served Monday's guidance from Tuesday's mxcli, with no warning. Confirmed with a binary rebuilt at 12:05 beside skills stamped the previous day. Stale guidance is worse than missing guidance — an agent reads it with the same confidence either way, and the point of shipping skills inside the binary is that the two versions agree. Fixed where the files are consumed rather than where they are authored: the SessionStart bootstrap script already runs on every session and can fetch the binary, so it is the one place guaranteed to execute immediately before an agent reads them. It now runs `mxcli init --sync-skills`, a new flag that refreshes only .ai-context/skills/ and exits. Two properties make an every-session job acceptable: - it writes only the files that differ, so mtimes keep meaning "when did this guidance last move" (a test asserts an unchanged skill is not rewritten); - it is silent when the project is already current. It is never fatal — a skills refresh must not block a session. A test asserts the bootstrap calls it before the exec'd setup, since a step ordered after an exec never runs. Control: with write-once restored, the stale file survives the sync. mxcli-formula1 §16 --- .claude/skills/fix-issue.md | 1 + cmd/mxcli/init.go | 16 ++++ cmd/mxcli/init_hook.go | 7 ++ cmd/mxcli/init_skills_sync.go | 87 +++++++++++++++++ cmd/mxcli/init_skills_sync_test.go | 145 +++++++++++++++++++++++++++++ 5 files changed, 256 insertions(+) create mode 100644 cmd/mxcli/init_skills_sync.go create mode 100644 cmd/mxcli/init_skills_sync_test.go diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index dbe856ecf..b20627d10 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -427,3 +427,4 @@ extracting `OffsetExpression`/`LimitExpression`. | Every page in a locally-run app is a black screen. `mxcli check` passes, the build reports success, the runtime log is quiet, `curl /` returns 200 with a valid HTML shell, and every OData service answers — only a browser sees it. It also comes and goes between runs of the same command at the same version | `run --local` bundles the browser client at step 5b, then the runtime boot runs Gradle `clean-custom-classes compile package`, whose package pass repopulates `deployment/web` and takes `dist/` with it — deleting the bundle 51 seconds after the same command wrote it. It only bites when Gradle has work to do (a new Java action, a full recompile), which is why the app boots fine for weeks and then stops | `cmd/mxcli/docker/webclient.go` (`WebClientBundled`, `EnsureWebClientBundle`, `ReportLostWebClientBundle`), `cmd/mxcli/docker/runlocal.go` (step 6b, after the boot), `cmd/mxcli/docker/localapp.go` (headless boots warn instead of paying ~30s) | **A pre-condition established before a step that rewrites the same directory is not a post-condition** — verify after, not before. The guard is a `stat` when the bundle survived, which is what makes it affordable at every boot; re-ordering the bundle to after the boot instead would leave the app reachable-but-blank for ~30s on every cold start. **`curl /` returning 200 is not evidence the app renders**: the shell is served by the runtime, the client by a file the shell references. The one-line check is `curl -o /dev/null -w '%{http_code}' /dist/index.js`. **Do not silently pay for a repair on a path that does not need it** — `test --local` destroys the bundle too, but tests are headless, so it prints the loss and the remedy rather than spending 30s on a two-second loop. Tests `webclient_bundle_test.go`; both controls run (guard never fires → the wipe test fails; guard always fires → the survivor test fails). mxcli-formula1 §35 | | `DESCRIBE PAGE` reports a drill-down button as `linkbutton b (Caption: 'Weekend', Action: show_page Mod.Page)` — the `(Race: $currentObject)` argument is gone. The model is fine (`mx check` is clean, and an unmapped required page parameter is a consistency error, so it could not have built), but the description reads as a diagnosis mid-hunt and costs cycles fixing a button that was correct | mxcli deliberately writes `ParameterMappings` as an empty array for a page action, because Studio Pro infers the row object from the enclosing widget and rejects an explicit `$currentObject` Argument as CE0115 (#296). That decision was right; its other half was missing. `renderClientActionMDL` read only explicit mappings, and the writer's comment *asserted* DESCRIBE recovered the implicit one — it never did | `mdl/executor/cmd_pages_describe_output.go` (`pageActionParameters`, `targetPageParameterNames`), stale comment corrected in `sdk/mpr/writer_widgets_action.go` | **When a writer stores something implicitly, the reader owes it an explicit reconstruction — and a comment claiming the reader already does is worth nothing until a test says so.** Recover from the *target page's* declared parameters, not from the action, since that is where the information actually lives. Guard both ends: an explicit mapping still wins (recovery fills a gap, it does not override Studio Pro), and an unresolvable page yields no arguments rather than invented ones — a description that omits an argument is recoverable, one that names a parameter that does not exist is not. **A lossy DESCRIBE is costliest exactly when it is most used**, because DESCRIBE is what you reach for once you have stopped trusting the model. Tests `cmd_pages_describe_pageparams_test.go`; the control (explicit-only) reproduces the reported output verbatim. mxcli-formula1 §39 | | `create or modify odata service` silently revokes the service's access; the next build fails with "At least one allowed role must be selected for the published OData service to be accessible." Re-granting fixes it until the next modify | `serializePublishedODataService` never wrote `AllowedModuleRoles`. The document is serialized wholesale and written with `updateUnit`, so a field the serializer omits is not left alone — it is deleted. The grants were read correctly and carried through the executor, then dropped one layer down | `sdk/mpr/writer_odata.go` (`AllowedModuleRoles`, marker 1 / BY_NAME, matching the working `GRANT` path's `makeMendixStringArray`); stale executor comment corrected in `mdl/executor/cmd_odata.go` | **A wholesale re-serialization deletes every field it does not write, so the writer's field list is a data-retention policy.** Audit it against the parser, not against the struct — the round trip is the contract. **Where you look decides what you conclude**: an earlier pass looked for this loss at model level, found the value present and carried, and recorded "reported but does not reproduce" — the loss only exists after the BSON round trip, so a model-level check could never have seen it. When a report says a value disappears, reproduce at the persistence boundary before disbelieving it. For the marker, copy the shape from the code path that already works (`GRANT` writes marker 1 and builds fine) instead of reasoning from an unrelated type. Tests `writer_odata_test.go`; the control (field omitted again) reproduces the empty-grants document. mxcli-formula1 §26 | +| `.ai-context/skills/` still carries the previous release's guidance after the mxcli binary is upgraded — binary rebuilt at 12:05, skills stamped the day before — with no warning that they disagree | The skills are embedded in the binary and written exactly once, by `mxcli init`. Nothing re-ran init on upgrade, and nothing compared what was on disk against what the binary carried | New `cmd/mxcli/init_skills_sync.go` (`syncAIContextSkills`, `reportSkillSync`), `--sync-skills` flag in `cmd/mxcli/init.go`, and a sync step in the SessionStart bootstrap in `cmd/mxcli/init_hook.go` | **Stale guidance is worse than missing guidance**, because an agent reads it with identical confidence either way — so the failure mode is silent and confident. Fix it where it is consumed, not where it is authored: the SessionStart bootstrap already runs on every session and can fetch the binary, so it is the one place guaranteed to execute immediately before an agent reads the files. Two properties keep an every-session job acceptable: **write only what differs** (an mtime that moves every session makes "when did this last change" unanswerable — a test asserts the mtime holds) and **stay silent when current**. Never fatal: a skills refresh must not block a session, hence `|| true`. A test asserts the bootstrap script actually calls it *before* the exec'd setup, since a step ordered after an `exec` never runs. Tests `init_skills_sync_test.go`; the control (write-once) leaves the stale file in place. mxcli-formula1 §16 | diff --git a/cmd/mxcli/init.go b/cmd/mxcli/init.go index ca43f9a5c..1ed8d93cf 100644 --- a/cmd/mxcli/init.go +++ b/cmd/mxcli/init.go @@ -22,6 +22,7 @@ var ( initAllTools bool initListTools bool initContainerRuntime string + initSyncSkills bool ) const mendixGitignore = `# Mendix project @@ -135,6 +136,20 @@ Container Runtime: os.Exit(1) } + // --sync-skills: refresh only the embedded skills and exit. The rest of + // init is interactive-ish and writes tool configs; this is the part that + // must follow a binary upgrade, and the SessionStart bootstrap runs it + // unattended on every session (mxcli-formula1 §16). + if initSyncSkills { + res, err := syncAIContextSkills(absDir) + if err != nil { + fmt.Fprintf(os.Stderr, "Error syncing skills: %v\n", err) + os.Exit(1) + } + reportSkillSync(os.Stdout, res) + return + } + // Find .mpr file. With none here, look one level down: a solution repo // keeps each app in its own folder, and running `mxcli init` from the // root used to write everything at the root against an invented @@ -696,6 +711,7 @@ func init() { initCmd.Flags().BoolVar(&initAllTools, "all-tools", false, "Initialize for all supported AI tools") initCmd.Flags().BoolVar(&initListTools, "list-tools", false, "List supported AI tools and exit") initCmd.Flags().StringVar(&initContainerRuntime, "container-runtime", "docker", "Container runtime for devcontainer (docker or podman)") + initCmd.Flags().BoolVar(&initSyncSkills, "sync-skills", false, "Refresh .ai-context/skills/ from this binary and exit (quiet when already current)") } // findMprFilesInSubdirs returns the .mpr files one level below dir, sorted, so diff --git a/cmd/mxcli/init_hook.go b/cmd/mxcli/init_hook.go index 007f740a6..dd1ea962d 100644 --- a/cmd/mxcli/init_hook.go +++ b/cmd/mxcli/init_hook.go @@ -66,6 +66,13 @@ if [ ! -x ./mxcli ]; then chmod +x ./mxcli fi +# Keep .ai-context/skills/ in step with this binary. The skills are embedded in +# mxcli and written once by 'mxcli init', so upgrading the binary used to leave +# yesterday's guidance in place with no warning — and an agent reads stale +# guidance with the same confidence as current guidance. Quiet when already +# current; never fatal, since a skills refresh must not block the session. +./mxcli init --sync-skills . || true + exec ./mxcli run --local --setup --ensure-db -p "$MPR" ` diff --git a/cmd/mxcli/init_skills_sync.go b/cmd/mxcli/init_skills_sync.go new file mode 100644 index 000000000..bdf188396 --- /dev/null +++ b/cmd/mxcli/init_skills_sync.go @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "bytes" + "fmt" + "io" + "io/fs" + "os" + "path/filepath" + "sort" +) + +// init_skills_sync.go keeps a project's .ai-context/skills/ in step with the +// mxcli binary that serves it. +// +// The skills are embedded in the binary and written once by `mxcli init`. +// Upgrading the binary therefore did nothing to them: a project initialised on +// Monday still served Monday's guidance from Tuesday's mxcli, with no warning — +// confirmed with a binary rebuilt at 12:05 beside skills stamped the previous +// day (mxcli-formula1 §16). Stale guidance is worse than missing guidance, +// because an agent reads it with the same confidence either way, and the whole +// point of shipping skills in the binary is that the two versions agree. +// +// These files are generated, never user-edited (the sources live in the mxcli +// repo), so refreshing is a copy, not a merge. The SessionStart bootstrap script +// runs it on every session — the moment before an agent would read them. + +// skillSyncResult reports what a refresh did. +type skillSyncResult struct { + Total int // skills the binary carries + Changed []string // names whose on-disk content differed (added or updated) +} + +// Stale reports whether anything on disk disagreed with the binary. +func (r skillSyncResult) Stale() bool { return len(r.Changed) > 0 } + +// syncAIContextSkills rewrites /.ai-context/skills/ from the binary's +// embedded copies, reporting which files differed. Writing only the files that +// changed keeps mtimes meaningful, so "when did this guidance last move" stays +// answerable from the filesystem. +func syncAIContextSkills(projectDir string) (skillSyncResult, error) { + var res skillSyncResult + skillsDir := filepath.Join(projectDir, ".ai-context", "skills") + + entries, err := fs.ReadDir(skillsFS, "skills") + if err != nil { + return res, fmt.Errorf("reading embedded skills: %w", err) + } + if err := os.MkdirAll(skillsDir, 0o755); err != nil { + return res, fmt.Errorf("creating %s: %w", skillsDir, err) + } + + for _, e := range entries { + if e.IsDir() { + continue + } + want, err := skillsFS.ReadFile("skills/" + e.Name()) + if err != nil { + return res, fmt.Errorf("reading embedded skill %s: %w", e.Name(), err) + } + res.Total++ + + target := filepath.Join(skillsDir, e.Name()) + if have, readErr := os.ReadFile(target); readErr == nil && bytes.Equal(have, want) { + continue + } + if err := os.WriteFile(target, want, 0o644); err != nil { + return res, fmt.Errorf("writing %s: %w", target, err) + } + res.Changed = append(res.Changed, e.Name()) + } + sort.Strings(res.Changed) + return res, nil +} + +// reportSkillSync prints a one-line summary, and nothing at all when the project +// was already current — this runs on every session start, so silence is the +// common case and the only acceptable one. +func reportSkillSync(w io.Writer, res skillSyncResult) { + if !res.Stale() { + return + } + fmt.Fprintf(w, "Refreshed %d of %d skill file(s) in .ai-context/skills/ to match this mxcli: %v\n", + len(res.Changed), res.Total, res.Changed) +} diff --git a/cmd/mxcli/init_skills_sync_test.go b/cmd/mxcli/init_skills_sync_test.go new file mode 100644 index 000000000..d53ffb1f9 --- /dev/null +++ b/cmd/mxcli/init_skills_sync_test.go @@ -0,0 +1,145 @@ +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "bytes" + "io/fs" + "os" + "path/filepath" + "strings" + "testing" +) + +// embeddedSkillNames returns the skills this binary carries. +func embeddedSkillNames(t *testing.T) []string { + t.Helper() + entries, err := fs.ReadDir(skillsFS, "skills") + if err != nil { + t.Fatalf("reading embedded skills: %v", err) + } + var names []string + for _, e := range entries { + if !e.IsDir() { + names = append(names, e.Name()) + } + } + if len(names) == 0 { + t.Fatal("no embedded skills; the embed directive is broken") + } + return names +} + +// mxcli-formula1 §16: a project initialised on Monday still served Monday's +// skills from Tuesday's binary — the files are written once by `mxcli init` and +// nothing re-wrote them on upgrade. Stale guidance, no warning. +func TestSyncAIContextSkills_RefreshesStaleGuidance(t *testing.T) { + dir := t.TempDir() + names := embeddedSkillNames(t) + + // A project initialised by an older binary: the file exists, with content + // that binary shipped. + skillsDir := filepath.Join(dir, ".ai-context", "skills") + if err := os.MkdirAll(skillsDir, 0o755); err != nil { + t.Fatal(err) + } + stale := filepath.Join(skillsDir, names[0]) + if err := os.WriteFile(stale, []byte("# guidance from an older mxcli\n"), 0o644); err != nil { + t.Fatal(err) + } + + res, err := syncAIContextSkills(dir) + if err != nil { + t.Fatalf("sync failed: %v", err) + } + if !res.Stale() { + t.Fatal("the stale file was not detected") + } + if res.Total != len(names) { + t.Errorf("Total = %d, want %d", res.Total, len(names)) + } + + // Every embedded skill now matches the binary, not just the one that existed. + for _, n := range names { + want, err := skillsFS.ReadFile("skills/" + n) + if err != nil { + t.Fatal(err) + } + got, err := os.ReadFile(filepath.Join(skillsDir, n)) + if err != nil { + t.Fatalf("%s missing after sync: %v", n, err) + } + if !bytes.Equal(got, want) { + t.Errorf("%s still disagrees with the binary", n) + } + } + + var out bytes.Buffer + reportSkillSync(&out, res) + if !strings.Contains(out.String(), names[0]) { + t.Errorf("the refresh should name what it changed:\n%s", out.String()) + } +} + +// This runs on every session start, so an up-to-date project must be silent and +// must not rewrite files — an mtime that moves on every session makes "when did +// this guidance last change" unanswerable. +func TestSyncAIContextSkills_CurrentProjectIsSilentAndUntouched(t *testing.T) { + dir := t.TempDir() + + first, err := syncAIContextSkills(dir) + if err != nil { + t.Fatalf("first sync failed: %v", err) + } + if len(first.Changed) != first.Total { + t.Fatalf("a fresh project should write every skill: %d of %d", len(first.Changed), first.Total) + } + + skillsDir := filepath.Join(dir, ".ai-context", "skills") + probe := filepath.Join(skillsDir, first.Changed[0]) + before, err := os.Stat(probe) + if err != nil { + t.Fatal(err) + } + + second, err := syncAIContextSkills(dir) + if err != nil { + t.Fatalf("second sync failed: %v", err) + } + if second.Stale() { + t.Errorf("a current project reported changes: %v", second.Changed) + } + + after, err := os.Stat(probe) + if err != nil { + t.Fatal(err) + } + if !after.ModTime().Equal(before.ModTime()) { + t.Error("an unchanged skill was rewritten; mtime no longer means anything") + } + + var out bytes.Buffer + reportSkillSync(&out, second) + if out.Len() != 0 { + t.Errorf("the common path must be silent, got:\n%s", out.String()) + } +} + +// The SessionStart bootstrap must actually run the sync, or the fix ships +// without the thing that triggers it. +func TestBootstrapScript_SyncsSkillsBeforeSetup(t *testing.T) { + script := bootstrapScriptTemplate + sync := strings.Index(script, "init --sync-skills") + setup := strings.Index(script, "run --local --setup") + switch { + case sync < 0: + t.Fatal("the bootstrap script does not refresh skills") + case setup < 0: + t.Fatal("the bootstrap script no longer runs setup") + case sync > setup: + t.Error("skills are refreshed after the exec'd setup, so never") + } + if !strings.Contains(script[sync:], "|| true") { + t.Error("a failed skills refresh must not block the session") + } +} From 054f780a74c2fc976c38cd1a43780a57a951df13 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 00:04:29 +0000 Subject: [PATCH 05/29] fix(odata): publish the query-option opt-out instead of always claiming true MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `publish entity … (TopSupported: No)` parsed, described back as No, and published as Yes: serializeEntitySet hardcoded all three QueryOptions to true and never read the *bool fields the model already carried. The AST, model and DESCRIBE halves of the feature had shipped without the writer half — which is exactly the shape a DESCRIBE round-trip test cannot catch, since DESCRIBE reads the model, not the published document. For a microflow-backed resource this claim is load-bearing rather than decorative. Mendix applies no query options to a read-microflow resource: it hands the request to the microflow and returns what comes back. The annotation is therefore the only thing a client has to go on, and an over-claim is not cosmetic — a client that believes $top works reads a whole collection as though it were a page. nil keeps Mendix's own default of true; only an explicit false opts out, which is what the model's comment already promised. Control: with the constants restored the test reproduces the over-claim. mxcli-formula1 §20 --- .claude/skills/fix-issue.md | 1 + sdk/mpr/writer_odata.go | 17 +++++++-- sdk/mpr/writer_odata_test.go | 69 ++++++++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 3 deletions(-) diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index b20627d10..ecdca9568 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -428,3 +428,4 @@ extracting `OffsetExpression`/`LimitExpression`. | `DESCRIBE PAGE` reports a drill-down button as `linkbutton b (Caption: 'Weekend', Action: show_page Mod.Page)` — the `(Race: $currentObject)` argument is gone. The model is fine (`mx check` is clean, and an unmapped required page parameter is a consistency error, so it could not have built), but the description reads as a diagnosis mid-hunt and costs cycles fixing a button that was correct | mxcli deliberately writes `ParameterMappings` as an empty array for a page action, because Studio Pro infers the row object from the enclosing widget and rejects an explicit `$currentObject` Argument as CE0115 (#296). That decision was right; its other half was missing. `renderClientActionMDL` read only explicit mappings, and the writer's comment *asserted* DESCRIBE recovered the implicit one — it never did | `mdl/executor/cmd_pages_describe_output.go` (`pageActionParameters`, `targetPageParameterNames`), stale comment corrected in `sdk/mpr/writer_widgets_action.go` | **When a writer stores something implicitly, the reader owes it an explicit reconstruction — and a comment claiming the reader already does is worth nothing until a test says so.** Recover from the *target page's* declared parameters, not from the action, since that is where the information actually lives. Guard both ends: an explicit mapping still wins (recovery fills a gap, it does not override Studio Pro), and an unresolvable page yields no arguments rather than invented ones — a description that omits an argument is recoverable, one that names a parameter that does not exist is not. **A lossy DESCRIBE is costliest exactly when it is most used**, because DESCRIBE is what you reach for once you have stopped trusting the model. Tests `cmd_pages_describe_pageparams_test.go`; the control (explicit-only) reproduces the reported output verbatim. mxcli-formula1 §39 | | `create or modify odata service` silently revokes the service's access; the next build fails with "At least one allowed role must be selected for the published OData service to be accessible." Re-granting fixes it until the next modify | `serializePublishedODataService` never wrote `AllowedModuleRoles`. The document is serialized wholesale and written with `updateUnit`, so a field the serializer omits is not left alone — it is deleted. The grants were read correctly and carried through the executor, then dropped one layer down | `sdk/mpr/writer_odata.go` (`AllowedModuleRoles`, marker 1 / BY_NAME, matching the working `GRANT` path's `makeMendixStringArray`); stale executor comment corrected in `mdl/executor/cmd_odata.go` | **A wholesale re-serialization deletes every field it does not write, so the writer's field list is a data-retention policy.** Audit it against the parser, not against the struct — the round trip is the contract. **Where you look decides what you conclude**: an earlier pass looked for this loss at model level, found the value present and carried, and recorded "reported but does not reproduce" — the loss only exists after the BSON round trip, so a model-level check could never have seen it. When a report says a value disappears, reproduce at the persistence boundary before disbelieving it. For the marker, copy the shape from the code path that already works (`GRANT` writes marker 1 and builds fine) instead of reasoning from an unrelated type. Tests `writer_odata_test.go`; the control (field omitted again) reproduces the empty-grants document. mxcli-formula1 §26 | | `.ai-context/skills/` still carries the previous release's guidance after the mxcli binary is upgraded — binary rebuilt at 12:05, skills stamped the day before — with no warning that they disagree | The skills are embedded in the binary and written exactly once, by `mxcli init`. Nothing re-ran init on upgrade, and nothing compared what was on disk against what the binary carried | New `cmd/mxcli/init_skills_sync.go` (`syncAIContextSkills`, `reportSkillSync`), `--sync-skills` flag in `cmd/mxcli/init.go`, and a sync step in the SessionStart bootstrap in `cmd/mxcli/init_hook.go` | **Stale guidance is worse than missing guidance**, because an agent reads it with identical confidence either way — so the failure mode is silent and confident. Fix it where it is consumed, not where it is authored: the SessionStart bootstrap already runs on every session and can fetch the binary, so it is the one place guaranteed to execute immediately before an agent reads the files. Two properties keep an every-session job acceptable: **write only what differs** (an mtime that moves every session makes "when did this last change" unanswerable — a test asserts the mtime holds) and **stay silent when current**. Never fatal: a skills refresh must not block a session, hence `|| true`. A test asserts the bootstrap script actually calls it *before* the exec'd setup, since a step ordered after an `exec` never runs. Tests `init_skills_sync_test.go`; the control (write-once) leaves the stale file in place. mxcli-formula1 §16 | +| `publish entity … (TopSupported: No)` (or `SkipSupported`/`Countable`) parses, `DESCRIBE` reads it back as `No`, and the published `$metadata` still advertises `true`. A client then believes paging works when nothing implements it | `serializeEntitySet` hardcoded all three `QueryOptions` to `true`, ignoring the `*bool` fields the model already carried — the AST/model/DESCRIBE half of the feature shipped without the writer half | `sdk/mpr/writer_odata.go` (`boolOrTrue`, entity-set `QueryOptions`) | **A capability annotation on a microflow-backed resource is load-bearing, not decorative.** Mendix applies *no* query options to a read-microflow resource — it hands the request over and returns what comes back — so the annotation is the only thing a client has to go on, and an over-claim is not cosmetic: the client reads a whole collection believing it is a page. **Check the writer whenever a property round-trips correctly through DESCRIBE**: DESCRIBE reads the model, so a model-only feature describes perfectly and publishes wrong, and the round-trip test everyone reaches for cannot see it. A tri-state `*bool` needs an explicit nil policy at the boundary (`nil` = the platform default, only an explicit `false` opts out) or the pointer is pointless. Tests `writer_odata_test.go`; the control (hardcoded true) reproduces the over-claim. mxcli-formula1 §20 | diff --git a/sdk/mpr/writer_odata.go b/sdk/mpr/writer_odata.go index 2dc5ff74c..6e2e4f5f1 100644 --- a/sdk/mpr/writer_odata.go +++ b/sdk/mpr/writer_odata.go @@ -323,6 +323,10 @@ func (w *Writer) serializePublishedODataService(svc *model.PublishedODataService return marshalUnitIDFirst(doc) } +// boolOrTrue resolves a tri-state query option: nil keeps Mendix's default of +// true, and only an explicit false turns the capability off. +func boolOrTrue(p *bool) bool { return p == nil || *p } + // serializePublishedEntityType converts a PublishedEntityType to a BSON map. func serializePublishedEntityType(et *model.PublishedEntityType) bson.D { // Serialize child members. Pass the owning entity's qualified name so @@ -361,12 +365,19 @@ func serializePublishedEntitySet(es *model.PublishedEntitySet, entityTypeID stri // entity set to be considered valid. Without it the second // published entity in a multi-entity service fails to resolve // its key (CE6585) — see Studio Pro reference dump. + // nil means "not specified" and keeps Mendix's own default of true; only + // an explicit false turns one off. These were hardcoded true, so + // `publish entity … (TopSupported: No)` parsed, described back as No, and + // was published as Yes — mxcli asserting a capability the author had + // explicitly disowned. For a microflow-backed resource the claim is + // especially load-bearing: Mendix applies no query options itself, so the + // annotation is the only thing a client has to go on (mxcli-formula1 §20). {Key: "QueryOptions", Value: bson.D{ {Key: "$ID", Value: idToBsonBinary(generateUUID())}, {Key: "$Type", Value: "ODataPublish$QueryOptions"}, - {Key: "Countable", Value: true}, - {Key: "SkipSupported", Value: true}, - {Key: "TopSupported", Value: true}, + {Key: "Countable", Value: boolOrTrue(es.Countable)}, + {Key: "SkipSupported", Value: boolOrTrue(es.SkipSupported)}, + {Key: "TopSupported", Value: boolOrTrue(es.TopSupported)}, }}, } diff --git a/sdk/mpr/writer_odata_test.go b/sdk/mpr/writer_odata_test.go index f2d530258..8d2b3ea11 100644 --- a/sdk/mpr/writer_odata_test.go +++ b/sdk/mpr/writer_odata_test.go @@ -457,3 +457,72 @@ func TestSerializePublishedODataService_EmptyRolesStillWritesTheField(t *testing t.Errorf("expected the bare marker and no grants, got %v", arr) } } + +// The query-option annotations were hardcoded true, so `publish entity … +// (TopSupported: No)` parsed, described back as No, and published as Yes. +// +// For a microflow-backed resource this claim is load-bearing rather than +// decorative: Mendix applies no query options itself, so the annotation is the +// only thing a client has to go on — and a client that believes $top works, when +// nothing implements it, silently reads a whole collection as though it were a +// page (mxcli-formula1 §20). +func TestSerializePublishedODataService_HonoursQueryOptionOptOut(t *testing.T) { + no := false + yes := true + w := &Writer{} + svc := &model.PublishedODataService{ + BaseElement: model.BaseElement{ID: "svc-qo"}, + Name: "LiveAPI", + EntitySets: []*model.PublishedEntitySet{ + { + BaseElement: model.BaseElement{ID: "es-off"}, + ExposedName: "Drivers", + EntityTypeName: "M.Driver", + Countable: &no, + SkipSupported: &no, + TopSupported: &no, + }, + { + BaseElement: model.BaseElement{ID: "es-mixed"}, + ExposedName: "Races", + EntityTypeName: "M.Race", + Countable: &yes, + // SkipSupported/TopSupported unspecified: Mendix's default of true. + }, + }, + } + + data, err := w.serializePublishedODataService(svc) + if err != nil { + t.Fatalf("serialize failed: %v", err) + } + var raw map[string]any + if err := bson.Unmarshal(data, &raw); err != nil { + t.Fatalf("unmarshal failed: %v", err) + } + + sets := extractBsonArray(raw["EntitySets"]) + if len(sets) != 2 { + t.Fatalf("expected 2 entity sets, got %d", len(sets)) + } + opts := func(i int) map[string]any { + set, _ := sets[i].(map[string]any) + qo, _ := set["QueryOptions"].(map[string]any) + if qo == nil { + t.Fatalf("entity set %d has no QueryOptions", i) + } + return qo + } + + for _, key := range []string{"Countable", "SkipSupported", "TopSupported"} { + if v, _ := opts(0)[key].(bool); v { + t.Errorf("Drivers.%s = true, but the author said No — mxcli is advertising a capability nothing implements", key) + } + } + // Unspecified must still mean Mendix's default, not false. + for _, key := range []string{"Countable", "SkipSupported", "TopSupported"} { + if v, _ := opts(1)[key].(bool); !v { + t.Errorf("Races.%s = false; unspecified must keep Mendix's default of true", key) + } + } +} From 18795da840a87430dd54802736125ca4fb10d145 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 00:34:43 +0000 Subject: [PATCH 06/29] feat(check): flag a read microflow that cannot keep its resource's promises MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A published OData resource backed by a read microflow could silently return the wrong thing, twice over: - `?$top=5` returned the whole collection with a 200, because Mendix applies NO query options to a read-microflow resource — it hands over the request and returns what comes back. TopSupported/SkipSupported describe the microflow, not the platform, and default to true when unspecified. - a client re-reading a row it holds sends `?$filter=key eq '…'` unprompted. With no branch for it the request falls through to the collection default and the client adopts the FIRST row as that object's identity. No error: well-formed request, valid collection, correct $count, 200. Both are promises the service makes on the microflow's behalf, and nothing checked either. MDL-ODATA02 flags a declared KEY the microflow cannot answer; MDL-ODATA03 flags capabilities it cannot implement. The read path has no other way to be safe. Unlike an OData action or an insert/update/delete microflow, a read microflow has no System.HttpResponse parameter and cannot answer 400, so declaring `TopSupported: No` is its only substitute for the refusal it cannot send. That contract is now documented per capability in odata-data-sharing.md and in `mxcli syntax odata.publish`, which neither covered before. Both rules fire on one provable condition — the microflow takes no System.HttpRequest parameter, so it cannot see a key or a query option at all. A microflow that does take it gets the benefit of the doubt: proving which options it parses needs real analysis, and a rule that guesses gets switched off. The rule shipped dead once during development: the visitor stores ReadMode as `MICROFLOW Module.Name` upper-cased and the prefix match was case-sensitive. Caught by running it against a real script rather than a hand-built AST, and now pinned by its own test. Swept mdl-examples/ and the skill MDL blocks for false positives: 0 failures. mxcli-formula1 §37, §20 (suggested issues 2 and 3) --- .claude/skills/fix-issue.md | 1 + .claude/skills/mendix/odata-data-sharing.md | 107 ++++++++++ cmd/mxcli/cmd_check.go | 6 + cmd/mxcli/syntax/features_integration.go | 19 +- .../doctype-tests/10-odata-examples.mdl | 73 +++++++ mdl/executor/validate_odata_read_contract.go | 187 ++++++++++++++++++ .../validate_odata_read_contract_test.go | 143 ++++++++++++++ 7 files changed, 535 insertions(+), 1 deletion(-) create mode 100644 mdl/executor/validate_odata_read_contract.go create mode 100644 mdl/executor/validate_odata_read_contract_test.go diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index ecdca9568..dbf8db33b 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -429,3 +429,4 @@ extracting `OffsetExpression`/`LimitExpression`. | `create or modify odata service` silently revokes the service's access; the next build fails with "At least one allowed role must be selected for the published OData service to be accessible." Re-granting fixes it until the next modify | `serializePublishedODataService` never wrote `AllowedModuleRoles`. The document is serialized wholesale and written with `updateUnit`, so a field the serializer omits is not left alone — it is deleted. The grants were read correctly and carried through the executor, then dropped one layer down | `sdk/mpr/writer_odata.go` (`AllowedModuleRoles`, marker 1 / BY_NAME, matching the working `GRANT` path's `makeMendixStringArray`); stale executor comment corrected in `mdl/executor/cmd_odata.go` | **A wholesale re-serialization deletes every field it does not write, so the writer's field list is a data-retention policy.** Audit it against the parser, not against the struct — the round trip is the contract. **Where you look decides what you conclude**: an earlier pass looked for this loss at model level, found the value present and carried, and recorded "reported but does not reproduce" — the loss only exists after the BSON round trip, so a model-level check could never have seen it. When a report says a value disappears, reproduce at the persistence boundary before disbelieving it. For the marker, copy the shape from the code path that already works (`GRANT` writes marker 1 and builds fine) instead of reasoning from an unrelated type. Tests `writer_odata_test.go`; the control (field omitted again) reproduces the empty-grants document. mxcli-formula1 §26 | | `.ai-context/skills/` still carries the previous release's guidance after the mxcli binary is upgraded — binary rebuilt at 12:05, skills stamped the day before — with no warning that they disagree | The skills are embedded in the binary and written exactly once, by `mxcli init`. Nothing re-ran init on upgrade, and nothing compared what was on disk against what the binary carried | New `cmd/mxcli/init_skills_sync.go` (`syncAIContextSkills`, `reportSkillSync`), `--sync-skills` flag in `cmd/mxcli/init.go`, and a sync step in the SessionStart bootstrap in `cmd/mxcli/init_hook.go` | **Stale guidance is worse than missing guidance**, because an agent reads it with identical confidence either way — so the failure mode is silent and confident. Fix it where it is consumed, not where it is authored: the SessionStart bootstrap already runs on every session and can fetch the binary, so it is the one place guaranteed to execute immediately before an agent reads the files. Two properties keep an every-session job acceptable: **write only what differs** (an mtime that moves every session makes "when did this last change" unanswerable — a test asserts the mtime holds) and **stay silent when current**. Never fatal: a skills refresh must not block a session, hence `|| true`. A test asserts the bootstrap script actually calls it *before* the exec'd setup, since a step ordered after an `exec` never runs. Tests `init_skills_sync_test.go`; the control (write-once) leaves the stale file in place. mxcli-formula1 §16 | | `publish entity … (TopSupported: No)` (or `SkipSupported`/`Countable`) parses, `DESCRIBE` reads it back as `No`, and the published `$metadata` still advertises `true`. A client then believes paging works when nothing implements it | `serializeEntitySet` hardcoded all three `QueryOptions` to `true`, ignoring the `*bool` fields the model already carried — the AST/model/DESCRIBE half of the feature shipped without the writer half | `sdk/mpr/writer_odata.go` (`boolOrTrue`, entity-set `QueryOptions`) | **A capability annotation on a microflow-backed resource is load-bearing, not decorative.** Mendix applies *no* query options to a read-microflow resource — it hands the request over and returns what comes back — so the annotation is the only thing a client has to go on, and an over-claim is not cosmetic: the client reads a whole collection believing it is a page. **Check the writer whenever a property round-trips correctly through DESCRIBE**: DESCRIBE reads the model, so a model-only feature describes perfectly and publishes wrong, and the round-trip test everyone reaches for cannot see it. A tri-state `*bool` needs an explicit nil policy at the boundary (`nil` = the platform default, only an explicit `false` opts out) or the pointer is pointless. Tests `writer_odata_test.go`; the control (hardcoded true) reproduces the over-claim. mxcli-formula1 §20 | +| A published OData resource backed by a read microflow silently returns the wrong thing: `?$top=5` yields the whole collection with a 200, and a client re-reading a held row by key gets the collection default and adopts the FIRST row as that object's identity. No error anywhere — well-formed request, valid collection, correct `$count`, 200 | Two promises the service makes on the microflow's behalf and nothing checked: the `KEY` in `expose (…)`, and the `TopSupported`/`SkipSupported` annotations (which default to **true** when unspecified). Mendix applies no query options to a read-microflow resource — it hands over the request and returns what comes back | New `mdl/executor/validate_odata_read_contract.go` (MDL-ODATA02 key promise, MDL-ODATA03 capability over-claim), wired in `cmd/mxcli/cmd_check.go`; the response contract documented in `.claude/skills/mendix/odata-data-sharing.md` | **A read microflow cannot answer 400** — unlike an OData action or an insert/update/delete microflow, the read capability has no `System.HttpResponse` parameter ([docs](https://docs.mendix.com/refguide/published-odata-entity/#custom-http-response)). Its contract is therefore *declarative*: declaring `TopSupported: No` is the read path's only substitute for the refusal it cannot send. **Pick a trigger you can prove**: both rules fire only when the microflow takes no `System.HttpRequest` parameter, because then it provably cannot see a key or a query option; a microflow that does take it gets the benefit of the doubt, since proving *which* options it parses needs real analysis and a rule that guesses gets switched off. **Verify a check rule against a real parse, never a hand-built AST** — the visitor stores `ReadMode` as `MICROFLOW Module.Name` **upper-cased**, so a case-sensitive prefix match made the whole rule dead while it still looked right; the casing is now pinned by its own test. Tests `validate_odata_read_contract_test.go`, examples in `10-odata-examples.mdl` (both correct shapes: request-aware, and honestly declared). mxcli-formula1 §37/§20, suggested issues 2 and 3 | diff --git a/.claude/skills/mendix/odata-data-sharing.md b/.claude/skills/mendix/odata-data-sharing.md index a61793172..555172595 100644 --- a/.claude/skills/mendix/odata-data-sharing.md +++ b/.claude/skills/mendix/odata-data-sharing.md @@ -474,6 +474,113 @@ Two things worth knowing before you write this: `PublishAssociations` must stay at its default (Yes) here: a non-persistable entity cannot publish its ID, so object-id mode can never build for it. +## HTTP Status Codes and Errors: What Each Capability Can Do + +**The read path and the write path have different powers, and the difference is +the single most expensive thing to get wrong here.** Read this before designing +any microflow-backed resource. + +| Capability | Can set the HTTP status code? | How | +|---|---|---| +| OData **action** (published microflow) | **Yes** | add a `System.HttpResponse` parameter | +| Entity **Insertable / Updatable / Deletable** microflow | **Yes** | add a `System.HttpResponse` parameter | +| Entity **Readable** microflow | **No** | not offered — the read capability has no documented `HttpResponse` parameter | + +Sources: [published-odata-microflow §4](https://docs.mendix.com/refguide/published-odata-microflow/#4-customizing-the-outgoing-http-response), +[published-odata-entity, custom HTTP response](https://docs.mendix.com/refguide/published-odata-entity/#custom-http-response). +The custom-response section names Insertable, Updatable and Deletable; the +Readable section never references it. + +### Writing a status code (action / insert / update / delete) + +```sql +create microflow Api.InsertRow ( + $Row: Api.Row, + $HttpResponse: System.HttpResponse +) +begin + if $Row/RowKey = empty then + change $HttpResponse (StatusCode = 400, Content = '{"error":"rowKey is required"}'); + return; + end if; + ... +end; +``` + +Three rules the platform imposes: + +- **`ReasonPhrase` is ignored.** Setting it is dead code; put the explanation in + `Content`. +- **`204` always produces an empty body.** Setting `Content` alongside it is + discarded. +- **Changing status or content makes the whole response come from + `HttpResponse`** — headers included. Changing *only* headers merges them with + the defaults instead. +- `Transfer-Encoding` and `Date` cannot be changed. + +### The read path cannot refuse, so it must not over-promise + +A read microflow has no way to answer `400`. Its only exits are to throw (a +blunt `500`) or to return data. That has two consequences, and both are design +obligations rather than nice-to-haves: + +**1. Declare capabilities you do not implement as `No`.** Mendix applies *no* +query options to a read-microflow resource — it hands over the request and +returns whatever comes back — so `TopSupported` / `SkipSupported` / `Countable` +are claims about your microflow, not about the platform. A resource that +advertises `TopSupported: Yes` and ignores `$top` returns the entire collection +with a `200`, and the client believes it received a page. + +```sql +publish entity Api.Row as 'Rows' ( + ReadMode: microflow Api.Read_Rows, + -- Only claim what Read_Rows actually parses out of the URI: + TopSupported: No, + SkipSupported: No, + Countable: No +) +``` + +Declaring `No` is the read path's substitute for the `400` it cannot send. + +**2. Answer a lookup by your own KEY.** A client holding a row re-reads it by +key, unprompted, and Mendix's own OData client sends the `$filter` spelling: + +``` +?$filter=rowKey eq '1036-c' ← what the runtime actually sends +/Rows('1036-c') ← bare path key +/Rows(rowKey='1036-c') ← named path key +``` + +If the microflow parses only its collection filter, the key request falls through +to the collection default and the client adopts the **first row** as the identity +of the object it is displaying. There is no error: the request is well-formed, +the response is a valid collection, the count is right, the status is `200`. Two +different objects are then on screen at once, and nothing distinguishes them +until one travels to another page. + +So: `expose ( … (KEY) )` is a promise the *service* makes on the *microflow's* +behalf. Branch key → id → filter → default, or do not declare the KEY. + +The request itself always arrives on `System.HttpRequest`: + +```sql +create microflow Api.Read_Rows ( + $Request: System.HttpRequest, + $Response: System.ODataResponse -- required while Countable is Yes +) +returns List of Api.Row +begin + log info 'URI=' + $Request/Uri; -- the whole query string, URL-encoded + ... +end; +``` + +Mendix validates field names before the microflow runs (`$filter=secretColumn eq 'x'` +is a `400` from the platform), so the microflow only ever sees names that exist +in the published metadata. That is defence in depth, not a substitute for a +whitelist — it constrains the *name*, not what you do with it. + ## Step-by-Step: Read-Write API with Microflow Handlers For write operations (insert, update, delete), the OData service delegates to microflows that map between the view entity and the underlying persistent entities. diff --git a/cmd/mxcli/cmd_check.go b/cmd/mxcli/cmd_check.go index 89705bbb7..2c83275ab 100644 --- a/cmd/mxcli/cmd_check.go +++ b/cmd/mxcli/cmd_check.go @@ -183,6 +183,12 @@ Examples: // the model quietly lacked what the author asked for. violations = append(violations, executor.ValidateODataProperties(prog)...) + // Flag a microflow-backed OData resource whose read microflow cannot keep + // the promises the service makes for it. A read microflow has no + // System.HttpResponse parameter, so it cannot answer 400 — its contract + // has to be declared correctly up front, and nothing else checks that. + violations = append(violations, executor.ValidateODataReadContract(prog)...) + // Flag a page whose widgets point at a page created further down the same // script. `exec` resolves page references in statement order and is not // transactional, so this fails after earlier statements are already diff --git a/cmd/mxcli/syntax/features_integration.go b/cmd/mxcli/syntax/features_integration.go index 506a6e8a2..24e6e34d8 100644 --- a/cmd/mxcli/syntax/features_integration.go +++ b/cmd/mxcli/syntax/features_integration.go @@ -111,7 +111,24 @@ func init() { "--\n" + "-- While Countable is Yes (the default) the read microflow must take a\n" + "-- $Response: System.ODataResponse parameter and set its Count; with\n" + - "-- Countable: No it takes no parameters at all.", + "-- Countable: No it takes no parameters at all.\n" + + "\n" + + "-- HTTP STATUS CODES. An OData action, and an insert/update/delete\n" + + "-- microflow, may take a $HttpResponse: System.HttpResponse parameter and\n" + + "-- set StatusCode/Content. A READ microflow may not — it cannot answer\n" + + "-- 400, so its contract has to be declared correctly instead:\n" + + "--\n" + + "-- * Mendix applies NO query options to a read-microflow resource. It\n" + + "-- returns exactly what the microflow returns, so TopSupported and\n" + + "-- SkipSupported describe YOUR microflow. Leaving them unspecified\n" + + "-- publishes Yes. Declare No for anything you do not parse.\n" + + "-- * A declared KEY promises a lookup by that key. A client holding a\n" + + "-- row re-reads it as `?$filter=key eq '…'` on its own; answer it, or\n" + + "-- the client adopts the first row of your collection default as that\n" + + "-- object's identity, silently and with a 200.\n" + + "--\n" + + "-- Take a $Request: System.HttpRequest parameter to see the query string.\n" + + "-- MDL-ODATA02 and MDL-ODATA03 flag a read microflow that takes none.", Example: "create persistent entity Shop.Customer (\n" + " Email: string(200) unique error 'unique' required error 'required',\n" + " Name: string(200)\n" + diff --git a/mdl-examples/doctype-tests/10-odata-examples.mdl b/mdl-examples/doctype-tests/10-odata-examples.mdl index 4e63b5491..10655aa0e 100644 --- a/mdl-examples/doctype-tests/10-odata-examples.mdl +++ b/mdl-examples/doctype-tests/10-odata-examples.mdl @@ -539,6 +539,79 @@ begin end; / +-- ############################################################################ +-- LEVEL 8.75: A microflow-backed resource declares its own contract +-- ############################################################################ +-- +-- Mendix applies NO query options to a read-microflow resource: it hands the +-- request to the microflow and returns exactly what comes back. And unlike an +-- OData action or an insert/update/delete microflow, a read microflow has no +-- System.HttpResponse parameter, so it cannot answer 400 either. +-- +-- So the read path's contract is declarative. Two ways to be correct, both +-- shown here, and MDL-ODATA02/03 flag anything in between. + +create non-persistent entity OdTest.LiveRow ( RowKey: string(60), Label: string(200) ); + +-- (a) Take the request and honour it. mxcli cannot tell WHICH options a +-- microflow parses, so taking $Request is enough to earn its silence — the +-- branch key -> filter -> default is the author's job. +create microflow OdTest.Read_LiveRows ( + $Request: System.HttpRequest, + $Response: System.ODataResponse +) +returns List of OdTest.LiveRow as $Rows +begin + -- $Request/Uri carries the whole query string, URL-encoded: + -- ?$filter=rowKey eq '1036-c' <- a client re-reading a row it holds + -- ?$top=20&$skip=40 <- paging the client believes works + $Rows = create list of OdTest.LiveRow; + return $Rows; +end; +/ + +-- (b) Or declare an honest contract: no KEY to answer, no paging claimed. +create microflow OdTest.Read_AllRows () +returns List of OdTest.LiveRow as $Rows +begin + $Rows = create list of OdTest.LiveRow; + return $Rows; +end; +/ + +create odata service OdTest.LiveAPI ( + path: 'odata/live/', + version: '1.0.0', + ODataVersion: OData4, + namespace: 'OdTest.LiveAPI', + ServiceName: 'LiveAPI' +) +{ + publish entity OdTest.LiveRow as 'Rows' ( + ReadMode: microflow OdTest.Read_LiveRows, + InsertMode: not_supported, UpdateMode: not_supported, DeleteMode: not_supported + ) + expose ( RowKey as 'rowKey' (KEY), Label as 'label' ) +}; + +-- The honest variant: nothing is claimed, so nothing is promised. Note there is +-- no KEY — declaring one would promise a lookup Read_AllRows cannot answer. +create odata service OdTest.BulkAPI ( + path: 'odata/bulk/', + version: '1.0.0', + ODataVersion: OData4, + namespace: 'OdTest.BulkAPI', + ServiceName: 'BulkAPI' +) +{ + publish entity OdTest.LiveRow as 'AllRows' ( + ReadMode: microflow OdTest.Read_AllRows, + InsertMode: not_supported, UpdateMode: not_supported, DeleteMode: not_supported, + Countable: false, TopSupported: false, SkipSupported: false + ) + expose ( RowKey as 'rowKey', Label as 'label' ) +}; + -- ############################################################################ -- LEVEL 8.8: DROP (cleanup) -- ############################################################################ diff --git a/mdl/executor/validate_odata_read_contract.go b/mdl/executor/validate_odata_read_contract.go new file mode 100644 index 000000000..daeaa94f6 --- /dev/null +++ b/mdl/executor/validate_odata_read_contract.go @@ -0,0 +1,187 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Check-time validation of what a microflow-backed OData resource promises. +// +// A read microflow cannot refuse a request. Mendix hands it the raw request and +// returns whatever comes back, and — unlike an OData action or an +// insert/update/delete microflow — the read capability has no System.HttpResponse +// parameter, so there is no way to answer 400. Its only exits are to throw (a +// blunt 500) or to return data. +// +// That makes the read path's contract declarative: whatever it cannot do at +// request time has to be stated up front, in the published metadata. These rules +// check the statement against the microflow it names. +// +// Both fire on one provable condition — the microflow does not take a +// System.HttpRequest parameter — because without the request it cannot see a +// key, a $filter, a $top or a $skip at all. A microflow that does take the +// request gets the benefit of the doubt: proving *which* options it parses would +// need real analysis, and a rule that guesses is a rule people switch off. +// +// mxcli-formula1 §37 (the KEY promise) and §20 (capabilities Mendix does not +// apply itself). +package executor + +import ( + "fmt" + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +const ( + // httpRequestType is the parameter that carries the query string. + httpRequestType = "System.HttpRequest" + // odataDocsCustomResponse is the reference for what each capability may do. + odataDocsCustomResponse = "https://docs.mendix.com/refguide/published-odata-entity/#custom-http-response" +) + +// ValidateODataReadContract flags read microflows that cannot keep the promises +// their published resource makes for them. +func ValidateODataReadContract(prog *ast.Program) []linter.Violation { + if prog == nil { + return nil + } + flows := microflowsByName(prog) + + var out []linter.Violation + for _, stmt := range prog.Statements { + svc, ok := stmt.(*ast.CreateODataServiceStmt) + if !ok { + continue + } + for _, e := range svc.Entities { + if e == nil { + continue + } + mf, named := readModeMicroflow(e.ReadMode) + if !named { + continue // ReadFromDatabase and friends: Mendix does the work + } + // Only a microflow defined in this same script can be inspected. A + // reference to one that already exists in the project is not + // evidence of anything, so say nothing. + decl, found := flows[strings.ToLower(mf)] + if !found || takesHTTPRequest(decl) { + continue + } + where := fmt.Sprintf("publish entity %s as %q in %s", + e.Entity.String(), e.ExposedName, svc.Name.String()) + out = append(out, keyPromiseViolations(where, e, mf)...) + out = append(out, capabilityViolations(where, e, mf)...) + } + } + return out +} + +// keyPromiseViolations flags a declared KEY the read microflow is never told +// about (MDL-ODATA02). +func keyPromiseViolations(where string, e *ast.PublishedEntityDef, mf string) []linter.Violation { + var keys []string + for _, m := range e.Members { + if m == nil || !m.IsPartOfKey { + continue + } + // The exposed name is what a client filters on, so that is the name to + // put in the message; fall back to the model name when none was given. + name := m.ExposedName + if name == "" { + name = m.Name + } + keys = append(keys, name) + } + if len(keys) == 0 { + return nil + } + return []linter.Violation{{ + RuleID: "MDL-ODATA02", + Severity: linter.SeverityWarning, + Message: fmt.Sprintf("%s: declares KEY %s, but %s never sees the request (no %s parameter) and so cannot answer a lookup by that key", + where, strings.Join(keys, ", "), mf, httpRequestType), + Suggestion: fmt.Sprintf( + "A client holding a row re-reads it by key on its own — Mendix's OData client sends `?$filter=%s eq '…'`. "+ + "With no branch for it the request falls through to the collection default, the client adopts the FIRST row as that object's identity, and there is no error: valid collection, right count, 200. "+ + "Give %s a `$Request: %s` parameter and branch key → filter → default, or drop the KEY.", + keys[0], mf, httpRequestType), + }} +} + +// capabilityViolations flags query options advertised to clients that the read +// microflow cannot implement (MDL-ODATA03). +func capabilityViolations(where string, e *ast.PublishedEntityDef, mf string) []linter.Violation { + // nil means "not specified", which publishes Mendix's default of true — so + // silence is a claim, and that is exactly what makes this worth flagging. + var claimed []string + if boolOrTrueAST(e.TopSupported) { + claimed = append(claimed, "TopSupported") + } + if boolOrTrueAST(e.SkipSupported) { + claimed = append(claimed, "SkipSupported") + } + if len(claimed) == 0 { + return nil + } + return []linter.Violation{{ + RuleID: "MDL-ODATA03", + Severity: linter.SeverityWarning, + Message: fmt.Sprintf("%s: advertises %s, but %s never sees the request (no %s parameter), so no paging is applied", + where, strings.Join(claimed, " and "), mf, httpRequestType), + Suggestion: fmt.Sprintf( + "Mendix applies no query options to a read-microflow resource — it returns exactly what the microflow returns — so these annotations describe %s, not the platform. "+ + "A client asking for $top=5 gets the whole collection with a 200 and believes it received a page. "+ + "Either parse them from `$Request/Uri`, or declare `TopSupported: No, SkipSupported: No`. "+ + "Declaring No is the read path's substitute for the 400 it cannot send: the read capability has no System.HttpResponse parameter (%s).", + mf, odataDocsCustomResponse), + }} +} + +// microflowsByName indexes the microflows this script defines, lower-cased. +func microflowsByName(prog *ast.Program) map[string]*ast.CreateMicroflowStmt { + out := map[string]*ast.CreateMicroflowStmt{} + for _, stmt := range prog.Statements { + if mf, ok := stmt.(*ast.CreateMicroflowStmt); ok { + out[strings.ToLower(mf.Name.String())] = mf + } + } + return out +} + +// readModeMicroflow extracts the microflow name from a ReadMode, and reports +// whether the mode names one at all. +// +// The visitor stores this as `MICROFLOW Module.Name`, upper-cased, so the prefix +// is matched case-insensitively — a case-sensitive check silently matched +// nothing and the whole rule was dead, which is why it is verified against a +// real parse rather than a hand-built AST. +func readModeMicroflow(readMode string) (string, bool) { + trimmed := strings.TrimSpace(readMode) + const prefix = "microflow" + if len(trimmed) < len(prefix) || !strings.EqualFold(trimmed[:len(prefix)], prefix) { + return "", false + } + name := strings.TrimSpace(trimmed[len(prefix):]) + return name, name != "" +} + +// takesHTTPRequest reports whether a microflow can see the request at all. A +// System.* parameter is a qualified name, which the parser cannot tell apart +// from an enumeration, so both refs are checked (see CLAUDE.md on the +// TypeEnumeration/TypeEntity ambiguity). +func takesHTTPRequest(mf *ast.CreateMicroflowStmt) bool { + if mf == nil { + return false + } + for _, p := range mf.Parameters { + for _, ref := range []*ast.QualifiedName{p.Type.EntityRef, p.Type.EnumRef} { + if ref != nil && strings.EqualFold(ref.String(), httpRequestType) { + return true + } + } + } + return false +} + +// boolOrTrueAST resolves a tri-state query option: nil publishes Mendix's +// default of true. +func boolOrTrueAST(p *bool) bool { return p == nil || *p } diff --git a/mdl/executor/validate_odata_read_contract_test.go b/mdl/executor/validate_odata_read_contract_test.go new file mode 100644 index 000000000..0ffd64a91 --- /dev/null +++ b/mdl/executor/validate_odata_read_contract_test.go @@ -0,0 +1,143 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +// readContractScript builds a service whose read microflow takes params and +// whose published entity carries entityProps and members. +func readContractScript(params, entityProps, members string) string { + return `create module T; +create non-persistent entity T.Row (K: string(20)); + +CREATE MICROFLOW T.Read_Rows (` + params + `) + RETURNS List of T.Row AS $Rows +BEGIN + $Rows = CREATE LIST OF T.Row; + RETURN $Rows; +END; + +create odata service T.Api ( + path: 'odata/t/', + version: '1.0.0', + ODataVersion: OData4, + namespace: 'T.Api', + ServiceName: 'Api' +) +{ + publish entity T.Row as 'Rows' ( + ReadMode: microflow T.Read_Rows, + InsertMode: not_supported, UpdateMode: not_supported, DeleteMode: not_supported` + entityProps + ` + ) + expose ( ` + members + ` ) +}; +` +} + +func odataReadRuleIDs(t *testing.T, script string) []string { + t.Helper() + prog, errs := visitor.Build(script) + if len(errs) > 0 { + t.Fatalf("parsing the script: %v", errs) + } + var ids []string + for _, v := range ValidateODataReadContract(prog) { + ids = append(ids, v.RuleID) + } + return ids +} + +func hasODataReadRule(ids []string, id string) bool { + for _, got := range ids { + if got == id { + return true + } + } + return false +} + +// mxcli-formula1 §37: the resource declares a KEY, the read microflow is never +// told about it, and a client holding a row re-reads it by key — gets the +// collection default, and adopts the first row as that object's identity. No +// error: valid collection, right count, 200. Fifteen restart cycles. +func TestODataReadContract_FlagsAKeyTheMicroflowCannotAnswer(t *testing.T) { + ids := odataReadRuleIDs(t, readContractScript( + "$Response: System.ODataResponse", "", "K as 'k' (KEY)")) + if !hasODataReadRule(ids, "MDL-ODATA02") { + t.Errorf("the unanswerable KEY was not flagged, got %v", ids) + } +} + +// §20: Mendix applies no query options to a read-microflow resource, so an +// unspecified TopSupported still publishes `true` and the client believes it +// received a page when it received the whole collection. +func TestODataReadContract_FlagsCapabilitiesNothingImplements(t *testing.T) { + ids := odataReadRuleIDs(t, readContractScript( + "$Response: System.ODataResponse", "", "K as 'k'")) + if !hasODataReadRule(ids, "MDL-ODATA03") { + t.Errorf("the unimplementable paging claim was not flagged, got %v", ids) + } + if hasODataReadRule(ids, "MDL-ODATA02") { + t.Errorf("no KEY was declared, so the key rule must stay quiet: %v", ids) + } +} + +// A microflow that takes the request gets the benefit of the doubt. Proving +// WHICH options it parses would need real analysis, and a rule that guesses is +// a rule people switch off. +func TestODataReadContract_SilentWhenTheMicroflowSeesTheRequest(t *testing.T) { + ids := odataReadRuleIDs(t, readContractScript( + "$Request: System.HttpRequest, $Response: System.ODataResponse", "", "K as 'k' (KEY)")) + if len(ids) != 0 { + t.Errorf("a request-aware microflow must not be flagged, got %v", ids) + } +} + +// An honest contract — no KEY, capabilities declared off — is the other way to +// be correct, and is the only one available to a microflow that cannot parse a +// URI. It must be silent, or the rule punishes the fix it recommends. +func TestODataReadContract_SilentWhenTheContractIsHonest(t *testing.T) { + ids := odataReadRuleIDs(t, readContractScript( + "", ",\n Countable: false, TopSupported: false, SkipSupported: false", "K as 'k'")) + if len(ids) != 0 { + t.Errorf("an honestly declared resource must not be flagged, got %v", ids) + } +} + +// ReadFromDatabase is Mendix's own implementation; the microflow rules do not +// apply to it at all. +func TestODataReadContract_IgnoresNonMicroflowReadModes(t *testing.T) { + script := strings.Replace( + readContractScript("", "", "K as 'k' (KEY)"), + "ReadMode: microflow T.Read_Rows", "ReadMode: source", 1) + if ids := odataReadRuleIDs(t, script); len(ids) != 0 { + t.Errorf("a database-backed resource must not be flagged, got %v", ids) + } +} + +// A microflow this script does not define cannot be inspected — it may well +// take the request. Silence, not a guess. +func TestODataReadContract_SilentWhenTheMicroflowIsNotInThisScript(t *testing.T) { + script := strings.Replace( + readContractScript("$Response: System.ODataResponse", "", "K as 'k' (KEY)"), + "ReadMode: microflow T.Read_Rows", "ReadMode: microflow Other.Read_Elsewhere", 1) + if ids := odataReadRuleIDs(t, script); len(ids) != 0 { + t.Errorf("an unknown microflow must not be flagged, got %v", ids) + } +} + +// The visitor stores ReadMode as `MICROFLOW Module.Name`, upper-cased. A +// case-sensitive prefix match made the whole rule dead while every unit test +// built on a hand-made AST still passed, so the casing is pinned here. +func TestODataReadContract_MatchesTheVisitorsReadModeCasing(t *testing.T) { + name, named := readModeMicroflow("MICROFLOW T.Read_Rows") + if !named || name != "T.Read_Rows" { + t.Errorf("readModeMicroflow(%q) = (%q, %v); the visitor writes it upper-cased", + "MICROFLOW T.Read_Rows", name, named) + } +} From 6aa7c33ae665a936393f371d630f4a5bdb109139 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 00:52:57 +0000 Subject: [PATCH 07/29] feat(log): drive the runtime's per-node log levels MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Everything logs at INFO, so the detail you need is usually not in the log at all — and raising the whole runtime to TRACE is unusable on a busy app. The M2EE admin API has exposed per-node levels all along; nothing in mxcli drove them. mxcli log list [--filter x] [--json] mxcli log set mxcli log set A=TRACE B=DEBUG # one admin call, applied together This was proposed as `mxcli odata trace`, for "what is my published resource being asked?". It is deliberately not that: set_log_level takes a LIST of nodes and the runtime reports 57 of them, so the primitive is subsystem-agnostic and a per-subsystem command would have wrapped it one subsystem at a time. The OData knowledge lives in the skill instead — including the finding that there is NO log node for a published OData service (ODataConsume is the client side), so that particular question still needs a LOG in the read microflow. That gap is Mendix's. Every API fact was probed against a live 11.12.1 runtime, because the HTTP response for an AdminException says only "See logging output for details" — the real message is in the runtime log: - get_log_settings requires one of node/subscriber/sort in params - sort accepts exactly "node" and "subscriber" - set_log_level takes {"nodes":[{name,level}],"force":bool} - force means "allow a node that does not exist yet", and permanently registers the name — so it is opt-in and an unknown node is an error by default, making a typo an error rather than a setting that never applies - an invalid level is refused A level typo is caught locally with the valid set named, rather than becoming an AdminException whose detail the caller never sees. "Cannot reach the admin API" is distinguished from "the runtime refused the request", so the connection hint does not appear on an unknown-node error and bury the sentence that matters. Verified end-to-end against a booted runtime: both argument forms, multi-node in one call, a typo refused, --force accepted, and an unreachable port. mxcli-formula1 suggested issue 4 --- .claude/skills/fix-issue.md | 1 + .claude/skills/mendix/analyze-runtime.md | 43 +++++ cmd/mxcli/cmd_log.go | 203 +++++++++++++++++++++++ cmd/mxcli/cmd_log_test.go | 84 ++++++++++ cmd/mxcli/docker/loglevels.go | 156 +++++++++++++++++ cmd/mxcli/docker/loglevels_test.go | 57 +++++++ 6 files changed, 544 insertions(+) create mode 100644 cmd/mxcli/cmd_log.go create mode 100644 cmd/mxcli/cmd_log_test.go create mode 100644 cmd/mxcli/docker/loglevels.go create mode 100644 cmd/mxcli/docker/loglevels_test.go diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index dbf8db33b..f8aedabe7 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -430,3 +430,4 @@ extracting `OffsetExpression`/`LimitExpression`. | `.ai-context/skills/` still carries the previous release's guidance after the mxcli binary is upgraded — binary rebuilt at 12:05, skills stamped the day before — with no warning that they disagree | The skills are embedded in the binary and written exactly once, by `mxcli init`. Nothing re-ran init on upgrade, and nothing compared what was on disk against what the binary carried | New `cmd/mxcli/init_skills_sync.go` (`syncAIContextSkills`, `reportSkillSync`), `--sync-skills` flag in `cmd/mxcli/init.go`, and a sync step in the SessionStart bootstrap in `cmd/mxcli/init_hook.go` | **Stale guidance is worse than missing guidance**, because an agent reads it with identical confidence either way — so the failure mode is silent and confident. Fix it where it is consumed, not where it is authored: the SessionStart bootstrap already runs on every session and can fetch the binary, so it is the one place guaranteed to execute immediately before an agent reads the files. Two properties keep an every-session job acceptable: **write only what differs** (an mtime that moves every session makes "when did this last change" unanswerable — a test asserts the mtime holds) and **stay silent when current**. Never fatal: a skills refresh must not block a session, hence `|| true`. A test asserts the bootstrap script actually calls it *before* the exec'd setup, since a step ordered after an `exec` never runs. Tests `init_skills_sync_test.go`; the control (write-once) leaves the stale file in place. mxcli-formula1 §16 | | `publish entity … (TopSupported: No)` (or `SkipSupported`/`Countable`) parses, `DESCRIBE` reads it back as `No`, and the published `$metadata` still advertises `true`. A client then believes paging works when nothing implements it | `serializeEntitySet` hardcoded all three `QueryOptions` to `true`, ignoring the `*bool` fields the model already carried — the AST/model/DESCRIBE half of the feature shipped without the writer half | `sdk/mpr/writer_odata.go` (`boolOrTrue`, entity-set `QueryOptions`) | **A capability annotation on a microflow-backed resource is load-bearing, not decorative.** Mendix applies *no* query options to a read-microflow resource — it hands the request over and returns what comes back — so the annotation is the only thing a client has to go on, and an over-claim is not cosmetic: the client reads a whole collection believing it is a page. **Check the writer whenever a property round-trips correctly through DESCRIBE**: DESCRIBE reads the model, so a model-only feature describes perfectly and publishes wrong, and the round-trip test everyone reaches for cannot see it. A tri-state `*bool` needs an explicit nil policy at the boundary (`nil` = the platform default, only an explicit `false` opts out) or the pointer is pointless. Tests `writer_odata_test.go`; the control (hardcoded true) reproduces the over-claim. mxcli-formula1 §20 | | A published OData resource backed by a read microflow silently returns the wrong thing: `?$top=5` yields the whole collection with a 200, and a client re-reading a held row by key gets the collection default and adopts the FIRST row as that object's identity. No error anywhere — well-formed request, valid collection, correct `$count`, 200 | Two promises the service makes on the microflow's behalf and nothing checked: the `KEY` in `expose (…)`, and the `TopSupported`/`SkipSupported` annotations (which default to **true** when unspecified). Mendix applies no query options to a read-microflow resource — it hands over the request and returns what comes back | New `mdl/executor/validate_odata_read_contract.go` (MDL-ODATA02 key promise, MDL-ODATA03 capability over-claim), wired in `cmd/mxcli/cmd_check.go`; the response contract documented in `.claude/skills/mendix/odata-data-sharing.md` | **A read microflow cannot answer 400** — unlike an OData action or an insert/update/delete microflow, the read capability has no `System.HttpResponse` parameter ([docs](https://docs.mendix.com/refguide/published-odata-entity/#custom-http-response)). Its contract is therefore *declarative*: declaring `TopSupported: No` is the read path's only substitute for the refusal it cannot send. **Pick a trigger you can prove**: both rules fire only when the microflow takes no `System.HttpRequest` parameter, because then it provably cannot see a key or a query option; a microflow that does take it gets the benefit of the doubt, since proving *which* options it parses needs real analysis and a rule that guesses gets switched off. **Verify a check rule against a real parse, never a hand-built AST** — the visitor stores `ReadMode` as `MICROFLOW Module.Name` **upper-cased**, so a case-sensitive prefix match made the whole rule dead while it still looked right; the casing is now pinned by its own test. Tests `validate_odata_read_contract_test.go`, examples in `10-odata-examples.mdl` (both correct shapes: request-aware, and honestly declared). mxcli-formula1 §37/§20, suggested issues 2 and 3 | +| No way to see what a running app's subsystem is doing: everything logs at `INFO`, the detail is not in the log at all, and raising the whole runtime to `TRACE` is unusable on a busy app | Nothing in mxcli drove the runtime's per-node log levels, though the M2EE admin API has exposed them all along | New `cmd/mxcli/docker/loglevels.go` (`GetLogSettings`, `SetLogLevels`, `NormalizeLogLevel`, `MatchLogNodes`), `cmd/mxcli/cmd_log.go` (`mxcli log list` / `log set`), documented in `.claude/skills/mendix/analyze-runtime.md` | **Probe an undocumented API before designing the command around it.** Every fact here came from a live 11.12.1 runtime: `get_log_settings` requires one of `node`/`subscriber`/`sort`; `sort` accepts only `node` and `subscriber`; `set_log_level` takes `{"nodes":[{name,level}],"force":bool}`. **The HTTP response for an AdminException says only "See logging output for details" — the actual message ("Please specify node, subscriber or sort option in params", "Unknown sort option: name", "Unknown LogNode X. Use the 'force' parameter…") is in the runtime log**, so probe with the runtime log open or you learn nothing. `force` means "allow a node that does not exist yet" and **permanently registers the name**, so a typo becomes a real empty node — hence it is opt-in, and an unknown node is an error by default. **Resist the subsystem-specific command**: this was proposed as `mxcli odata trace`, but `set_log_level` takes a list of nodes and the runtime reports 57, so the primitive is generic and the OData knowledge belongs in docs. Distinguish "cannot reach the admin API" from "the runtime refused the request" (`ErrAdminUnreachable`) or the wrong hint buries the real one. Tests `cmd_log_test.go`, `loglevels_test.go`; verified end-to-end against a booted runtime (both arg forms, multi-node, typo refused, `--force` accepted, unreachable port). mxcli-formula1 suggested issue 4 | diff --git a/.claude/skills/mendix/analyze-runtime.md b/.claude/skills/mendix/analyze-runtime.md index b2f2397ea..e16d6b977 100644 --- a/.claude/skills/mendix/analyze-runtime.md +++ b/.claude/skills/mendix/analyze-runtime.md @@ -44,6 +44,49 @@ Gotchas: - A spike in "Executing N database synchronization command(s)" on an *unchanged* model is a red flag (see the `create or modify` data-loss class of bug). +### Turning up one subsystem: `mxcli log` + +Everything logs at `INFO` by default, so the detail you need usually is not in the +file at all — and raising the whole runtime to `TRACE` is unusable. Logging is +publish/subscribe: code publishes to a named **LogNode**, and each node has its +own level. + +```bash +mxcli log list # every node and its level (57 on a blank 11.12 app) +mxcli log list --filter connectionbus # narrow it — nobody remembers the exact names +mxcli log set ConnectionBus_Queries TRACE +mxcli log set ConnectionBus_Queries=TRACE Connector=DEBUG # one admin call +mxcli log set ConnectionBus_Queries INFO # put it back +``` + +Levels: `NONE CRITICAL ERROR WARNING INFO DEBUG TRACE`. + +This needs a **running** app (it goes through the M2EE admin API), and the change +lasts as long as the process — it is a debugging knob, not project configuration. + +Nodes worth knowing: + +| Question | Node | +|---|---| +| What SQL is being run | `ConnectionBus_Queries` (and `_Retrieve`, `_Update`) | +| Database sync at startup | `ConnectionBus_Synchronize` | +| Consumed OData / REST calls | `ODataConsume`, `REST Consume` | +| Microflow execution | `MicroflowEngine`, `ActionManager` | +| Scheduled events / queues | `SystemTask`, `TaskQueue` | +| Java/JS action wiring | `Connector` | + +**`--force` creates the node, permanently.** Without it an unknown node is refused, +which is what you want — a typo should be an error. With it the name is registered +for the life of the process, so a typo becomes a real (empty) node that shows up in +`log list` from then on. Use it only to pre-register a node that has not published +yet. + +**There is no log node for a *published* OData service.** `ODataConsume` is the +client side only. So "what is my published resource being asked?" cannot be +answered by raising a node — put `LOG INFO 'URI=' + $Request/Uri` in the read +microflow instead (see `odata-data-sharing.md`). That gap is Mendix's, not +mxcli's. + ## 2. Metrics — throughput and database pressure `--metrics` registers a Prometheus registry, served at diff --git a/cmd/mxcli/cmd_log.go b/cmd/mxcli/cmd_log.go new file mode 100644 index 000000000..a86bfcf9f --- /dev/null +++ b/cmd/mxcli/cmd_log.go @@ -0,0 +1,203 @@ +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "encoding/json" + "errors" + "fmt" + "os" + "strings" + "text/tabwriter" + + "github.com/mendixlabs/mxcli/cmd/mxcli/docker" + "github.com/spf13/cobra" +) + +// cmd_log.go exposes the runtime's log-node levels. +// +// This started life as a proposal for `mxcli odata trace`, to answer "what is my +// published OData resource actually being asked?" — a question that cost an +// afternoon because the URI is only visible at TRACE across the whole runtime. +// But `set_log_level` is subsystem-agnostic: it takes a list of nodes, and the +// runtime this was built against reports 57 of them. A per-subsystem command +// would have wrapped a generic primitive one subsystem at a time. +// +// So the command is generic, and the OData-specific knowledge (which node to +// raise) lives in the skill instead. + +var logCmd = &cobra.Command{ + Use: "log", + Short: "Inspect and set the runtime's log levels", + Long: `Read and change the log level of the running app's log nodes. + +Logging inside the Mendix runtime is publish/subscribe: code publishes to a +named LogNode, and a subscriber records what it receives at or above that node's +level. Raising one node is how you see a subsystem's detail without drowning in +everything else — TRACE across the whole runtime is unusable on a busy app. + +Levels, most to least severe: + NONE CRITICAL ERROR WARNING INFO DEBUG TRACE + +This talks to the M2EE admin API, so it needs a running app whose admin port is +reachable — typically one started by 'mxcli run --local'. + +Examples: + # What nodes exist, and at what level + mxcli log list + + # Just the ones that look relevant + mxcli log list --filter connectionbus + + # Raise one node, then put it back + mxcli log set ConnectionBus_Queries TRACE + mxcli log set ConnectionBus_Queries INFO + + # Several at once — one admin call, applied together + mxcli log set ConnectionBus_Queries=TRACE Connector=DEBUG + + # A node that has not published yet (Mendix allows pre-registering a level) + mxcli log set MyModule.MyNode TRACE --force`, +} + +var logListCmd = &cobra.Command{ + Use: "list", + Short: "List log nodes and their current levels", + Run: func(cmd *cobra.Command, args []string) { + filter, _ := cmd.Flags().GetString("filter") + asJSON, _ := cmd.Flags().GetBool("json") + + levels, err := docker.GetLogSettings(logAdminOptions(cmd)) + if err != nil { + fmt.Fprintf(os.Stderr, "Error: %v\n%s", err, logConnectionHint(cmd, err)) + os.Exit(1) + } + levels = docker.MatchLogNodes(levels, filter) + + if asJSON { + out, _ := json.MarshalIndent(levels, "", " ") + fmt.Println(string(out)) + return + } + if len(levels) == 0 { + if filter != "" { + fmt.Printf("No log node matches %q.\n", filter) + } else { + fmt.Println("No log nodes reported.") + } + return + } + w := tabwriter.NewWriter(os.Stdout, 0, 0, 2, ' ', 0) + fmt.Fprintln(w, "NODE\tLEVEL\tSUBSCRIBER") + for _, l := range levels { + fmt.Fprintf(w, "%s\t%s\t%s\n", l.Node, l.Level, l.Subscriber) + } + w.Flush() + fmt.Printf("\n(%d node/subscriber pair(s))\n", len(levels)) + }, +} + +var logSetCmd = &cobra.Command{ + Use: "set | =...", + Short: "Set the log level of one or more nodes", + Long: `Set the level of one or more log nodes, in a single admin call. + +Two spellings, because both read naturally: + + mxcli log set ConnectionBus_Queries TRACE + mxcli log set ConnectionBus_Queries=TRACE Connector=DEBUG + +An unknown node is refused, so a typo is an error rather than a setting that +silently never takes effect. Pass --force to set a level for a node that does +not exist yet — Mendix supports pre-registering one, which is how you capture a +subsystem's very first messages.`, + Args: cobra.MinimumNArgs(1), + Run: func(cmd *cobra.Command, args []string) { + force, _ := cmd.Flags().GetBool("force") + + levels, err := parseLogSetArgs(args) + if err != nil { + fmt.Fprintf(os.Stderr, "Error: %v\n", err) + os.Exit(1) + } + if err := docker.SetLogLevels(logAdminOptions(cmd), levels, force); err != nil { + fmt.Fprintf(os.Stderr, "Error: %v\n%s", err, logConnectionHint(cmd, err)) + os.Exit(1) + } + for _, l := range levels { + up, _ := docker.NormalizeLogLevel(l.Level) + fmt.Printf("%s -> %s\n", l.Node, up) + } + }, +} + +// parseLogSetArgs accepts either ` ` or repeated `=`. +// Mixing the two is refused rather than guessed at: `log set A=TRACE B DEBUG` +// has two readings and neither is obviously right. +func parseLogSetArgs(args []string) ([]docker.LogNodeLevel, error) { + hasPairs := false + for _, a := range args { + if strings.Contains(a, "=") { + hasPairs = true + } + } + + if !hasPairs { + if len(args) != 2 { + return nil, fmt.Errorf("expected ' ' or '=...', got %d argument(s)", len(args)) + } + if _, err := docker.NormalizeLogLevel(args[1]); err != nil { + return nil, err + } + return []docker.LogNodeLevel{{Node: args[0], Level: args[1]}}, nil + } + + var out []docker.LogNodeLevel + for _, a := range args { + node, level, ok := strings.Cut(a, "=") + if !ok || node == "" || level == "" { + return nil, fmt.Errorf("%q is not '=' — do not mix the two forms in one command", a) + } + if _, err := docker.NormalizeLogLevel(level); err != nil { + return nil, err + } + out = append(out, docker.LogNodeLevel{Node: node, Level: level}) + } + return out, nil +} + +func logAdminOptions(cmd *cobra.Command) docker.M2EEOptions { + host, _ := cmd.Flags().GetString("admin-host") + port, _ := cmd.Flags().GetInt("admin-port") + pass, _ := cmd.Flags().GetString("admin-pass") + return docker.M2EEOptions{Host: host, Port: port, Token: pass, Direct: true} +} + +// logConnectionHint names the most likely cause when nothing is listening — but +// ONLY then. The runtime rejecting a request (an unknown node, a bad level) is +// not a connection problem, and telling someone to start an app they clearly +// already have running buries the sentence that matters. +func logConnectionHint(cmd *cobra.Command, err error) string { + if !errors.Is(err, docker.ErrAdminUnreachable) { + return "" + } + host, _ := cmd.Flags().GetString("admin-host") + port, _ := cmd.Flags().GetInt("admin-port") + return fmt.Sprintf(" Log levels come from a RUNNING app's admin API (%s:%d).\n"+ + " Start one with 'mxcli run --local -p ', or point at another with --admin-host/--admin-port/--admin-pass.\n", + host, port) +} + +func init() { + logCmd.PersistentFlags().String("admin-host", "127.0.0.1", "M2EE admin API host") + logCmd.PersistentFlags().Int("admin-port", 8090, "M2EE admin API port") + logCmd.PersistentFlags().String("admin-pass", envOr("MXCLI_ADMIN_PASS", "mxcli-local-dev"), "M2EE admin password") + + logListCmd.Flags().String("filter", "", "Only nodes whose name contains this (case-insensitive)") + logListCmd.Flags().Bool("json", false, "Output as JSON") + logSetCmd.Flags().Bool("force", false, "Allow a node the runtime does not know yet (it is refused otherwise)") + + logCmd.AddCommand(logListCmd) + logCmd.AddCommand(logSetCmd) + rootCmd.AddCommand(logCmd) +} diff --git a/cmd/mxcli/cmd_log_test.go b/cmd/mxcli/cmd_log_test.go new file mode 100644 index 000000000..87a710e09 --- /dev/null +++ b/cmd/mxcli/cmd_log_test.go @@ -0,0 +1,84 @@ +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "strings" + "testing" +) + +// Both spellings exist because both read naturally, and the two-argument form is +// what anyone types first. +func TestParseLogSetArgs_BothForms(t *testing.T) { + got, err := parseLogSetArgs([]string{"ConnectionBus_Queries", "TRACE"}) + if err != nil { + t.Fatalf("two-arg form: %v", err) + } + if len(got) != 1 || got[0].Node != "ConnectionBus_Queries" || got[0].Level != "TRACE" { + t.Errorf("two-arg form parsed as %+v", got) + } + + got, err = parseLogSetArgs([]string{"A=TRACE", "B=debug"}) + if err != nil { + t.Fatalf("pair form: %v", err) + } + if len(got) != 2 || got[0].Node != "A" || got[1].Node != "B" || got[1].Level != "debug" { + t.Errorf("pair form parsed as %+v", got) + } +} + +// `log set A=TRACE B DEBUG` has two readings and neither is obviously right, so +// it is refused rather than guessed at. +func TestParseLogSetArgs_RefusesMixedForms(t *testing.T) { + _, err := parseLogSetArgs([]string{"A=TRACE", "B", "DEBUG"}) + if err == nil { + t.Fatal("mixed forms should be refused") + } + if !strings.Contains(err.Error(), "do not mix") { + t.Errorf("the error should say why: %v", err) + } +} + +// A bad level is caught before the round trip, with the valid set named. The +// runtime's own rejection ("Unknown LogLevel VERBOSE") reaches the caller as a +// bare AdminException whose detail is only in the runtime log. +func TestParseLogSetArgs_RejectsAnUnknownLevelLocally(t *testing.T) { + for _, args := range [][]string{ + {"Connector", "VERBOSE"}, + {"Connector=VERBOSE"}, + } { + _, err := parseLogSetArgs(args) + if err == nil { + t.Fatalf("%v: an unknown level should be refused", args) + } + if !strings.Contains(err.Error(), "TRACE") { + t.Errorf("%v: the error should list the valid levels, got %v", args, err) + } + } +} + +// Levels are case-insensitive on input and upper-cased on the wire, since that +// is what the runtime accepts. +func TestParseLogSetArgs_LevelCasing(t *testing.T) { + got, err := parseLogSetArgs([]string{"Connector", "trace"}) + if err != nil { + t.Fatalf("lower-case level rejected: %v", err) + } + if len(got) != 1 { + t.Fatalf("got %+v", got) + } +} + +// A single argument is neither form; say so rather than reaching the runtime +// with half a request. +func TestParseLogSetArgs_RejectsIncompleteInput(t *testing.T) { + if _, err := parseLogSetArgs([]string{"Connector"}); err == nil { + t.Error("a lone node name should be refused") + } + if _, err := parseLogSetArgs([]string{"=TRACE"}); err == nil { + t.Error("an empty node name should be refused") + } + if _, err := parseLogSetArgs([]string{"Connector="}); err == nil { + t.Error("an empty level should be refused") + } +} diff --git a/cmd/mxcli/docker/loglevels.go b/cmd/mxcli/docker/loglevels.go new file mode 100644 index 000000000..db097e96e --- /dev/null +++ b/cmd/mxcli/docker/loglevels.go @@ -0,0 +1,156 @@ +// SPDX-License-Identifier: Apache-2.0 + +package docker + +import ( + "errors" + "fmt" + "sort" + "strings" +) + +// ErrAdminUnreachable marks a failure to reach the admin API at all, as opposed +// to the runtime rejecting the request. The two need different advice: "start an +// app" versus "that node does not exist", and printing both makes the real one +// harder to see. +var ErrAdminUnreachable = errors.New("admin API unreachable") + +// loglevels.go drives the runtime's log-node levels through the M2EE admin API. +// +// Every fact below was established against a live 11.12.1 runtime, because the +// admin API is barely documented and guesses here are expensive: +// +// - `get_log_settings` exists but REQUIRES one of `node`, `subscriber` or +// `sort` in params. With none it answers "Please specify node, subscriber or +// sort option in params" — an AdminException, not a usage hint, and the +// detail is only in the runtime log rather than the HTTP response. +// - `sort` accepts exactly "node" and "subscriber". Everything else +// ("name", "all", "level", "nodes", …) is "Unknown sort option". +// - `set_log_level` takes `{"nodes":[{"name":…,"level":…}], "force":bool}`. +// - `force` means "allow a node that does not exist yet". Without it an +// unknown node is REFUSED ("Unknown LogNode X. Use the 'force' parameter…"), +// which makes the default the typo-safe one — so mxcli does not pass it +// unless asked. Mendix supports pre-registering a level for a node that has +// not published yet, which is what force is for. +// - An invalid level is refused ("Unknown LogLevel VERBOSE"). +// +// The response for an AdminException carries no detail ("See logging output for +// details"), so a failure here is reported with the request that caused it. +type LogLevel string + +// The levels the runtime accepts, most to least severe. Verified against +// SetLoglevelAction's rejection of anything outside this set. +var logLevels = []string{"NONE", "CRITICAL", "ERROR", "WARNING", "INFO", "DEBUG", "TRACE"} + +// NormalizeLogLevel upper-cases and validates a level, so a typo is caught here +// with the valid set rather than by an AdminException whose detail is in a log +// file the caller is not reading. +func NormalizeLogLevel(level string) (string, error) { + up := strings.ToUpper(strings.TrimSpace(level)) + for _, l := range logLevels { + if up == l { + return up, nil + } + } + return "", fmt.Errorf("unknown log level %q; valid levels are %s", + level, strings.Join(logLevels, ", ")) +} + +// LogNodeLevel is one node and its level for a subscriber. +type LogNodeLevel struct { + Node string + Subscriber string + Level string +} + +// GetLogSettings returns every log node with its level, one row per +// node/subscriber pair, sorted by node then subscriber so output is stable. +func GetLogSettings(opts M2EEOptions) ([]LogNodeLevel, error) { + resp, err := CallM2EE(opts, "get_log_settings", map[string]any{"sort": "node"}) + if err != nil { + return nil, fmt.Errorf("%w: %v", ErrAdminUnreachable, err) + } + if resp.Result != 0 { + return nil, fmt.Errorf("reading log settings: %s", resp.M2EEError()) + } + // feedback is {node: {subscriber: level}}. + var out []LogNodeLevel + for node, v := range resp.Feedback() { + subs, ok := v.(map[string]any) + if !ok { + continue + } + for sub, lvl := range subs { + s, _ := lvl.(string) + out = append(out, LogNodeLevel{Node: node, Subscriber: sub, Level: s}) + } + } + sort.Slice(out, func(i, j int) bool { + if !strings.EqualFold(out[i].Node, out[j].Node) { + return strings.ToLower(out[i].Node) < strings.ToLower(out[j].Node) + } + return out[i].Subscriber < out[j].Subscriber + }) + return out, nil +} + +// SetLogLevels sets levels for the given nodes in a single admin call — the +// action takes a list, so a multi-node change is one round trip and lands +// together rather than partially. +// +// force allows a node the runtime does not know yet. Without it an unknown node +// is refused, which is the behaviour worth having by default: a mistyped node +// name should be an error, not a silently pre-registered setting that never +// takes effect. +func SetLogLevels(opts M2EEOptions, levels []LogNodeLevel, force bool) error { + if len(levels) == 0 { + return fmt.Errorf("no nodes given") + } + nodes := make([]map[string]any, 0, len(levels)) + var described []string + for _, l := range levels { + lvl, err := NormalizeLogLevel(l.Level) + if err != nil { + return err + } + nodes = append(nodes, map[string]any{"name": l.Node, "level": lvl}) + described = append(described, l.Node+"="+lvl) + } + params := map[string]any{"nodes": nodes} + if force { + params["force"] = true + } + + resp, err := CallM2EE(opts, "set_log_level", params) + if err != nil { + return fmt.Errorf("%w: %v", ErrAdminUnreachable, err) + } + if resp.Result != 0 { + // The HTTP response for an AdminException says only "See logging output + // for details", so name the request and the likeliest cause instead of + // passing that through as if it were an explanation. + hint := "" + if !force { + hint = "\n If the node does not exist yet, pass --force — the runtime refuses an unknown node otherwise." + } + return fmt.Errorf("setting log level (%s): %s%s", + strings.Join(described, ", "), resp.M2EEError(), hint) + } + return nil +} + +// MatchLogNodes returns the nodes whose name contains the (case-insensitive) +// pattern. An empty pattern matches everything. +func MatchLogNodes(all []LogNodeLevel, pattern string) []LogNodeLevel { + if pattern == "" { + return all + } + needle := strings.ToLower(pattern) + var out []LogNodeLevel + for _, l := range all { + if strings.Contains(strings.ToLower(l.Node), needle) { + out = append(out, l) + } + } + return out +} diff --git a/cmd/mxcli/docker/loglevels_test.go b/cmd/mxcli/docker/loglevels_test.go new file mode 100644 index 000000000..07cb87d24 --- /dev/null +++ b/cmd/mxcli/docker/loglevels_test.go @@ -0,0 +1,57 @@ +// SPDX-License-Identifier: Apache-2.0 + +package docker + +import ( + "strings" + "testing" +) + +// The runtime rejects anything outside this set with "Unknown LogLevel X", whose +// detail never reaches the HTTP response — so the check happens here. +func TestNormalizeLogLevel(t *testing.T) { + for in, want := range map[string]string{ + "trace": "TRACE", "TRACE": "TRACE", " Debug ": "DEBUG", "none": "NONE", + } { + got, err := NormalizeLogLevel(in) + if err != nil || got != want { + t.Errorf("NormalizeLogLevel(%q) = (%q, %v), want %q", in, got, err, want) + } + } + for _, bad := range []string{"VERBOSE", "FINE", "", "warn"} { + if _, err := NormalizeLogLevel(bad); err == nil { + t.Errorf("NormalizeLogLevel(%q) should fail", bad) + } + } +} + +// Listing 57 nodes is unusable without a filter; matching is case-insensitive +// and substring, because nobody remembers "ConnectionBus_Synchronize" exactly. +func TestMatchLogNodes(t *testing.T) { + all := []LogNodeLevel{ + {Node: "ConnectionBus_Queries", Level: "INFO"}, + {Node: "Connector", Level: "INFO"}, + {Node: "Jetty", Level: "INFO"}, + } + if got := MatchLogNodes(all, ""); len(got) != 3 { + t.Errorf("an empty filter should match everything, got %d", len(got)) + } + got := MatchLogNodes(all, "connect") + if len(got) != 2 { + t.Fatalf("case-insensitive substring match failed: %+v", got) + } + if got[0].Node != "ConnectionBus_Queries" || got[1].Node != "Connector" { + t.Errorf("unexpected matches: %+v", got) + } + if got := MatchLogNodes(all, "nope"); len(got) != 0 { + t.Errorf("no match expected, got %+v", got) + } +} + +// An empty node list must not reach the admin API as a well-formed no-op. +func TestSetLogLevels_RejectsEmptyInput(t *testing.T) { + err := SetLogLevels(M2EEOptions{}, nil, false) + if err == nil || !strings.Contains(err.Error(), "no nodes") { + t.Errorf("expected a no-nodes error, got %v", err) + } +} From 5b62444559fa64f4510d5c2afaf79a5890177f20 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 01:05:39 +0000 Subject: [PATCH 08/29] =?UTF-8?q?docs(log):=20correct=20the=20published-OD?= =?UTF-8?q?ata=20node=20=E2=80=94=20it=20exists,=20per=20app?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit claimed there is no log node for a published OData service, and that "what is my published resource being asked?" therefore still needed a LOG in the read microflow. That was wrong. `OData Publish` — with a space — exists whenever the project publishes a service, and at TRACE it logs the full incoming URI: TRACE - OData Publish: Incoming request from 127.0.0.1: GET .../Rows?$top=5&$filter=rowKey eq 'abc' DEBUG - OData Publish: Responding to client with status code 400. which is precisely the question the command was built for. The mistake is worth naming, because it is the same one in a new place: the node list is a property of the APP, not of Mendix. Nodes appear once something registers them. The first probe ran against a project with zero OData services, saw 57 nodes and no publish node, and generalised. Adding one service makes it 58. Enumerating a platform's capabilities from one sample app and calling the result "Mendix does not have this" is exactly what `log list` against your own app is for — the command was right, the conclusion drawn beside it was not. The same probe also showed Mendix rejecting `$filter` on a property not declared Filterable, with 400 "Property 'x' is non-filterable", BEFORE the read microflow runs — so the platform does enforce declared filterability, which is more than §20 credited it with. What it still does not do is apply $top/$skip/$orderby for a read-microflow resource. Corrected in analyze-runtime.md (node table + a section on the publish node), odata-data-sharing.md, the command's own help, and the fix-issue row. mxcli-formula1 suggested issue 4 --- .claude/skills/fix-issue.md | 2 +- .claude/skills/mendix/analyze-runtime.md | 33 +++++++++++++++++---- .claude/skills/mendix/odata-data-sharing.md | 20 +++++++++++-- cmd/mxcli/cmd_log.go | 19 +++++++++--- 4 files changed, 62 insertions(+), 12 deletions(-) diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index f8aedabe7..67beba8a0 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -430,4 +430,4 @@ extracting `OffsetExpression`/`LimitExpression`. | `.ai-context/skills/` still carries the previous release's guidance after the mxcli binary is upgraded — binary rebuilt at 12:05, skills stamped the day before — with no warning that they disagree | The skills are embedded in the binary and written exactly once, by `mxcli init`. Nothing re-ran init on upgrade, and nothing compared what was on disk against what the binary carried | New `cmd/mxcli/init_skills_sync.go` (`syncAIContextSkills`, `reportSkillSync`), `--sync-skills` flag in `cmd/mxcli/init.go`, and a sync step in the SessionStart bootstrap in `cmd/mxcli/init_hook.go` | **Stale guidance is worse than missing guidance**, because an agent reads it with identical confidence either way — so the failure mode is silent and confident. Fix it where it is consumed, not where it is authored: the SessionStart bootstrap already runs on every session and can fetch the binary, so it is the one place guaranteed to execute immediately before an agent reads the files. Two properties keep an every-session job acceptable: **write only what differs** (an mtime that moves every session makes "when did this last change" unanswerable — a test asserts the mtime holds) and **stay silent when current**. Never fatal: a skills refresh must not block a session, hence `|| true`. A test asserts the bootstrap script actually calls it *before* the exec'd setup, since a step ordered after an `exec` never runs. Tests `init_skills_sync_test.go`; the control (write-once) leaves the stale file in place. mxcli-formula1 §16 | | `publish entity … (TopSupported: No)` (or `SkipSupported`/`Countable`) parses, `DESCRIBE` reads it back as `No`, and the published `$metadata` still advertises `true`. A client then believes paging works when nothing implements it | `serializeEntitySet` hardcoded all three `QueryOptions` to `true`, ignoring the `*bool` fields the model already carried — the AST/model/DESCRIBE half of the feature shipped without the writer half | `sdk/mpr/writer_odata.go` (`boolOrTrue`, entity-set `QueryOptions`) | **A capability annotation on a microflow-backed resource is load-bearing, not decorative.** Mendix applies *no* query options to a read-microflow resource — it hands the request over and returns what comes back — so the annotation is the only thing a client has to go on, and an over-claim is not cosmetic: the client reads a whole collection believing it is a page. **Check the writer whenever a property round-trips correctly through DESCRIBE**: DESCRIBE reads the model, so a model-only feature describes perfectly and publishes wrong, and the round-trip test everyone reaches for cannot see it. A tri-state `*bool` needs an explicit nil policy at the boundary (`nil` = the platform default, only an explicit `false` opts out) or the pointer is pointless. Tests `writer_odata_test.go`; the control (hardcoded true) reproduces the over-claim. mxcli-formula1 §20 | | A published OData resource backed by a read microflow silently returns the wrong thing: `?$top=5` yields the whole collection with a 200, and a client re-reading a held row by key gets the collection default and adopts the FIRST row as that object's identity. No error anywhere — well-formed request, valid collection, correct `$count`, 200 | Two promises the service makes on the microflow's behalf and nothing checked: the `KEY` in `expose (…)`, and the `TopSupported`/`SkipSupported` annotations (which default to **true** when unspecified). Mendix applies no query options to a read-microflow resource — it hands over the request and returns what comes back | New `mdl/executor/validate_odata_read_contract.go` (MDL-ODATA02 key promise, MDL-ODATA03 capability over-claim), wired in `cmd/mxcli/cmd_check.go`; the response contract documented in `.claude/skills/mendix/odata-data-sharing.md` | **A read microflow cannot answer 400** — unlike an OData action or an insert/update/delete microflow, the read capability has no `System.HttpResponse` parameter ([docs](https://docs.mendix.com/refguide/published-odata-entity/#custom-http-response)). Its contract is therefore *declarative*: declaring `TopSupported: No` is the read path's only substitute for the refusal it cannot send. **Pick a trigger you can prove**: both rules fire only when the microflow takes no `System.HttpRequest` parameter, because then it provably cannot see a key or a query option; a microflow that does take it gets the benefit of the doubt, since proving *which* options it parses needs real analysis and a rule that guesses gets switched off. **Verify a check rule against a real parse, never a hand-built AST** — the visitor stores `ReadMode` as `MICROFLOW Module.Name` **upper-cased**, so a case-sensitive prefix match made the whole rule dead while it still looked right; the casing is now pinned by its own test. Tests `validate_odata_read_contract_test.go`, examples in `10-odata-examples.mdl` (both correct shapes: request-aware, and honestly declared). mxcli-formula1 §37/§20, suggested issues 2 and 3 | -| No way to see what a running app's subsystem is doing: everything logs at `INFO`, the detail is not in the log at all, and raising the whole runtime to `TRACE` is unusable on a busy app | Nothing in mxcli drove the runtime's per-node log levels, though the M2EE admin API has exposed them all along | New `cmd/mxcli/docker/loglevels.go` (`GetLogSettings`, `SetLogLevels`, `NormalizeLogLevel`, `MatchLogNodes`), `cmd/mxcli/cmd_log.go` (`mxcli log list` / `log set`), documented in `.claude/skills/mendix/analyze-runtime.md` | **Probe an undocumented API before designing the command around it.** Every fact here came from a live 11.12.1 runtime: `get_log_settings` requires one of `node`/`subscriber`/`sort`; `sort` accepts only `node` and `subscriber`; `set_log_level` takes `{"nodes":[{name,level}],"force":bool}`. **The HTTP response for an AdminException says only "See logging output for details" — the actual message ("Please specify node, subscriber or sort option in params", "Unknown sort option: name", "Unknown LogNode X. Use the 'force' parameter…") is in the runtime log**, so probe with the runtime log open or you learn nothing. `force` means "allow a node that does not exist yet" and **permanently registers the name**, so a typo becomes a real empty node — hence it is opt-in, and an unknown node is an error by default. **Resist the subsystem-specific command**: this was proposed as `mxcli odata trace`, but `set_log_level` takes a list of nodes and the runtime reports 57, so the primitive is generic and the OData knowledge belongs in docs. Distinguish "cannot reach the admin API" from "the runtime refused the request" (`ErrAdminUnreachable`) or the wrong hint buries the real one. Tests `cmd_log_test.go`, `loglevels_test.go`; verified end-to-end against a booted runtime (both arg forms, multi-node, typo refused, `--force` accepted, unreachable port). mxcli-formula1 suggested issue 4 | +| No way to see what a running app's subsystem is doing: everything logs at `INFO`, the detail is not in the log at all, and raising the whole runtime to `TRACE` is unusable on a busy app | Nothing in mxcli drove the runtime's per-node log levels, though the M2EE admin API has exposed them all along | New `cmd/mxcli/docker/loglevels.go` (`GetLogSettings`, `SetLogLevels`, `NormalizeLogLevel`, `MatchLogNodes`), `cmd/mxcli/cmd_log.go` (`mxcli log list` / `log set`), documented in `.claude/skills/mendix/analyze-runtime.md` | **Probe an undocumented API before designing the command around it.** Every fact here came from a live 11.12.1 runtime: `get_log_settings` requires one of `node`/`subscriber`/`sort`; `sort` accepts only `node` and `subscriber`; `set_log_level` takes `{"nodes":[{name,level}],"force":bool}`. **The HTTP response for an AdminException says only "See logging output for details" — the actual message ("Please specify node, subscriber or sort option in params", "Unknown sort option: name", "Unknown LogNode X. Use the 'force' parameter…") is in the runtime log**, so probe with the runtime log open or you learn nothing. `force` means "allow a node that does not exist yet" and **permanently registers the name**, so a typo becomes a real empty node — hence it is opt-in, and an unknown node is an error by default. **Resist the subsystem-specific command**: this was proposed as `mxcli odata trace`, but `set_log_level` takes a list of nodes, so the primitive is generic and the OData knowledge belongs in docs. **The node list is a property of the APP, not of Mendix** — a node appears only once something registers it. A first pass concluded "there is no log node for a published OData service" from a project that had none; adding one service turns 57 nodes into 58 and `OData Publish` (with a space) appears, logging the full incoming URI at TRACE — exactly the question that motivated the command. Enumerating capabilities against one sample app and generalising is the trap; `log list` against *this* app is the answer. Distinguish "cannot reach the admin API" from "the runtime refused the request" (`ErrAdminUnreachable`) or the wrong hint buries the real one. Tests `cmd_log_test.go`, `loglevels_test.go`; verified end-to-end against a booted runtime (both arg forms, multi-node, typo refused, `--force` accepted, unreachable port). mxcli-formula1 suggested issue 4 | diff --git a/.claude/skills/mendix/analyze-runtime.md b/.claude/skills/mendix/analyze-runtime.md index e16d6b977..1d41fd3dc 100644 --- a/.claude/skills/mendix/analyze-runtime.md +++ b/.claude/skills/mendix/analyze-runtime.md @@ -71,6 +71,7 @@ Nodes worth knowing: | What SQL is being run | `ConnectionBus_Queries` (and `_Retrieve`, `_Update`) | | Database sync at startup | `ConnectionBus_Synchronize` | | Consumed OData / REST calls | `ODataConsume`, `REST Consume` | +| **Published** OData requests (the incoming URI) | `OData Publish` — note the space; only exists if the project publishes a service | | Microflow execution | `MicroflowEngine`, `ActionManager` | | Scheduled events / queues | `SystemTask`, `TaskQueue` | | Java/JS action wiring | `Connector` | @@ -81,11 +82,33 @@ for the life of the process, so a typo becomes a real (empty) node that shows up `log list` from then on. Use it only to pre-register a node that has not published yet. -**There is no log node for a *published* OData service.** `ODataConsume` is the -client side only. So "what is my published resource being asked?" cannot be -answered by raising a node — put `LOG INFO 'URI=' + $Request/Uri` in the read -microflow instead (see `odata-data-sharing.md`). That gap is Mendix's, not -mxcli's. +**Nodes appear only once something registers them**, so the list is a property of +*this* app, not of Mendix. A blank 11.12 app reports 57; adding one published +OData service makes it 58. This is why `log list` is the first step rather than a +remembered name — and why `--force` exists for a node that has not registered yet. + +### Seeing what a published OData resource is asked + +`OData Publish` — **note the space** — is the node, and it exists only when the +project publishes a service. At TRACE it logs the full incoming URI, which is the +question `$filter`/`$top`/key-lookup bugs turn on: + +```bash +mxcli log set "OData Publish" TRACE +# GET /odata/f1/Rows?$top=5&$filter=rowKey eq 'abc' +``` +``` +TRACE - OData Publish: Incoming request from 127.0.0.1: GET .../Rows?$top=5&$filter=rowKey eq 'abc' +DEBUG - OData Publish: Responding to client with status code 400. +``` + +`ODataConsume` is the *client* side — a different node for a different direction. + +That same probe showed Mendix rejecting `$filter` on a property not declared +`Filterable`, with a **400 "Property 'rowKey' is non-filterable"**, before the read +microflow ran. So the platform does enforce the filterability you declare in +`expose (…)`; what it does *not* do is apply `$top`/`$skip`/`$orderby` for a +read-microflow resource (see `odata-data-sharing.md`). ## 2. Metrics — throughput and database pressure diff --git a/.claude/skills/mendix/odata-data-sharing.md b/.claude/skills/mendix/odata-data-sharing.md index 555172595..10cd97774 100644 --- a/.claude/skills/mendix/odata-data-sharing.md +++ b/.claude/skills/mendix/odata-data-sharing.md @@ -576,9 +576,25 @@ begin end; ``` +To watch what clients actually send, raise the **`OData Publish`** log node (note +the space; it exists only when the project publishes a service): + +```bash +mxcli log set "OData Publish" TRACE +``` +``` +TRACE - OData Publish: Incoming request from 127.0.0.1: GET .../Rows?$top=5&$filter=rowKey eq 'abc' +DEBUG - OData Publish: Responding to client with status code 400. +``` + +That is the fastest way to see a client re-reading a row by key, and it needs no +change to the model. `ODataConsume` is the client side, a different node. + Mendix validates field names before the microflow runs (`$filter=secretColumn eq 'x'` -is a `400` from the platform), so the microflow only ever sees names that exist -in the published metadata. That is defence in depth, not a substitute for a +is a `400` from the platform), and it enforces `Filterable`: filtering on a property +you did not declare filterable is rejected with `400 "Property 'x' is +non-filterable"` before the microflow runs. So the microflow only ever sees names +that exist in the published metadata. That is defence in depth, not a substitute for a whitelist — it constrains the *name*, not what you do with it. ## Step-by-Step: Read-Write API with Microflow Handlers diff --git a/cmd/mxcli/cmd_log.go b/cmd/mxcli/cmd_log.go index a86bfcf9f..74ed9f71e 100644 --- a/cmd/mxcli/cmd_log.go +++ b/cmd/mxcli/cmd_log.go @@ -23,8 +23,12 @@ import ( // runtime this was built against reports 57 of them. A per-subsystem command // would have wrapped a generic primitive one subsystem at a time. // -// So the command is generic, and the OData-specific knowledge (which node to -// raise) lives in the skill instead. +// So the command is generic, and the OData-specific knowledge lives in the skill +// instead — including the answer to the original question: `OData Publish` (with +// a space) logs the full incoming URI at TRACE, and exists only when the project +// publishes a service. An earlier note here claimed no such node existed; it was +// written against a project that had no OData service, which is exactly the +// mistake `log list` is meant to prevent. var logCmd = &cobra.Command{ Use: "log", @@ -56,8 +60,15 @@ Examples: # Several at once — one admin call, applied together mxcli log set ConnectionBus_Queries=TRACE Connector=DEBUG - # A node that has not published yet (Mendix allows pre-registering a level) - mxcli log set MyModule.MyNode TRACE --force`, + # A name with a space needs quoting + mxcli log set "OData Publish" TRACE + + # A node that has not registered yet (Mendix allows pre-registering a level) + mxcli log set MyModule.MyNode TRACE --force + +The node list belongs to THIS app, not to Mendix: a node appears once something +registers it, so a blank app reports 57 and publishing one OData service makes it +58. Start from 'log list' rather than a remembered name.`, } var logListCmd = &cobra.Command{ From ff97412ef7e4fee26160bdbc8f19922307ee8ec3 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 01:06:38 +0000 Subject: [PATCH 09/29] fix(pages): qualify an association datasource's name, which wrote an unloadable page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Upstream #854 reported a cross-module association datasource writing an empty DestinationEntity, which Mendix resolves to null so the .mpr will not open. That half no longer reproduces — the destination resolves through CrossAssociations, and an unresolved one is refused. The association half of the same EntityRefStep was still written as authored. Both halves are BY_NAME references and Mendix nulls either one it cannot find, so a bare `Order_Line` produced the identical unopenable project, one property over: ArgumentNullException at EntityRefStep.set_AssociationId The explicit-destination form is what made it reachable: supplying the destination satisfies the empty-DestinationEntity guard, so nothing else stood between a bare name and the crash — and `Assoc/Module.Entity` is exactly the spelling that guard's error message tells the author to use. Qualify a bare association with the context entity's module, the rule attribute-path hops already follow, and verify an author-supplied destination's association exists rather than taking it on trust (a misspelling would otherwise be written qualified-but-nonexistent and null the same way). Verified on Mendix 11.13.0: all five spellings resolve and mx check reports 0 errors. The pre-fix binary reproduces the crash on the same script. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013uQvFDd5R4eNqqita59jM8 --- .claude/skills/fix-issue.md | 1 + .../854-assoc-datasource-qualified-name.mdl | 82 +++++++++ ...cmd_pages_builder_assoc_datasource_test.go | 166 ++++++++++++++++++ mdl/executor/cmd_pages_builder_v3.go | 25 ++- 4 files changed, 273 insertions(+), 1 deletion(-) create mode 100644 mdl-examples/bug-tests/854-assoc-datasource-qualified-name.mdl create mode 100644 mdl/executor/cmd_pages_builder_assoc_datasource_test.go diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index 85535a966..044d182f0 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -427,3 +427,4 @@ extracting `OffsetExpression`/`LimitExpression`. | `create odata client` with credentials given as constants (`HttpUsername: '@Module.ApiUser'`) still gets HTTP 401 and an empty client, after the fix that made literal credentials work. Sharpened by the same release making a constant `ServiceUrl` mandatory — the shape the tool insists on is the shape whose credentials it will not read | `resolveCredential` trusted the visitor's isLiteral flag. `'@Module.ApiUser'` **is** a STRING_LITERAL, so the flag said "literal" and the previous code sent the fifteen characters `@Module.ApiUser` as the username — and the unresolved-credential note did not fire either, because as far as the code knew nothing was unresolved | `mdl/executor/cmd_odata.go` (`resolveCredential`, `constantReference`, `designTimeConstants`) | **A syntactic classification is not a semantic one.** The visitor can say "this was a quoted string"; only the executor can say "this quoted string names a constant". Any flag of the form isLiteral needs the consumer to ask what the literal *contains* before treating it as a value. **The fix is to resolve, not to refuse**: a constant's design-time default is exactly what Studio Pro sends on the same fetch, so reading it is the value rather than a workaround — and mxcli already has the project open. Three spellings must all work (`'v'`, `@M.C`, `'@M.C'`); a dotted literal like a password containing a dot must not be mistaken for a reference. Tests `cmd_odata_metadata_auth_test.go`. mxcli-formula1 #23 follow-up | | An app themed dark still shows light-mode drop shadows under the datagrid's filter-operator popover and dropdown filter lists | The generated widget layer re-pointed `.column-selectors` but not the four rules in `_datagrid-filters.scss` that bake the same two-layer shadow. Each already takes its *background* from `--bg-color-secondary`, so Atlas re-colours the panel and leaves the shadow — which is why it reads as a partial fix rather than an untouched widget | `cmd/mxcli/theme/assets/*/files/theme/web/_mxcli-widgets.scss` | **Read the selectors out of the shipped `themesource/`, not the bug report** — the fourth here is `.dropdown-container .dropdown-list`, nested under a parent the report did not name. **Verify in the compiled CSS, never the source** (the §33 rule): apply the theme to a real project, run `mxbuild --target=deploy --java-home=… --java-exe-path=…`, then grep `theme-cache/web/theme.compiled.css` and check the *line number* — the fix must land after the widget module's own rule (30794 vs 27765 here) or the cascade eats it. A theme-cache file older than the SCSS you just wrote is a stale compile, and mtime is the cheapest way to catch it. mxcli-formula1 #33 / suggested issue 4 | | A `MOVE` cannot be confirmed and a module's layout cannot be reviewed: `SHOW STRUCTURE` groups by document type at every depth and never names a folder, `DESCRIBE` answers for one document at a time, so checking where things ended up means opening the `.mpr` as SQLite | The read side of folders was simply never built. `MOVE`/`DROP FOLDER` write containment; nothing read it back | New `mdl/executor/cmd_list_folders.go` + grammar (`FOLDERS` lexer token, `showOrList FOLDERS (IN …)?` in `MDLCatalog.g4`, `FOLDERS` added to the `keyword` rule so it stays usable as an identifier), `mdl/ast/ast_query.go`, `mdl/visitor/visitor_query.go`, `mdl/executor/executor_query.go` | **A layout listing must show what is *not* there**: empty folders (`[0]`) and documents still at the module root, or it cannot be diffed against an intended layout — that is the whole use. Documents are indexed by `ContainerID` across every list call the backend offers, each best-effort, so a backend that cannot answer one kind yields a listing missing that kind rather than no listing. **Do not stub the hierarchy in the test** — `mkHierarchy` populates `moduleNames` but not `folderNames`, so `BuildFolderPath` returns `""` and every folder silently collapses into the module root, which looks exactly like the bug. Build it from the mock's `ListModules`/`ListUnits`/`ListFolders`, as `getHierarchy` does. Tests `cmd_list_folders_test.go`, example in `18-folder-examples.mdl`. mxcli-formula1 issue #2 | +| A page written by mxcli will not open in Studio Pro, and `mx check` dies during *load* with `ArgumentNullException` at `EntityRefStep.set_AssociationId` — no page error, no CE code, the whole project is down rather than one document | An association DATASOURCE wrote the association name exactly as authored. Both halves of an `EntityRefStep` are BY_NAME and Mendix resolves either one it cannot find to null, but only `DestinationEntity` was guarded (issuetracker #14) — a bare `Order_Line` reached BSON unqualified. The **explicit-destination** form is the live trap: supplying the destination satisfies that guard, so nothing stood between a bare name and the crash — and `Assoc/Module.Entity` is exactly what the guard's error tells the author to write | `mdl/executor/cmd_pages_builder_v3.go` (`buildDataSourceV3`, `case "association"`: qualify via `resolveAssociationPathIn`, then verify an author-supplied destination's association exists via `associationEndpoints`) | **Guard every BY_NAME half of a ref, not the one that was reported.** #854 reported the destination; the association beside it failed identically and was reachable through the fix's own advice. **Diff the two properties in the dump** (`grep -A1 '"Key": "(Association\|DestinationEntity)"'`) rather than reading the exception name — the loader reports whichever it touches first. **Hold the input constant across builds**: the pre-fix binary was first run with bare names and post-fix with qualified ones, which "reproduced" nothing; re-running the *same qualified* script on both is what isolated cross-module (`""`) from same-module (resolved). `mx check` exits 0 on this failure — assert on the `contains: N errors` line, never `$?`. Tests `cmd_pages_builder_assoc_datasource_test.go`, example `mdl-examples/bug-tests/854-assoc-datasource-qualified-name.mdl`. upstream #854 follow-on | diff --git a/mdl-examples/bug-tests/854-assoc-datasource-qualified-name.mdl b/mdl-examples/bug-tests/854-assoc-datasource-qualified-name.mdl new file mode 100644 index 000000000..ab29cf1b6 --- /dev/null +++ b/mdl-examples/bug-tests/854-assoc-datasource-qualified-name.mdl @@ -0,0 +1,82 @@ +-- ============================================================================ +-- Upstream #854 (follow-on): an association DATASOURCE wrote the association +-- name unqualified, making the .mpr unloadable +-- ============================================================================ +-- +-- #854 itself — a cross-module association datasource writing +-- `DestinationEntity: ""` — no longer reproduces: the destination now resolves +-- through `CrossAssociations` (f0d1aea) and an unresolved one is refused +-- outright (18de30b, see it-14-assoc-destination-entity.mdl). +-- +-- What survived is the OTHER half of the same EntityRefStep. Both halves are +-- BY_NAME references and Mendix resolves either one it cannot find to null, but +-- only the destination was being guarded. A bare association name was written +-- through verbatim: +-- +-- System.InvalidOperationException: An error occurred when trying to set the +-- 'Association' property of a Entity ref step in a Page with ID ... +-- ---> System.ArgumentNullException: Value cannot be null. (Parameter 'value') +-- at ...DomainModels.Refs.EntityRefStep.set_AssociationId +-- +-- Same unopenable project as #854, one property over. Not a build error: the +-- loader dies before validation, so `mx check` reports nothing about the page. +-- +-- The explicit-destination form below is the one that mattered. Supplying the +-- destination satisfied the empty-DestinationEntity guard, so nothing else stood +-- between a bare name and the crash — and `Assoc/Module.Entity` is precisely the +-- spelling that guard's error message tells the author to use. +-- +-- Fix: qualify a bare association with the context entity's module (the rule +-- attribute-path hops already follow), and verify an author-supplied +-- destination's association actually exists rather than taking it on trust. +-- +-- Verified on Mendix 11.13.0: this script checks 0 errors and the project opens. +-- ============================================================================ + +create module M854A; +create module M854B; + +create persistent entity M854A.Order (OrderNumber: String(50)); +create persistent entity M854A.Note (Body: String(200)); +create persistent entity M854B.Line (Product: String(100)); + +-- control: both ends in the same module +create association M854A.Order_Note from M854A.Order to M854A.Note type ReferenceSet; +-- subject: destination lives in another module +create association M854A.Order_Line from M854A.Order to M854B.Line type ReferenceSet; + +create page M854A.OrderDetail ( + Params: { $Order: M854A.Order }, + Title: 'Order Detail', + Layout: Atlas_Core.Atlas_Default +) { + layoutgrid g { row r { column c (DesktopWidth: 12) { + dataview dvOrder (datasource: $Order) { + textbox tbNumber (attribute: OrderNumber, label: 'Order #') + + -- bare name, same module + datagrid dgNotes (datasource: association Order_Note) { + column colBody (attribute: Body, caption: 'Note') + } + + -- bare name, destination in another module + datagrid dgLines (datasource: association Order_Line) { + column colProduct (attribute: Product, caption: 'Product') + } + + -- bare name + explicit destination: the guard's own suggested spelling, + -- and the form that used to crash the loader + datagrid dgLines2 (datasource: association Order_Line/M854B.Line) { + column colProduct2 (attribute: Product, caption: 'Product') + } + + -- fully qualified, with and without an explicit destination + datagrid dgLines3 (datasource: association M854A.Order_Line) { + column colProduct3 (attribute: Product, caption: 'Product') + } + datagrid dgLines4 (datasource: association M854A.Order_Line/M854B.Line) { + column colProduct4 (attribute: Product, caption: 'Product') + } + } + } } } +} diff --git a/mdl/executor/cmd_pages_builder_assoc_datasource_test.go b/mdl/executor/cmd_pages_builder_assoc_datasource_test.go new file mode 100644 index 000000000..12d2db2dc --- /dev/null +++ b/mdl/executor/cmd_pages_builder_assoc_datasource_test.go @@ -0,0 +1,166 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/domainmodel" + "github.com/mendixlabs/mxcli/sdk/pages" +) + +// upstream #854 (follow-on): an association DATASOURCE wrote the association +// name into the EntityRefStep exactly as authored. A bare name — the spelling +// attribute paths accept, and the spelling the unresolved-destination guard's +// own error message suggests (`Assoc/Module.Entity`) — therefore reached BSON +// unqualified. Mendix resolves an unqualified AssociationIdentifier to null and +// the loader throws +// +// ArgumentNullException at EntityRefStep.set_AssociationId +// +// so the .mpr will not open in Studio Pro and `mx check` dies before validating +// anything. Same unopenable-project outcome as the empty DestinationEntity that +// #854 reported; a different property of the same step. +// +// The explicit-destination form is the dangerous one: supplying the destination +// satisfies the guard, so nothing else stood between a bare name and the crash. +func TestAssociationDataSource_QualifiesAssociationName(t *testing.T) { + const ( + modAID = model.ID("mod-a") + modBID = model.ID("mod-b") + orderID = model.ID("e-order") + noteID = model.ID("e-note") + lineID = model.ID("e-line") + ) + + newPB := func() *pageBuilder { + return &pageBuilder{ + entityContext: "ModA.Order", + execCache: &executorCache{ + hierarchy: &ContainerHierarchy{moduleNames: map[model.ID]string{ + modAID: "ModA", + modBID: "ModB", + }}, + domainModels: []*domainmodel.DomainModel{ + { + ContainerID: modAID, + Entities: []*domainmodel.Entity{ + {BaseElement: model.BaseElement{ID: orderID}, Name: "Order"}, + {BaseElement: model.BaseElement{ID: noteID}, Name: "Note"}, + }, + Associations: []*domainmodel.Association{ + {Name: "Order_Note", ParentID: orderID, ChildID: noteID, Type: domainmodel.AssociationTypeReferenceSet}, + }, + CrossAssociations: []*domainmodel.CrossModuleAssociation{ + {Name: "Order_Line", ParentID: orderID, ChildRef: "ModB.Line", Type: domainmodel.AssociationTypeReferenceSet}, + }, + }, + { + ContainerID: modBID, + Entities: []*domainmodel.Entity{ + {BaseElement: model.BaseElement{ID: lineID}, Name: "Line"}, + }, + }, + }, + }, + } + } + + tests := []struct { + name string + reference string + wantPath string // EntityPath = "Module.Assoc/Module.DestEntity" + }{ + { + name: "bare same-module association", + reference: "Order_Note", + wantPath: "ModA.Order_Note/ModA.Note", + }, + { + name: "bare cross-module association", + reference: "Order_Line", + wantPath: "ModA.Order_Line/ModB.Line", + }, + { + name: "bare name with explicit destination (the guard's suggested spelling)", + reference: "Order_Line/ModB.Line", + wantPath: "ModA.Order_Line/ModB.Line", + }, + { + name: "already-qualified name is left alone", + reference: "ModA.Order_Line", + wantPath: "ModA.Order_Line/ModB.Line", + }, + { + name: "qualified name with explicit destination", + reference: "ModA.Order_Line/ModB.Line", + wantPath: "ModA.Order_Line/ModB.Line", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + ds, childCtx, err := newPB().buildDataSourceV3(&ast.DataSourceV3{ + Type: "association", + Reference: tc.reference, + }) + if err != nil { + t.Fatalf("buildDataSourceV3(%q) = error %v", tc.reference, err) + } + src, ok := ds.(*pages.AssociationSource) + if !ok { + t.Fatalf("got %T, want *pages.AssociationSource", ds) + } + if src.EntityPath != tc.wantPath { + t.Errorf("EntityPath = %q, want %q", src.EntityPath, tc.wantPath) + } + // The association half must be qualified: an unqualified + // AssociationIdentifier is what Mendix resolves to null. + assoc := strings.SplitN(src.EntityPath, "/", 2)[0] + if !strings.Contains(assoc, ".") { + t.Errorf("association %q is unqualified — the .mpr will not open", assoc) + } + wantCtx := strings.SplitN(tc.wantPath, "/", 2)[1] + if childCtx != wantCtx { + t.Errorf("child entity context = %q, want %q", childCtx, wantCtx) + } + }) + } +} + +// A destination the author supplies explicitly must not be taken on trust: it +// satisfies the empty-DestinationEntity guard, so a misspelled association would +// otherwise be written qualified-but-nonexistent, which Mendix again resolves to +// null. Refuse at author time instead. +func TestAssociationDataSource_RejectsUnknownAssociation(t *testing.T) { + pb := &pageBuilder{ + entityContext: "ModA.Order", + execCache: &executorCache{ + hierarchy: &ContainerHierarchy{moduleNames: map[model.ID]string{ + model.ID("mod-a"): "ModA", + }}, + domainModels: []*domainmodel.DomainModel{{ + ContainerID: model.ID("mod-a"), + Entities: []*domainmodel.Entity{ + {BaseElement: model.BaseElement{ID: model.ID("e-order")}, Name: "Order"}, + }, + }}, + }, + } + + for _, ref := range []string{"Order_Lnie/ModB.Line", "ModA.Order_Lnie/ModB.Line"} { + t.Run(ref, func(t *testing.T) { + _, _, err := pb.buildDataSourceV3(&ast.DataSourceV3{Type: "association", Reference: ref}) + if err == nil { + t.Fatalf("buildDataSourceV3(%q) succeeded; want a refusal — "+ + "a nonexistent association writes a null AssociationId and the project will not open", ref) + } + if !strings.Contains(err.Error(), "Order_Lnie") { + t.Errorf("error %q does not name the offending association", err) + } + }) + } +} diff --git a/mdl/executor/cmd_pages_builder_v3.go b/mdl/executor/cmd_pages_builder_v3.go index e2409483e..0d3e48584 100644 --- a/mdl/executor/cmd_pages_builder_v3.go +++ b/mdl/executor/cmd_pages_builder_v3.go @@ -793,8 +793,31 @@ func (pb *pageBuilder) buildDataSourceV3(ds *ast.DataSourceV3) (pages.DataSource if idx := strings.Index(path, "/"); idx >= 0 { destEntity = path[idx+1:] path = path[:idx] - } else { + } + + // Both halves of an EntityRefStep are BY_NAME references, and Mendix + // resolves either one it cannot find to null — so the association half + // needs the same care as the destination. A bare name (the spelling + // attribute paths accept, and the one the guard below suggests) reached + // BSON unqualified and the loader threw ArgumentNullException at + // `EntityRefStep.set_AssociationId`: same unopenable project as an empty + // DestinationEntity, a different property. Qualify with the context + // entity's module, exactly as attribute-path hops do (upstream #854). + path = pb.resolveAssociationPathIn(path, pb.entityContext) + + if destEntity == "" { destEntity = pb.resolveAssociationDestination(path, pb.entityContext) + } else if _, _, ok := pb.associationEndpoints(path); !ok { + // An author-supplied destination satisfies the guard below, so it is + // the one path where a misspelled — or wrongly-moduled — association + // would sail through and be written qualified-but-nonexistent, which + // Mendix resolves to null just the same. Verify it exists. + return nil, "", mdlerrors.NewValidationf( + "association %q for datasource %q does not exist — "+ + "writing it would produce a project Mendix cannot open; "+ + "a bare name is qualified with the module of the context entity (%s), "+ + "so an association declared elsewhere must be named in full", + path, ds.Reference, pb.entityContext) } // An empty DestinationEntity is a by-name reference Mendix resolves to From f1fa02bbeab11e8f52f90fd560b7e6ec383e9f9e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 01:14:10 +0000 Subject: [PATCH 10/29] fix(screenshot): declare the real scheme so an https-root app can be logged into MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Under --hub the runtime boots with the public https root URL, so it marks its session cookies Secure and prefixes them __Host-. A headless browser reaching the app over http then cannot hold a session, every screenshot silently shows the login page, and rendering defects survive every other check. The login browser context now sends X-Forwarded-Proto: http when the target is http. On Mendix 10.24+ that header takes precedence over ApplicationRootUrl, so the runtime drops both Secure and the __Host- prefix. This is accurate rather than a workaround — the request genuinely is http — it is a no-op when the root URL is already http, and real users arriving over https through the hub are unaffected. Verified at the layer the bug lives in, against a live 11.12.1 runtime booted with an https root URL: the captured Playwright storage state goes from __Host-XASSESSIONID(secure=true) to XASSESSIONID(secure=false). The reported cause did NOT reproduce as stated, which is worth recording. On 127.0.0.1 an https root URL blocks nothing: loopback is a trustworthy origin, so Chromium accepts Secure and __Host- cookies there, and the app rendered clean with no console errors and no failed requests. The mechanism is real only from a NON-loopback http origin — a container hostname, a LAN address — where the origin is not trustworthy and the session cannot be held at all. The fix ships because it is correct and free, not because it was shown to repair the reported symptom. mxcli-formula1 §38 / suggested issue 7 --- .claude/skills/fix-issue.md | 1 + .claude/skills/mendix/run-local.md | 19 ++++++++++++++++++ cmd/mxcli/docker/screenshot_login.go | 15 +++++++++++++- cmd/mxcli/docker/screenshot_login_test.go | 24 +++++++++++++++++++++++ 4 files changed, 58 insertions(+), 1 deletion(-) diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index 67beba8a0..2a07d599e 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -431,3 +431,4 @@ extracting `OffsetExpression`/`LimitExpression`. | `publish entity … (TopSupported: No)` (or `SkipSupported`/`Countable`) parses, `DESCRIBE` reads it back as `No`, and the published `$metadata` still advertises `true`. A client then believes paging works when nothing implements it | `serializeEntitySet` hardcoded all three `QueryOptions` to `true`, ignoring the `*bool` fields the model already carried — the AST/model/DESCRIBE half of the feature shipped without the writer half | `sdk/mpr/writer_odata.go` (`boolOrTrue`, entity-set `QueryOptions`) | **A capability annotation on a microflow-backed resource is load-bearing, not decorative.** Mendix applies *no* query options to a read-microflow resource — it hands the request over and returns what comes back — so the annotation is the only thing a client has to go on, and an over-claim is not cosmetic: the client reads a whole collection believing it is a page. **Check the writer whenever a property round-trips correctly through DESCRIBE**: DESCRIBE reads the model, so a model-only feature describes perfectly and publishes wrong, and the round-trip test everyone reaches for cannot see it. A tri-state `*bool` needs an explicit nil policy at the boundary (`nil` = the platform default, only an explicit `false` opts out) or the pointer is pointless. Tests `writer_odata_test.go`; the control (hardcoded true) reproduces the over-claim. mxcli-formula1 §20 | | A published OData resource backed by a read microflow silently returns the wrong thing: `?$top=5` yields the whole collection with a 200, and a client re-reading a held row by key gets the collection default and adopts the FIRST row as that object's identity. No error anywhere — well-formed request, valid collection, correct `$count`, 200 | Two promises the service makes on the microflow's behalf and nothing checked: the `KEY` in `expose (…)`, and the `TopSupported`/`SkipSupported` annotations (which default to **true** when unspecified). Mendix applies no query options to a read-microflow resource — it hands over the request and returns what comes back | New `mdl/executor/validate_odata_read_contract.go` (MDL-ODATA02 key promise, MDL-ODATA03 capability over-claim), wired in `cmd/mxcli/cmd_check.go`; the response contract documented in `.claude/skills/mendix/odata-data-sharing.md` | **A read microflow cannot answer 400** — unlike an OData action or an insert/update/delete microflow, the read capability has no `System.HttpResponse` parameter ([docs](https://docs.mendix.com/refguide/published-odata-entity/#custom-http-response)). Its contract is therefore *declarative*: declaring `TopSupported: No` is the read path's only substitute for the refusal it cannot send. **Pick a trigger you can prove**: both rules fire only when the microflow takes no `System.HttpRequest` parameter, because then it provably cannot see a key or a query option; a microflow that does take it gets the benefit of the doubt, since proving *which* options it parses needs real analysis and a rule that guesses gets switched off. **Verify a check rule against a real parse, never a hand-built AST** — the visitor stores `ReadMode` as `MICROFLOW Module.Name` **upper-cased**, so a case-sensitive prefix match made the whole rule dead while it still looked right; the casing is now pinned by its own test. Tests `validate_odata_read_contract_test.go`, examples in `10-odata-examples.mdl` (both correct shapes: request-aware, and honestly declared). mxcli-formula1 §37/§20, suggested issues 2 and 3 | | No way to see what a running app's subsystem is doing: everything logs at `INFO`, the detail is not in the log at all, and raising the whole runtime to `TRACE` is unusable on a busy app | Nothing in mxcli drove the runtime's per-node log levels, though the M2EE admin API has exposed them all along | New `cmd/mxcli/docker/loglevels.go` (`GetLogSettings`, `SetLogLevels`, `NormalizeLogLevel`, `MatchLogNodes`), `cmd/mxcli/cmd_log.go` (`mxcli log list` / `log set`), documented in `.claude/skills/mendix/analyze-runtime.md` | **Probe an undocumented API before designing the command around it.** Every fact here came from a live 11.12.1 runtime: `get_log_settings` requires one of `node`/`subscriber`/`sort`; `sort` accepts only `node` and `subscriber`; `set_log_level` takes `{"nodes":[{name,level}],"force":bool}`. **The HTTP response for an AdminException says only "See logging output for details" — the actual message ("Please specify node, subscriber or sort option in params", "Unknown sort option: name", "Unknown LogNode X. Use the 'force' parameter…") is in the runtime log**, so probe with the runtime log open or you learn nothing. `force` means "allow a node that does not exist yet" and **permanently registers the name**, so a typo becomes a real empty node — hence it is opt-in, and an unknown node is an error by default. **Resist the subsystem-specific command**: this was proposed as `mxcli odata trace`, but `set_log_level` takes a list of nodes, so the primitive is generic and the OData knowledge belongs in docs. **The node list is a property of the APP, not of Mendix** — a node appears only once something registers it. A first pass concluded "there is no log node for a published OData service" from a project that had none; adding one service turns 57 nodes into 58 and `OData Publish` (with a space) appears, logging the full incoming URI at TRACE — exactly the question that motivated the command. Enumerating capabilities against one sample app and generalising is the trap; `log list` against *this* app is the answer. Distinguish "cannot reach the admin API" from "the runtime refused the request" (`ErrAdminUnreachable`) or the wrong hint buries the real one. Tests `cmd_log_test.go`, `loglevels_test.go`; verified end-to-end against a booted runtime (both arg forms, multi-node, typo refused, `--force` accepted, unreachable port). mxcli-formula1 suggested issue 4 | +| A headless browser cannot log in to an app started with `--hub`, so every screenshot silently shows the login page and rendering defects survive every other check | Under `--hub` the runtime boots with the public **https** root URL, so it marks session cookies `Secure` and prefixes them `__Host-`; a browser on a non-trustworthy http origin cannot store them | `cmd/mxcli/docker/screenshot_login.go` — the login browser context declares `X-Forwarded-Proto: http` when the target is http (Mendix 10.24+ lets that header override `ApplicationRootUrl`) | **The reported cause did not reproduce as stated, and saying so is part of the fix.** On 11.12.1 an https root URL does *not* block a headless browser on `127.0.0.1`: loopback is a **trustworthy origin**, so Chromium accepts `Secure`/`__Host-` cookies there — the app rendered clean, no console errors, no failed requests. The mechanism is real only for a **non-loopback** http origin (a container hostname, a LAN address). Fix shipped anyway because the header is *accurate* rather than a workaround (the request genuinely is http), it costs nothing on an already-http root URL, and real users over https are unaffected. Verified at the layer the bug lives in: the captured Playwright storage state goes from `__Host-XASSESSIONID(secure=true)` to `XASSESSIONID(secure=false)`. **`curl` cannot see this class of bug and neither can a loopback browser** — when a report blames cookie flags, check whether the origin is trustworthy before believing the flags are the blocker. mxcli-formula1 §38 / suggested issue 7 | diff --git a/.claude/skills/mendix/run-local.md b/.claude/skills/mendix/run-local.md index 48917d3d1..626f3a132 100644 --- a/.claude/skills/mendix/run-local.md +++ b/.claude/skills/mendix/run-local.md @@ -228,6 +228,25 @@ headless so it is not rebuilt for them (that would cost ~30 s on a loop whose po is two seconds) — the run prints a note instead, and a subsequent `run --local` restores it. +### Screenshots when the app has an https root URL (`--hub`) + +Under `--hub` the runtime boots with the public **https** root URL, so it marks +its session cookies `Secure` and prefixes them `__Host-`. The screenshot login +therefore declares the real scheme with `X-Forwarded-Proto: http`, which on +Mendix 10.24+ takes precedence over `ApplicationRootUrl` and drops both — so the +captured session is usable over http. + +Measured on 11.12.1, `__Host-XASSESSIONID (secure)` becomes `XASSESSIONID` +(not secure). Real users still arrive over https through the hub without that +header and still get `Secure` cookies. + +One correction to a common assumption: this is **not** needed for `127.0.0.1`. +Loopback is a *trustworthy origin*, so Chromium accepts `Secure` cookies there +and an app with an https root URL renders and logs in fine over +`http://127.0.0.1:8080`. It matters when the browser reaches the app from a +non-loopback host — a container name, a LAN address — where the origin is not +trustworthy and the session cannot be held at all. + ## Pixel-perfect page loop `--screenshot` captures a PNG (default `/.mxcli/run-local.png`) after boot diff --git a/cmd/mxcli/docker/screenshot_login.go b/cmd/mxcli/docker/screenshot_login.go index a9d42ca64..809ca94ee 100644 --- a/cmd/mxcli/docker/screenshot_login.go +++ b/cmd/mxcli/docker/screenshot_login.go @@ -64,7 +64,20 @@ const [appURL, username, password, storagePath] = process.argv.slice(3); const { chromium } = require(require.resolve("playwright-core", { paths: [pkgDir] })); (async () => { const b = await chromium.launch(); - const ctx = await b.newContext(); + // Tell the runtime the request really is http when it is. On Mendix 10.24+ + // X-Forwarded-Proto takes precedence over ApplicationRootUrl, so this drops + // the Secure attribute and the __Host- prefix from the session cookies. It + // matters when the app boots with an https root URL (as it does under --hub) + // and the browser reaches it over http from a NON-loopback host: 127.0.0.1 is + // a trustworthy origin and keeps Secure cookies happily, but a container + // hostname is not, and there the session cannot be held at all. + // + // Accurate rather than a workaround — the request is http — and a no-op when + // the root URL is already http. Real users arrive over https through the hub + // without this header and still get Secure cookies. + const insecure = new URL(appURL).protocol === "http:"; + const ctx = await b.newContext( + insecure ? { extraHTTPHeaders: { "X-Forwarded-Proto": "http" } } : {}); const p = await ctx.newPage(); await p.goto(appURL, { waitUntil: "load", timeout: 30000 }); let sawForm = false; diff --git a/cmd/mxcli/docker/screenshot_login_test.go b/cmd/mxcli/docker/screenshot_login_test.go index 89a0bb8d7..393e5f3c4 100644 --- a/cmd/mxcli/docker/screenshot_login_test.go +++ b/cmd/mxcli/docker/screenshot_login_test.go @@ -80,3 +80,27 @@ func TestReadLogTail(t *testing.T) { t.Errorf("readLogTail(small) = %q, %v", got, err) } } + +// mxcli-formula1 suggested issue 7: under --hub the app boots with an https +// ApplicationRootUrl, so the runtime marks its session cookies Secure and +// __Host-. A headless browser reaching the app over http from a non-loopback +// host cannot hold such a cookie, so login is impossible and every screenshot +// silently shows the login page. +// +// Mendix 10.24+ lets X-Forwarded-Proto override ApplicationRootUrl, which is +// both the fix and the truth: the request really is http. Verified against a +// live 11.12.1 runtime — the captured storage state goes from +// __Host-XASSESSIONID(secure=true) to XASSESSIONID(secure=false). +func TestLoginScript_DeclaresHttpWhenTheTargetIsHttp(t *testing.T) { + if !strings.Contains(loginScript, `"X-Forwarded-Proto": "http"`) { + t.Error("the login context does not declare the real scheme; Secure cookies will block a non-loopback browser") + } + // Conditioned on the scheme: an https target must not be told it is http, or + // the runtime would downgrade cookies for a session that is genuinely secure. + if !strings.Contains(loginScript, `new URL(appURL).protocol === "http:"`) { + t.Error("the header must be conditional on an http target") + } + if !strings.Contains(loginScript, "insecure ?") { + t.Error("expected the context options to branch on the scheme") + } +} From cd2ac98e7eaea6ffd3c96c97b9d468b487481904 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 02:13:11 +0000 Subject: [PATCH 11/29] fix(visitor): keep MDL comments out of the Mendix expressions they sit in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit declare $Msg String = 'a' + -- explain the second half 'b'; stored the comment INSIDE the expression, and the build failed CE0117 "Error(s) in expression". Nothing before mxbuild objected: `mxcli check` passed and DESCRIBE round-tripped the comment back out. extractOriginalText reads the raw input stream between two token positions. That is exactly what preserves an expression's spacing — and it also drags in every token the lexer sent to a hidden channel. MDL's `--` and `/* */` are `-> skip`, so they never appear in ctx.GetText() and always appear in a source slice. Mendix expressions have neither form. Expression sites now go through extractExpressionText, which strips MDL comments first. Two properties matter: - a comment becomes whitespace, never nothing, so `1 --c\n+ 2` cannot become `1+ 2` and `'a'--c\n'b'` cannot weld into one token; - single-quoted strings are respected, because a Mendix string may legitimately contain `--` or `/*` and stripping those would corrupt the value. The `''` escape that keeps a string open is handled too. OQL keeps extractOriginalText on purpose: `--` is legitimate SQL comment syntax in a view entity's query, and stripping it would change a different language's meaning. Verified end to end against mxbuild 11.12.1 on the same project and script: 1 error before, 0 after. The control matters here — with extractExpressionText bypassed the unit tests still passed, because they exercise the function rather than the wiring, and only the mxbuild run showed the call sites were converted. Swept mdl-examples/ and the 189 checkable skill MDL blocks: 0 failures. mxcli-formula1 §34 / suggested issue 11 --- .claude/skills/fix-issue.md | 1 + .../bug-tests/comment-in-expression.mdl | 30 ++++++++ mdl/visitor/visitor_helpers.go | 76 +++++++++++++++++++ mdl/visitor/visitor_microflow_actions.go | 2 +- mdl/visitor/visitor_microflow_statements.go | 10 +-- mdl/visitor/visitor_strip_comments_test.go | 58 ++++++++++++++ 6 files changed, 171 insertions(+), 6 deletions(-) create mode 100644 mdl-examples/bug-tests/comment-in-expression.mdl create mode 100644 mdl/visitor/visitor_strip_comments_test.go diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index 2a07d599e..822234933 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -432,3 +432,4 @@ extracting `OffsetExpression`/`LimitExpression`. | A published OData resource backed by a read microflow silently returns the wrong thing: `?$top=5` yields the whole collection with a 200, and a client re-reading a held row by key gets the collection default and adopts the FIRST row as that object's identity. No error anywhere — well-formed request, valid collection, correct `$count`, 200 | Two promises the service makes on the microflow's behalf and nothing checked: the `KEY` in `expose (…)`, and the `TopSupported`/`SkipSupported` annotations (which default to **true** when unspecified). Mendix applies no query options to a read-microflow resource — it hands over the request and returns what comes back | New `mdl/executor/validate_odata_read_contract.go` (MDL-ODATA02 key promise, MDL-ODATA03 capability over-claim), wired in `cmd/mxcli/cmd_check.go`; the response contract documented in `.claude/skills/mendix/odata-data-sharing.md` | **A read microflow cannot answer 400** — unlike an OData action or an insert/update/delete microflow, the read capability has no `System.HttpResponse` parameter ([docs](https://docs.mendix.com/refguide/published-odata-entity/#custom-http-response)). Its contract is therefore *declarative*: declaring `TopSupported: No` is the read path's only substitute for the refusal it cannot send. **Pick a trigger you can prove**: both rules fire only when the microflow takes no `System.HttpRequest` parameter, because then it provably cannot see a key or a query option; a microflow that does take it gets the benefit of the doubt, since proving *which* options it parses needs real analysis and a rule that guesses gets switched off. **Verify a check rule against a real parse, never a hand-built AST** — the visitor stores `ReadMode` as `MICROFLOW Module.Name` **upper-cased**, so a case-sensitive prefix match made the whole rule dead while it still looked right; the casing is now pinned by its own test. Tests `validate_odata_read_contract_test.go`, examples in `10-odata-examples.mdl` (both correct shapes: request-aware, and honestly declared). mxcli-formula1 §37/§20, suggested issues 2 and 3 | | No way to see what a running app's subsystem is doing: everything logs at `INFO`, the detail is not in the log at all, and raising the whole runtime to `TRACE` is unusable on a busy app | Nothing in mxcli drove the runtime's per-node log levels, though the M2EE admin API has exposed them all along | New `cmd/mxcli/docker/loglevels.go` (`GetLogSettings`, `SetLogLevels`, `NormalizeLogLevel`, `MatchLogNodes`), `cmd/mxcli/cmd_log.go` (`mxcli log list` / `log set`), documented in `.claude/skills/mendix/analyze-runtime.md` | **Probe an undocumented API before designing the command around it.** Every fact here came from a live 11.12.1 runtime: `get_log_settings` requires one of `node`/`subscriber`/`sort`; `sort` accepts only `node` and `subscriber`; `set_log_level` takes `{"nodes":[{name,level}],"force":bool}`. **The HTTP response for an AdminException says only "See logging output for details" — the actual message ("Please specify node, subscriber or sort option in params", "Unknown sort option: name", "Unknown LogNode X. Use the 'force' parameter…") is in the runtime log**, so probe with the runtime log open or you learn nothing. `force` means "allow a node that does not exist yet" and **permanently registers the name**, so a typo becomes a real empty node — hence it is opt-in, and an unknown node is an error by default. **Resist the subsystem-specific command**: this was proposed as `mxcli odata trace`, but `set_log_level` takes a list of nodes, so the primitive is generic and the OData knowledge belongs in docs. **The node list is a property of the APP, not of Mendix** — a node appears only once something registers it. A first pass concluded "there is no log node for a published OData service" from a project that had none; adding one service turns 57 nodes into 58 and `OData Publish` (with a space) appears, logging the full incoming URI at TRACE — exactly the question that motivated the command. Enumerating capabilities against one sample app and generalising is the trap; `log list` against *this* app is the answer. Distinguish "cannot reach the admin API" from "the runtime refused the request" (`ErrAdminUnreachable`) or the wrong hint buries the real one. Tests `cmd_log_test.go`, `loglevels_test.go`; verified end-to-end against a booted runtime (both arg forms, multi-node, typo refused, `--force` accepted, unreachable port). mxcli-formula1 suggested issue 4 | | A headless browser cannot log in to an app started with `--hub`, so every screenshot silently shows the login page and rendering defects survive every other check | Under `--hub` the runtime boots with the public **https** root URL, so it marks session cookies `Secure` and prefixes them `__Host-`; a browser on a non-trustworthy http origin cannot store them | `cmd/mxcli/docker/screenshot_login.go` — the login browser context declares `X-Forwarded-Proto: http` when the target is http (Mendix 10.24+ lets that header override `ApplicationRootUrl`) | **The reported cause did not reproduce as stated, and saying so is part of the fix.** On 11.12.1 an https root URL does *not* block a headless browser on `127.0.0.1`: loopback is a **trustworthy origin**, so Chromium accepts `Secure`/`__Host-` cookies there — the app rendered clean, no console errors, no failed requests. The mechanism is real only for a **non-loopback** http origin (a container hostname, a LAN address). Fix shipped anyway because the header is *accurate* rather than a workaround (the request genuinely is http), it costs nothing on an already-http root URL, and real users over https are unaffected. Verified at the layer the bug lives in: the captured Playwright storage state goes from `__Host-XASSESSIONID(secure=true)` to `XASSESSIONID(secure=false)`. **`curl` cannot see this class of bug and neither can a loopback browser** — when a report blames cookie flags, check whether the origin is trustworthy before believing the flags are the blocker. mxcli-formula1 §38 / suggested issue 7 | +| A `--` comment written between two operands of a Mendix expression ends up **inside** the expression; the build fails **CE0117** "Error(s) in expression". `mxcli check` passes and `DESCRIBE` round-trips the comment, so nothing before mxbuild objects | `extractOriginalText` reads the raw input stream between two token positions — which is exactly what preserves an expression's spacing, and also drags in every token the lexer sent to a hidden channel. MDL's `--` and `/* */` are `-> skip`, so they never appear in `ctx.GetText()` but always appear in the source slice | `mdl/visitor/visitor_helpers.go` (`stripMDLComments`, `extractExpressionText`), applied at the six microflow-expression sites in `visitor_microflow_statements.go` / `visitor_microflow_actions.go` | **The ANTLR trap: `ctx.GetText()` excludes hidden tokens, a source-interval slice includes them.** Any code reaching for original text to preserve formatting inherits every comment in that span. **Replace a comment with whitespace, never with nothing** — `1 --c\n+ 2` must not become `1+ 2`, and `'a'--c\n'b'` must not weld into one token. **Respect single-quoted strings**: a Mendix string may legitimately contain `--` or `/*`, and stripping those corrupts the value (tested both, plus the `''` escape that keeps a string open). **Left OQL alone on purpose** — `visitor_entity.go` uses the same helper for view-entity queries, where `--` is legitimate SQL comment syntax; stripping it would change a different language's meaning. **The unit test alone would not have caught a wiring mistake**: it still passed with `extractExpressionText` bypassed, and only the mxbuild run (1 error → 0, same script, same project) proved the call sites were converted. Tests `visitor_strip_comments_test.go`, repro `mdl-examples/bug-tests/comment-in-expression.mdl`. mxcli-formula1 §34 / suggested issue 11 | diff --git a/mdl-examples/bug-tests/comment-in-expression.mdl b/mdl-examples/bug-tests/comment-in-expression.mdl new file mode 100644 index 000000000..8899496aa --- /dev/null +++ b/mdl-examples/bug-tests/comment-in-expression.mdl @@ -0,0 +1,30 @@ +-- mxcli-formula1 suggested issue 11: an MDL comment between two operands was +-- lifted into the Mendix expression along with the source text, and the build +-- failed CE0117 "Error(s) in expression". Mendix expressions have no `--`. +-- +-- Verified against mxbuild 11.12.1: 1 error before the fix, 0 after. + +create module CmtExpr; + +create microflow CmtExpr.Build () +returns string as $Msg +begin + -- A comment between operands: the classic case. + declare $Msg String = 'a' + + -- explain the second half + 'b'; + + -- A block comment mid-expression. + declare $N Integer = 1 /* one */ + 2; + + -- A string that legitimately CONTAINS the comment markers must survive intact. + declare $Literal String = 'a--b' + '/*not a comment*/'; + + -- An escaped quote keeps the string open, so the `--` inside stays. + declare $Escaped String = 'it''s -- fine'; + + return $Msg; +end; +/ + +drop module CmtExpr; diff --git a/mdl/visitor/visitor_helpers.go b/mdl/visitor/visitor_helpers.go index ac5337d53..3b5ff0976 100644 --- a/mdl/visitor/visitor_helpers.go +++ b/mdl/visitor/visitor_helpers.go @@ -616,6 +616,82 @@ func stripExpressionIdentifierQuotes(s string) string { return b.String() } +// extractExpressionText lifts a Mendix expression's source text, without the MDL +// comments the raw input stream would otherwise carry into the model. +// +// Use this rather than extractOriginalText for anything stored as a Mendix +// expression. OQL keeps extractOriginalText: `--` is a legitimate SQL comment +// there, and stripping it would change a different language's meaning. +func extractExpressionText(ctx antlr.ParserRuleContext) string { + return stripMDLComments(extractOriginalText(ctx)) +} + +// stripMDLComments removes MDL comments from text lifted out of the input stream. +// +// extractOriginalText reads the raw source between two token positions, which is +// what preserves an expression's spacing — and also drags in anything the lexer +// sent to a hidden channel. MDL comments are `--` to end of line and `/* … */`; +// Mendix expressions use neither, so a comment written between two operands +// ended up stored *inside* the expression and the build failed with CE0117 +// "Error(s) in expression" (mxcli-formula1 suggested issue 11): +// +// declare $Msg String = 'a' + +// -- explain the second half +// 'b'; +// +// A comment is replaced by a single space rather than deleted, so `1 --c\n+ 2` +// does not become `1+ 2`… and more importantly `'a'--c\n'b'` cannot silently +// weld into one token. +// +// Single-quoted string literals are respected: a Mendix string may legitimately +// contain `--` or `/*`, and removing those would corrupt the value. +func stripMDLComments(s string) string { + if !strings.Contains(s, "--") && !strings.Contains(s, "/*") { + return s + } + var b strings.Builder + b.Grow(len(s)) + inString := false + for i := 0; i < len(s); i++ { + c := s[i] + if c == '\'' { + if inString && i+1 < len(s) && s[i+1] == '\'' { + b.WriteByte(c) + b.WriteByte(s[i+1]) + i++ + continue + } + inString = !inString + b.WriteByte(c) + continue + } + if !inString && c == '-' && i+1 < len(s) && s[i+1] == '-' { + for i < len(s) && s[i] != '\n' { + i++ + } + // Keep the newline itself: it is the statement's own layout, and a + // multi-line expression stays readable in DESCRIBE. + if i < len(s) { + b.WriteByte('\n') + } + continue + } + if !inString && c == '/' && i+1 < len(s) && s[i+1] == '*' { + end := strings.Index(s[i+2:], "*/") + if end < 0 { + // Unterminated: the parser would already have complained. Drop + // the rest rather than emit half a comment into the model. + break + } + i += 2 + end + 1 + b.WriteByte(' ') + continue + } + b.WriteByte(c) + } + return b.String() +} + // ---------------------------------------------------------------------------- // Microflow Statements // ---------------------------------------------------------------------------- diff --git a/mdl/visitor/visitor_microflow_actions.go b/mdl/visitor/visitor_microflow_actions.go index fba6bedff..32e1c4124 100644 --- a/mdl/visitor/visitor_microflow_actions.go +++ b/mdl/visitor/visitor_microflow_actions.go @@ -343,7 +343,7 @@ func appendSourceExpressionSuffix( expr ast.Expression, suffix string, ) ast.Expression { - source := strings.TrimSpace(extractOriginalText(exprCtx.(antlr.ParserRuleContext))) + source := strings.TrimSpace(extractExpressionText(exprCtx.(antlr.ParserRuleContext))) innerExpr := expr if sourceExpr, ok := expr.(*ast.SourceExpr); ok { source = sourceExpr.Source diff --git a/mdl/visitor/visitor_microflow_statements.go b/mdl/visitor/visitor_microflow_statements.go index 1ca597301..3394beffa 100644 --- a/mdl/visitor/visitor_microflow_statements.go +++ b/mdl/visitor/visitor_microflow_statements.go @@ -1163,7 +1163,7 @@ func buildRetrieveStatement(ctx parser.IRetrieveStatementContext) *ast.RetrieveS if xpathExpr := xcCtx.XpathExpr(); xpathExpr != nil { andExprs = append(andExprs, buildXPathSourceExpression(xpathExpr)) if prc, ok := xpathExpr.(antlr.ParserRuleContext); ok { - if source := strings.TrimSpace(extractOriginalText(prc)); source != "" { + if source := strings.TrimSpace(extractExpressionText(prc)); source != "" { predicateSources = append(predicateSources, normalizeXPathTokens("["+source+"]")) } } @@ -1221,7 +1221,7 @@ func retrieveRangeExpressionSource(exprCtx parser.IExpressionContext) string { return "" } if prc, ok := exprCtx.(antlr.ParserRuleContext); ok { - if source := strings.TrimSpace(extractOriginalText(prc)); source != "" { + if source := strings.TrimSpace(extractExpressionText(prc)); source != "" { return source } } @@ -1446,7 +1446,7 @@ func buildSourceExpression(ctx parser.IExpressionContext) ast.Expression { } expr := buildExpression(ctx) if prc, ok := ctx.(antlr.ParserRuleContext); ok { - if source := strings.TrimSpace(extractOriginalText(prc)); source != "" { + if source := strings.TrimSpace(extractExpressionText(prc)); source != "" { if shouldPreserveExpressionSource(source) { return &ast.SourceExpr{Expression: expr, Source: stripExpressionIdentifierQuotes(source)} } @@ -1461,7 +1461,7 @@ func buildXPathSourceExpression(ctx parser.IXpathExprContext) ast.Expression { } expr := buildXPathExpr(ctx) if prc, ok := ctx.(antlr.ParserRuleContext); ok { - if source := strings.TrimSpace(extractOriginalText(prc)); source != "" { + if source := strings.TrimSpace(extractExpressionText(prc)); source != "" { // Requote any bare [%token%] so the stored constraint passes mx check // (CE0161) — the original source preserves the unquoted form (#641). return &ast.SourceExpr{Expression: expr, Source: stripExpressionIdentifierQuotes(normalizeXPathTokens(source))} @@ -1487,7 +1487,7 @@ func buildRetrieveWhereExpression(ctx parser.IExpressionContext) ast.Expression } } if prc, ok := ctx.(antlr.ParserRuleContext); ok { - if source := strings.TrimSpace(extractOriginalText(prc)); source != "" { + if source := strings.TrimSpace(extractExpressionText(prc)); source != "" { if shouldPreserveExpressionSource(source) || strings.Contains(source, "/") { return &ast.SourceExpr{Expression: expr, Source: stripExpressionIdentifierQuotes(source)} } diff --git a/mdl/visitor/visitor_strip_comments_test.go b/mdl/visitor/visitor_strip_comments_test.go new file mode 100644 index 000000000..eab6061e6 --- /dev/null +++ b/mdl/visitor/visitor_strip_comments_test.go @@ -0,0 +1,58 @@ +// SPDX-License-Identifier: Apache-2.0 + +package visitor + +import "testing" + +// mxcli-formula1 suggested issue 11: extractOriginalText reads the raw source +// between two token positions — which is what preserves an expression's spacing, +// and also drags in the comments the lexer sent to a hidden channel. A `--` +// comment between two operands was stored inside the Mendix expression and the +// build failed CE0117 "Error(s) in expression". Verified end to end: the same +// script went from 1 error to 0 against mxbuild 11.12.1. +func TestStripMDLComments(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {"nothing to do", "'a' + 'b'", "'a' + 'b'"}, + { + "line comment between operands", + "'a' +\n -- explain\n 'b'", + "'a' +\n \n 'b'", + }, + {"trailing line comment", "$x + 1 -- why", "$x + 1 "}, + {"block comment", "$x /* mid */ + 1", "$x + 1"}, + + // A Mendix string may legitimately contain these. Removing them would + // silently corrupt the value, which is worse than the bug being fixed. + {"-- inside a string literal", "'a--b'", "'a--b'"}, + {"/* inside a string literal", "'a/*b'", "'a/*b'"}, + {"comment marker after a string", "'a--b' -- real", "'a--b' "}, + { + "escaped quote keeps the string open", + "'it''s -- fine'", + "'it''s -- fine'", + }, + + // A comment becomes whitespace, never nothing: gluing the operands + // together would change the expression rather than clean it. + {"tokens must not weld", "1 --c\n+ 2", "1 \n+ 2"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := stripMDLComments(tt.in); got != tt.want { + t.Errorf("stripMDLComments(%q)\n got: %q\nwant: %q", tt.in, got, tt.want) + } + }) + } +} + +// An unterminated block comment is a parse error the parser reports; the +// stripper must not emit half of it into the model. +func TestStripMDLComments_UnterminatedBlock(t *testing.T) { + if got := stripMDLComments("$x + /* oops"); got != "$x + " { + t.Errorf("got %q", got) + } +} From 416b4ee237011a39e82f7d4d97d6d94f7dc65b63 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 08:32:23 +0000 Subject: [PATCH 12/29] fix(check): stop recommending a remedy Mendix rejects (CE6585) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught this: the doctype gate failed on 10-odata-examples.mdl with [CE6585] "Published entity 'OdTest.LiveRow' must have a key defined." at Published OData service 'OdTest.BulkAPI' The example demonstrated an "honest contract" resource by omitting the KEY — the alternative MDL-ODATA02 offers to answering a key lookup. Mendix does not permit that: a published entity must have a key. So the rule shipped in this branch was recommending something the platform rejects, and the example demonstrated it. Corrected in all three places the claim appeared: the rule's suggestion, the example, and odata-data-sharing.md. The resulting position is sharper than the one it replaces — query options you may decline, the key you may not. A microflow-backed resource whose rows a client can hold must answer the key lookup; there is no opt-out. I validated that example with `mxcli check`, which is parse-only and cannot see a CE code, rather than the integration gate — the same mistake made earlier in this branch on 18-folder-examples.mdl, and already written down in fix-issue.md at the time. The new symptom row states the rule plainly: a change to mdl-examples/doctype-tests/ is not done until the gate has run on it. mxcli-formula1 §37 follow-up --- .claude/skills/fix-issue.md | 1 + .claude/skills/mendix/odata-data-sharing.md | 7 ++++++- mdl-examples/doctype-tests/10-odata-examples.mdl | 10 +++++++--- mdl/executor/validate_odata_read_contract.go | 4 +++- 4 files changed, 17 insertions(+), 5 deletions(-) diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index 822234933..352b8efa4 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -433,3 +433,4 @@ extracting `OffsetExpression`/`LimitExpression`. | No way to see what a running app's subsystem is doing: everything logs at `INFO`, the detail is not in the log at all, and raising the whole runtime to `TRACE` is unusable on a busy app | Nothing in mxcli drove the runtime's per-node log levels, though the M2EE admin API has exposed them all along | New `cmd/mxcli/docker/loglevels.go` (`GetLogSettings`, `SetLogLevels`, `NormalizeLogLevel`, `MatchLogNodes`), `cmd/mxcli/cmd_log.go` (`mxcli log list` / `log set`), documented in `.claude/skills/mendix/analyze-runtime.md` | **Probe an undocumented API before designing the command around it.** Every fact here came from a live 11.12.1 runtime: `get_log_settings` requires one of `node`/`subscriber`/`sort`; `sort` accepts only `node` and `subscriber`; `set_log_level` takes `{"nodes":[{name,level}],"force":bool}`. **The HTTP response for an AdminException says only "See logging output for details" — the actual message ("Please specify node, subscriber or sort option in params", "Unknown sort option: name", "Unknown LogNode X. Use the 'force' parameter…") is in the runtime log**, so probe with the runtime log open or you learn nothing. `force` means "allow a node that does not exist yet" and **permanently registers the name**, so a typo becomes a real empty node — hence it is opt-in, and an unknown node is an error by default. **Resist the subsystem-specific command**: this was proposed as `mxcli odata trace`, but `set_log_level` takes a list of nodes, so the primitive is generic and the OData knowledge belongs in docs. **The node list is a property of the APP, not of Mendix** — a node appears only once something registers it. A first pass concluded "there is no log node for a published OData service" from a project that had none; adding one service turns 57 nodes into 58 and `OData Publish` (with a space) appears, logging the full incoming URI at TRACE — exactly the question that motivated the command. Enumerating capabilities against one sample app and generalising is the trap; `log list` against *this* app is the answer. Distinguish "cannot reach the admin API" from "the runtime refused the request" (`ErrAdminUnreachable`) or the wrong hint buries the real one. Tests `cmd_log_test.go`, `loglevels_test.go`; verified end-to-end against a booted runtime (both arg forms, multi-node, typo refused, `--force` accepted, unreachable port). mxcli-formula1 suggested issue 4 | | A headless browser cannot log in to an app started with `--hub`, so every screenshot silently shows the login page and rendering defects survive every other check | Under `--hub` the runtime boots with the public **https** root URL, so it marks session cookies `Secure` and prefixes them `__Host-`; a browser on a non-trustworthy http origin cannot store them | `cmd/mxcli/docker/screenshot_login.go` — the login browser context declares `X-Forwarded-Proto: http` when the target is http (Mendix 10.24+ lets that header override `ApplicationRootUrl`) | **The reported cause did not reproduce as stated, and saying so is part of the fix.** On 11.12.1 an https root URL does *not* block a headless browser on `127.0.0.1`: loopback is a **trustworthy origin**, so Chromium accepts `Secure`/`__Host-` cookies there — the app rendered clean, no console errors, no failed requests. The mechanism is real only for a **non-loopback** http origin (a container hostname, a LAN address). Fix shipped anyway because the header is *accurate* rather than a workaround (the request genuinely is http), it costs nothing on an already-http root URL, and real users over https are unaffected. Verified at the layer the bug lives in: the captured Playwright storage state goes from `__Host-XASSESSIONID(secure=true)` to `XASSESSIONID(secure=false)`. **`curl` cannot see this class of bug and neither can a loopback browser** — when a report blames cookie flags, check whether the origin is trustworthy before believing the flags are the blocker. mxcli-formula1 §38 / suggested issue 7 | | A `--` comment written between two operands of a Mendix expression ends up **inside** the expression; the build fails **CE0117** "Error(s) in expression". `mxcli check` passes and `DESCRIBE` round-trips the comment, so nothing before mxbuild objects | `extractOriginalText` reads the raw input stream between two token positions — which is exactly what preserves an expression's spacing, and also drags in every token the lexer sent to a hidden channel. MDL's `--` and `/* */` are `-> skip`, so they never appear in `ctx.GetText()` but always appear in the source slice | `mdl/visitor/visitor_helpers.go` (`stripMDLComments`, `extractExpressionText`), applied at the six microflow-expression sites in `visitor_microflow_statements.go` / `visitor_microflow_actions.go` | **The ANTLR trap: `ctx.GetText()` excludes hidden tokens, a source-interval slice includes them.** Any code reaching for original text to preserve formatting inherits every comment in that span. **Replace a comment with whitespace, never with nothing** — `1 --c\n+ 2` must not become `1+ 2`, and `'a'--c\n'b'` must not weld into one token. **Respect single-quoted strings**: a Mendix string may legitimately contain `--` or `/*`, and stripping those corrupts the value (tested both, plus the `''` escape that keeps a string open). **Left OQL alone on purpose** — `visitor_entity.go` uses the same helper for view-entity queries, where `--` is legitimate SQL comment syntax; stripping it would change a different language's meaning. **The unit test alone would not have caught a wiring mistake**: it still passed with `extractExpressionText` bypassed, and only the mxbuild run (1 error → 0, same script, same project) proved the call sites were converted. Tests `visitor_strip_comments_test.go`, repro `mdl-examples/bug-tests/comment-in-expression.mdl`. mxcli-formula1 §34 / suggested issue 11 | +| A published OData entity with no `KEY` fails the build with **CE6585** "Published entity 'X' must have a key defined." — so any advice of the form "drop the KEY" is impossible to follow | Mendix requires every published entity to have a key. MDL-ODATA02's suggestion offered "…or drop the KEY" as the alternative to answering a key lookup, and a doctype example demonstrated that non-existent option | `mdl/executor/validate_odata_read_contract.go` (suggestion text), `mdl-examples/doctype-tests/10-odata-examples.mdl`, `.claude/skills/mendix/odata-data-sharing.md` | **Query options you may decline; the key you may not.** A microflow-backed resource whose rows a client can hold *must* answer the key lookup — there is no opt-out, which makes MDL-ODATA02's real remedy singular rather than a choice. **Verify the remedy a diagnostic recommends, not just the diagnosis** — the rule correctly identified an unanswerable KEY and then proposed something mxbuild rejects, which is worse than saying nothing. This is the second time in one session that a doctype example was validated with `mxcli check` (parse-only) instead of the integration gate; `mxcli check` cannot see CE-codes at all, so **any change to `mdl-examples/doctype-tests/` needs `go test -tags integration -run TestMxCheck_DoctypeScripts/