From 861be36fd9e1bae494e0f6d7340da6bf6ffa7ba2 Mon Sep 17 00:00:00 2001 From: Ako Date: Thu, 8 Oct 2026 08:41:28 +0000 Subject: [PATCH 01/24] fix(check): count joins inside a REST call's error handler (MDL-FLOW02) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getErrorHandlerBody kept its own list of statements with an ON ERROR clause and lacked the REST statements (and call workflow, the mapping statements, commit, delete, …). The merge/join label walk read handler bodies through it, so a `join` in a REST call's custom error handler went unseen and its merge was reported as "nothing joins it". That is the retry loop of Studio Pro-authored OIDC.GET/POST/PUT/PATCH/DELETE and AWSAuthentication.POST_v1_GetCallerIdentity_ValidateCredentials, which build at 0 errors; exec refused their describe output. getErrorHandlerBody now reads the shared StatementErrorHandling table (mdl/exprcheck/adapters), as stmtErrorHandling already does. Co-Authored-By: Claude Opus 5.5 --- .../fix-issue/findings/mdl-executor.jsonl | 1 + .../bug-tests/check-fp-rest-handler-join.mdl | 41 ++++++++++ mdl/executor/validate.go | 55 +++----------- .../validate_microflow_merge_join_test.go | 76 +++++++++++++++++++ 4 files changed, 128 insertions(+), 45 deletions(-) create mode 100644 mdl-examples/bug-tests/check-fp-rest-handler-join.mdl create mode 100644 mdl/executor/validate_microflow_merge_join_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index 5cdf70b25..8376ee72b 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -884,3 +884,4 @@ {"area": "mdl/executor", "date": "2026-10-07", "symptom": "A Microflow-typed Java action argument with `)` on the next line — `call java action M.DoSomething(Flow = M.SUB_Target⏎ );` — reported on 0.23/0.24 as passing check/exec and then failing mx check with CE1613 \"The selected microflow 'MyModule.SUB_Target⏎' no longer exists\"; on current main the same script is REFUSED by both `check --references` and exec as \"parameter Flow is of type Microflow, which takes a microflow name ('Module.Microflow'), not MyModule.SUB_Target\" — the message names a valid microflow name. `Flow = empty⏎)` was refused too (or, without the guard, stored the text `empty` as a microflow reference).", "cause": "buildCallArgumentList keeps the whitespace between an argument's last token and `)` by wrapping the value in ast.SourceExpr (expression round-trip). The Microflow-typed path type-switched on the wrapper: microflowParamArgRefusal (#1210) and isEmptyJavaActionArgument saw a SourceExpr, not a QualifiedNameExpr / LiteralExpr. The original stored-newline form had already been removed by #898 (mendixexpr.String trims a SourceExpr's trailing whitespace), which is why the reported symptom no longer reproduces and the bug now surfaces as a false refusal instead.", "file": "`mdl/executor/javaaction_flow_param.go` (`javaActionArgumentValue`, `microflowParamArgRefusal`), `mdl/executor/cmd_microflows_builder_calls.go` (`isEmptyJavaActionArgument`, Microflow-typed mapping), `mdl/executor/javaaction_flow_param_whitespace_test.go`", "insight": "Any executor decision that type-switches on a call/member argument (`arg.Value.(type)`) must unwrap ast.SourceExpr first: the visitor wraps the value only when whitespace follows it, so the hand-built QualifiedNameExpr/LiteralExpr in existing unit tests never exercises the wrapped form and every such switch passes its tests. Build test arguments with visitor.Build from real MDL, with and without a line break before `)`. Wrong turn to skip: the issue blames the lexer/name text — the name is parsed cleanly; the trailing whitespace is a deliberate SourceExpr suffix, and stripping it in the visitor would break describe round-trip layout (#898). Control: reverting the fix with #1210's guard stubbed no longer reproduces the newline on this tree (#898 trims it), so the live regression is the refusal, which the reverted fix reproduces in both the builder test and the check/exec agreement test.", "refs": ["mendixlabs/mxcli#1282", "mendixlabs/mxcli#1210", "#898"], "ce": ["CE1613"]} {"area": "mdl/executor", "date": "2026-10-07", "symptom": "`mxcli check script.mdl -p app.mpr --references` printed \"Check passed!\" for `grant read * on entity System.Nope to M.R`, for a grant to `M.NopeRole`, and for every other GRANT/REVOKE form naming a missing entity, document, member or role; also for `create user role R ( ModuleRoles: (M.NopeRole) )`, `alter user role … add module roles`, and a demo user with an unknown user role or entity (measured on Evora Factory Management, 10.24.15). exec refused the grants after the earlier statements were written; the user-role and demo-user shapes it wrote unresolved (CE1613 at build).", "cause": "No validate path resolved a security statement's names. validateWithContext's switch had no case for any of them (only the role-free validateCrossModuleGrant ran), and the user-role/demo-user executors store module and user role names verbatim without resolving them, so neither check nor exec stood between a typo and the model.", "file": "`mdl/executor/validate_grant_refs.go` (`validateGrantReferences`), wired into `validateProgramWithWarnings` in `validate.go`; test `check_grant_references_pedapp_test.go`", "insight": "Two things made the obvious version wrong. (1) What check refuses has to be what exec refuses, form by form: exec refuses an unknown role on a GRANT and on an entity REVOKE, but a document REVOKE (and `alter user role … drop module roles`) of an unknown role is a reported no-op, which keeps cleanup scripts re-runnable after the role is dropped — refusing it would make check louder than exec. Likewise every System entity is refused by exec (refuseSystemEntityGrant), so `grant … on entity System.User` must be refused by check too, while System MODULE roles in a user role must pass. (2) A script's own effects are not only its CREATE statements: creating a microflow/nanoflow/page in a module with no module roles auto-creates `.User` (defaultDocumentAccessRoles), and doctype-tests/02b grants to exactly that role — the first version flagged 10 statements there. Walking the program in statement order (not the whole-program scriptContext) gives the forward-reference hint for free. Performance trap: reading every module's security costs ~5s on Evora; read only the modules a statement names (~0.1s).", "refs": ["mdl-examples/bug-tests/check-grant-unknown-reference-refused.mdl", "mdl-examples/bug-tests/check-grant-unknown-reference.mdl", "ako/mxcli#1020"]} {"area": "mdl/executor", "date": "2026-10-07", "symptom": "A DataGrid 2 column's `Visible:` expression is stored as `true` (always visible) for every spelling but the old quoted one: `Visible: $showPrices`, `Visible: if … then … else …`, `visible: not(…)`, `Visible: [cond]`. check clean, exec reports \"Created page\", describe shows no Visible. `alter page … set (Visible: ) on grid column(…)` is refused as \"column property VisibleIf not found\"; `insert` of such a column drops it, and drops `Visible: false` too.", "cause": "The visitor lowers every expression spelling of Visible to the key VisibleIf (the page-widget conditional-visibility key) and keeps only plain values under Visible. The column writers read only visible/Visible: the widget engine by schema key + types.ItemPropertyAliases (no VisibleIf alias), the ALTER mutator's resolveColumnPropertyKey the same table, and widgetobj.BuildDataGrid2Column (ALTER insert/replace via buildColumnSpecFromAST) the exact key \"Visible\" as a string only, so a bool false fell to the default too. The mirror image of widget-visible-expression.mdl, where page widgets read VisibleIf and dropped Visible.", "file": "`mdl/types/widget_item_aliases.go` (`visible` ← `VisibleIf`), `mdl/executor/widget_engine.go` (WidgetDefGeneratorVersion 18), `mdl/executor/cmd_pages_builder_v3_widgets.go` (`columnSpecProperties`), `mdl/executor/cmd_pages_describe_output.go` (column Visible via widgetConditionMDL), `mdl/executor/validate_column_visible_scope.go` (MDL-WIDGET43)", "insight": "When the visitor lowers one MDL property to two AST keys by value shape, every consumer must read both — grep the consumers of the key the visitor writes, not of the property name. Making the value persist exposed what the drop had hidden: a column's visible expression has no row object (no dataSource in the widget schema, unlike columnClass), so every $currentObject example became CE0117 at build — measured on 11.14.0 — and needed a check rule (MDL-WIDGET43) in the same change. The widget def's per-item `dataSource` field is the signal for whether $currentObject exists.", "refs": ["mdl-examples/bug-tests/datagrid-column-visible-expression.mdl", "mdl-examples/bug-tests/widget-visible-expression.mdl"]} +{"area": "mdl/executor", "date": "2026-10-08", "symptom": "`mxcli check` reports MDL-FLOW02 \"merge rejoin1: nothing joins it\" when the only `join rejoin1;` sits in the custom error handler of a `call rest service` \u2014 the retry loop of Studio Pro-authored OIDC.GET/POST/PUT/PATCH/DELETE and AWSAuthentication.POST_v1_GetCallerIdentity_ValidateCredentials (Evora, 0 errors under mx check), so exec refused their describe output", "cause": "getErrorHandlerBody (validate.go) kept its own type switch of statements with an ON ERROR clause and had drifted: no RestCallStmt, SendRestRequestStmt, CallWorkflowStmt, mapping statements, commit, delete, \u2026 Every walk built on it \u2014 merge/join labels, reference collection, import-mapping range, layout lanes \u2014 skipped those handler bodies", "file": "`mdl/executor/validate.go` (`getErrorHandlerBody` now delegates to `stmtErrorHandling` \u2192 `adapters.StatementErrorHandling`)", "insight": "The adapters table already existed precisely because per-walk lists drift (its comment says so, citing MDL076); this was a fifth copy nobody had pointed at it. When a rule misses a construct inside a handler, grep for every accessor of ErrorHandling.Body before patching the one switch in front of you. The control that the rule still fires (merge with no join anywhere) is in the same test file", "refs": ["mdl-examples/bug-tests/check-fp-rest-handler-join.mdl"], "rules": ["MDL-FLOW02"]} diff --git a/mdl-examples/bug-tests/check-fp-rest-handler-join.mdl b/mdl-examples/bug-tests/check-fp-rest-handler-join.mdl new file mode 100644 index 000000000..c48cbbc0d --- /dev/null +++ b/mdl-examples/bug-tests/check-fp-rest-handler-join.mdl @@ -0,0 +1,41 @@ +-- ============================================================================ +-- MDL-FLOW02 false positive: a `join` inside a REST call's error handler +-- ============================================================================ +-- +-- Symptom (before fix): `mxcli check` reported "merge rejoin1: nothing joins +-- it. [MDL-FLOW02]" when the only join to the merge sat inside the custom +-- error handler of a `call rest service`. That is the retry loop Studio Pro- +-- authored OIDC microflows use (OIDC.GET/POST/PUT/PATCH/DELETE in Evora +-- Factory Management, 0 errors under mx check), so exec refused to re-apply +-- their own describe output. +-- +-- Cause: the label walk read handler bodies through getErrorHandlerBody, a +-- hand-kept list of statement types that had no REST call (nor call workflow, +-- the mapping statements, commit, delete, ...). It now reads the shared +-- StatementErrorHandling table. +-- +-- Usage (expected to PASS check): +-- mxcli check mdl-examples/bug-tests/check-fp-rest-handler-join.mdl +-- ============================================================================ + +mdl 1; + +create module Probe; + +create microflow Probe.GetWithOneRetry ($Url: String) +returns Boolean +begin + declare $Retried Boolean = false; + merge rejoin1; + $Response = call rest service get '{1}' with ({1} = $Url) ( + Timeout: 300, + ) returns response on error without rollback begin + if not($Retried) then + set $Retried = true; + join rejoin1; + else + return false; + end if; + end error; + return true; +end; diff --git a/mdl/executor/validate.go b/mdl/executor/validate.go index bc9e759b8..b3b3d6011 100644 --- a/mdl/executor/validate.go +++ b/mdl/executor/validate.go @@ -1776,52 +1776,17 @@ func (c *flowRefCollector) collectFromStatements(stmts []ast.MicroflowStatement) } // getErrorHandlerBody returns the custom error handler body if present, or nil. +// +// It reads the clause through stmtErrorHandling — the one table of which +// statements carry an ON ERROR clause — rather than keeping its own list. Its +// own list had drifted: it lacked the REST statements (and call workflow, the +// mapping statements, commit, delete, …), so every walk built on it skipped +// those handler bodies. The visible failure was MDL-FLOW02 reporting +// "merge X: nothing joins it" when the only join sat in a REST call's error +// handler — the retry loop Studio Pro-authored OIDC microflows use. func getErrorHandlerBody(stmt ast.MicroflowStatement) []ast.MicroflowStatement { - switch s := stmt.(type) { - case *ast.CreateObjectStmt: - if s.ErrorHandling != nil && s.ErrorHandling.Body != nil { - return s.ErrorHandling.Body - } - case *ast.RetrieveStmt: - if s.ErrorHandling != nil && s.ErrorHandling.Body != nil { - return s.ErrorHandling.Body - } - case *ast.CallMicroflowStmt: - if s.ErrorHandling != nil && s.ErrorHandling.Body != nil { - return s.ErrorHandling.Body - } - case *ast.CallNanoflowStmt: - if s.ErrorHandling != nil && s.ErrorHandling.Body != nil { - return s.ErrorHandling.Body - } - case *ast.CallJavaActionStmt: - if s.ErrorHandling != nil && s.ErrorHandling.Body != nil { - return s.ErrorHandling.Body - } - case *ast.DownloadFileStmt: - if s.ErrorHandling != nil && s.ErrorHandling.Body != nil { - return s.ErrorHandling.Body - } - case *ast.SynchronizeStmt: - if s.ErrorHandling != nil && s.ErrorHandling.Body != nil { - return s.ErrorHandling.Body - } - case *ast.SendEmailStmt: - if s.ErrorHandling != nil && s.ErrorHandling.Body != nil { - return s.ErrorHandling.Body - } - case *ast.CallJavaScriptActionStmt: - if s.ErrorHandling != nil && s.ErrorHandling.Body != nil { - return s.ErrorHandling.Body - } - case *ast.CallWebServiceStmt: - if s.ErrorHandling != nil && s.ErrorHandling.Body != nil { - return s.ErrorHandling.Body - } - case *ast.ExecuteDatabaseQueryStmt: - if s.ErrorHandling != nil && s.ErrorHandling.Body != nil { - return s.ErrorHandling.Body - } + if eh := stmtErrorHandling(stmt); eh != nil && eh.Body != nil { + return eh.Body } return nil } diff --git a/mdl/executor/validate_microflow_merge_join_test.go b/mdl/executor/validate_microflow_merge_join_test.go new file mode 100644 index 000000000..c9fff500a --- /dev/null +++ b/mdl/executor/validate_microflow_merge_join_test.go @@ -0,0 +1,76 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +func mergeJoinRuleHits(t *testing.T, src string) []string { + t.Helper() + prog, errs := visitor.Build(src) + if len(errs) > 0 { + t.Fatalf("parse errors: %v", errs) + } + var hits []string + for _, v := range ValidateMicroflow(prog.Statements[0].(*ast.CreateMicroflowStmt)) { + if strings.HasPrefix(v.RuleID, "MDL-FLOW0") { + hits = append(hits, v.RuleID+": "+v.Message) + } + } + return hits +} + +// A `join` inside the custom error handler of a REST call is an incoming path +// of its merge. The label walk read handler bodies through a hand-kept list of +// statement types that had no REST call in it, so the join went unseen and the +// merge was reported as joined by nothing — on six Studio Pro-authored +// microflows in Evora Factory Management (OIDC.GET/POST/PUT/PATCH/DELETE, +// AWSAuthentication.POST_v1_GetCallerIdentity_ValidateCredentials) that build at +// 0 errors, which made exec refuse their own describe output. +func TestMergeJoinLabels_JoinInsideRestCallErrorHandler(t *testing.T) { + src := `create microflow M.Retry ($Url: String) +returns Boolean +begin + declare $Retried Boolean = false; + merge rejoin1; + $Response = call rest service get '{1}' with ({1} = $Url) ( + Timeout: 300, + ) returns response on error without rollback begin + if not($Retried) then + set $Retried = true; + join rejoin1; + else + return false; + end if; + end error; + return true; +end;` + if hits := mergeJoinRuleHits(t, src); len(hits) > 0 { + t.Errorf("a join in a REST call's error handler was not counted: %v", hits) + } +} + +// Control: the rule still reports a merge that nothing joins. +func TestMergeJoinLabels_UnjoinedMergeStillReported(t *testing.T) { + src := `create microflow M.Retry ($Url: String) +returns Boolean +begin + declare $Retried Boolean = false; + merge rejoin1; + $Response = call rest service get '{1}' with ({1} = $Url) ( + Timeout: 300, + ) returns response on error without rollback begin + return false; + end error; + return true; +end;` + hits := mergeJoinRuleHits(t, src) + if len(hits) != 1 || !strings.Contains(hits[0], "nothing joins it") { + t.Errorf("want one MDL-FLOW02 'nothing joins it', got %v", hits) + } +} From 65a9fb10887bca66c9814b9c52a590adeae68b64 Mon Sep 17 00:00:00 2001 From: Ako Date: Thu, 8 Oct 2026 08:41:28 +0000 Subject: [PATCH 02/24] fix(check): a void microflow/nanoflow call's output name declares nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #953 taught the duplicate-variable check (MDL063) that a call to a void Java/JavaScript action keeps an inert output name. Studio Pro stores calls to void microflows and nanoflows the same way (ResultVariableName with UseReturnVariable=true), and the resolver never looked at them: Evora's OIDC.webCallback, OIDC.SUB_HandleUserProvisioning and two Teamcenter microflows (0 errors under mx check) failed check, and describe headed webCallback with "duplicate output variable … model is invalid". Measured on mxbuild 10.24.15 and 11.13.0: two void microflow calls of one name 0, void call then declare 0, two void nanoflow calls 0, reading a void microflow call's output CE0109, two String microflow calls CE0111. voidCodeActions now resolves called microflows and nanoflows (script first, then the project), which also extends MDL093 and describe's duplicate-output header to them. Co-Authored-By: Claude Opus 5.5 --- .../fix-issue/findings/mdl-executor.jsonl | 1 + .../check-fp-void-microflow-output.mdl | 59 +++++++ mdl/executor/validate_void_code_calls.go | 96 +++++++++-- mdl/executor/validate_void_flow_calls_test.go | 151 ++++++++++++++++++ 4 files changed, 295 insertions(+), 12 deletions(-) create mode 100644 mdl-examples/bug-tests/check-fp-void-microflow-output.mdl create mode 100644 mdl/executor/validate_void_flow_calls_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index 5cdf70b25..ed3e2c194 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -884,3 +884,4 @@ {"area": "mdl/executor", "date": "2026-10-07", "symptom": "A Microflow-typed Java action argument with `)` on the next line — `call java action M.DoSomething(Flow = M.SUB_Target⏎ );` — reported on 0.23/0.24 as passing check/exec and then failing mx check with CE1613 \"The selected microflow 'MyModule.SUB_Target⏎' no longer exists\"; on current main the same script is REFUSED by both `check --references` and exec as \"parameter Flow is of type Microflow, which takes a microflow name ('Module.Microflow'), not MyModule.SUB_Target\" — the message names a valid microflow name. `Flow = empty⏎)` was refused too (or, without the guard, stored the text `empty` as a microflow reference).", "cause": "buildCallArgumentList keeps the whitespace between an argument's last token and `)` by wrapping the value in ast.SourceExpr (expression round-trip). The Microflow-typed path type-switched on the wrapper: microflowParamArgRefusal (#1210) and isEmptyJavaActionArgument saw a SourceExpr, not a QualifiedNameExpr / LiteralExpr. The original stored-newline form had already been removed by #898 (mendixexpr.String trims a SourceExpr's trailing whitespace), which is why the reported symptom no longer reproduces and the bug now surfaces as a false refusal instead.", "file": "`mdl/executor/javaaction_flow_param.go` (`javaActionArgumentValue`, `microflowParamArgRefusal`), `mdl/executor/cmd_microflows_builder_calls.go` (`isEmptyJavaActionArgument`, Microflow-typed mapping), `mdl/executor/javaaction_flow_param_whitespace_test.go`", "insight": "Any executor decision that type-switches on a call/member argument (`arg.Value.(type)`) must unwrap ast.SourceExpr first: the visitor wraps the value only when whitespace follows it, so the hand-built QualifiedNameExpr/LiteralExpr in existing unit tests never exercises the wrapped form and every such switch passes its tests. Build test arguments with visitor.Build from real MDL, with and without a line break before `)`. Wrong turn to skip: the issue blames the lexer/name text — the name is parsed cleanly; the trailing whitespace is a deliberate SourceExpr suffix, and stripping it in the visitor would break describe round-trip layout (#898). Control: reverting the fix with #1210's guard stubbed no longer reproduces the newline on this tree (#898 trims it), so the live regression is the refusal, which the reverted fix reproduces in both the builder test and the check/exec agreement test.", "refs": ["mendixlabs/mxcli#1282", "mendixlabs/mxcli#1210", "#898"], "ce": ["CE1613"]} {"area": "mdl/executor", "date": "2026-10-07", "symptom": "`mxcli check script.mdl -p app.mpr --references` printed \"Check passed!\" for `grant read * on entity System.Nope to M.R`, for a grant to `M.NopeRole`, and for every other GRANT/REVOKE form naming a missing entity, document, member or role; also for `create user role R ( ModuleRoles: (M.NopeRole) )`, `alter user role … add module roles`, and a demo user with an unknown user role or entity (measured on Evora Factory Management, 10.24.15). exec refused the grants after the earlier statements were written; the user-role and demo-user shapes it wrote unresolved (CE1613 at build).", "cause": "No validate path resolved a security statement's names. validateWithContext's switch had no case for any of them (only the role-free validateCrossModuleGrant ran), and the user-role/demo-user executors store module and user role names verbatim without resolving them, so neither check nor exec stood between a typo and the model.", "file": "`mdl/executor/validate_grant_refs.go` (`validateGrantReferences`), wired into `validateProgramWithWarnings` in `validate.go`; test `check_grant_references_pedapp_test.go`", "insight": "Two things made the obvious version wrong. (1) What check refuses has to be what exec refuses, form by form: exec refuses an unknown role on a GRANT and on an entity REVOKE, but a document REVOKE (and `alter user role … drop module roles`) of an unknown role is a reported no-op, which keeps cleanup scripts re-runnable after the role is dropped — refusing it would make check louder than exec. Likewise every System entity is refused by exec (refuseSystemEntityGrant), so `grant … on entity System.User` must be refused by check too, while System MODULE roles in a user role must pass. (2) A script's own effects are not only its CREATE statements: creating a microflow/nanoflow/page in a module with no module roles auto-creates `.User` (defaultDocumentAccessRoles), and doctype-tests/02b grants to exactly that role — the first version flagged 10 statements there. Walking the program in statement order (not the whole-program scriptContext) gives the forward-reference hint for free. Performance trap: reading every module's security costs ~5s on Evora; read only the modules a statement names (~0.1s).", "refs": ["mdl-examples/bug-tests/check-grant-unknown-reference-refused.mdl", "mdl-examples/bug-tests/check-grant-unknown-reference.mdl", "ako/mxcli#1020"]} {"area": "mdl/executor", "date": "2026-10-07", "symptom": "A DataGrid 2 column's `Visible:` expression is stored as `true` (always visible) for every spelling but the old quoted one: `Visible: $showPrices`, `Visible: if … then … else …`, `visible: not(…)`, `Visible: [cond]`. check clean, exec reports \"Created page\", describe shows no Visible. `alter page … set (Visible: ) on grid column(…)` is refused as \"column property VisibleIf not found\"; `insert` of such a column drops it, and drops `Visible: false` too.", "cause": "The visitor lowers every expression spelling of Visible to the key VisibleIf (the page-widget conditional-visibility key) and keeps only plain values under Visible. The column writers read only visible/Visible: the widget engine by schema key + types.ItemPropertyAliases (no VisibleIf alias), the ALTER mutator's resolveColumnPropertyKey the same table, and widgetobj.BuildDataGrid2Column (ALTER insert/replace via buildColumnSpecFromAST) the exact key \"Visible\" as a string only, so a bool false fell to the default too. The mirror image of widget-visible-expression.mdl, where page widgets read VisibleIf and dropped Visible.", "file": "`mdl/types/widget_item_aliases.go` (`visible` ← `VisibleIf`), `mdl/executor/widget_engine.go` (WidgetDefGeneratorVersion 18), `mdl/executor/cmd_pages_builder_v3_widgets.go` (`columnSpecProperties`), `mdl/executor/cmd_pages_describe_output.go` (column Visible via widgetConditionMDL), `mdl/executor/validate_column_visible_scope.go` (MDL-WIDGET43)", "insight": "When the visitor lowers one MDL property to two AST keys by value shape, every consumer must read both — grep the consumers of the key the visitor writes, not of the property name. Making the value persist exposed what the drop had hidden: a column's visible expression has no row object (no dataSource in the widget schema, unlike columnClass), so every $currentObject example became CE0117 at build — measured on 11.14.0 — and needed a check rule (MDL-WIDGET43) in the same change. The widget def's per-item `dataSource` field is the signal for whether $currentObject exists.", "refs": ["mdl-examples/bug-tests/datagrid-column-visible-expression.mdl", "mdl-examples/bug-tests/widget-visible-expression.mdl"]} +{"area": "mdl/executor", "date": "2026-10-08", "symptom": "`mxcli check` reports MDL063 \"'$Variable_1' is created twice \u2026 CE0111\" for two calls to a microflow that returns nothing, and describe heads the flow with \"duplicate output variable \u2026 model is invalid\". Evora's OIDC.webCallback, OIDC.SUB_HandleUserProvisioning and two Teamcenter microflows (0 errors under mx check) were refused", "cause": "#953 taught voidCodeActions that a void Java/JavaScript call's output name declares nothing, but only for code actions. Studio Pro stores calls to void microflows/nanoflows the same way (ResultVariableName + UseReturnVariable=true), and the resolver never looked at them", "file": "`mdl/executor/validate_void_code_calls.go` (`resolveFlow`, `flowKey`; `callResolution` and `actionIsVoidCall` cover CallMicroflow/CallNanoflow)", "insight": "A fix keyed on one action kind should be re-asked of every action kind with an output name. Measured on mxbuild 10.24.15 and 11.13.0: two void microflow calls of one name 0, void call then declare 0, two void nanoflow calls 0, reading a void microflow call's output CE0109 (so MDL093 now covers it), two String microflow calls CE0111 (control). Resolution goes script first, then GetRawUnitByName+ParseMicroflowBSON; an unresolvable callee still counts under check's strict policy", "refs": ["mdl-examples/bug-tests/check-fp-void-microflow-output.mdl", "ako/mxcli#953"], "ce": ["CE0111", "CE0109"], "rules": ["MDL063", "MDL093"]} diff --git a/mdl-examples/bug-tests/check-fp-void-microflow-output.mdl b/mdl-examples/bug-tests/check-fp-void-microflow-output.mdl new file mode 100644 index 000000000..94472a5ca --- /dev/null +++ b/mdl-examples/bug-tests/check-fp-void-microflow-output.mdl @@ -0,0 +1,59 @@ +-- ============================================================================ +-- MDL063 false positive: the output name of a call to a VOID microflow +-- ============================================================================ +-- +-- Symptom (before fix): `mxcli check` reported "'$Variable_1' is created twice +-- in this microflow … CE0111" for calls to a microflow that returns nothing. +-- Studio Pro stores an output name on such a call (UseReturnVariable=true) and +-- the name declares no variable — the same case ako/mxcli#953 fixed for void +-- Java/JavaScript actions. Evora Factory Management (0 errors under mx check) +-- has it in OIDC.webCallback, OIDC.SUB_HandleUserProvisioning and two +-- Teamcenter microflows; exec refused their own describe output, and describe +-- headed OIDC.webCallback with "duplicate output variable … model is invalid". +-- +-- Measured (mxbuild 10.24.15 and 11.13.0): +-- two void microflow calls, same output name 0 errors +-- void microflow call, then declare of the name 0 errors +-- two void nanoflow calls, same output name 0 errors +-- void microflow call, then reading its output CE0109 <- MDL093 +-- two String microflow calls, same output name CE0111 <- MDL063 (control) +-- +-- Usage (expected to PASS check): +-- mxcli check mdl-examples/bug-tests/check-fp-void-microflow-output.mdl +-- ============================================================================ + +mdl 1; + +create module Probe; + +create microflow Probe.ShowError ($Message: String) +begin + log error node 'Probe' $Message; +end; + +create nanoflow Probe.Refresh ($Message: String) +begin + log info node 'Probe' $Message; +end; + +create microflow Probe.CallVoidTwice ($Ok: Boolean) +begin + if $Ok then + $Variable_1 = call microflow Probe.ShowError(Message = 'first'); + else + $Variable_1 = call microflow Probe.ShowError(Message = 'second'); + end if; +end; + +create microflow Probe.VoidCallThenDeclare () +begin + $W = call microflow Probe.ShowError(Message = 'x'); + declare $W String = 'now a real variable'; + log info node 'Probe' $W; +end; + +create nanoflow Probe.CallVoidNanoflowTwice () +begin + $N = call nanoflow Probe.Refresh(Message = 'a'); + $N = call nanoflow Probe.Refresh(Message = 'b'); +end; diff --git a/mdl/executor/validate_void_code_calls.go b/mdl/executor/validate_void_code_calls.go index e56f7f823..dc128cea8 100644 --- a/mdl/executor/validate_void_code_calls.go +++ b/mdl/executor/validate_void_code_calls.go @@ -9,6 +9,7 @@ import ( "github.com/mendixlabs/mxcli/mdl/ast" "github.com/mendixlabs/mxcli/mdl/backend" "github.com/mendixlabs/mxcli/mdl/linter" + "github.com/mendixlabs/mxcli/model" "github.com/mendixlabs/mxcli/sdk/microflows" ) @@ -38,6 +39,16 @@ import ( // Whatever that policy, only a call KNOWN to be void counts for the CE0109 // rule (MDL093): "possibly void" must never turn a use of the output into an // error. +// +// Calls to a void MICROFLOW or NANOFLOW are the same case. Studio Pro stores +// them with an output name and UseReturnVariable=true (Evora Factory +// Management's OIDC.webCallback calls the void ACT_ShowCusomExceptionMessage +// three times as `$Variable_1`), and mxbuild treats the name as inert. Measured +// on mxbuild 10.24.15 and 11.13.0: two void microflow calls of one name, a void +// call then a declare of it, and two void nanoflow calls are 0 errors; reading +// a void microflow call's output is CE0109; two String microflow calls are +// CE0111 (control). Without the flows in here, check refused those stored +// microflows' own describe output with MDL063. type voidCodeActions struct { // script holds the actions the script itself creates, keyed by // codeActionKey, valued true when the action returns Void. @@ -60,6 +71,20 @@ type voidness struct { known bool // the action was found, so void is an answer and not a default } +// flowKey keys a microflow or nanoflow in the same maps as the code actions. +func flowKey(nanoflow bool, qn string) string { + if nanoflow { + return "nf:" + qn + } + return "mf:" + qn +} + +// flowReturnsVoid reports whether a stored flow's return type is Void: no +// return type at all, or an explicit VoidType. +func flowReturnsVoid(rt microflows.DataType) bool { + return rt == nil || rt.GetTypeName() == "Void" +} + func codeActionKey(javaScript bool, qn string) string { if javaScript { return "js:" + qn @@ -80,6 +105,10 @@ func newVoidCodeActions(prog *ast.Program, open func() backend.FullBackend) *voi r.script[codeActionKey(false, s.Name.String())] = s.ReturnType.Kind == ast.TypeVoid case *ast.CreateJavaScriptActionStmt: r.script[codeActionKey(true, s.Name.String())] = s.ReturnType.Kind == ast.TypeVoid + case *ast.CreateMicroflowStmt: + r.script[flowKey(false, s.Name.String())] = s.ReturnType == nil || s.ReturnType.Type.Kind == ast.TypeVoid + case *ast.CreateNanoflowStmt: + r.script[flowKey(true, s.Name.String())] = s.ReturnType == nil || s.ReturnType.Type.Kind == ast.TypeVoid } } return r @@ -105,7 +134,48 @@ func (r *voidCodeActions) resolve(javaScript bool, qn string) voidness { if r == nil || qn == "" { return voidness{} } - key := codeActionKey(javaScript, qn) + return r.lookup(codeActionKey(javaScript, qn), func(b backend.FullBackend) voidness { + if javaScript { + if a, err := b.ReadJavaScriptActionByName(qn); err == nil && a != nil && a.ReturnType != nil { + return voidness{void: a.ReturnType.TypeString() == "Void", known: true} + } + return voidness{} + } + // The Java action reader returns a nil ReturnType for Void + // (codeActionReturnTypeFromGen); the JavaScript one a VoidType. + if a, err := b.ReadJavaActionByName(qn); err == nil && a != nil { + return voidness{void: a.ReturnType == nil || a.ReturnType.TypeString() == "Void", known: true} + } + return voidness{} + }) +} + +// resolveFlow looks a called microflow or nanoflow up: in the script first, +// then in the project. +func (r *voidCodeActions) resolveFlow(nanoflow bool, qn string) voidness { + if r == nil || qn == "" { + return voidness{} + } + return r.lookup(flowKey(nanoflow, qn), func(b backend.FullBackend) voidness { + objectType := "microflow" + if nanoflow { + objectType = "nanoflow" + } + raw, err := b.GetRawUnitByName(objectType, qn) + if err != nil || raw == nil || len(raw.Contents) == 0 { + return voidness{} + } + mf, err := b.ParseMicroflowBSON(raw.Contents, model.ID(raw.ID), "") + if err != nil || mf == nil { + return voidness{} + } + return voidness{void: flowReturnsVoid(mf.ReturnType), known: true} + }) +} + +// lookup answers key from the script, the caches, and finally the project +// through read. +func (r *voidCodeActions) lookup(key string, read func(backend.FullBackend) voidness) voidness { if v, ok := r.script[key]; ok { return voidness{void: v, known: true} } @@ -118,17 +188,7 @@ func (r *voidCodeActions) resolve(javaScript bool, qn string) voidness { } var got voidness if b := r.project(); b != nil { - if javaScript { - if a, err := b.ReadJavaScriptActionByName(qn); err == nil && a != nil && a.ReturnType != nil { - got = voidness{void: a.ReturnType.TypeString() == "Void", known: true} - } - } else { - // The Java action reader returns a nil ReturnType for Void - // (codeActionReturnTypeFromGen); the JavaScript one a VoidType. - if a, err := b.ReadJavaActionByName(qn); err == nil && a != nil { - got = voidness{void: a.ReturnType == nil || a.ReturnType.TypeString() == "Void", known: true} - } - } + got = read(b) } r.cache[key] = got r.shared.put(key, got) @@ -151,6 +211,10 @@ func (r *voidCodeActions) callResolution(s ast.MicroflowStatement) (v voidness, return r.resolve(false, st.ActionName.String()), true case *ast.CallJavaScriptActionStmt: return r.resolve(true, st.ActionName.String()), true + case *ast.CallMicroflowStmt: + return r.resolveFlow(false, st.MicroflowName.String()), true + case *ast.CallNanoflowStmt: + return r.resolveFlow(true, st.NanoflowName.String()), true } return voidness{}, false } @@ -177,6 +241,14 @@ func (r *voidCodeActions) actionIsVoidCall(action any) bool { return r.treatAsVoid(r.resolve(false, a.JavaAction)) case *microflows.JavaScriptActionCallAction: return r.treatAsVoid(r.resolve(true, a.JavaScriptAction)) + case *microflows.MicroflowCallAction: + if a.MicroflowCall != nil { + return r.treatAsVoid(r.resolveFlow(false, a.MicroflowCall.Microflow)) + } + case *microflows.NanoflowCallAction: + if a.NanoflowCall != nil { + return r.treatAsVoid(r.resolveFlow(true, a.NanoflowCall.Nanoflow)) + } } return false } diff --git a/mdl/executor/validate_void_flow_calls_test.go b/mdl/executor/validate_void_flow_calls_test.go new file mode 100644 index 000000000..04ed84d9c --- /dev/null +++ b/mdl/executor/validate_void_flow_calls_test.go @@ -0,0 +1,151 @@ +// SPDX-License-Identifier: Apache-2.0 + +// A call to a VOID microflow or nanoflow is the same case as #953's void +// Java/JavaScript action: Studio Pro keeps an output name on it (stored with +// UseReturnVariable=true), and that name declares no variable. Measured on +// mxbuild 10.24.15 and 11.13.0, fresh project, one microflow per case: +// +// two calls to a void microflow, same output name 0 errors +// void microflow call `$W = …`, then `declare $W` 0 errors +// two calls to a void nanoflow, same output name 0 errors +// void microflow call `$Z = …`, then reading `$Z` CE0109 "Undefined variable 'Z'" +// two calls to a String microflow, same output name CE0111 (control) +// +// Evora Factory Management (Mendix 10.24, 0 errors under mx check) carries the +// shape in OIDC.webCallback, OIDC.SUB_HandleUserProvisioning and two Teamcenter +// microflows; `check` refused their own describe output with MDL063. +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend" + "github.com/mendixlabs/mxcli/mdl/backend/mock" + "github.com/mendixlabs/mxcli/mdl/linter" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +func mfCall(out, flow string) *ast.CallMicroflowStmt { + return &ast.CallMicroflowStmt{OutputVariable: out, MicroflowName: qn("M", flow)} +} + +func nfCall(out, flow string) *ast.CallNanoflowStmt { + return &ast.CallNanoflowStmt{OutputVariable: out, NanoflowName: qn("M", flow)} +} + +func flowDecls() []ast.Statement { + str := &ast.MicroflowReturnType{Type: ast.DataType{Kind: ast.TypeString}} + return []ast.Statement{ + &ast.CreateMicroflowStmt{Name: qn("M", "MfVoid")}, + &ast.CreateMicroflowStmt{Name: qn("M", "MfVoidTyped"), + ReturnType: &ast.MicroflowReturnType{Type: ast.DataType{Kind: ast.TypeVoid}}}, + &ast.CreateMicroflowStmt{Name: qn("M", "MfString"), ReturnType: str}, + &ast.CreateNanoflowStmt{Name: qn("M", "NfVoid")}, + &ast.CreateNanoflowStmt{Name: qn("M", "NfString"), ReturnType: str}, + } +} + +func rulesHit(vs []linter.Violation, rule string) int { + n := 0 + for _, v := range vs { + if v.RuleID == rule { + n++ + } + } + return n +} + +func TestMDL063_VoidFlowCallOutputsAreNotDeclarations(t *testing.T) { + str := ast.DataType{Kind: ast.TypeString} + lit := &ast.LiteralExpr{Kind: ast.LiteralString, Value: "x"} + cases := []struct { + name string + flow ast.Statement + want int + }{ + {"two void microflow calls, same name", &ast.CreateMicroflowStmt{Name: qn("M", "Caller"), + Body: []ast.MicroflowStatement{mfCall("V", "MfVoid"), mfCall("V", "MfVoid")}}, 0}, + {"void microflow (explicit Void) twice", &ast.CreateMicroflowStmt{Name: qn("M", "Caller"), + Body: []ast.MicroflowStatement{mfCall("V", "MfVoidTyped"), mfCall("V", "MfVoidTyped")}}, 0}, + {"void microflow call then declare", &ast.CreateMicroflowStmt{Name: qn("M", "Caller"), + Body: []ast.MicroflowStatement{mfCall("W", "MfVoid"), + &ast.DeclareStmt{Variable: "W", Type: str, InitialValue: lit}}}, 0}, + {"control: two String microflow calls", &ast.CreateMicroflowStmt{Name: qn("M", "Caller"), + Body: []ast.MicroflowStatement{mfCall("Y", "MfString"), mfCall("Y", "MfString")}}, 1}, + {"two void nanoflow calls", &ast.CreateNanoflowStmt{Name: qn("M", "NCaller"), + Body: []ast.MicroflowStatement{nfCall("N", "NfVoid"), nfCall("N", "NfVoid")}}, 0}, + {"control: two String nanoflow calls", &ast.CreateNanoflowStmt{Name: qn("M", "NCaller"), + Body: []ast.MicroflowStatement{nfCall("N", "NfString"), nfCall("N", "NfString")}}, 1}, + {"control: unresolvable microflow still counts", &ast.CreateMicroflowStmt{Name: qn("M", "Caller"), + Body: []ast.MicroflowStatement{mfCall("R", "Elsewhere"), mfCall("R", "Elsewhere")}}, 1}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + prog := &ast.Program{Statements: append(flowDecls(), tc.flow)} + if got := rulesHit(ValidateProgram(prog, ""), "MDL063"); got != tc.want { + t.Fatalf("MDL063 count = %d, want %d", got, tc.want) + } + }) + } +} + +// Reading a void call's output is the CE0109 the measurement found, so MDL093 +// covers microflow calls as it covers Java/JavaScript ones. +func TestMDL093_ReadOfVoidMicroflowOutput(t *testing.T) { + read := &ast.LogStmt{Level: ast.LogInfo, Node: &ast.LiteralExpr{Kind: ast.LiteralString, Value: "P"}, + Message: &ast.VariableExpr{Name: "Z"}} + prog := &ast.Program{Statements: append(flowDecls(), &ast.CreateMicroflowStmt{Name: qn("M", "Caller"), + Body: []ast.MicroflowStatement{mfCall("Z", "MfVoid"), read}})} + if got := rulesHit(ValidateProgram(prog, ""), voidCallOutputRule); got != 1 { + t.Fatalf("%s count = %d, want 1", voidCallOutputRule, got) + } + ctrl := &ast.Program{Statements: append(flowDecls(), &ast.CreateMicroflowStmt{Name: qn("M", "Caller"), + Body: []ast.MicroflowStatement{mfCall("Z", "MfString"), read}})} + if got := rulesHit(ValidateProgram(ctrl, ""), voidCallOutputRule); got != 0 { + t.Fatalf("control: %s fired on a String microflow's output", voidCallOutputRule) + } +} + +// Stored flows are read from the project by qualified name. +func TestVoidCodeActions_ReadsStoredFlowReturnType(t *testing.T) { + stored := map[string]microflows.DataType{ + "microflow:OIDC.ACT_ShowCusomExceptionMessage": nil, + "microflow:M.Typed": µflows.VoidType{}, + "microflow:M.Str": µflows.StringType{}, + "nanoflow:M.NVoid": nil, + } + b := &mock.MockBackend{ + GetRawUnitByNameFunc: func(objectType, name string) (*types.RawUnitInfo, error) { + if _, ok := stored[objectType+":"+name]; ok { + return &types.RawUnitInfo{ID: objectType + ":" + name, Contents: []byte{1}}, nil + } + return nil, nil + }, + ParseMicroflowBSONFunc: func(_ []byte, unitID, _ model.ID) (*microflows.Microflow, error) { + return µflows.Microflow{ReturnType: stored[string(unitID)]}, nil + }, + } + r := newVoidCodeActions(nil, func() backend.FullBackend { return b }) + check := func(s ast.MicroflowStatement, want bool, what string) { + t.Helper() + if got := r.callIsVoid(s); got != want { + t.Errorf("%s: callIsVoid = %v, want %v", what, got, want) + } + } + check(&ast.CallMicroflowStmt{OutputVariable: "V", MicroflowName: qn("OIDC", "ACT_ShowCusomExceptionMessage")}, true, "stored void microflow") + check(mfCall("V", "Typed"), true, "stored VoidType microflow") + check(mfCall("V", "Str"), false, "control: stored String microflow") + check(mfCall("V", "Missing"), false, "control: a microflow the project does not have") + check(nfCall("V", "NVoid"), true, "stored void nanoflow") + check(nfCall("V", "Typed"), false, "control: a microflow name is not a nanoflow") + + // describe's duplicate-output header asks the same question of the stored action. + act := µflows.MicroflowCallAction{ResultVariableName: "Variable_1", UseReturnVariable: true, + MicroflowCall: µflows.MicroflowCall{Microflow: "OIDC.ACT_ShowCusomExceptionMessage"}} + if !r.actionIsVoidCall(act) { + t.Error("describe: stored call to a void microflow not recognised") + } +} From ca7562b8ec13e88741ade50c6b195dfdfbf3c93d Mon Sep 17 00:00:00 2001 From: Ako Date: Thu, 8 Oct 2026 08:41:28 +0000 Subject: [PATCH 03/24] fix(check): MDL004 on a stored void microflow's return value is a warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Studio Pro keeps an end event's return value when a microflow's return type is set to Nothing, and hides the field. Evora stores five such microflows; describe renders the value faithfully and MDL004 refused it, so exec would not re-apply an untouched microflow. mxbuild 10.24.15 and 11.13.0 build the shape at 0 errors. The value is not inert — the runtime returns it, and a workflow branching on the call fails at instance start — so new code keeps the error. Like DropSettledCommitNotes for MDL067, check -p and exec now downgrade it to a warning only for a microflow the project already stores with at least as many valued end events. Co-Authored-By: Claude Opus 5.5 --- .../fix-issue/findings/mdl-executor.jsonl | 1 + cmd/mxcli/cmd_check.go | 3 + cmd/mxcli/exec_preflight.go | 3 + .../check-fp-void-return-value.fail.mdl | 36 ++++++ mdl/executor/validate_microflow.go | 3 +- mdl/executor/validate_void_return_settled.go | 111 ++++++++++++++++ .../validate_void_return_settled_test.go | 121 ++++++++++++++++++ 7 files changed, 276 insertions(+), 2 deletions(-) create mode 100644 mdl-examples/bug-tests/check-fp-void-return-value.fail.mdl create mode 100644 mdl/executor/validate_void_return_settled.go create mode 100644 mdl/executor/validate_void_return_settled_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index 5cdf70b25..4628ba576 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -884,3 +884,4 @@ {"area": "mdl/executor", "date": "2026-10-07", "symptom": "A Microflow-typed Java action argument with `)` on the next line — `call java action M.DoSomething(Flow = M.SUB_Target⏎ );` — reported on 0.23/0.24 as passing check/exec and then failing mx check with CE1613 \"The selected microflow 'MyModule.SUB_Target⏎' no longer exists\"; on current main the same script is REFUSED by both `check --references` and exec as \"parameter Flow is of type Microflow, which takes a microflow name ('Module.Microflow'), not MyModule.SUB_Target\" — the message names a valid microflow name. `Flow = empty⏎)` was refused too (or, without the guard, stored the text `empty` as a microflow reference).", "cause": "buildCallArgumentList keeps the whitespace between an argument's last token and `)` by wrapping the value in ast.SourceExpr (expression round-trip). The Microflow-typed path type-switched on the wrapper: microflowParamArgRefusal (#1210) and isEmptyJavaActionArgument saw a SourceExpr, not a QualifiedNameExpr / LiteralExpr. The original stored-newline form had already been removed by #898 (mendixexpr.String trims a SourceExpr's trailing whitespace), which is why the reported symptom no longer reproduces and the bug now surfaces as a false refusal instead.", "file": "`mdl/executor/javaaction_flow_param.go` (`javaActionArgumentValue`, `microflowParamArgRefusal`), `mdl/executor/cmd_microflows_builder_calls.go` (`isEmptyJavaActionArgument`, Microflow-typed mapping), `mdl/executor/javaaction_flow_param_whitespace_test.go`", "insight": "Any executor decision that type-switches on a call/member argument (`arg.Value.(type)`) must unwrap ast.SourceExpr first: the visitor wraps the value only when whitespace follows it, so the hand-built QualifiedNameExpr/LiteralExpr in existing unit tests never exercises the wrapped form and every such switch passes its tests. Build test arguments with visitor.Build from real MDL, with and without a line break before `)`. Wrong turn to skip: the issue blames the lexer/name text — the name is parsed cleanly; the trailing whitespace is a deliberate SourceExpr suffix, and stripping it in the visitor would break describe round-trip layout (#898). Control: reverting the fix with #1210's guard stubbed no longer reproduces the newline on this tree (#898 trims it), so the live regression is the refusal, which the reverted fix reproduces in both the builder test and the check/exec agreement test.", "refs": ["mendixlabs/mxcli#1282", "mendixlabs/mxcli#1210", "#898"], "ce": ["CE1613"]} {"area": "mdl/executor", "date": "2026-10-07", "symptom": "`mxcli check script.mdl -p app.mpr --references` printed \"Check passed!\" for `grant read * on entity System.Nope to M.R`, for a grant to `M.NopeRole`, and for every other GRANT/REVOKE form naming a missing entity, document, member or role; also for `create user role R ( ModuleRoles: (M.NopeRole) )`, `alter user role … add module roles`, and a demo user with an unknown user role or entity (measured on Evora Factory Management, 10.24.15). exec refused the grants after the earlier statements were written; the user-role and demo-user shapes it wrote unresolved (CE1613 at build).", "cause": "No validate path resolved a security statement's names. validateWithContext's switch had no case for any of them (only the role-free validateCrossModuleGrant ran), and the user-role/demo-user executors store module and user role names verbatim without resolving them, so neither check nor exec stood between a typo and the model.", "file": "`mdl/executor/validate_grant_refs.go` (`validateGrantReferences`), wired into `validateProgramWithWarnings` in `validate.go`; test `check_grant_references_pedapp_test.go`", "insight": "Two things made the obvious version wrong. (1) What check refuses has to be what exec refuses, form by form: exec refuses an unknown role on a GRANT and on an entity REVOKE, but a document REVOKE (and `alter user role … drop module roles`) of an unknown role is a reported no-op, which keeps cleanup scripts re-runnable after the role is dropped — refusing it would make check louder than exec. Likewise every System entity is refused by exec (refuseSystemEntityGrant), so `grant … on entity System.User` must be refused by check too, while System MODULE roles in a user role must pass. (2) A script's own effects are not only its CREATE statements: creating a microflow/nanoflow/page in a module with no module roles auto-creates `.User` (defaultDocumentAccessRoles), and doctype-tests/02b grants to exactly that role — the first version flagged 10 statements there. Walking the program in statement order (not the whole-program scriptContext) gives the forward-reference hint for free. Performance trap: reading every module's security costs ~5s on Evora; read only the modules a statement names (~0.1s).", "refs": ["mdl-examples/bug-tests/check-grant-unknown-reference-refused.mdl", "mdl-examples/bug-tests/check-grant-unknown-reference.mdl", "ako/mxcli#1020"]} {"area": "mdl/executor", "date": "2026-10-07", "symptom": "A DataGrid 2 column's `Visible:` expression is stored as `true` (always visible) for every spelling but the old quoted one: `Visible: $showPrices`, `Visible: if … then … else …`, `visible: not(…)`, `Visible: [cond]`. check clean, exec reports \"Created page\", describe shows no Visible. `alter page … set (Visible: ) on grid column(…)` is refused as \"column property VisibleIf not found\"; `insert` of such a column drops it, and drops `Visible: false` too.", "cause": "The visitor lowers every expression spelling of Visible to the key VisibleIf (the page-widget conditional-visibility key) and keeps only plain values under Visible. The column writers read only visible/Visible: the widget engine by schema key + types.ItemPropertyAliases (no VisibleIf alias), the ALTER mutator's resolveColumnPropertyKey the same table, and widgetobj.BuildDataGrid2Column (ALTER insert/replace via buildColumnSpecFromAST) the exact key \"Visible\" as a string only, so a bool false fell to the default too. The mirror image of widget-visible-expression.mdl, where page widgets read VisibleIf and dropped Visible.", "file": "`mdl/types/widget_item_aliases.go` (`visible` ← `VisibleIf`), `mdl/executor/widget_engine.go` (WidgetDefGeneratorVersion 18), `mdl/executor/cmd_pages_builder_v3_widgets.go` (`columnSpecProperties`), `mdl/executor/cmd_pages_describe_output.go` (column Visible via widgetConditionMDL), `mdl/executor/validate_column_visible_scope.go` (MDL-WIDGET43)", "insight": "When the visitor lowers one MDL property to two AST keys by value shape, every consumer must read both — grep the consumers of the key the visitor writes, not of the property name. Making the value persist exposed what the drop had hidden: a column's visible expression has no row object (no dataSource in the widget schema, unlike columnClass), so every $currentObject example became CE0117 at build — measured on 11.14.0 — and needed a check rule (MDL-WIDGET43) in the same change. The widget def's per-item `dataSource` field is the signal for whether $currentObject exists.", "refs": ["mdl-examples/bug-tests/datagrid-column-visible-expression.mdl", "mdl-examples/bug-tests/widget-visible-expression.mdl"]} +{"area": "mdl/executor", "date": "2026-10-08", "symptom": "`mxcli check` reports MDL004 \"return has a value but microflow does not declare a return type\" on the describe output of five untouched Evora microflows (OIDC.SUB_HandlePrivateKey, TcConnector.HandleSSOLoginMicroflow, three TeamcenterToolkit ones), so exec would not re-apply them", "cause": "The stored model really is that way: MicroflowReturnType is DataTypes$VoidType while an EndEvent keeps a ReturnValue (Studio Pro keeps it when the return type is set to Nothing and hides the field). Describe renders it faithfully; mxbuild 10.24.15 and 11.13.0 accept it, also when exec writes it fresh", "file": "`mdl/executor/validate_void_return_settled.go` (`SettleStoredVoidReturnValues`, `StoredVoidReturns`), wired in `cmd/mxcli/cmd_check.go` and `cmd/mxcli/exec_preflight.go` beside DropSettledCommitNotes", "insight": "Classify before relaxing: this is a real defect Studio Pro tolerates, not a check bug \u2014 the runtime does return the value, and a workflow branching on the call fails at instance start (write-workflows skill). So new code keeps the error and only a flow the project already stores with at least as many valued end events is downgraded to a warning, the 'settled' pattern MDL067 uses. Reading the BSON settled which of describer-drops-the-type vs genuinely-absent it was", "refs": ["mdl-examples/bug-tests/check-fp-void-return-value.fail.mdl"], "rules": ["MDL004"]} diff --git a/cmd/mxcli/cmd_check.go b/cmd/mxcli/cmd_check.go index 3abd57624..4bbbe6a8f 100644 --- a/cmd/mxcli/cmd_check.go +++ b/cmd/mxcli/cmd_check.go @@ -283,6 +283,9 @@ func runCheckFile(cmd *cobra.Command, filePath string) int { exec = e if b := exec.Backend(); b != nil { violations = executor.DropSettledCommitNotes(violations, prog, executor.NewStoredCommitEvents(b)) + // MDL004 on a void microflow's end-event value is a warning for a flow + // already stored that way: re-applying it changes nothing. + violations = executor.SettleStoredVoidReturnValues(violations, prog, executor.NewStoredVoidReturns(b)) violations = append(violations, executor.StoredTaskClaimViolations(prog, b)...) } } diff --git a/cmd/mxcli/exec_preflight.go b/cmd/mxcli/exec_preflight.go index 164953124..5068ad822 100644 --- a/cmd/mxcli/exec_preflight.go +++ b/cmd/mxcli/exec_preflight.go @@ -37,6 +37,9 @@ func execPreflight(exec *executor.Executor, prog *ast.Program, projectPath, scri if exec != nil { if b := exec.Backend(); b != nil { violations = executor.DropSettledCommitNotes(violations, prog, executor.NewStoredCommitEvents(b)) + // MDL004 on a void microflow's end-event value is a warning for a flow + // already stored that way: re-applying it changes nothing. + violations = executor.SettleStoredVoidReturnValues(violations, prog, executor.NewStoredVoidReturns(b)) // A called microflow the script does not create is read from the // project for MDL-WORKFLOW10 (ako/mxcli#943). violations = append(violations, executor.StoredTaskClaimViolations(prog, b)...) diff --git a/mdl-examples/bug-tests/check-fp-void-return-value.fail.mdl b/mdl-examples/bug-tests/check-fp-void-return-value.fail.mdl new file mode 100644 index 000000000..24f145ad4 --- /dev/null +++ b/mdl-examples/bug-tests/check-fp-void-return-value.fail.mdl @@ -0,0 +1,36 @@ +-- ============================================================================ +-- MDL004 on a void microflow's return value: error for new code, warning for +-- a microflow the project already stores that way +-- ============================================================================ +-- +-- Studio Pro keeps an end event's return value when a microflow's return type +-- is changed to Nothing, and hides the field. Evora Factory Management +-- (Mendix 10.24, 0 errors under mx check) stores five such microflows +-- (OIDC.SUB_HandlePrivateKey, TcConnector.HandleSSOLoginMicroflow, +-- TeamcenterToolkit.CreateRelation, .Configuration_Validate, +-- .GetPropertiesForModelObjectList), and describe prints the value faithfully +-- as `return ;` in a microflow with no `returns` clause. MDL004 refused +-- that output, so exec would not re-apply an untouched microflow. +-- +-- Measured: mxbuild 10.24.15 and 11.13.0 build the shape at 0 errors, also +-- when exec writes it fresh. The value is still returned at runtime, and a +-- workflow branching on such a microflow fails at instance start (see the +-- write-workflows skill) — so this file, which is NEW code, stays an error. +-- With `-p` against a project that already stores the microflow with that +-- many valued end events, MDL004 is reported as a warning and exec proceeds. +-- +-- Usage (expected to FAIL check — no project, so nothing is settled): +-- mxcli check mdl-examples/bug-tests/check-fp-void-return-value.fail.mdl +-- ============================================================================ + +create module Probe; +create enumeration Probe.Assertion ( PRIVATE_KEY 'Private key', SECRET 'Secret' ); + +create or modify microflow Probe.HandlePrivateKey ($IsJwt: Boolean) +begin + if $IsJwt then + return Probe.Assertion.PRIVATE_KEY; -- MDL004: no `returns` clause + else + return; + end if; +end; diff --git a/mdl/executor/validate_microflow.go b/mdl/executor/validate_microflow.go index 3e5d5f206..793f55105 100644 --- a/mdl/executor/validate_microflow.go +++ b/mdl/executor/validate_microflow.go @@ -1213,8 +1213,7 @@ func (v *microflowValidator) checkReturn(stmt *ast.ReturnStmt) { return } } - v.addViolation("MDL004", linter.SeverityError, - "return has a value but microflow does not declare a return type", + v.addViolation("MDL004", linter.SeverityError, voidReturnValueMessage, "Remove the return value or add a return type to the microflow") return } diff --git a/mdl/executor/validate_void_return_settled.go b/mdl/executor/validate_void_return_settled.go new file mode 100644 index 000000000..554c9c84b --- /dev/null +++ b/mdl/executor/validate_void_return_settled.go @@ -0,0 +1,111 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend" + "github.com/mendixlabs/mxcli/mdl/linter" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// voidReturnValueMessage is MDL004's report of `return ` in a microflow +// that declares no return type. SettleStoredVoidReturnValues matches on it. +const voidReturnValueMessage = "return has a value but microflow does not declare a return type" + +// voidReturnSource is what SettleStoredVoidReturnValues asks the project. +type voidReturnSource interface { + // VoidReturnValues reports how many end events of the stored microflow + // carry a return value while the microflow returns Void; ok is false when + // the project has no such microflow. + VoidReturnValues(qualifiedName string) (n int, ok bool) +} + +// SettleStoredVoidReturnValues downgrades MDL004's "return has a value but +// microflow does not declare a return type" to a warning for a microflow the +// project already stores that way. +// +// Studio Pro keeps an end event's return value when the microflow's return +// type is changed to Nothing, and hides the field; mxbuild 10.24.15 and +// 11.13.0 build it at 0 errors, and so does a fresh one exec writes (measured). +// Evora Factory Management stores five such microflows, so `exec` refused to +// re-apply their own `describe` output — a statement that changes nothing. +// +// The value is not inert, which is why new code keeps the error: the runtime +// returns it, and a workflow branching on the call fails at instance start +// (`Trying to compare VoidConditionValue$(”) to BooleanValue('true')`, see the +// write-workflows skill). For a stored flow the warning says the same thing +// without blocking a round trip; it stays an error when there is no project, the +// flow is new, or the script adds more valued returns than are stored. +func SettleStoredVoidReturnValues(violations []linter.Violation, prog *ast.Program, stored voidReturnSource) []linter.Violation { + if stored == nil || prog == nil { + return violations + } + reported := map[string]int{} + for _, v := range violations { + if isVoidReturnValue(v) { + reported[v.Location.DocumentName]++ + } + } + settled := map[string]bool{} + for flow, n := range reported { + if have, ok := stored.VoidReturnValues(flow); ok && have >= n { + settled[flow] = true + } + } + if len(settled) == 0 { + return violations + } + out := make([]linter.Violation, 0, len(violations)) + for _, v := range violations { + if isVoidReturnValue(v) && settled[v.Location.DocumentName] { + v.Severity = linter.SeverityWarning + v.Message = voidReturnValueMessage + " — already stored this way (Studio Pro keeps an end " + + "event's value after the return type is set to Nothing, and hides it; mxbuild accepts it). " + + "The runtime still returns the value, so a workflow that branches on this microflow fails at " + + "instance start" + } + out = append(out, v) + } + return out +} + +func isVoidReturnValue(v linter.Violation) bool { + return v.RuleID == "MDL004" && v.Message == voidReturnValueMessage && v.Location.DocumentType == "microflow" +} + +// StoredVoidReturns answers voidReturnSource from a project. +type StoredVoidReturns struct{ b backend.FullBackend } + +// NewStoredVoidReturns reads stored microflows through b. +func NewStoredVoidReturns(b backend.FullBackend) *StoredVoidReturns { + return &StoredVoidReturns{b: b} +} + +// VoidReturnValues implements voidReturnSource. +func (s *StoredVoidReturns) VoidReturnValues(qualifiedName string) (int, bool) { + if s == nil || s.b == nil { + return 0, false + } + raw, err := s.b.GetRawUnitByName("microflow", qualifiedName) + if err != nil || raw == nil || len(raw.Contents) == 0 { + return 0, false + } + mf, err := s.b.ParseMicroflowBSON(raw.Contents, model.ID(raw.ID), "") + if err != nil || mf == nil { + return 0, false + } + if mf.ReturnType != nil && mf.ReturnType.GetTypeName() != "Void" { + return 0, true + } + n := 0 + if mf.ObjectCollection != nil { + for _, obj := range mf.ObjectCollection.Objects { + if end, ok := obj.(*microflows.EndEvent); ok && end.ReturnValue != "" { + n++ + } + } + } + return n, true +} diff --git a/mdl/executor/validate_void_return_settled_test.go b/mdl/executor/validate_void_return_settled_test.go new file mode 100644 index 000000000..0ca1c9f27 --- /dev/null +++ b/mdl/executor/validate_void_return_settled_test.go @@ -0,0 +1,121 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend/mock" + "github.com/mendixlabs/mxcli/mdl/linter" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/mdl/visitor" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +type fakeVoidReturns map[string]int + +func (f fakeVoidReturns) VoidReturnValues(qn string) (int, bool) { + n, ok := f[qn] + return n, ok +} + +const voidValueFlow = `create or modify microflow OIDC.SUB_HandlePrivateKey ($C: Boolean) +begin + if $C then + return OIDC.ENU_ClientAssertion.PRIVATE_KEY; + else + return; + end if; +end;` + +func voidValueViolations(t *testing.T) (*ast.Program, []linter.Violation) { + t.Helper() + prog, errs := visitor.Build(voidValueFlow) + if len(errs) > 0 { + t.Fatalf("parse: %v", errs) + } + return prog, ValidateProgram(prog, "") +} + +func severityOfVoidValue(vs []linter.Violation) (linter.Severity, bool) { + for _, v := range vs { + if v.RuleID == "MDL004" && strings.HasPrefix(v.Message, voidReturnValueMessage) { + return v.Severity, true + } + } + return 0, false +} + +// A void microflow whose end event carries a value is what Evora Factory +// Management stores in OIDC.SUB_HandlePrivateKey and four other microflows — +// a leftover of an earlier return type, hidden by Studio Pro and built at 0 +// errors by mxbuild 10.24.15 and 11.13.0 (measured, also for a fresh one written +// by exec). Re-applying the microflow's own describe output changes nothing, +// so for a flow already stored that way the refusal is a warning: the value is +// still reported, but exec no longer refuses an untouched flow. +func TestSettleStoredVoidReturnValues_StoredFlowIsAWarning(t *testing.T) { + prog, vs := voidValueViolations(t) + if sev, ok := severityOfVoidValue(vs); !ok || sev != linter.SeverityError { + t.Fatalf("precondition: MDL004 void-value error expected, got %v (found=%v)", sev, ok) + } + got := SettleStoredVoidReturnValues(vs, prog, fakeVoidReturns{"OIDC.SUB_HandlePrivateKey": 1}) + if sev, ok := severityOfVoidValue(got); !ok || sev != linter.SeverityWarning { + t.Fatalf("stored void flow with a return value: want a warning, got %v (found=%v)", sev, ok) + } +} + +// Controls: new code keeps the error — the runtime does return the value, and +// a workflow that branches on the call fails at instance start (write-workflows +// skill), so authoring it is still refused. +func TestSettleStoredVoidReturnValues_NewCodeStaysAnError(t *testing.T) { + prog, vs := voidValueViolations(t) + cases := map[string]voidReturnSource{ + "no project": nil, + "flow not in the project": fakeVoidReturns{}, + "stored flow has no such value": fakeVoidReturns{"OIDC.SUB_HandlePrivateKey": 0}, + } + for name, src := range cases { + t.Run(name, func(t *testing.T) { + got := SettleStoredVoidReturnValues(vs, prog, src) + if sev, ok := severityOfVoidValue(got); !ok || sev != linter.SeverityError { + t.Fatalf("want the error kept, got %v (found=%v)", sev, ok) + } + }) + } +} + +// The project reader counts valued end events of a stored void microflow and +// answers 0 for one that declares a return type. +func TestStoredVoidReturns_CountsValuedEndEvents(t *testing.T) { + end := func(v string) *microflows.EndEvent { return µflows.EndEvent{ReturnValue: v} } + stored := map[string]*microflows.Microflow{ + "M.Void": {ObjectCollection: µflows.MicroflowObjectCollection{ + Objects: []microflows.MicroflowObject{end("M.E.A\n"), end(""), end("false")}}}, + "M.Typed": {ReturnType: µflows.BooleanType{}, ObjectCollection: µflows.MicroflowObjectCollection{ + Objects: []microflows.MicroflowObject{end("true")}}}, + } + b := &mock.MockBackend{ + GetRawUnitByNameFunc: func(_, name string) (*types.RawUnitInfo, error) { + if _, ok := stored[name]; ok { + return &types.RawUnitInfo{ID: name, Contents: []byte{1}}, nil + } + return nil, nil + }, + ParseMicroflowBSONFunc: func(_ []byte, id, _ model.ID) (*microflows.Microflow, error) { + return stored[string(id)], nil + }, + } + s := NewStoredVoidReturns(b) + if n, ok := s.VoidReturnValues("M.Void"); !ok || n != 2 { + t.Errorf("M.Void: got %d,%v want 2,true", n, ok) + } + if n, ok := s.VoidReturnValues("M.Typed"); !ok || n != 0 { + t.Errorf("M.Typed: got %d,%v want 0,true", n, ok) + } + if _, ok := s.VoidReturnValues("M.Missing"); ok { + t.Error("M.Missing reported as stored") + } +} From 230dd5c1d37e52232178cea28603f9e0d580adb0 Mon Sep 17 00:00:00 2001 From: Ako Date: Thu, 8 Oct 2026 08:46:59 +0000 Subject: [PATCH 04/24] fix(xpath): store an underscore-led enumeration value as a string literal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Studio Pro names a True/False enumeration's values `_True` and `_False` and stores a constraint on one as `[A = '_True']`; describe prints it as `[A = M.E._True]`. The stored-form rewrite (normalizeXPathEnumRefs) matched only value names starting with a letter, so exec wrote the qualified name verbatim — CE0161 on mxbuild 10.24.15 — and `check -p` correctly refused it as "neither an attribute nor an association". Evora's AgentCommons.DS_Agent_GetDeployedModels is the instance. The value part of xpathEnumRefRe now allows a leading underscore, as xpathQualifiedNameRe already did. Co-Authored-By: Claude Opus 5.5 --- .../fix-issue/findings/mdl-executor.jsonl | 1 + .../check-fp-xpath-underscore-enum-value.mdl | 33 ++++++++++++++ mdl/executor/cmd_microflows_helpers.go | 6 ++- .../xpath_enum_underscore_value_test.go | 43 +++++++++++++++++++ 4 files changed, 82 insertions(+), 1 deletion(-) create mode 100644 mdl-examples/bug-tests/check-fp-xpath-underscore-enum-value.mdl create mode 100644 mdl/executor/xpath_enum_underscore_value_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index 5cdf70b25..98aa976e2 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -884,3 +884,4 @@ {"area": "mdl/executor", "date": "2026-10-07", "symptom": "A Microflow-typed Java action argument with `)` on the next line — `call java action M.DoSomething(Flow = M.SUB_Target⏎ );` — reported on 0.23/0.24 as passing check/exec and then failing mx check with CE1613 \"The selected microflow 'MyModule.SUB_Target⏎' no longer exists\"; on current main the same script is REFUSED by both `check --references` and exec as \"parameter Flow is of type Microflow, which takes a microflow name ('Module.Microflow'), not MyModule.SUB_Target\" — the message names a valid microflow name. `Flow = empty⏎)` was refused too (or, without the guard, stored the text `empty` as a microflow reference).", "cause": "buildCallArgumentList keeps the whitespace between an argument's last token and `)` by wrapping the value in ast.SourceExpr (expression round-trip). The Microflow-typed path type-switched on the wrapper: microflowParamArgRefusal (#1210) and isEmptyJavaActionArgument saw a SourceExpr, not a QualifiedNameExpr / LiteralExpr. The original stored-newline form had already been removed by #898 (mendixexpr.String trims a SourceExpr's trailing whitespace), which is why the reported symptom no longer reproduces and the bug now surfaces as a false refusal instead.", "file": "`mdl/executor/javaaction_flow_param.go` (`javaActionArgumentValue`, `microflowParamArgRefusal`), `mdl/executor/cmd_microflows_builder_calls.go` (`isEmptyJavaActionArgument`, Microflow-typed mapping), `mdl/executor/javaaction_flow_param_whitespace_test.go`", "insight": "Any executor decision that type-switches on a call/member argument (`arg.Value.(type)`) must unwrap ast.SourceExpr first: the visitor wraps the value only when whitespace follows it, so the hand-built QualifiedNameExpr/LiteralExpr in existing unit tests never exercises the wrapped form and every such switch passes its tests. Build test arguments with visitor.Build from real MDL, with and without a line break before `)`. Wrong turn to skip: the issue blames the lexer/name text — the name is parsed cleanly; the trailing whitespace is a deliberate SourceExpr suffix, and stripping it in the visitor would break describe round-trip layout (#898). Control: reverting the fix with #1210's guard stubbed no longer reproduces the newline on this tree (#898 trims it), so the live regression is the refusal, which the reverted fix reproduces in both the builder test and the check/exec agreement test.", "refs": ["mendixlabs/mxcli#1282", "mendixlabs/mxcli#1210", "#898"], "ce": ["CE1613"]} {"area": "mdl/executor", "date": "2026-10-07", "symptom": "`mxcli check script.mdl -p app.mpr --references` printed \"Check passed!\" for `grant read * on entity System.Nope to M.R`, for a grant to `M.NopeRole`, and for every other GRANT/REVOKE form naming a missing entity, document, member or role; also for `create user role R ( ModuleRoles: (M.NopeRole) )`, `alter user role … add module roles`, and a demo user with an unknown user role or entity (measured on Evora Factory Management, 10.24.15). exec refused the grants after the earlier statements were written; the user-role and demo-user shapes it wrote unresolved (CE1613 at build).", "cause": "No validate path resolved a security statement's names. validateWithContext's switch had no case for any of them (only the role-free validateCrossModuleGrant ran), and the user-role/demo-user executors store module and user role names verbatim without resolving them, so neither check nor exec stood between a typo and the model.", "file": "`mdl/executor/validate_grant_refs.go` (`validateGrantReferences`), wired into `validateProgramWithWarnings` in `validate.go`; test `check_grant_references_pedapp_test.go`", "insight": "Two things made the obvious version wrong. (1) What check refuses has to be what exec refuses, form by form: exec refuses an unknown role on a GRANT and on an entity REVOKE, but a document REVOKE (and `alter user role … drop module roles`) of an unknown role is a reported no-op, which keeps cleanup scripts re-runnable after the role is dropped — refusing it would make check louder than exec. Likewise every System entity is refused by exec (refuseSystemEntityGrant), so `grant … on entity System.User` must be refused by check too, while System MODULE roles in a user role must pass. (2) A script's own effects are not only its CREATE statements: creating a microflow/nanoflow/page in a module with no module roles auto-creates `.User` (defaultDocumentAccessRoles), and doctype-tests/02b grants to exactly that role — the first version flagged 10 statements there. Walking the program in statement order (not the whole-program scriptContext) gives the forward-reference hint for free. Performance trap: reading every module's security costs ~5s on Evora; read only the modules a statement names (~0.1s).", "refs": ["mdl-examples/bug-tests/check-grant-unknown-reference-refused.mdl", "mdl-examples/bug-tests/check-grant-unknown-reference.mdl", "ako/mxcli#1020"]} {"area": "mdl/executor", "date": "2026-10-07", "symptom": "A DataGrid 2 column's `Visible:` expression is stored as `true` (always visible) for every spelling but the old quoted one: `Visible: $showPrices`, `Visible: if … then … else …`, `visible: not(…)`, `Visible: [cond]`. check clean, exec reports \"Created page\", describe shows no Visible. `alter page … set (Visible: ) on grid column(…)` is refused as \"column property VisibleIf not found\"; `insert` of such a column drops it, and drops `Visible: false` too.", "cause": "The visitor lowers every expression spelling of Visible to the key VisibleIf (the page-widget conditional-visibility key) and keeps only plain values under Visible. The column writers read only visible/Visible: the widget engine by schema key + types.ItemPropertyAliases (no VisibleIf alias), the ALTER mutator's resolveColumnPropertyKey the same table, and widgetobj.BuildDataGrid2Column (ALTER insert/replace via buildColumnSpecFromAST) the exact key \"Visible\" as a string only, so a bool false fell to the default too. The mirror image of widget-visible-expression.mdl, where page widgets read VisibleIf and dropped Visible.", "file": "`mdl/types/widget_item_aliases.go` (`visible` ← `VisibleIf`), `mdl/executor/widget_engine.go` (WidgetDefGeneratorVersion 18), `mdl/executor/cmd_pages_builder_v3_widgets.go` (`columnSpecProperties`), `mdl/executor/cmd_pages_describe_output.go` (column Visible via widgetConditionMDL), `mdl/executor/validate_column_visible_scope.go` (MDL-WIDGET43)", "insight": "When the visitor lowers one MDL property to two AST keys by value shape, every consumer must read both — grep the consumers of the key the visitor writes, not of the property name. Making the value persist exposed what the drop had hidden: a column's visible expression has no row object (no dataSource in the widget schema, unlike columnClass), so every $currentObject example became CE0117 at build — measured on 11.14.0 — and needed a check rule (MDL-WIDGET43) in the same change. The widget def's per-item `dataSource` field is the signal for whether $currentObject exists.", "refs": ["mdl-examples/bug-tests/datagrid-column-visible-expression.mdl", "mdl-examples/bug-tests/widget-visible-expression.mdl"]} +{"area": "mdl/executor", "date": "2026-10-08", "symptom": "`mxcli check -p` refuses the describe output of Evora's AgentCommons.DS_Agent_GetDeployedModels: \"the constraint names GenAICommons.ENUM_ModelSupport._True, which is neither an attribute nor an association of it \u2026 CE0161\". With --no-check, exec writes `[A = M.E._True]` verbatim and mxbuild 10.24.15 reports CE0161 \u2014 the round trip corrupts a Studio Pro-authored retrieve", "cause": "normalizeXPathEnumRefs' xpathEnumRefRe required every part of `Module.Enum.Value` to start with a letter. Studio Pro names a True/False enumeration's values `_True`/`_False` (the bare words are reserved) and stores `[A = '_True']`; describe prints the qualified name, and the writer then left it unrewritten. xpathQualifiedNameRe (the project-aware path used by access rules) already allowed `_`", "file": "`mdl/executor/cmd_microflows_helpers.go` (`xpathEnumRefRe`)", "insight": "A check that looks like a false positive can be the only witness of a writer bug: check validates retrieveXPathConstraint's output, which is exactly what the builder stores, so 'check refuses describe output' meant 'exec would corrupt this'. Before relaxing a reference rule on a describe round trip, exec the same text into a scratch project with --no-check and read back the stored BSON. Two regexes describing one token class (xpathEnumRefRe vs xpathQualifiedNameRe) had drifted on the leading character", "refs": ["mdl-examples/bug-tests/check-fp-xpath-underscore-enum-value.mdl"], "ce": ["CE0161"]} diff --git a/mdl-examples/bug-tests/check-fp-xpath-underscore-enum-value.mdl b/mdl-examples/bug-tests/check-fp-xpath-underscore-enum-value.mdl new file mode 100644 index 000000000..69477e74b --- /dev/null +++ b/mdl-examples/bug-tests/check-fp-xpath-underscore-enum-value.mdl @@ -0,0 +1,33 @@ +-- ============================================================================ +-- XPath enumeration value with a leading underscore (`_True`) is stored as-is +-- ============================================================================ +-- +-- Symptom (before fix): `mxcli check -p` refused the describe output of +-- Evora's AgentCommons.DS_Agent_GetDeployedModels — "the constraint names +-- GenAICommons.ENUM_ModelSupport._True, which is neither an attribute nor an +-- association of it … CE0161". The check was right about what exec would +-- write: the stored-form rewrite turned `Module.Enum.Value` into `'Value'` +-- only when the value started with a letter, so `_True` was written verbatim +-- and mxbuild 10.24.15 reported CE0161 (measured). Studio Pro names a +-- True/False enumeration's values `_True` / `_False`, and stores the +-- constraint as `[SupportsSystemPrompt = '_True']`. +-- +-- After fix: stored as `[Supports = '_True']`; check passes, the project +-- builds at 0 errors on mxbuild 10.24.15 and 11.13.0. +-- +-- Usage (expected to PASS check): +-- mxcli check mdl-examples/bug-tests/check-fp-xpath-underscore-enum-value.mdl +-- ============================================================================ + +mdl 1; + +create module Probe; +create enumeration Probe.Support ( _True 'True', _False 'False', Unknown 'Unknown' ); +create persistent entity Probe.Model ( Supports: enum Probe.Support ); + +create microflow Probe.GetSupported () +returns list of Probe.Model +begin + retrieve $L from Probe.Model where [Supports = Probe.Support._True]; + return $L; +end; diff --git a/mdl/executor/cmd_microflows_helpers.go b/mdl/executor/cmd_microflows_helpers.go index 6aed4eb8f..d3caaefe9 100644 --- a/mdl/executor/cmd_microflows_helpers.go +++ b/mdl/executor/cmd_microflows_helpers.go @@ -166,7 +166,11 @@ func qualifiedNameToXPath(e *ast.QualifiedNameExpr) string { // xpathEnumRefRe matches 3-part qualified enum value references like Module.EnumName.Value // in a raw XPath string. These must be replaced with string literals for database queries. -var xpathEnumRefRe = regexp.MustCompile(`[A-Za-z][A-Za-z0-9_]*\.[A-Za-z][A-Za-z0-9_]*\.[A-Za-z][A-Za-z0-9_]*`) +// +// The value may start with an underscore: Studio Pro names a True/False +// enumeration's values `_True` and `_False`. Requiring a letter there wrote +// `[A = M.E._True]` verbatim, which mxbuild rejects with CE0161. +var xpathEnumRefRe = regexp.MustCompile(`[A-Za-z][A-Za-z0-9_]*\.[A-Za-z][A-Za-z0-9_]*\.[A-Za-z_][A-Za-z0-9_]*`) // normalizeXPathEnumRefs converts 3-part qualified enum value references in a raw XPath // string to the string literal format that Mendix database queries require. diff --git a/mdl/executor/xpath_enum_underscore_value_test.go b/mdl/executor/xpath_enum_underscore_value_test.go new file mode 100644 index 000000000..cbf2120ca --- /dev/null +++ b/mdl/executor/xpath_enum_underscore_value_test.go @@ -0,0 +1,43 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +// An enumeration value may start with an underscore — Studio Pro names the +// values of a True/False/Unknown enumeration `_True` and `_False`, since the +// bare words are reserved (GenAICommons.ENUM_ModelSupport). XPath compares the +// attribute with the value's name, so `[A = M.E._True]` is stored as +// `[A = '_True']`, which is what Evora's AgentCommons.DS_Agent_GetDeployedModels +// stores and describe prints back as the qualified name. +// +// The stored-form rewrite matched only parts starting with a letter, so the +// value was written verbatim: `[Supports = Probe.Support._True]`, CE0161 on +// mxbuild 10.24.15 (measured), and `check -p` reported the name as "neither an +// attribute nor an association". The writer and the check share +// retrieveXPathConstraint, so the check was telling the truth about the write. +func TestRetrieveXPathConstraint_UnderscoreEnumValue(t *testing.T) { + cases := map[string]string{ + "Unknown": "[Supports = 'Unknown']", // control: an ordinary value + "_True": "[Supports = '_True']", + "_False": "[Supports = '_False']", + } + for value, want := range cases { + prog, errs := visitor.Build("create microflow M.F () begin retrieve $L from M.Model where [Supports = M.Support." + value + "]; end;") + if len(errs) > 0 { + t.Fatalf("parse: %v", errs) + } + r := prog.Statements[0].(*ast.CreateMicroflowStmt).Body[0].(*ast.RetrieveStmt) + if got := retrieveXPathConstraint(r.Where, "M.Model"); got != want { + t.Errorf("%s: stored %q, want %q", value, got, want) + } + } + if got := normalizeXPathEnumRefs("[A = M.E._True or B = M.E.Open]"); got != "[A = '_True' or B = 'Open']" { + t.Errorf("normalizeXPathEnumRefs: got %q", got) + } +} From d11a40fe267bedec6340107aa03bf8b43b6eabb8 Mon Sep 17 00:00:00 2001 From: Ako Date: Thu, 8 Oct 2026 11:56:24 +0000 Subject: [PATCH 05/24] fix(exprcheck): lex `:` as Mendix division so comparisons over it type as Boolean `mxcli check -p` reported E009 (Boolean slot, Decimal expression) on `hoursBetween(...) : 4 < $Max` in Evora's ConversationalUI.*_GetGranularity microflows, which Studio Pro and mxbuild accept. The expression checker's lexer did not know `:` and emitted a TokError. parseMul saw no operator, parseCmp saw no comparison, and Parse skips the trailing-token error for TokError, so the expression was silently truncated to `hoursBetween(...)` and typed as Decimal. The MDL grammar itself already parsed `:` at the multiplicative level; only the checker's second parser had drifted. `:` is now TokColon, parsed at the one left-associative multiplicative level with `*`, `div` and `mod`, typed as Decimal like `div`, and treated as a recovery boundary. Co-Authored-By: Claude Opus 5.5 --- .../skills/fix-issue/findings/mdl-other.jsonl | 1 + .../exprcheck-colon-division-precedence.mdl | 35 +++++ mdl/exprcheck/colon_division_test.go | 129 ++++++++++++++++++ mdl/exprcheck/infer.go | 2 +- mdl/exprcheck/lexer.go | 5 + mdl/exprcheck/parser.go | 14 +- mdl/exprcheck/recovery.go | 2 +- mdl/exprcheck/unknown_funcs.go | 2 +- 8 files changed, 181 insertions(+), 9 deletions(-) create mode 100644 mdl-examples/bug-tests/exprcheck-colon-division-precedence.mdl create mode 100644 mdl/exprcheck/colon_division_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-other.jsonl b/.claude/skills/fix-issue/findings/mdl-other.jsonl index b889534f2..a20349f8a 100644 --- a/.claude/skills/fix-issue/findings/mdl-other.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-other.jsonl @@ -105,3 +105,4 @@ {"area": "mdl/exprcheck", "date": "2026-10-07", "symptom": "`declare $D DateTime = currentDateTime();` passes `mxcli check` and `exec`, then mx check on 11.14.0 fails `[error] [CE0117] \"Error(s) in expression.\" at Create variable activity 'Create Date and time variable'` — in a microflow and a nanoflow alike", "cause": "funcTable listed `currentDateTime` as a zero-argument built-in. Mendix has no such function; the current time is the `[%CurrentDateTime%]` token. funcTable is MDL044's sole allow-list, so the entry silenced the one rule that would have caught it", "file": "`mdl/exprcheck/func_checker.go` (entry removed), `mdl/exprcheck/unknown_funcs.go` (tokenFuncs → FuncRef.Token), `mdl/executor/validate_microflow.go` (MDL044 hint), bug tests `current-datetime-function.fail.mdl` / `current-datetime-token.mdl`", "insight": "Found by accident while measuring #1216: a default of currentDateTime() made parseDateTime look like it rejected a third argument. Before blaming the outer construct, build the inner expression on its own. The removal alone would give a useless hint — nearestFunc offers a spelling match, and the right answer is a token rather than a function — so name the token in the hint. Nothing in the repo emitted or recommended currentDateTime() (every example uses the token), which suggests the entry came from transcription, like the year()/month()/trunc() entries before it. The remaining unverified extraction names (dayOfYear, hour, …) are the same risk.", "ce": ["CE0117"], "rules": ["MDL044"]} {"area": "mdl/linter", "date": "2026-10-07", "symptom": "A Starlark lint rule cannot tell which project languages are enabled: strings() returns rows for every stored translation (a fresh en_US-only 11.12.5 app already has 124 nl_NL rows), enabling nl_NL changes nothing a rule sees, and a rule calling languages() is skipped with 'undefined: languages (a builtin this mxcli does not have — the rule may need a newer mxcli)'", "cause": "No builtin exposed Settings$LanguageSettings.Languages; LintReader had no GetProjectSettings, so the only language data a rule could reach was the catalog's strings table, which indexes stored translations, not enabled languages", "fix": "languages() builtin returning struct language{code,is_default,check_completeness} from LintReader.GetProjectSettings (added to the interface; the backend already implements it). Classified CatalogFast; a reader error fails the rule instead of answering []", "insight": "Stored translations are not the enabled set: Studio Pro and the starter app keep texts in languages the project never enabled, so any 'per language' check built on strings() over-reports. Project-level facts belong on the reader path (like project_security()), not in the catalog, so they need no FULL build. A new builtin trips three guards at once — builtinModes, the skill's builtin list and its struct table — which is the checklist", "issue": "mendixlabs/mxcli#1306", "file": "mdl/linter/starlark.go (builtinLanguages); mdl/linter/context.go (LintReader)", "test": "mdl/linter/starlark_languages_test.go"} {"area": "mdl/catalog", "date": "2026-10-07", "symptom": "activities_for() / CATALOG.ACTIVITIES returns every call and delete with its target empty: `MicroflowCallAction: action_ref=\"\" entity_ref=\"\" service_ref=\"\"` (also NanoflowCallAction, JavaActionCallAction, JavaScriptActionCallAction, DeleteObjectAction); no column says which call runs in a task queue; a nanoflow's JavaScript action call has no refs_from() row. refs_from() names the call/delete targets fine, and the lint run reports nothing.", "cause": "describeAction (builder_microflows.go) filled ActionRef/EntityRef only for REST/web service/OData calls and create/retrieve; the call and delete cases were never written. The delete's entity was resolvable all along via buildVarEntityMap, which only the refs builder used. microflowActionRef had no JavaScriptActionCallAction case.", "file": "`mdl/catalog/builder_microflows.go` (describeAction, insertFlowActivities), `mdl/catalog/builder_references.go` (microflowActionRef), `mdl/catalog/tables.go` (QueueRef, schema 23)", "insight": "Two builders derive per-action facts from the same parsed actions -- the refs switch and the activities switch -- and drift independently: an action can have its target in refs and an empty column in activities. When a row field is empty, check whether the other switch already resolves it before writing new resolution; here the delete's entity reused buildVarEntityMap (params, create/retrieve outputs, loop iterators) per flow. The queue lives on MicroflowCall.QueueSettings.Queue (and JavaActionCallAction.QueueSettings), not a top-level Queue. A new activities column breaks hand-built `CREATE TABLE activities` fixtures in linter tests (COALESCE does not cover a missing column) and needs a CatalogSchemaVersion bump. Control: HEAD binary on the issue's repro prints exactly the reported empty rows.", "refs": ["mendixlabs/mxcli#1305", "mendixlabs/mxcli#1266", "mendixlabs/mxcli#1267"]} +{"area": "mdl/exprcheck", "date": "2026-10-08", "symptom": "`mxcli check -p` reports E009 \"This position requires Boolean, but the expression has kind Decimal\" on `hoursBetween(…) : 4 < $Max` (Evora ConversationalUI.TokenMonitorSettings_GetGranularity / TraceFiltering_GetGranularity, 7 conditions). Studio Pro and mxbuild accept them; `mxcli check` without -p passes, so only project-backed checks show it.", "cause": "Neither of the two obvious suspects (wrong `:` precedence, or wrong typing of a comparison over a division). The MDL grammar already had `:` in multiplicativeExpression; the separate expression checker's hand-written lexer (mdl/exprcheck/lexer.go) did not know the character and emitted a TokError. parseMul saw no operator, parseCmp saw no comparison, and Parse deliberately skips E014 for a trailing TokError — so the tree was just `hoursBetween(…)` (measured: shape `hoursBetween(...)` before, `(< (: hoursBetween(...) 4) Max)` after), a Decimal, in a Boolean slot.", "file": "mdl/exprcheck/lexer.go (TokColon), parser.go (parseMul, inferKind), infer.go, unknown_funcs.go, recovery.go", "fix": "Lex `:` as TokColon; parseMul takes it at the one left-associative multiplicative level with `*`, `div`, `mod`; every switch that names \"div\" (inferKind → Decimal, SourceIsArithmeticDecimal, the unknown-funcs decimal test) also names \":\"; TokColon is a recovery boundary.", "insight": "An error token that the top level is told to tolerate is a silent truncation: everything after it is never parsed, so any rule that types the whole expression is typing a prefix. When a checker exempts a token kind from the trailing-token error to avoid false positives, the false positive moves to the type rules instead of going away. Two parsers for one language (ANTLR grammar + exprcheck) drift independently — the grammar had `:` all along. Test: TestColonDivision_* asserts the parse tree as an S-expression rather than a kind, since a kind can be right for the wrong tree; reverting the fix fails all three. Evora describe → `check -p` clean and re-exec reports `2 documents already in sync`, .mpr sha unchanged.", "refs": [], "rules": ["E009"]} diff --git a/mdl-examples/bug-tests/exprcheck-colon-division-precedence.mdl b/mdl-examples/bug-tests/exprcheck-colon-division-precedence.mdl new file mode 100644 index 000000000..f10b16c51 --- /dev/null +++ b/mdl-examples/bug-tests/exprcheck-colon-division-precedence.mdl @@ -0,0 +1,35 @@ +mdl 1; +-- ============================================================================ +-- `:` division in a comparison — must PASS `mxcli check -p` +-- ============================================================================ +-- +-- `:` is Mendix division, the same operator as `div` (spelled after ÷, since +-- `/` is member navigation). Evora's ConversationalUI.*_GetGranularity +-- microflows decide on `hoursBetween(…) : 4 < $MaxNumberOfBins`, which Studio +-- Pro and mxbuild accept. `mxcli check -p` reported E009 ("requires Boolean, but +-- the expression has kind Decimal") on every such condition: the expression +-- type checker's lexer did not know `:`, the parse stopped there silently, and +-- only `hoursBetween(…)` was typed. +-- +-- The rule needs a project (it types `$` variables against the model), so a +-- plain `mxcli check` of this file passed even while the bug was live; the +-- precedence assertions are Go tests in mdl/exprcheck/colon_division_test.go. +-- Run against a project: `mxcli check this.mdl -p app.mpr` — no E009. +-- ============================================================================ + +CREATE MODULE BugColonDiv; + +CREATE OR MODIFY MICROFLOW BugColonDiv.GetBucket ($DateFrom: DateTime, $DateTo: DateTime, $Max: Integer) +RETURNS String +BEGIN + IF hoursBetween($DateTo, $DateFrom) : 4 < $Max THEN + RETURN 'quarter-day'; + ELSIF hoursBetween($DateTo, $DateFrom) div 24 < $Max THEN + RETURN 'day'; + ELSIF $Max + hoursBetween($DateTo, $DateFrom) : 168 * 2 >= $Max THEN + RETURN 'fortnight'; + ELSIF $Max mod 7 = 0 THEN + RETURN 'week-aligned'; + END IF; + RETURN 'month'; +END; diff --git a/mdl/exprcheck/colon_division_test.go b/mdl/exprcheck/colon_division_test.go new file mode 100644 index 000000000..6f882a679 --- /dev/null +++ b/mdl/exprcheck/colon_division_test.go @@ -0,0 +1,129 @@ +// SPDX-License-Identifier: Apache-2.0 + +package exprcheck + +import ( + "fmt" + "strings" + "testing" +) + +// `:` is Mendix division — the same operator as `div`, always Decimal +// (docs.mendix.com/refguide/arithmetic-expressions: "You can use either the +// `div` or colon ( `:` ) syntax"). The checker's lexer did not know the +// character: it became a TokError, parseMul did not see an operator, parseCmp +// did not see a comparison, and Parse deliberately ignores a trailing TokError +// — so `hoursBetween(…) : 4 < $Max` was checked as just `hoursBetween(…)`, a +// Decimal, and a Boolean decision slot reported E009 on an expression Studio +// Pro and mxbuild accept (Evora, ConversationalUI.*_GetGranularity). + +// shape renders a parse tree as an S-expression so precedence is asserted +// directly rather than inferred from a type. +func shape(e RobustExpr) string { + switch n := e.(type) { + case *BinExpr: + return fmt.Sprintf("(%s %s %s)", n.Op, shape(n.L), shape(n.R)) + case *UnaryExpr: + return fmt.Sprintf("(%s %s)", n.Op, shape(n.Operand)) + case *ParenExpr: + return shape(n.Inner) + case *VariableExpr: + return n.Name + case *NumberLit: + return n.Value + case *AttributePathExpr: + return n.Variable + "/" + strings.Join(n.Path, "/") + case *CallExpr: + args := make([]string, len(n.Args)) + for i, a := range n.Args { + args[i] = shape(a) + } + return n.Name + "(" + strings.Join(args, ", ") + ")" + case *RecoveredExpr: + return "" + } + return fmt.Sprintf("<%T>", e) +} + +func numericScope(c *Context) { + c.Scope = mapScope{ + "Max": KindInteger, "a": KindInteger, "b": KindInteger, + "c": KindInteger, "d": KindInteger, + } +} + +func parseTree(src string) RobustExpr { + ctx := Context{} + numericScope(&ctx) + e, _ := NewParser().Parse(src, ctx) + return e +} + +// The exact Evora expression, in the Boolean slot it sits in. +func TestColonDivision_EvoraGranularityConditionIsBoolean(t *testing.T) { + const src = "hoursBetween($TokenMonitorSettings/DateTo, $TokenMonitorSettings/DateFrom) : 4 < $Max" + hs := parseIn(t, src, "IfStmt.Condition", numericScope) + if len(hs) != 0 { + t.Errorf("valid Mendix condition reported hints %v: %+v", codes(hs), hs) + } + want := "(< (: hoursBetween(TokenMonitorSettings/DateTo, TokenMonitorSettings/DateFrom) 4) Max)" + if got := shape(parseTree(src)); got != want { + t.Errorf("parse tree\n got %s\n want %s", got, want) + } +} + +func TestColonDivision_Precedence(t *testing.T) { + for _, tc := range []struct{ src, want string }{ + // `:` without a comparison. + {"$a : $b", "(: a b)"}, + // Multiplicative binds tighter than additive… + {"$a + $b : $c < $d", "(< (+ a (: b c)) d)"}, + {"$a - $b : $c", "(- a (: b c))"}, + {"$a : $b - $c", "(- (: a b) c)"}, + // …and all multiplicative operators share one left-associative level. + {"$a : $b * $c", "(* (: a b) c)"}, + {"$a * $b : $c", "(: (* a b) c)"}, + {"$a : $b div $c", "(div (: a b) c)"}, + {"$a mod $b : $c", "(: (mod a b) c)"}, + // CONTROLS: the operators that already worked. + {"$a div 4 < $Max", "(< (div a 4) Max)"}, + {"$a mod 4 < $Max", "(< (mod a 4) Max)"}, + {"$a + $b * $c < $d", "(< (+ a (* b c)) d)"}, + } { + if got := shape(parseTree(tc.src)); got != tc.want { + t.Errorf("%q\n got %s\n want %s", tc.src, got, tc.want) + } + } +} + +func TestColonDivision_TypesLikeDiv(t *testing.T) { + ctx := Context{} + numericScope(&ctx) + for _, tc := range []struct { + src string + want TypeKind + }{ + {"$a : $b", KindDecimal}, // Integer : Integer is Decimal, as with div + {"$a div $b", KindDecimal}, + {"$a mod $b", KindInteger}, + {"$a + $b : $c < $d", KindBoolean}, + {"$a : $b * $c", KindDecimal}, + } { + if got := inferKind(parseTree(tc.src), ctx); got != tc.want { + t.Errorf("%q: kind %s, want %s", tc.src, typeKindName(got), typeKindName(tc.want)) + } + } + // The Boolean slot accepts each comparison, and still rejects a bare + // division (the rule is not simply switched off). + for _, src := range []string{"$a : 4 < $Max", "$a div 4 < $Max", "$a mod 4 < $Max", "$a + $b : $c < $d"} { + if hs := parseIn(t, src, "IfStmt.Condition", numericScope); hasCode(hs, "E009") { + t.Errorf("%q: unexpected E009: %v", src, codes(hs)) + } + } + if hs := parseIn(t, "$a : 4", "IfStmt.Condition", numericScope); !hasCode(hs, "E009") { + t.Errorf("a bare division in a Boolean slot must still be E009: %v", codes(hs)) + } + if !SourceIsArithmeticDecimal("$a : $b", map[string]TypeKind{"a": KindInteger, "b": KindInteger}) { + t.Error("SourceIsArithmeticDecimal does not treat `:` as division") + } +} diff --git a/mdl/exprcheck/infer.go b/mdl/exprcheck/infer.go index 8c223233d..e155cb446 100644 --- a/mdl/exprcheck/infer.go +++ b/mdl/exprcheck/infer.go @@ -52,7 +52,7 @@ func SourceIsArithmeticDecimal(src string, vars map[string]TypeKind) bool { return false } switch be.Op { - case "+", "-", "*", "div", "mod": + case "+", "-", "*", "div", ":", "mod": return inferKind(be, ctx) == KindDecimal } return false diff --git a/mdl/exprcheck/lexer.go b/mdl/exprcheck/lexer.go index 6ae7904c8..4908dae60 100644 --- a/mdl/exprcheck/lexer.go +++ b/mdl/exprcheck/lexer.go @@ -26,6 +26,8 @@ const ( TokPlus TokMinus TokStar + // TokColon is `:`, Mendix division (the same operator as `div`). + TokColon TokEq TokNeq TokLt @@ -141,6 +143,9 @@ func Lex(src string) []Token { case c == '*': push(TokStar, "*", p) advance(1) + case c == ':': + push(TokColon, ":", p) + advance(1) case c == '=': push(TokEq, "=", p) advance(1) diff --git a/mdl/exprcheck/parser.go b/mdl/exprcheck/parser.go index d479a26a3..3224be1d1 100644 --- a/mdl/exprcheck/parser.go +++ b/mdl/exprcheck/parser.go @@ -28,10 +28,10 @@ func (p *parserImpl) Parse(src string, ctx Context) (RobustExpr, []Hint) { // Detect unconsumed trailing tokens (e.g. "emptyor" parsed as a variable, // leaving "$X = ''" silently abandoned). This indicates a structural parse // error — most commonly a keyword glued to an adjacent token without whitespace. - // TokError tokens (unrecognised characters such as ':') are excluded: the - // parser's E007 recovery already handles them inline; re-reporting them here - // would produce false positives for valid expressions that use characters - // the lexer does not model (e.g. "$Total : $Count" with Mendix ':' division). + // TokError tokens (unrecognised characters) are excluded: the parser's E007 + // recovery already handles them inline. That exclusion is also why ':' had + // to become an operator rather than stay a TokError: as an error token it + // ended the parse silently, so `x : 4 < $Max` was checked as just `x`. if t := s.Peek(); t.Kind != TokEOF && t.Kind != TokError { // This hint used to carry no code, no document and no microflow: the // location held only a line and column, which are the offsets WITHIN the @@ -341,8 +341,10 @@ func parseMul(s *Stream, ctx Context) (RobustExpr, []Hint) { left, hints := parseUnary(s, ctx) for { t := s.Peek() + // One left-associative level for every multiplicative operator: `*`, + // `div`, `:` (division, spelled after ÷) and `mod`. isDivMod := t.Kind == TokIdent && (t.Text == "div" || t.Text == "mod") - if t.Kind != TokStar && !isDivMod { + if t.Kind != TokStar && t.Kind != TokColon && !isDivMod { break } op := s.Consume().Text @@ -751,7 +753,7 @@ func inferKind(e RobustExpr, ctx Context) TypeKind { switch n.Op { case "AND", "OR", "=", "!=", "<", "<=", ">", ">=": return KindBoolean - case "div": + case "div", ":": // Mendix division always yields a Decimal, even Integer div Integer. // Assigning the result to an Integer/Long fails mx check with CE0117. return KindDecimal diff --git a/mdl/exprcheck/recovery.go b/mdl/exprcheck/recovery.go index 86e14a339..deb0697fe 100644 --- a/mdl/exprcheck/recovery.go +++ b/mdl/exprcheck/recovery.go @@ -22,7 +22,7 @@ func consumeUntilSafe(s *Stream) string { func isSafeBoundary(t Token) bool { switch t.Kind { - case TokEOF, TokRParen, TokComma, TokPlus, TokMinus, TokStar, + case TokEOF, TokRParen, TokComma, TokPlus, TokMinus, TokStar, TokColon, TokEq, TokNeq, TokLt, TokLe, TokGt, TokGe: return true case TokIdent: diff --git a/mdl/exprcheck/unknown_funcs.go b/mdl/exprcheck/unknown_funcs.go index c98df7e00..89b07dc6f 100644 --- a/mdl/exprcheck/unknown_funcs.go +++ b/mdl/exprcheck/unknown_funcs.go @@ -177,7 +177,7 @@ func SourceRejectedForIntegerTarget(src string, vars map[string]TypeKind) bool { switch n := root.(type) { case *BinExpr: switch n.Op { - case "+", "-", "*", "div", "mod": + case "+", "-", "*", "div", ":", "mod": return inferKind(n, ctx) == KindDecimal } case *CallExpr: From 32693c76872d5a23b4f1323d01a0f940a8468ec0 Mon Sep 17 00:00:00 2001 From: Ako Date: Thu, 8 Oct 2026 12:05:24 +0000 Subject: [PATCH 06/24] perf(check): read the project settings only for a script that writes translations checkTranslationTargets read the project settings (for the source language) on every `check -p`, and only consults them for a `create translations` statement. Skip the read when the script has none. Co-Authored-By: Claude Opus 5.5 --- mdl/executor/validate_exec_refusals.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/mdl/executor/validate_exec_refusals.go b/mdl/executor/validate_exec_refusals.go index 6592c6640..9a205152c 100644 --- a/mdl/executor/validate_exec_refusals.go +++ b/mdl/executor/validate_exec_refusals.go @@ -117,6 +117,18 @@ func cloneJarDependencies(ms *types.ModuleSettings) *types.ModuleSettings { // later plain CREATE of that language is decided by the script, not the // project — the prediction stops at both. func checkTranslationTargets(ctx *ExecContext, prog *ast.Program) []error { + // The project settings are read only for a script that writes + // translations; every other script would pay a settings read for nothing. + hasTranslations := false + for _, stmt := range prog.Statements { + if _, ok := stmt.(*ast.CreateTranslationsStmt); ok { + hasTranslations = true + break + } + } + if !hasTranslations { + return nil + } src := sourceLanguage(ctx) if src == "" { return nil From 569a2c979f45ac31bb934f4c38dc0f5a2507af2b Mon Sep 17 00:00:00 2001 From: Ako Date: Thu, 8 Oct 2026 12:05:25 +0000 Subject: [PATCH 07/24] perf(check): list only the document kinds the script's statements ask about MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check -p` on a 140 MB MPR v1 project spent ~9 s in CheckProjectConflicts for a one-flow `create or modify` script: loadProjectNameSets listed all ~40 document kinds, and every module's security, before reading a statement — though only a plain CREATE is ever flagged. CheckProjectNameClashes then listed its 13 kinds up front too. - projectNameSets.setFor lists a kind on first use and caches it. CheckProjectConflicts asks only for the kinds a plain CREATE names; a DROP records a dropped project name only when a plain CREATE of its kind follows (droppedFromProject is read nowhere else). - CheckProjectNameClashes lists a kind only if a create, rename or move can read it (the union of nameSpaceKinds over those statements). A drop on another kind updates a throwaway set nothing reads. Semantics unchanged: a lazily-loaded set plus the same mutations equals the eagerly-loaded one. `create or modify microflow` still lists the flow name space (microflow/nanoflow/rule), since a cross-kind clash is CE0122 whether or not the statement modifies. Tests count the listings through a MockBackend whose List* fields are wrapped by reflection: on main the or-modify script issues 36 listings; now 0 for a kind with no shared name space, 3 for a microflow. Co-Authored-By: Claude Opus 5.5 --- .../fix-issue/findings/mdl-executor.jsonl | 1 + mdl/executor/validate_duplicates.go | 378 +++++------------- .../validate_duplicates_listing_test.go | 170 ++++++++ mdl/executor/validate_name_clash.go | 33 +- 4 files changed, 311 insertions(+), 271 deletions(-) create mode 100644 mdl/executor/validate_duplicates_listing_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index 6e1cdf091..7d64c328b 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -886,3 +886,4 @@ {"area": "mdl/executor", "date": "2026-10-07", "symptom": "A DataGrid 2 column's `Visible:` expression is stored as `true` (always visible) for every spelling but the old quoted one: `Visible: $showPrices`, `Visible: if … then … else …`, `visible: not(…)`, `Visible: [cond]`. check clean, exec reports \"Created page\", describe shows no Visible. `alter page … set (Visible: ) on grid column(…)` is refused as \"column property VisibleIf not found\"; `insert` of such a column drops it, and drops `Visible: false` too.", "cause": "The visitor lowers every expression spelling of Visible to the key VisibleIf (the page-widget conditional-visibility key) and keeps only plain values under Visible. The column writers read only visible/Visible: the widget engine by schema key + types.ItemPropertyAliases (no VisibleIf alias), the ALTER mutator's resolveColumnPropertyKey the same table, and widgetobj.BuildDataGrid2Column (ALTER insert/replace via buildColumnSpecFromAST) the exact key \"Visible\" as a string only, so a bool false fell to the default too. The mirror image of widget-visible-expression.mdl, where page widgets read VisibleIf and dropped Visible.", "file": "`mdl/types/widget_item_aliases.go` (`visible` ← `VisibleIf`), `mdl/executor/widget_engine.go` (WidgetDefGeneratorVersion 18), `mdl/executor/cmd_pages_builder_v3_widgets.go` (`columnSpecProperties`), `mdl/executor/cmd_pages_describe_output.go` (column Visible via widgetConditionMDL), `mdl/executor/validate_column_visible_scope.go` (MDL-WIDGET43)", "insight": "When the visitor lowers one MDL property to two AST keys by value shape, every consumer must read both — grep the consumers of the key the visitor writes, not of the property name. Making the value persist exposed what the drop had hidden: a column's visible expression has no row object (no dataSource in the widget schema, unlike columnClass), so every $currentObject example became CE0117 at build — measured on 11.14.0 — and needed a check rule (MDL-WIDGET43) in the same change. The widget def's per-item `dataSource` field is the signal for whether $currentObject exists.", "refs": ["mdl-examples/bug-tests/datagrid-column-visible-expression.mdl", "mdl-examples/bug-tests/widget-visible-expression.mdl"]} {"area": "mdl/executor", "date": "2026-10-08", "symptom": "`describe microflow AgentCommons.PromptToUse_ApplyVariable` (Evora Factory Management, MPR v1, Mendix 10.24) runs past a 5-minute timeout, while the app's other 1,800+ flows — including ones with 4x its McCabe and 5x its activities — describe in seconds. The flow is small: 3 decisions, 2 merges, no loops", "cause": "Not graph complexity. Two of its decisions call a rule (`if AgentCommons.Rule_PromptToUse_ContainsVariable(...)`), and every build asks backend.IsRule for each `if Module.Name(...)`. The canonical describe rebuilds the flow once per derived-layout round (9 here), and the layout read cache did not memoise IsRule. modelsdk IsRule resolved the module of EVERY rule before comparing names; each resolution (moduleNameFor) listed the whole project once, then the modules once per folder level. On MPR v1 every listing reads every unit's contents from SQLite (137 MB), so one IsRule took 53 s on a 29-rule app: 2 splits x 9 rounds x 53 s", "file": "`mdl/backend/modelsdk/microflow.go` (IsRule: compare bare name before resolving the module), `mdl/backend/modelsdk/domainmodel_write.go` (moduleNameFor: list modules once, not per ancestor; moduleNameListings counter), `mdl/executor/cmd_microflows_derived_layout_backend.go` (layoutCheckBackend.IsRule memo); tests TestIsRule_ResolvesOneModule, TestDescribe_DerivedLayoutAsksIsRuleOncePerName; repro `mdl-examples/bug-tests/describe-rule-split-timeout.mdl`", "insight": "When one document is pathologically slow, compare it with the LARGEST documents before theorising about its graph: this one was among the smallest, so the cost had to be a per-construct backend lookup multiplied by rounds, not a traversal. A goroutine dump (SIGQUIT) at a few points showed the same IsRule frame every time, and timing a single call (53 s) settled it before any profile. The multiplier chain is the lesson: per-statement lookup x rebuild rounds x per-candidate project listing x per-ancestor listing — each factor harmless alone, and the derived-layout read cache must memoise EVERY backend read the builder makes, not just the ones the first profile showed. Filter by the cheap key (bare name) before the expensive one (module resolution). Tests count calls/listings, not wall time; reverting gives 32 IsRule calls over 8 rounds (want 1) and 12 project listings for one lookup (want 2). Measured: >5 min (killed) -> 6-10 s, byte-identical output; describe -> exec -> Unchanged, MPR byte-identical"} {"area":"mdl/executor","date":"2026-10-08","symptom":"`mxcli check --references` printed \"Check passed!\" (exit 0) for `grant create on entity MyModule.NoSuchEntity to MyModule.User` and a grant to `MyModule.NoSuchRole`; `exec` wrote the valid grant above them, then failed with \"Error: entity not found: MyModule.NoSuchEntity\" / \"module role not found: MyModule.NoSuchRole\" (reported on v0.25.0, Mendix 11.12.4). Issue mendixlabs/mxcli#1333","cause":"Already fixed before the report reached main: 6731d9df (validateGrantReferences) landed the day the issue was filed, after v0.25.0 was cut. Nothing in the executor changed for #1333.","file":"`mdl/executor/validate_grant_refs.go` (fix, 6731d9df); test added `cmd/mxcli/exec_preflight_grant_refs_test.go`","insight":"**Run the reported script on current main before touching code** — and, when it passes, build the parent of the suspected fixing commit and run it there too: that one control turns \"cannot reproduce\" into \"fixed by \", and `git tag --contains ` tells the reporter which release carries it. The pre-fix binary reproduced the report verbatim (Check passed, exit 0; exec writes grant 1, then entity not found). What was still missing was the issue's actual harm — a valid statement written before the bad one — pinned at the layer it lives in: the existing tests drive the reference check through `agreeCheck`, not `execPreflight`, so nothing asserted exec refuses the MIXED script before writing. The new test does, and fails when the `validateGrantReferences` line in `validate.go` is commented out. Repro `mdl-examples/bug-tests/1333-grant-missing-entity-or-role.mdl`.","refs":["mdl-examples/bug-tests/1333-grant-missing-entity-or-role.mdl","cmd/mxcli/exec_preflight_grant_refs_test.go"]} +{"area": "mdl/executor", "date": "2026-10-08", "symptom": "`mxcli check script.mdl -p app.mpr` on a large MPR v1 project (Evora, 140 MB) takes ~15-17 s for a one-flow `create or modify microflow`; ~9 s of it in CheckProjectConflicts, although an or-modify statement is never a same-kind conflict", "cause": "loadProjectNameSets listed every one of ~40 document kinds (plus every module's security, ~5 s alone) before looking at the script, and CheckProjectNameClashes did the same for its 13 kinds; checkTranslationTargets read the project settings for every script. Each listing is a full scan of the stored model on MPR v1", "file": "mdl/executor/validate_duplicates.go (projectNameSets.setFor now lists on first use; CheckProjectConflicts reads only kinds a plain CREATE names, drops gated on that), validate_name_clash.go (CheckProjectNameClashes lists only the name spaces a create/rename/move asks about), validate_exec_refusals.go (settings read only with a create translations), validate_duplicates_listing_test.go", "insight": "A check that preloads 'everything it might consult' scales with the project, not the script. The semantics-preserving rewrite is lazy-per-key plus a static pre-pass over the statements for which keys can ever be READ: mutations (drop/rename) on a key nothing reads may go to a throwaway set. Prove it with a call-counting MockBackend (reflect over the List*Func fields, fill nil ones), not a timer — on main the or-modify script issued 36 listings, after it issues the 3 of the flow name space (CE0122 microflow/nanoflow/rule clash still applies to or-modify) and 0 for a kind with no shared name space", "refs": ["fix/check-conflict-scan-speed", "ako/mxcli#1042"]} diff --git a/mdl/executor/validate_duplicates.go b/mdl/executor/validate_duplicates.go index 2aa57dbf8..bc144caa1 100644 --- a/mdl/executor/validate_duplicates.go +++ b/mdl/executor/validate_duplicates.go @@ -455,136 +455,142 @@ func CheckScriptDuplicates(prog *ast.Program) []linter.Violation { // Phase 2: CheckProjectConflicts // ---------------------------------------------------------------------------- -// projectNameSets holds sets of qualified names already present in the project, -// loaded once before the walk. +// projectNameSets answers which qualified names of a kind the project already +// holds. Each kind is listed on first use and kept: a listing is a scan of the +// stored model (on a 140 MB MPR v1 project ~0.1 s per document kind, and ~5 s +// for module roles, which read every module's security), and the conflict +// check asks about only the kinds its script plainly creates — usually none. type projectNameSets struct { - entities map[string]bool - enumerations map[string]bool - constants map[string]bool - microflows map[string]bool - nanoflows map[string]bool - pages map[string]bool - snippets map[string]bool - layouts map[string]bool - javaActions map[string]bool - workflows map[string]bool - businessEvents map[string]bool - publishedRest map[string]bool - jsonStructures map[string]bool - importMappings map[string]bool - exportMappings map[string]bool - dataTransformers map[string]bool - agentModels map[string]bool - knowledgeBases map[string]bool - consumedMcp map[string]bool - agents map[string]bool - imageCollections map[string]bool - associations map[string]bool - rules map[string]bool - javaScriptActs map[string]bool - moduleRoles map[string]bool - demoUsers map[string]bool - userRoles map[string]bool - configurations map[string]bool - menus map[string]bool - queues map[string]bool - scheduledEvents map[string]bool - regexes map[string]bool - messageColls map[string]bool - messageDocs map[string]bool - dbConnections map[string]bool - restClients map[string]bool - odataClients map[string]bool - odataServices map[string]bool + ctx *ExecContext + h *ContainerHierarchy + loaded map[string]map[string]bool } -// projectSetFor returns the existence set for the given doc-type key, or nil -// if we don't check project conflicts for that type. +// setFor returns the existence set for the given doc-type key, listing that +// kind on first use, or nil if we don't check project conflicts for that type. func (ps *projectNameSets) setFor(docType string) map[string]bool { + if set, ok := ps.loaded[docType]; ok { + return set + } + if ps.h == nil { + // No hierarchy, no qualified names — callers treat nil as "no conflicts". + return nil + } + ctx, h := ps.ctx, ps.h + var set map[string]bool switch docType { case "entity": - return ps.entities + set = buildEntityQualifiedNames(ctx) case "enumeration": - return ps.enumerations + set = listedQualifiedNames(h, ctx.Backend.ListEnumerations) case "constant": - return ps.constants + set = listedQualifiedNames(h, ctx.Backend.ListConstants) case "microflow": - return ps.microflows + set = buildMicroflowQualifiedNames(ctx) case "nanoflow": - return ps.nanoflows + set = buildNanoflowQualifiedNames(ctx) case "page": - return ps.pages + set = buildPageQualifiedNames(ctx) case "snippet": - return ps.snippets + set = buildSnippetQualifiedNames(ctx) case "layout": - return ps.layouts + set = buildLayoutQualifiedNames(ctx) case "javaaction": - return ps.javaActions + set = buildJavaActionQualifiedNames(ctx) case "workflow": - return ps.workflows + set = listedQualifiedNames(h, ctx.Backend.ListWorkflows) case "business-event-service": - return ps.businessEvents + set = listedQualifiedNames(h, ctx.Backend.ListBusinessEventServices) case "published-rest-service": - return ps.publishedRest + set = listedQualifiedNames(h, ctx.Backend.ListPublishedRestServices) case "json-structure": - return ps.jsonStructures + set = listedQualifiedNames(h, ctx.Backend.ListJsonStructures) case "import-mapping": - return ps.importMappings + set = listedQualifiedNames(h, ctx.Backend.ListImportMappings) case "export-mapping": - return ps.exportMappings + set = listedQualifiedNames(h, ctx.Backend.ListExportMappings) case "data-transformer": - return ps.dataTransformers + set = listedQualifiedNames(h, ctx.Backend.ListDataTransformers) case "agent-model": - return ps.agentModels + set = listedQualifiedNames(h, ctx.Backend.ListAgentEditorModels) case "knowledge-base": - return ps.knowledgeBases + set = listedQualifiedNames(h, ctx.Backend.ListAgentEditorKnowledgeBases) case "consumed-mcp-service": - return ps.consumedMcp + set = listedQualifiedNames(h, ctx.Backend.ListAgentEditorConsumedMCPServices) case "agent": - return ps.agents + set = listedQualifiedNames(h, ctx.Backend.ListAgentEditorAgents) case "image-collection": - return ps.imageCollections + set = listedQualifiedNames(h, ctx.Backend.ListImageCollections) case "association": - return ps.associations + // Intra-module and cross-module. + set = buildAssociationQualifiedNames(ctx) case "rule": - return ps.rules + set = buildRuleQualifiedNames(ctx) case "javascriptaction": - return ps.javaScriptActs + set = buildJavaScriptActionQualifiedNames(ctx) case "module-role": - return ps.moduleRoles - case "demo-user": - return ps.demoUsers - case "user-role": - return ps.userRoles + set = buildModuleRoleQualifiedNames(ctx) + case "demo-user", "user-role": + // The same lookups execCreateDemoUser and execCreateUserRole refuse on. + set = map[string]bool{} + if sec, err := ctx.Backend.GetProjectSecurity(); err == nil && sec != nil { + if docType == "demo-user" { + for _, du := range sec.DemoUsers { + if du != nil { + set[du.UserName] = true + } + } + } else { + for _, ur := range sec.UserRoles { + if ur != nil { + set[ur.Name] = true + } + } + } + } case "configuration": - return ps.configurations + // Project settings, not documents, so not module-qualified. + set = map[string]bool{} + if st, err := ctx.Backend.GetProjectSettings(); err == nil && st != nil && st.Configuration != nil { + for _, cfg := range st.Configuration.Configurations { + if cfg != nil { + set[cfg.Name] = true + } + } + } + // The documents whose handlers refuse a plain CREATE of an existing one, + // read through the same Backend lists the handlers search (ako/mxcli#557). case "menu": - return ps.menus + set = listedQualifiedNames(h, ctx.Backend.ListMenuDocuments) case "queue": - return ps.queues + set = listedQualifiedNames(h, ctx.Backend.ListQueues) case "scheduled-event": - return ps.scheduledEvents + set = listedQualifiedNames(h, ctx.Backend.ListScheduledEvents) case "regular-expression": - return ps.regexes + set = listedQualifiedNames(h, ctx.Backend.ListRegularExpressions) case "message-definition-collection": - return ps.messageColls + set = listedQualifiedNames(h, ctx.Backend.ListMessageDefinitionCollections) case "message-definition": - return ps.messageDocs + set = listedQualifiedNames(h, ctx.Backend.ListMessageDefinitionDocuments) case "database-connection": - return ps.dbConnections + set = listedQualifiedNames(h, ctx.Backend.ListDatabaseConnections) case "rest-client": - return ps.restClients + set = listedQualifiedNames(h, ctx.Backend.ListConsumedRestServices) case "odata-client": - return ps.odataClients + set = listedQualifiedNames(h, ctx.Backend.ListConsumedODataServices) case "odata-service": - return ps.odataServices + set = listedQualifiedNames(h, ctx.Backend.ListPublishedODataServices) + default: + // "module" is deliberately absent: CREATE MODULE on an existing module + // is a no-op that prints "already exists" and exits 0, so `create + // module M;` is the standard script preamble. Flagging it would be a + // false positive on essentially every script. + // TestEveryCreateDocTypeIsProjectChecked carries this exemption + // explicitly so it stays a decision rather than an omission. + return nil } - // "module" is deliberately absent: CREATE MODULE on an existing module is a - // no-op that prints "already exists" and exits 0, so `create module M;` is - // the standard script preamble. Flagging it would be a false positive on - // essentially every script. TestEveryCreateDocTypeIsProjectChecked carries - // this exemption explicitly so it stays a decision rather than an omission. - return nil + ps.loaded[docType] = set + return set } // execFoldsExistingName lists the kinds whose exec handler finds the stored @@ -652,187 +658,13 @@ func listedQualifiedNames[T any](h *ContainerHierarchy, list func() ([]*T, error return out } -// loadProjectNameSets queries the project for all existing document names. +// loadProjectNameSets prepares the project-side name lookup. Nothing is listed +// here: setFor lists a kind the first time it is asked about. func loadProjectNameSets(ctx *ExecContext) *projectNameSets { - ps := &projectNameSets{} - h, err := getHierarchy(ctx) - if err != nil { - // Return empty sets — callers treat empty as "no conflicts". - return ps - } - - ps.entities = buildEntityQualifiedNames(ctx) - ps.microflows = buildMicroflowQualifiedNames(ctx) - ps.nanoflows = buildNanoflowQualifiedNames(ctx) - ps.pages = buildPageQualifiedNames(ctx) - ps.snippets = buildSnippetQualifiedNames(ctx) - ps.layouts = buildLayoutQualifiedNames(ctx) - ps.javaActions = buildJavaActionQualifiedNames(ctx) - - // Enumerations - ps.enumerations = make(map[string]bool) - if enums, err := ctx.Backend.ListEnumerations(); err == nil { - for _, e := range enums { - ps.enumerations[h.GetQualifiedName(e.ContainerID, e.Name)] = true - } - } - - // Constants - ps.constants = make(map[string]bool) - if consts, err := ctx.Backend.ListConstants(); err == nil { - for _, c := range consts { - ps.constants[h.GetQualifiedName(c.ContainerID, c.Name)] = true - } - } - - // Workflows - ps.workflows = make(map[string]bool) - if wfs, err := ctx.Backend.ListWorkflows(); err == nil { - for _, w := range wfs { - ps.workflows[h.GetQualifiedName(w.ContainerID, w.Name)] = true - } - } - - // Business event services - ps.businessEvents = make(map[string]bool) - if bes, err := ctx.Backend.ListBusinessEventServices(); err == nil { - for _, b := range bes { - ps.businessEvents[h.GetQualifiedName(b.ContainerID, b.Name)] = true - } - } - - // Published REST services - ps.publishedRest = make(map[string]bool) - if prs, err := ctx.Backend.ListPublishedRestServices(); err == nil { - for _, p := range prs { - ps.publishedRest[h.GetQualifiedName(p.ContainerID, p.Name)] = true - } - } - - // JSON structures - ps.jsonStructures = make(map[string]bool) - if jss, err := ctx.Backend.ListJsonStructures(); err == nil { - for _, j := range jss { - ps.jsonStructures[h.GetQualifiedName(j.ContainerID, j.Name)] = true - } - } - - // Import mappings - ps.importMappings = make(map[string]bool) - if ims, err := ctx.Backend.ListImportMappings(); err == nil { - for _, m := range ims { - ps.importMappings[h.GetQualifiedName(m.ContainerID, m.Name)] = true - } - } - - // Export mappings - ps.exportMappings = make(map[string]bool) - if ems, err := ctx.Backend.ListExportMappings(); err == nil { - for _, m := range ems { - ps.exportMappings[h.GetQualifiedName(m.ContainerID, m.Name)] = true - } - } - - // Data transformers - ps.dataTransformers = make(map[string]bool) - if dts, err := ctx.Backend.ListDataTransformers(); err == nil { - for _, d := range dts { - ps.dataTransformers[h.GetQualifiedName(d.ContainerID, d.Name)] = true - } - } - - // Agent editor: models - ps.agentModels = make(map[string]bool) - if ms, err := ctx.Backend.ListAgentEditorModels(); err == nil { - for _, m := range ms { - ps.agentModels[h.GetQualifiedName(m.ContainerID, m.Name)] = true - } - } - - // Agent editor: knowledge bases - ps.knowledgeBases = make(map[string]bool) - if kbs, err := ctx.Backend.ListAgentEditorKnowledgeBases(); err == nil { - for _, k := range kbs { - ps.knowledgeBases[h.GetQualifiedName(k.ContainerID, k.Name)] = true - } + ps := &projectNameSets{ctx: ctx, loaded: map[string]map[string]bool{}} + if h, err := getHierarchy(ctx); err == nil { + ps.h = h } - - // Agent editor: consumed MCP services - ps.consumedMcp = make(map[string]bool) - if svcs, err := ctx.Backend.ListAgentEditorConsumedMCPServices(); err == nil { - for _, s := range svcs { - ps.consumedMcp[h.GetQualifiedName(s.ContainerID, s.Name)] = true - } - } - - // Agent editor: agents - ps.agents = make(map[string]bool) - if ags, err := ctx.Backend.ListAgentEditorAgents(); err == nil { - for _, a := range ags { - ps.agents[h.GetQualifiedName(a.ContainerID, a.Name)] = true - } - } - - // Associations (intra-module and cross-module) - ps.associations = buildAssociationQualifiedNames(ctx) - - // Rules - ps.rules = buildRuleQualifiedNames(ctx) - - // JavaScript actions - ps.javaScriptActs = buildJavaScriptActionQualifiedNames(ctx) - - // Module roles - ps.moduleRoles = buildModuleRoleQualifiedNames(ctx) - - // Demo users and user roles: the same lookups execCreateDemoUser and - // execCreateUserRole refuse on. - ps.demoUsers = make(map[string]bool) - ps.userRoles = make(map[string]bool) - if sec, err := ctx.Backend.GetProjectSecurity(); err == nil && sec != nil { - for _, du := range sec.DemoUsers { - if du != nil { - ps.demoUsers[du.UserName] = true - } - } - for _, ur := range sec.UserRoles { - if ur != nil { - ps.userRoles[ur.Name] = true - } - } - } - - // Configurations: project settings, not documents, so not module-qualified. - ps.configurations = make(map[string]bool) - if st, err := ctx.Backend.GetProjectSettings(); err == nil && st != nil && st.Configuration != nil { - for _, cfg := range st.Configuration.Configurations { - if cfg != nil { - ps.configurations[cfg.Name] = true - } - } - } - - // The documents whose handlers refuse a plain CREATE of an existing one, - // read through the same Backend lists the handlers search (ako/mxcli#557). - ps.menus = listedQualifiedNames(h, ctx.Backend.ListMenuDocuments) - ps.queues = listedQualifiedNames(h, ctx.Backend.ListQueues) - ps.scheduledEvents = listedQualifiedNames(h, ctx.Backend.ListScheduledEvents) - ps.regexes = listedQualifiedNames(h, ctx.Backend.ListRegularExpressions) - ps.messageColls = listedQualifiedNames(h, ctx.Backend.ListMessageDefinitionCollections) - ps.messageDocs = listedQualifiedNames(h, ctx.Backend.ListMessageDefinitionDocuments) - ps.dbConnections = listedQualifiedNames(h, ctx.Backend.ListDatabaseConnections) - ps.restClients = listedQualifiedNames(h, ctx.Backend.ListConsumedRestServices) - ps.odataClients = listedQualifiedNames(h, ctx.Backend.ListConsumedODataServices) - ps.odataServices = listedQualifiedNames(h, ctx.Backend.ListPublishedODataServices) - - // Image collections - ps.imageCollections = make(map[string]bool) - if ics, err := ctx.Backend.ListImageCollections(); err == nil { - for _, ic := range ics { - ps.imageCollections[h.GetQualifiedName(ic.ContainerID, ic.Name)] = true - } - } - return ps } @@ -851,6 +683,16 @@ func CheckProjectConflicts(ctx *ExecContext, prog *ast.Program) []error { } ps := loadProjectNameSets(ctx) + // Only a plain CREATE is ever flagged, so only the kinds plainly created + // need their stored names — and a DROP matters only to a later plain + // CREATE of its kind. A script of `create or modify` statements lists + // nothing here. + plainlyCreated := map[string]bool{} + for _, stmt := range prog.Statements { + if dt, _, idempotent := stmtCreateInfo(stmt); dt != "" && !idempotent { + plainlyCreated[dt] = true + } + } reg := newNameRegistry() droppedFromProject := newNameRegistry() var errs []error @@ -873,7 +715,7 @@ func CheckProjectConflicts(ctx *ExecContext, prog *ast.Program) []error { // record it so a subsequent CREATE is not flagged as a conflict. if dt, name := stmtDropInfo(stmt); dt != "" { reg.remove(dt, name) - if projectSetHas(dt, ps.setFor(dt), name) { + if plainlyCreated[dt] && projectSetHas(dt, ps.setFor(dt), name) { droppedFromProject.add(dt, name, stmtNum) } continue diff --git a/mdl/executor/validate_duplicates_listing_test.go b/mdl/executor/validate_duplicates_listing_test.go new file mode 100644 index 000000000..64dcbf02e --- /dev/null +++ b/mdl/executor/validate_duplicates_listing_test.go @@ -0,0 +1,170 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "reflect" + "sort" + "strings" + "sync" + "testing" + + "github.com/mendixlabs/mxcli/mdl/backend/mock" +) + +// The project-side existence checks behind `check -p` list documents to learn +// which names the project already holds. Each listing is a scan of the stored +// model — on a 140 MB MPR v1 project, ~0.1 s for a document kind and ~5 s for +// the module roles, which read every module's security — and CheckProjectConflicts +// used to issue all ~40 of them before looking at the script, although it only +// ever consults the kinds a plain CREATE names. A one-flow `create or modify` +// paid ~9 s for listings whose answers nothing read. +// +// These tests count the listings instead of timing them: a listing that is not +// needed is a listing that must not happen. + +// countListings wraps every List*, GetProjectSecurity, GetModuleSecurity and +// GetProjectSettings function field of mb so each call is counted by name. A field the test left nil +// is filled with a function returning zero values, so an unconfigured listing +// is still counted rather than erroring out of sight. +type listingCounter struct { + mu sync.Mutex + calls map[string]int +} + +func (c *listingCounter) names() []string { + c.mu.Lock() + defer c.mu.Unlock() + var out []string + for n := range c.calls { + out = append(out, n) + } + sort.Strings(out) + return out +} + +// hierarchyListings are what resolving a container's qualified name reads; they +// are not a question about which names a kind holds. +var hierarchyListings = map[string]bool{"ListModules": true, "ListFolders": true, "ListUnits": true} + +func countListings(mb *mock.MockBackend) *listingCounter { + c := &listingCounter{calls: map[string]int{}} + v := reflect.ValueOf(mb).Elem() + t := v.Type() + for i := 0; i < t.NumField(); i++ { + f := t.Field(i) + if f.Type.Kind() != reflect.Func || !strings.HasSuffix(f.Name, "Func") { + continue + } + name := strings.TrimSuffix(f.Name, "Func") + if !strings.HasPrefix(name, "List") && name != "GetProjectSecurity" && + name != "GetModuleSecurity" && name != "GetProjectSettings" { + continue + } + if hierarchyListings[name] { + continue + } + ft := f.Type + inner := reflect.New(ft).Elem() // a copy: the field itself is replaced below + inner.Set(v.Field(i)) + wrapped := reflect.MakeFunc(ft, func(args []reflect.Value) []reflect.Value { + c.mu.Lock() + c.calls[name]++ + c.mu.Unlock() + if !inner.IsNil() { + return inner.Call(args) + } + out := make([]reflect.Value, ft.NumOut()) + for j := range out { + out[j] = reflect.Zero(ft.Out(j)) + } + return out + }) + v.Field(i).Set(wrapped) + } + return c +} + +func TestCheckProjectConflicts_ListsOnlyWhatTheScriptAsks(t *testing.T) { + cases := []struct { + name string + script string + // want is exactly the set of listings the check may issue. + want []string + }{ + { + // `or modify` is never a same-kind conflict, and a regular + // expression shares no name space with another kind: nothing to ask. + name: "or-modify of a kind with no shared name space", + script: `create or modify regular expression M.Pattern ( + Expression: '^[a-z]+$' +);`, + want: nil, + }, + { + // `or modify` is still a cross-kind clash if a nanoflow or rule + // holds the name (CE0122), so the flow name space is read — and + // nothing else. + name: "or-modify of one microflow", + script: `create or modify microflow M.Probe () +begin + log info node 'Probe' 'hello'; +end;`, + want: []string{"ListMicroflows", "ListNanoflows", "ListRules"}, + }, + { + name: "plain create of a workflow", + script: `create workflow M.BrandNewWF begin end workflow;`, + want: []string{"ListWorkflows"}, + }, + { + // A drop of a kind no plain create follows needs no same-kind set. + name: "drop then or-modify of a kind with no shared name space", + script: `drop workflow M.ExistingWF; +create or modify regular expression M.Pattern ( + Expression: '^[a-z]+$' +);`, + want: nil, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx, _ := setupProjectConflictCtx(t) + counter := countListings(ctx.Backend.(*mock.MockBackend)) + if msgs := conflictErrorMessages(ctx, tc.script, t); len(msgs) != 0 { + t.Fatalf("expected no conflicts, got %v", msgs) + } + got := counter.names() + want := append([]string(nil), tc.want...) + sort.Strings(want) + if !reflect.DeepEqual(got, want) && !(len(got) == 0 && len(want) == 0) { + t.Errorf("listings issued:\n got %v\n want %v", got, want) + } + }) + } +} + +// The control: narrowing what is listed must not narrow what is found. A plain +// create of a stored name is still flagged, and a cross-kind clash in the flow +// name space is still found by a script that names only a microflow. +func TestCheckProjectConflicts_ListingNarrowedStillFlags(t *testing.T) { + ctx, _ := setupProjectConflictCtx(t) + counter := countListings(ctx.Backend.(*mock.MockBackend)) + assertHasConflict(t, ctx, `create workflow M.ExistingWF begin end workflow;`, "M.ExistingWF") + if got := counter.names(); !reflect.DeepEqual(got, []string{"ListWorkflows"}) { + t.Errorf("listings issued: got %v, want [ListWorkflows]", got) + } + + ctx, _ = setupProjectConflictCtx(t) + // M.ExistingRule is a rule; a microflow of the same name is CE0122. + assertHasConflict(t, ctx, `create or modify microflow M.ExistingRule () +begin + log info node 'Probe' 'hello'; +end;`, "M.ExistingRule") + + ctx, _ = setupProjectConflictCtx(t) + // Drop, then a plain create of the same name: not a conflict, because the + // drop removed it — which needs the dropped kind's set to be read. + assertNoConflicts(t, ctx, `drop workflow M.ExistingWF; +create workflow M.ExistingWF begin end workflow;`) +} diff --git a/mdl/executor/validate_name_clash.go b/mdl/executor/validate_name_clash.go index 948becae0..ec8350b15 100644 --- a/mdl/executor/validate_name_clash.go +++ b/mdl/executor/validate_name_clash.go @@ -438,15 +438,42 @@ func CheckProjectNameClashes(ctx *ExecContext, prog *ast.Program) []error { if !ctx.Connected() { return nil } - project := map[string]*nameSet{} + // A kind is listed the first time a statement asks about it, and only if + // some statement can ask: a create, rename or move reads the kinds of its + // name space and nothing else. A drop, rename or move still updates a + // kind nothing reads — into an empty set, since what it holds is never + // consulted. Listing every kind up front cost a one-flow script ten + // listings it did not need. + checked := map[string]bool{} for _, k := range nameClashCheckedKinds { - project[k] = loadNameSet(ctx, k) + checked[k] = true } + asked := map[string]bool{} + for _, stmt := range prog.Statements { + var dt string + switch s := stmt.(type) { + case *ast.RenameStmt: + dt, _, _, _ = renameTarget(s) + case *ast.MoveStmt: + dt, _, _, _ = moveTarget(s) + default: + dt, _, _ = stmtCreateKind(stmt) + } + for _, k := range nameSpaceKinds(dt) { + asked[k] = true + } + } + project := map[string]*nameSet{} sets := func(kind string) *nameSet { if s, ok := project[kind]; ok { return s } - s := newNameSet(nil) + var s *nameSet + if checked[kind] && asked[kind] { + s = loadNameSet(ctx, kind) + } else { + s = newNameSet(nil) + } project[kind] = s return s } From 8fd7f9d4fded3ce33882273d6f709b25550ebf4a Mon Sep 17 00:00:00 2001 From: Ako Date: Thu, 8 Oct 2026 13:18:47 +0000 Subject: [PATCH 08/24] fix(describe): join a crossed merge wherever a branch reaches it A crossed merge (Mode 2) is joined on arrival by traverseFlow, traverseFlowUntilMerge and continueAfterSplitJoin, but five other places walked THROUGH a merge without asking: continueAfterNestedSplitJoin, the split-type continuation at both levels, and the guard-clause false-branch skip at both levels. When the crossing sat inside another branch, the crossed merge's suffix was printed inline there, and then either - the walk marked it visited, its section was never declared, and the description ended in `merge shared1; join shared2;` with nothing declaring shared2 (Evora SnowflakeIntegration / SnowflakeRESTSQL .Request_Validation: MDL-FLOW02, MDL003), or - the walk ran on a case/split-type arm's private visited copy, so the section printed the suffix a second time (Evora AgentCommons.Tool_Validate: one stored MicroflowSelection_GetCreate call, two in the description, MDL063). Every pass-through now asks joinCrossedMerge first, and a labelled merge is no longer skipped by the guard-clause continuation. The faithful Tool_Validate description then failed check for a second reason: the section after the `split type` reads $IsValid, declared in the arm that joins it. The check-time validator scoped variables by text order; a `merge