From 79faf2d87833ded335c8ead8e05571780331a6e1 Mon Sep 17 00:00:00 2001 From: Ako Date: Sat, 3 Oct 2026 17:42:56 +0000 Subject: [PATCH 01/15] fix: count workflow activities with one walk in list workflows, show structure and the catalog (#963) list workflows and show structure recursed over outcome flows only and skipped boundary-event flows and event sub-processes, so TestApp Workflow1 listed 5 activities where the catalog counted 8. The catalog's walk moves to wfnames.WalkActivities/CountActivities and all three use it. Co-Authored-By: Claude Opus 5.5 --- .../fix-issue/findings/mdl-executor.jsonl | 1 + CHANGELOG.md | 1 + mdl/backend/wfnames/walk.go | 69 +++++++++++++ mdl/catalog/builder_workflows.go | 17 +--- mdl/catalog/workflow_walk.go | 39 +------- mdl/executor/cmd_structure.go | 53 +--------- mdl/executor/cmd_workflows.go | 63 +----------- mdl/executor/cmd_workflows_count_test.go | 99 +++++++++++++++++++ 8 files changed, 185 insertions(+), 157 deletions(-) create mode 100644 mdl/backend/wfnames/walk.go create mode 100644 mdl/executor/cmd_workflows_count_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index 4dc21c1e0a..045a13388d 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -850,3 +850,4 @@ {"area": "mdl/executor", "date": "2026-10-03", "symptom": "ako/mxcli#950 item 1: a page button described as `Action: delete close page` executes to a Forms$DeleteClientAction with ClosePage=false — a describe → exec round trip silently stops the button closing its page; check, exec and mx check are all clean", "cause": "buildClientActionV3Base's `delete` case did not copy action.ClosePage, although the visitor sets it and the writer (clientActionToGen) writes it; save/cancel copied it", "file": "`mdl/executor/cmd_pages_builder_v3.go` (buildClientActionV3Base, case \"delete\")", "insight": "The audit of the other cases found no other dropped visitor flag, but two hard-coded ones describe cannot express: complete task always writes ClosePage/Commit true and show page/create object write NumberOfPagesToClose2 \"\" — every Studio Pro instance in TestApp/PedApp has those values, so they are latent, not live. A text round trip could not have caught this: describe printed the flag correctly and the second describe of the exec'd page printed `delete`, which looks like a user edit — read the stored ClosePage", "refs": ["ako/mxcli#950"]} {"area": "mdl/executor", "date": "2026-10-03", "symptom": "ako/mxcli#950 item 2: describe of a flow ending `return [%CurrentUser%];` prints `return $[%CurrentUser%];`, which does not parse; `return if … then … else …` likewise became `return $if …` (two TestApp WorkflowCommons microflows)", "cause": "formatActivity's EndEvent branch added `$` to any return value without one of + ' \" ( ) — a character blacklist standing in for 'is a bare variable name'", "file": "`mdl/executor/cmd_microflows_format_action.go` (isBareReturnVariable)", "insight": "Restore a stripped sigil only for the positive shape it was stripped from (a bare name, optionally /path); a blacklist of characters lets every new expression form through. The other `$`-adding sites in describe prefix variable-NAME fields, not expressions, and are safe", "refs": ["ako/mxcli#950"]} {"area": "mdl/executor", "date": "2026-10-03", "symptom": "ako/mxcli#950 item 3: describe of a navigation list prints item actions as `show_page 'Mod.Page'` (does not parse — TestApp Rules.Entity_Menu, 3 syntax errors); with that fixed, exec refuses the description with `item inside navigationlist requires a name` because Studio Pro leaves items unnamed", "cause": "extractNavigationListItemAction had a private copy of the page-action rendering in the legacy form instead of the shared renderClientActionMDL; buildNavigationListItemV3 required a name Studio Pro never stores; and once exec accepted it, the writer wrote `Name: \"\"` and no ConditionalVisibilitySettings where Studio Pro stores no Name key and a null slot (6 of 6 items in TestApp), so GetPut still rewrote the snippet", "file": "`mdl/executor/cmd_pages_describe_parse.go` (extractNavigationListItemAction), `mdl/executor/cmd_pages_builder_v3_widgets.go` (buildNavigationListItemV3), `mdl/executor/cmd_pages_describe_output.go` (item header), `mdl/backend/modelsdk/widget_write.go` (navListItemToGen, Forms$NavigationListItem NullFields)", "insight": "Fixing the reported parse error only exposed the next law: the issue said the empty item name 'parses fine', which was true and irrelevant — exec refused it, and after that the writer rewrote it. An unnamed item with no Name key passes mx check at 11.14.0, contrary to the old ledger note that the key is mandatory (that applies to a NAMED item's key, not its absence). Run the whole describe → check → exec → describe chain on the Studio Pro-authored document before declaring a round-trip bug fixed", "refs": ["ako/mxcli#950"]} +{"area": "mdl/executor", "date": "2026-10-03", "symptom": "`list workflows` and `show structure` report fewer workflow activities than the catalog's workflows_data (TestApp Workflow1: 5 vs 8)", "cause": "cmd_workflows.go countFlowActivities and cmd_structure.go countStructureFlowActivities each recursed over outcome flows only, skipping boundary-event flows and event sub-processes; the catalog had moved to a shared walk in #937 and the executor copies were left behind", "file": "`mdl/backend/wfnames/walk.go` (`WalkActivities`, `CountActivities`), `mdl/executor/cmd_workflows.go`, `mdl/executor/cmd_structure.go`, `mdl/catalog/workflow_walk.go`", "insight": "Duplicate-resolver drift: fixing one copy of a traversal (#937) left two private copies answering differently. The walk now lives in wfnames, which both catalog and executor already import, so there is one place to add a new sub-flow slot. Grep for every recursion over `workflows.Flow` when one is fixed.", "refs": ["ako/mxcli#963", "ako/mxcli#937"]} diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ac2b20156..c9298c0f4b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **`list workflows` and `show structure` count every activity of a workflow** (ako/mxcli#963) — including those on a boundary-event path and in an event sub-process, as the catalog's `workflows_data` has since #937. TestApp `Workflow1` listed 5 activities where the catalog counted 8; all three now share one walk (`wfnames.WalkActivities`). - **`docker check` no longer modifies the project** (ako/mxcli#951) — `mx update-widgets` and `mx check` now run on a temporary copy, for MPR v1 and v2 alike, and what mx prints names the project's own paths. Before, a check rewrote an MPR v1 project's `.mpr` permanently (only v2 was restored from a snapshot), and `mx check` itself rewrote `theme-cache/` and created `deployment/sass/` even with `--no-update-widgets`. The output now says that widget definitions were normalised on a copy, and that a CE0463 the stored project still has is therefore not reported: `--no-update-widgets` checks the project as stored, `mxcli fix widgets` applies the normalisation (ako/mxcli#568, #646). Build output, caches and VCS folders are not copied; the copy goes to `$TMPDIR` and is removed afterwards. - **Parallel mxcli runs on one project no longer fail to save the catalog cache** (ako/mxcli#951) — eight parallel `lint` runs on a fresh copy printed `failed to create table catalog_meta: table catalog_meta already exists` or `database is locked`. The cache is now written to a temporary file next to it and renamed into place, so every run saves and a reader sees the old cache or the new one, never a half-written file; opening a current cache no longer writes to it. - **A lint rule that fails no longer costs the project score** (ako/mxcli#952) — a Starlark rule reading a struct field this mxcli does not expose (a rule written for a newer mxcli, such as one using `document_noun_title` under v0.24.0) is reported at info level as `rule needs a newer mxcli ()` instead of an error. All rule failures are kept out of `mxcli report`'s score, summary and categories and listed in their own "Rules That Could Not Run" section (`ruleFailures` in JSON); other failures stay `Starlark rule error` errors in `mxcli lint`. A configured rule severity no longer applies to the rule's own failure. A rule file that fails to load on an undefined name says the rule may need a newer mxcli. Measured on PedApp with v0.24.0 and rules from main: QUAL004 and CUSTOM002 crashed and scored as 2 errors. diff --git a/mdl/backend/wfnames/walk.go b/mdl/backend/wfnames/walk.go new file mode 100644 index 0000000000..f4616ae2f1 --- /dev/null +++ b/mdl/backend/wfnames/walk.go @@ -0,0 +1,69 @@ +// SPDX-License-Identifier: Apache-2.0 + +package wfnames + +import "github.com/mendixlabs/mxcli/sdk/workflows" + +// WalkActivities visits every activity of a workflow, depth-first: the main +// flow, every sub-flow an activity carries (outcome flows and boundary-event +// flows, per SubFlows), and every event sub-process. +// +// It is the one traversal every reader of a stored workflow shares — the +// catalog's refs and activity counts, `list workflows` and `show structure`. +// Each used to carry its own recursion over outcomes only, so they disagreed: +// a microflow called from a boundary-event path had no inbound edge +// (mendixlabs/mxcli#1269), and `list workflows` counted 5 activities where the +// catalog counted 8 (ako/mxcli#963). A new place that holds a flow is added to +// SubFlows or here, and every caller sees it. +func WalkActivities(wf *workflows.Workflow, visit func(workflows.WorkflowActivity)) { + if wf == nil { + return + } + walkFlow(wf.Flow, visit) + for _, esp := range wf.EventSubProcesses { + if esp != nil { + walkFlow(esp.Flow, visit) + } + } +} + +func walkFlow(flow *workflows.Flow, visit func(workflows.WorkflowActivity)) { + if flow == nil { + return + } + for _, act := range flow.Activities { + if act == nil { + continue + } + visit(act) + for _, sub := range SubFlows(act) { + walkFlow(sub, visit) + } + } +} + +// ActivityCounts is the per-type tally of a workflow's activities over every +// flow WalkActivities reaches. +type ActivityCounts struct { + Total int + UserTasks int + MicroflowCalls int // call-microflow and system tasks + Decisions int +} + +// CountActivities tallies a workflow's activities with WalkActivities. +func CountActivities(wf *workflows.Workflow) ActivityCounts { + var c ActivityCounts + WalkActivities(wf, func(act workflows.WorkflowActivity) { + c.Total++ + switch act.(type) { + case *workflows.UserTask: + c.UserTasks++ + case *workflows.CallMicroflowTask, *workflows.SystemTask: + c.MicroflowCalls++ + case *workflows.ExclusiveSplitActivity: + c.Decisions++ + } + }) + return c +} diff --git a/mdl/catalog/builder_workflows.go b/mdl/catalog/builder_workflows.go index 0405a6796d..0ff45c8a74 100644 --- a/mdl/catalog/builder_workflows.go +++ b/mdl/catalog/builder_workflows.go @@ -3,6 +3,7 @@ package catalog import ( + "github.com/mendixlabs/mxcli/mdl/backend/wfnames" "github.com/mendixlabs/mxcli/sdk/workflows" ) @@ -71,18 +72,8 @@ func (b *Builder) buildWorkflows() error { } // countWorkflowActivityTypes counts activity types in a workflow, over every -// flow it holds (see walkWorkflowActivities). +// flow it holds (see wfnames.WalkActivities). func countWorkflowActivityTypes(wf *workflows.Workflow) (total, userTasks, microflowCalls, decisions int) { - walkWorkflowActivities(wf, func(act workflows.WorkflowActivity) { - total++ - switch act.(type) { - case *workflows.UserTask: - userTasks++ - case *workflows.CallMicroflowTask, *workflows.SystemTask: - microflowCalls++ - case *workflows.ExclusiveSplitActivity: - decisions++ - } - }) - return + c := wfnames.CountActivities(wf) + return c.Total, c.UserTasks, c.MicroflowCalls, c.Decisions } diff --git a/mdl/catalog/workflow_walk.go b/mdl/catalog/workflow_walk.go index a0fb0f28c7..00ba51824c 100644 --- a/mdl/catalog/workflow_walk.go +++ b/mdl/catalog/workflow_walk.go @@ -7,43 +7,6 @@ import ( "github.com/mendixlabs/mxcli/sdk/workflows" ) -// walkWorkflowActivities visits every activity of a workflow, depth-first: the -// main flow, every sub-flow an activity carries (outcome flows and -// boundary-event flows, per wfnames.SubFlows), and every event sub-process. -// -// It is the one traversal the catalog's workflow passes share. The refs walk and -// the activity counts each had their own recursion over outcomes only, so a -// microflow called from a boundary-event path or an event sub-process had no -// inbound edge and was reported dead by graph_dead_assets (mendixlabs/mxcli#1269, -// workflows row). A new place that holds a flow is added to wfnames.SubFlows or -// here, and both passes see it. -func walkWorkflowActivities(wf *workflows.Workflow, visit func(workflows.WorkflowActivity)) { - if wf == nil { - return - } - walkWorkflowFlow(wf.Flow, visit) - for _, esp := range wf.EventSubProcesses { - if esp != nil { - walkWorkflowFlow(esp.Flow, visit) - } - } -} - -func walkWorkflowFlow(flow *workflows.Flow, visit func(workflows.WorkflowActivity)) { - if flow == nil { - return - } - for _, act := range flow.Activities { - if act == nil { - continue - } - visit(act) - for _, sub := range wfnames.SubFlows(act) { - walkWorkflowFlow(sub, visit) - } - } -} - // workflowDocRef is one reference a workflow makes to another document. type workflowDocRef struct { TargetType, TargetName, RefKind string @@ -75,7 +38,7 @@ func workflowDocRefs(wf *workflows.Workflow) []workflowDocRef { } } - walkWorkflowActivities(wf, func(act workflows.WorkflowActivity) { + wfnames.WalkActivities(wf, func(act workflows.WorkflowActivity) { switch a := act.(type) { case *workflows.UserTask: add(RefObjectPage, a.Page, RefKindShowPage) diff --git a/mdl/executor/cmd_structure.go b/mdl/executor/cmd_structure.go index 4501445d89..dd22d89132 100644 --- a/mdl/executor/cmd_structure.go +++ b/mdl/executor/cmd_structure.go @@ -8,6 +8,7 @@ import ( "strings" "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend/wfnames" "github.com/mendixlabs/mxcli/mdl/catalog" mdlerrors "github.com/mendixlabs/mxcli/mdl/errors" "github.com/mendixlabs/mxcli/mdl/types" @@ -1016,55 +1017,11 @@ func structureWorkflows(ctx *ExecContext, moduleName string, wfs []*workflows.Wo } } -// countStructureWorkflowActivities counts activity types in a workflow for structure output. +// countStructureWorkflowActivities counts activity types in a workflow for +// structure output, over every flow it holds — the catalog's walk (wfnames). func countStructureWorkflowActivities(wf *workflows.Workflow) (total, userTasks, microflowCalls, decisions int) { - if wf.Flow == nil { - return - } - countStructureFlowActivities(wf.Flow, &total, &userTasks, µflowCalls, &decisions) - return -} - -// countStructureFlowActivities recursively counts activity types in a flow. -func countStructureFlowActivities(flow *workflows.Flow, total, userTasks, microflowCalls, decisions *int) { - if flow == nil { - return - } - for _, act := range flow.Activities { - *total++ - switch a := act.(type) { - case *workflows.UserTask: - *userTasks++ - for _, outcome := range a.Outcomes { - countStructureFlowActivities(outcome.Flow, total, userTasks, microflowCalls, decisions) - } - case *workflows.CallMicroflowTask: - *microflowCalls++ - for _, outcome := range a.Outcomes { - if outcome != nil { - countStructureFlowActivities(outcome.GetFlow(), total, userTasks, microflowCalls, decisions) - } - } - case *workflows.SystemTask: - *microflowCalls++ - for _, outcome := range a.Outcomes { - if outcome != nil { - countStructureFlowActivities(outcome.GetFlow(), total, userTasks, microflowCalls, decisions) - } - } - case *workflows.ExclusiveSplitActivity: - *decisions++ - for _, outcome := range a.Outcomes { - if outcome != nil { - countStructureFlowActivities(outcome.GetFlow(), total, userTasks, microflowCalls, decisions) - } - } - case *workflows.ParallelSplitActivity: - for _, outcome := range a.Outcomes { - countStructureFlowActivities(outcome.Flow, total, userTasks, microflowCalls, decisions) - } - } - } + c := wfnames.CountActivities(wf) + return c.Total, c.UserTasks, c.MicroflowCalls, c.Decisions } // ============================================================================ diff --git a/mdl/executor/cmd_workflows.go b/mdl/executor/cmd_workflows.go index 318fed2a0e..6fe941c001 100644 --- a/mdl/executor/cmd_workflows.go +++ b/mdl/executor/cmd_workflows.go @@ -9,6 +9,7 @@ import ( "strings" "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend/wfnames" mdlerrors "github.com/mendixlabs/mxcli/mdl/errors" "github.com/mendixlabs/mxcli/mdl/visitor" "github.com/mendixlabs/mxcli/sdk/workflows" @@ -70,65 +71,11 @@ func listWorkflows(ctx *ExecContext, moduleName string) error { return writeResult(ctx, result) } -// countWorkflowActivities counts total activities, user tasks, and decisions in a workflow. +// countWorkflowActivities counts total activities, user tasks, and decisions in +// a workflow, over every flow it holds — the catalog's walk (wfnames). func countWorkflowActivities(wf *workflows.Workflow) (total, userTasks, decisions int) { - if wf.Flow == nil { - return - } - countFlowActivities(wf.Flow, &total, &userTasks, &decisions) - return -} - -// countFlowActivities recursively counts activities in a flow and its sub-flows. -func countFlowActivities(flow *workflows.Flow, total, userTasks, decisions *int) { - if flow == nil { - return - } - for _, act := range flow.Activities { - *total++ - switch a := act.(type) { - case *workflows.UserTask: - *userTasks++ - for _, outcome := range a.Outcomes { - countFlowActivities(outcome.Flow, total, userTasks, decisions) - } - case *workflows.ExclusiveSplitActivity: - *decisions++ - for _, outcome := range a.Outcomes { - if co, ok := outcome.(*workflows.BooleanConditionOutcome); ok { - countFlowActivities(co.Flow, total, userTasks, decisions) - } else if co, ok := outcome.(*workflows.EnumerationValueConditionOutcome); ok { - countFlowActivities(co.Flow, total, userTasks, decisions) - } else if co, ok := outcome.(*workflows.VoidConditionOutcome); ok { - countFlowActivities(co.Flow, total, userTasks, decisions) - } - } - case *workflows.ParallelSplitActivity: - for _, outcome := range a.Outcomes { - countFlowActivities(outcome.Flow, total, userTasks, decisions) - } - case *workflows.CallMicroflowTask: - for _, outcome := range a.Outcomes { - if co, ok := outcome.(*workflows.BooleanConditionOutcome); ok { - countFlowActivities(co.Flow, total, userTasks, decisions) - } else if co, ok := outcome.(*workflows.EnumerationValueConditionOutcome); ok { - countFlowActivities(co.Flow, total, userTasks, decisions) - } else if co, ok := outcome.(*workflows.VoidConditionOutcome); ok { - countFlowActivities(co.Flow, total, userTasks, decisions) - } - } - case *workflows.SystemTask: - for _, outcome := range a.Outcomes { - if co, ok := outcome.(*workflows.BooleanConditionOutcome); ok { - countFlowActivities(co.Flow, total, userTasks, decisions) - } else if co, ok := outcome.(*workflows.EnumerationValueConditionOutcome); ok { - countFlowActivities(co.Flow, total, userTasks, decisions) - } else if co, ok := outcome.(*workflows.VoidConditionOutcome); ok { - countFlowActivities(co.Flow, total, userTasks, decisions) - } - } - } - } + c := wfnames.CountActivities(wf) + return c.Total, c.UserTasks, c.Decisions } // describeWorkflow handles DESCRIBE WORKFLOW command. diff --git a/mdl/executor/cmd_workflows_count_test.go b/mdl/executor/cmd_workflows_count_test.go new file mode 100644 index 0000000000..beeb7de0cf --- /dev/null +++ b/mdl/executor/cmd_workflows_count_test.go @@ -0,0 +1,99 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/sdk/workflows" +) + +// `list workflows` and `show structure` count a workflow's activities with the +// same walk as the catalog (ako/mxcli#963): activities on a boundary-event path +// and in an event sub-process are activities of the workflow. +func boundaryAndESPWorkflow() *workflows.Workflow { + call := func(mf string) *workflows.CallMicroflowTask { return &workflows.CallMicroflowTask{Microflow: mf} } + return &workflows.Workflow{ + Name: "WF", + Flow: &workflows.Flow{Activities: []workflows.WorkflowActivity{ + &workflows.UserTask{ + BoundaryEvents: []*workflows.BoundaryEvent{{ + Flow: &workflows.Flow{Activities: []workflows.WorkflowActivity{call("M.InBoundary")}}, + }}, + }, + &workflows.ExclusiveSplitActivity{}, + }}, + EventSubProcesses: []*workflows.EventSubProcess{{ + Flow: &workflows.Flow{Activities: []workflows.WorkflowActivity{ + &workflows.EventSubProcessStartActivity{}, + &workflows.UserTask{}, + }}, + }}, + } +} + +// outcomeOnlyWorkflow is the control: no boundary events, no event +// sub-process, so the old outcome-only recursion and the shared walk agree. +func outcomeOnlyWorkflow() *workflows.Workflow { + return &workflows.Workflow{ + Name: "WF", + Flow: &workflows.Flow{Activities: []workflows.WorkflowActivity{ + &workflows.UserTask{Outcomes: []*workflows.UserTaskOutcome{{ + Flow: &workflows.Flow{Activities: []workflows.WorkflowActivity{&workflows.CallMicroflowTask{}}}, + }}}, + &workflows.ExclusiveSplitActivity{}, + }}, + } +} + +func TestCountWorkflowActivities_BoundaryAndEventSubProcess(t *testing.T) { + cases := []struct { + name string + wf *workflows.Workflow + total, ut, calls, decs int + }{ + // main: user task, decision = 2; boundary: call = 1; ESP: start, user task = 2 + {"boundary+esp", boundaryAndESPWorkflow(), 5, 2, 1, 1}, + {"control: outcomes only", outcomeOnlyWorkflow(), 3, 1, 1, 1}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + total, ut, decs := countWorkflowActivities(c.wf) + if total != c.total || ut != c.ut || decs != c.decs { + t.Errorf("list workflows counts = (%d, %d, %d), want (%d, %d, %d)", total, ut, decs, c.total, c.ut, c.decs) + } + stotal, sut, scalls, sdecs := countStructureWorkflowActivities(c.wf) + if stotal != c.total || sut != c.ut || scalls != c.calls || sdecs != c.decs { + t.Errorf("show structure counts = (%d, %d, %d, %d), want (%d, %d, %d, %d)", stotal, sut, scalls, sdecs, c.total, c.ut, c.calls, c.decs) + } + }) + } +} + +// On TestApp, Workflow1 has boundary events: the catalog has always counted 8 +// activities for it, `list workflows` counted 5. Every other workflow is the +// control — it must count the same either way. +func TestCountWorkflowActivities_TestAppWorkflow1(t *testing.T) { + exec, _ := openTestAppCopy(t) + wfs, err := exec.backend.ListWorkflows() + if err != nil { + t.Fatal(err) + } + found := false + for _, wf := range wfs { + total, _, _ := countWorkflowActivities(wf) + stotal, _, _, _ := countStructureWorkflowActivities(wf) + if stotal != total { + t.Errorf("%s: show structure %d != list workflows %d", wf.Name, stotal, total) + } + if wf.Name == "Workflow1" { + found = true + if total != 8 { + t.Errorf("Workflow1 activities = %d, want 8 (the catalog's count)", total) + } + } + } + if !found { + t.Fatal("TestApp has no Workflow1") + } +} From 493bb7ce8bb00d51eddf2d8668809fbc31ca7165 Mon Sep 17 00:00:00 2001 From: Ako Date: Sat, 3 Oct 2026 17:46:03 +0000 Subject: [PATCH 02/15] feat(lint): expose total_activity_count on the Starlark microflow struct (#963) The catalog has carried TotalActivityCount (loop bodies included) since #940; microflows() now hands it to rules for microflows, nanoflows and rules alike. Co-Authored-By: Claude Opus 5.5 --- .../skills/mendix/write-lint-rules/SKILL.md | 3 +- CHANGELOG.md | 1 + mdl/linter/context.go | 11 ++-- mdl/linter/context_document_filter_test.go | 2 +- mdl/linter/context_test.go | 4 +- mdl/linter/rules/empty_test.go | 6 +- mdl/linter/starlark.go | 4 +- mdl/linter/starlark_documented_values_test.go | 8 +-- mdl/linter/starlark_flow_noun_test.go | 14 ++-- .../starlark_total_activity_count_test.go | 66 +++++++++++++++++++ 10 files changed, 97 insertions(+), 22 deletions(-) create mode 100644 mdl/linter/starlark_total_activity_count_test.go diff --git a/.claude/skills/mendix/write-lint-rules/SKILL.md b/.claude/skills/mendix/write-lint-rules/SKILL.md index 3b34fc88ac..9914e5cb6e 100644 --- a/.claude/skills/mendix/write-lint-rules/SKILL.md +++ b/.claude/skills/mendix/write-lint-rules/SKILL.md @@ -205,7 +205,8 @@ def check(): | `description` | string | Documentation text | | `return_type` | string | Return type | | `parameter_count` | int | Number of parameters | -| `activity_count` | int | Number of activities | +| `activity_count` | int | Number of activities at the top level of the flow, excluding start/end events and merges. A loop counts as one; its body is not counted | +| `total_activity_count` | int | `activity_count` plus every activity inside a loop, at any depth — the size of the flow including loop bodies. Equal to `activity_count` for a flow without loops | | `complexity` | int | McCabe cyclomatic complexity | | `document_noun` | string | `"microflow"`, `"nanoflow"` or `"rule"` — for mid-sentence use in a message | | `document_noun_title` | string | `"Microflow"`, `"Nanoflow"` or `"Rule"` — for `document_type=` and a message that opens with it | diff --git a/CHANGELOG.md b/CHANGELOG.md index c9298c0f4b..a2d2244ed2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -133,6 +133,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added +- **`total_activity_count` on the Starlark microflow struct** (ako/mxcli#963) — the catalog's `TotalActivityCount` (loop bodies included, at any depth) for every flow `microflows()` yields: microflows, nanoflows and rules. `activity_count` keeps counting a loop as one activity. - **A project records which mxcli wrote its tooling, and an older binary says so** (ako/mxcli#952) — `mxcli init` and every `init --sync-skills` write `.ai-context/mxcli-tooling.json` (version, build time, date; rewritten only when the version changes). Any command that opens the project with `-p` and a binary **older** than the stamp warns once on stderr, naming both versions and how to update; `init --sync-skills` from an older binary **refuses** instead of rolling the skills, rules and CLAUDE.md back. Releases compare by number, nightlies by tag date, a release against a nightly by build date; dev builds are never reported. Binaries from v0.24.0 and earlier cannot read the stamp, so the regenerated `.claude/bootstrap-mxcli.sh` checks it before choosing a binary: an older `mxcli` on PATH is not linked in (it downloads `MXCLI_TAG` instead), an older `./mxcli` is replaced, and the download lands through a temporary file so a `./mxcli` symlink never has it written through into the PATH binary. - **`init --sync-skills` (alias `--sync`) refreshes the bundled lint rules and the mxcli section of CLAUDE.md / AGENTS.md** (ako/mxcli#952) — it used to refresh only the skills, so a project kept the lint rules and guidance of whichever mxcli first initialised it. Bundled rules are recognised by file name; your own rules beside them are never touched. CLAUDE.md and AGENTS.md are now written between `` / `` markers, and only that section is refreshed — by the sync and by a re-run of `mxcli init` — so project notes outside the markers survive. A file written before the markers is left alone by the sync, with a note; run `mxcli init` once to adopt them. - **`mxcli init` and `mxcli new` create `mdlsource/`** (ako/mxcli#952) — the directory the generated CLAUDE.md says scripts live in, with a README. diff --git a/mdl/linter/context.go b/mdl/linter/context.go index f72d76276f..459a417763 100644 --- a/mdl/linter/context.go +++ b/mdl/linter/context.go @@ -599,8 +599,11 @@ type Microflow struct { Description string ReturnType string ParameterCount int - ActivityCount int - Complexity int // McCabe cyclomatic complexity + ActivityCount int // top level only; a loop counts as one + // TotalActivityCount is ActivityCount plus every activity inside a loop, + // at any depth (mendixlabs/mxcli#1266). + TotalActivityCount int + Complexity int // McCabe cyclomatic complexity } // DocumentNoun is what to call this document in a lint message and in @@ -636,7 +639,7 @@ func (ctx *LintContext) Microflows() iter.Seq[Microflow] { rows, err := ctx.db.Query(fmt.Sprintf(` SELECT mf.Id, mf.Name, mf.QualifiedName, mf.ModuleName, mf.Folder, mf.MicroflowType, mf.Description, mf.ReturnType, - mf.ParameterCount, mf.ActivityCount, mf.Complexity + mf.ParameterCount, mf.ActivityCount, mf.TotalActivityCount, mf.Complexity FROM microflows mf LEFT JOIN modules m ON mf.ModuleName = m.Name WHERE %s AND %s @@ -652,7 +655,7 @@ func (ctx *LintContext) Microflows() iter.Seq[Microflow] { var mf Microflow var desc, retType, folder sql.NullString err := rows.Scan(&mf.ID, &mf.Name, &mf.QualifiedName, &mf.ModuleName, &folder, - &mf.MicroflowType, &desc, &retType, &mf.ParameterCount, &mf.ActivityCount, &mf.Complexity) + &mf.MicroflowType, &desc, &retType, &mf.ParameterCount, &mf.ActivityCount, &mf.TotalActivityCount, &mf.Complexity) if err != nil { ctx.recordQueryError("Microflows (row scan)", err) continue diff --git a/mdl/linter/context_document_filter_test.go b/mdl/linter/context_document_filter_test.go index caa921795a..48543b2247 100644 --- a/mdl/linter/context_document_filter_test.go +++ b/mdl/linter/context_document_filter_test.go @@ -31,7 +31,7 @@ import ( func twoInOneModuleDB(t *testing.T) catalog.CatalogDB { t.Helper() db := setupModuleFilterDB(t) - if _, err := db.Exec(`INSERT INTO microflows VALUES (?, ?, ?, ?, '', 'Microflow', '', '', 0, 0, 0)`, + if _, err := db.Exec(`INSERT INTO microflows VALUES (?, ?, ?, ?, '', 'Microflow', '', '', 0, 0, 0, 0)`, "ModB_mf2", "ModB_Sibling", "ModB.Sibling", "ModB"); err != nil { t.Fatalf("insert sibling microflow: %v", err) } diff --git a/mdl/linter/context_test.go b/mdl/linter/context_test.go index e37ceb1fcd..72ea32cf65 100644 --- a/mdl/linter/context_test.go +++ b/mdl/linter/context_test.go @@ -44,7 +44,7 @@ func setupModuleFilterDB(t *testing.T) catalog.CatalogDB { _, err = db.Exec(`CREATE TABLE microflows ( Id TEXT, Name TEXT, QualifiedName TEXT, ModuleName TEXT, Folder TEXT, MicroflowType TEXT, Description TEXT, ReturnType TEXT, - ParameterCount INTEGER, ActivityCount INTEGER, Complexity INTEGER + ParameterCount INTEGER, ActivityCount INTEGER, TotalActivityCount INTEGER DEFAULT 0, Complexity INTEGER )`) if err != nil { t.Fatalf("create microflows table: %v", err) @@ -63,7 +63,7 @@ func setupModuleFilterDB(t *testing.T) catalog.CatalogDB { mod+"_e", mod+"_Entity", mod+".Entity", mod); err != nil { t.Fatalf("insert entity for %s: %v", mod, err) } - if _, err := db.Exec(`INSERT INTO microflows VALUES (?, ?, ?, ?, '', 'Microflow', '', '', 0, 0, 0)`, + if _, err := db.Exec(`INSERT INTO microflows VALUES (?, ?, ?, ?, '', 'Microflow', '', '', 0, 0, 0, 0)`, mod+"_mf", mod+"_Flow", mod+".Flow", mod); err != nil { t.Fatalf("insert microflow for %s: %v", mod, err) } diff --git a/mdl/linter/rules/empty_test.go b/mdl/linter/rules/empty_test.go index 4b05cb9faa..b19960af61 100644 --- a/mdl/linter/rules/empty_test.go +++ b/mdl/linter/rules/empty_test.go @@ -33,14 +33,16 @@ func setupMicroflowsDB(t *testing.T, rows [][]any) catalog.CatalogDB { _, err = db.Exec(`CREATE TABLE microflows ( Id TEXT, Name TEXT, QualifiedName TEXT, ModuleName TEXT, Folder TEXT, MicroflowType TEXT, Description TEXT, ReturnType TEXT, - ParameterCount INTEGER, ActivityCount INTEGER, Complexity INTEGER + ParameterCount INTEGER, ActivityCount INTEGER, TotalActivityCount INTEGER DEFAULT 0, Complexity INTEGER )`) if err != nil { t.Fatalf("failed to create microflows table: %v", err) } for _, row := range rows { - _, err := db.Exec(`INSERT INTO microflows VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + _, err := db.Exec(`INSERT INTO microflows (Id, Name, QualifiedName, ModuleName, Folder, + MicroflowType, Description, ReturnType, ParameterCount, ActivityCount, Complexity) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, row...) if err != nil { t.Fatalf("failed to insert row: %v", err) diff --git a/mdl/linter/starlark.go b/mdl/linter/starlark.go index 769aa7242e..f8aeefbbb1 100644 --- a/mdl/linter/starlark.go +++ b/mdl/linter/starlark.go @@ -950,7 +950,9 @@ func microflowToStarlark(mf Microflow) starlark.Value { "return_type": starlark.String(mf.ReturnType), "parameter_count": starlark.MakeInt(mf.ParameterCount), "activity_count": starlark.MakeInt(mf.ActivityCount), - "complexity": starlark.MakeInt(mf.Complexity), + // Loop bodies included, at any depth; activity_count counts a loop as one. + "total_activity_count": starlark.MakeInt(mf.TotalActivityCount), + "complexity": starlark.MakeInt(mf.Complexity), // microflows() yields all three flow flavours, so a rule naming the // document in a message or a location must not hardcode "Microflow". // Title case matches the document_type spelling Starlark rules use. diff --git a/mdl/linter/starlark_documented_values_test.go b/mdl/linter/starlark_documented_values_test.go index 36f45864e6..4a3ddd8a7a 100644 --- a/mdl/linter/starlark_documented_values_test.go +++ b/mdl/linter/starlark_documented_values_test.go @@ -118,11 +118,11 @@ func everyKindFixtureDB(t *testing.T) catalog.CatalogDB { `CREATE TABLE microflows ( Id TEXT, Name TEXT, QualifiedName TEXT, ModuleName TEXT, Folder TEXT, MicroflowType TEXT, Description TEXT, ReturnType TEXT, - ParameterCount INTEGER, ActivityCount INTEGER, Complexity INTEGER)`, + ParameterCount INTEGER, ActivityCount INTEGER, TotalActivityCount INTEGER DEFAULT 0, Complexity INTEGER)`, `INSERT INTO microflows VALUES - ('f1','MF','Sales.MF','Sales','','MICROFLOW','','',0,0,1), - ('f2','NF','Sales.NF','Sales','','NANOFLOW','','',0,0,1), - ('f3','RU','Sales.RU','Sales','','RULE','','',0,0,1)`, + ('f1','MF','Sales.MF','Sales','','MICROFLOW','','',0,0,0,1), + ('f2','NF','Sales.NF','Sales','','NANOFLOW','','',0,0,0,1), + ('f3','RU','Sales.RU','Sales','','RULE','','',0,0,0,1)`, } for _, s := range stmts { if _, err := db.Exec(s); err != nil { diff --git a/mdl/linter/starlark_flow_noun_test.go b/mdl/linter/starlark_flow_noun_test.go index 191a244635..53c6287820 100644 --- a/mdl/linter/starlark_flow_noun_test.go +++ b/mdl/linter/starlark_flow_noun_test.go @@ -115,14 +115,14 @@ func flowKindsFixtureDB(t *testing.T) catalog.CatalogDB { `CREATE TABLE microflows ( Id TEXT, Name TEXT, QualifiedName TEXT, ModuleName TEXT, Folder TEXT, MicroflowType TEXT, Description TEXT, ReturnType TEXT, - ParameterCount INTEGER, ActivityCount INTEGER, Complexity INTEGER)`, + ParameterCount INTEGER, ActivityCount INTEGER, TotalActivityCount INTEGER DEFAULT 0, Complexity INTEGER)`, `INSERT INTO microflows VALUES - ('f1','Big_MF','Sales.Big_MF','Sales','','MICROFLOW','','',0,100,50), - ('f2','Big_NF','Sales.Big_NF','Sales','','NANOFLOW','','',0,100,50), - ('f3','Big_RU','Sales.Big_RU','Sales','','RULE','','',0,100,50), - ('f4','ACT_MF','Sales.ACT_MF','Sales','','MICROFLOW','','',0,100,50), - ('f5','ACT_NF','Sales.ACT_NF','Sales','','NANOFLOW','','',0,100,50), - ('f6','ACT_RU','Sales.ACT_RU','Sales','','RULE','','',0,100,50)`, + ('f1','Big_MF','Sales.Big_MF','Sales','','MICROFLOW','','',0,100,100,50), + ('f2','Big_NF','Sales.Big_NF','Sales','','NANOFLOW','','',0,100,100,50), + ('f3','Big_RU','Sales.Big_RU','Sales','','RULE','','',0,100,100,50), + ('f4','ACT_MF','Sales.ACT_MF','Sales','','MICROFLOW','','',0,100,100,50), + ('f5','ACT_NF','Sales.ACT_NF','Sales','','NANOFLOW','','',0,100,100,50), + ('f6','ACT_RU','Sales.ACT_RU','Sales','','RULE','','',0,100,100,50)`, `CREATE TABLE activities ( Id TEXT, Name TEXT, Caption TEXT, ActivityType TEXT, ActionType TEXT, MicroflowId TEXT, MicroflowQualifiedName TEXT, ModuleName TEXT, EntityRef TEXT, diff --git a/mdl/linter/starlark_total_activity_count_test.go b/mdl/linter/starlark_total_activity_count_test.go new file mode 100644 index 0000000000..b067208bf9 --- /dev/null +++ b/mdl/linter/starlark_total_activity_count_test.go @@ -0,0 +1,66 @@ +// SPDX-License-Identifier: Apache-2.0 + +package linter_test + +import ( + "os" + "path/filepath" + "sort" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/catalog" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// total_activity_count is the catalog's TotalActivityCount (loop bodies +// included, mendixlabs/mxcli#1266) on the struct microflows() yields for every +// flow flavour (ako/mxcli#963). A flow with a loop has total > activity_count; +// the control without a loop has them equal. +func TestStarlarkTotalActivityCount(t *testing.T) { + cat, err := catalog.New() + if err != nil { + t.Fatal(err) + } + defer cat.Close() + db := cat.CatalogDB() + if _, err := db.Exec(`INSERT INTO microflows_data (Id, Name, QualifiedName, ModuleName, MicroflowType, ActivityCount, TotalActivityCount) + VALUES ('mf', 'MF_Loop', 'T.MF_Loop', 'T', 'MICROFLOW', 2, 5), + ('nf', 'NF_Loop', 'T.NF_Loop', 'T', 'NANOFLOW', 1, 3), + ('ru', 'RU_Loop', 'T.RU_Loop', 'T', 'RULE', 1, 2), + ('fl', 'MF_Flat', 'T.MF_Flat', 'T', 'MICROFLOW', 4, 4)`); err != nil { + t.Fatalf("fixture: %v", err) + } + + rule := `RULE_ID = "TEST01" +RULE_NAME = "Total" +DESCRIPTION = "probe" +CATEGORY = "quality" +SEVERITY = "info" + +def check(): + return [violation(message = "%s|%d|%d" % (mf.name, mf.activity_count, mf.total_activity_count)) + for mf in microflows()] +` + path := filepath.Join(t.TempDir(), "total.star") + if err := os.WriteFile(path, []byte(rule), 0o644); err != nil { + t.Fatal(err) + } + r, err := linter.LoadStarlarkRule(path) + if err != nil { + t.Fatalf("LoadStarlarkRule: %v", err) + } + ctx := linter.NewLintContextFromDB(db) + var got []string + for _, v := range r.Check(ctx) { + got = append(got, v.Message) + } + if errs := ctx.QueryErrors(); len(errs) > 0 { + t.Fatalf("query errors: %v", errs) + } + sort.Strings(got) + want := []string{"MF_Flat|4|4", "MF_Loop|2|5", "NF_Loop|1|3", "RU_Loop|1|2"} + if strings.Join(got, "\n") != strings.Join(want, "\n") { + t.Errorf("got:\n%s\nwant:\n%s", strings.Join(got, "\n"), strings.Join(want, "\n")) + } +} From ac4e096f09b8f1a01bfc53e6bb9af910fc83f60a Mon Sep 17 00:00:00 2001 From: Ako Date: Sat, 3 Oct 2026 17:47:49 +0000 Subject: [PATCH 03/15] fix(check): report a read of a void action call's output name (MDL093, CE0109) A call to a void Java/JavaScript action keeps its output name and declares nothing (#953), so reading the name is CE0109 "Undefined variable" in mxbuild 11.13.0. check now reports it for microflows and nanoflows when the script or the project says the action is void. The void resolver returns whether it knows the action, so an unresolvable call is never reported. Part of #962 (item 1). Co-Authored-By: Claude Opus 5.5 --- .../fix-issue/findings/mdl-executor.jsonl | 1 + .../write-microflows/reference/pitfalls.md | 3 +- CHANGELOG.md | 1 + cmd/mxcli/check_void_calls_test.go | 57 ++++++++ mdl/executor/validate_microflow.go | 3 + mdl/executor/validate_nanoflow.go | 1 + mdl/executor/validate_void_call_output.go | 122 ++++++++++++++++++ .../validate_void_call_output_test.go | 103 +++++++++++++++ mdl/executor/validate_void_code_calls.go | 82 +++++++++--- 9 files changed, 353 insertions(+), 20 deletions(-) create mode 100644 mdl/executor/validate_void_call_output.go create mode 100644 mdl/executor/validate_void_call_output_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index 4dc21c1e0a..249d772378 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -850,3 +850,4 @@ {"area": "mdl/executor", "date": "2026-10-03", "symptom": "ako/mxcli#950 item 1: a page button described as `Action: delete close page` executes to a Forms$DeleteClientAction with ClosePage=false — a describe → exec round trip silently stops the button closing its page; check, exec and mx check are all clean", "cause": "buildClientActionV3Base's `delete` case did not copy action.ClosePage, although the visitor sets it and the writer (clientActionToGen) writes it; save/cancel copied it", "file": "`mdl/executor/cmd_pages_builder_v3.go` (buildClientActionV3Base, case \"delete\")", "insight": "The audit of the other cases found no other dropped visitor flag, but two hard-coded ones describe cannot express: complete task always writes ClosePage/Commit true and show page/create object write NumberOfPagesToClose2 \"\" — every Studio Pro instance in TestApp/PedApp has those values, so they are latent, not live. A text round trip could not have caught this: describe printed the flag correctly and the second describe of the exec'd page printed `delete`, which looks like a user edit — read the stored ClosePage", "refs": ["ako/mxcli#950"]} {"area": "mdl/executor", "date": "2026-10-03", "symptom": "ako/mxcli#950 item 2: describe of a flow ending `return [%CurrentUser%];` prints `return $[%CurrentUser%];`, which does not parse; `return if … then … else …` likewise became `return $if …` (two TestApp WorkflowCommons microflows)", "cause": "formatActivity's EndEvent branch added `$` to any return value without one of + ' \" ( ) — a character blacklist standing in for 'is a bare variable name'", "file": "`mdl/executor/cmd_microflows_format_action.go` (isBareReturnVariable)", "insight": "Restore a stripped sigil only for the positive shape it was stripped from (a bare name, optionally /path); a blacklist of characters lets every new expression form through. The other `$`-adding sites in describe prefix variable-NAME fields, not expressions, and are safe", "refs": ["ako/mxcli#950"]} {"area": "mdl/executor", "date": "2026-10-03", "symptom": "ako/mxcli#950 item 3: describe of a navigation list prints item actions as `show_page 'Mod.Page'` (does not parse — TestApp Rules.Entity_Menu, 3 syntax errors); with that fixed, exec refuses the description with `item inside navigationlist requires a name` because Studio Pro leaves items unnamed", "cause": "extractNavigationListItemAction had a private copy of the page-action rendering in the legacy form instead of the shared renderClientActionMDL; buildNavigationListItemV3 required a name Studio Pro never stores; and once exec accepted it, the writer wrote `Name: \"\"` and no ConditionalVisibilitySettings where Studio Pro stores no Name key and a null slot (6 of 6 items in TestApp), so GetPut still rewrote the snippet", "file": "`mdl/executor/cmd_pages_describe_parse.go` (extractNavigationListItemAction), `mdl/executor/cmd_pages_builder_v3_widgets.go` (buildNavigationListItemV3), `mdl/executor/cmd_pages_describe_output.go` (item header), `mdl/backend/modelsdk/widget_write.go` (navListItemToGen, Forms$NavigationListItem NullFields)", "insight": "Fixing the reported parse error only exposed the next law: the issue said the empty item name 'parses fine', which was true and irrelevant — exec refused it, and after that the writer rewrote it. An unnamed item with no Name key passes mx check at 11.14.0, contrary to the old ledger note that the key is mandatory (that applies to a NAMED item's key, not its absence). Run the whole describe → check → exec → describe chain on the Studio Pro-authored document before declaring a round-trip bug fixed", "refs": ["ako/mxcli#950"]} +{"area": "mdl/executor", "date": "2026-10-03", "symptom": "ako/mxcli#962 item 1: `$V = call java action M.VoidAction(...)` then `log ... + $V` (or `$V` as a JS call argument) passes `check` and `exec`, then mxbuild 11.13.0 fails CE0109 \"Undefined variable 'V'\"", "cause": "#953 taught MDL063 that a void call's output name declares nothing, but no rule read the other half: the name cannot be READ either. The resolver only answered void/not-void, so 'unknown' and 'non-void' were the same answer", "file": "`mdl/executor/validate_void_call_output.go` (checkVoidCallOutputUse, MDL093); `mdl/executor/validate_void_code_calls.go` (resolve -> voidness{void, known})", "fix": "MDL093: collect output names of calls KNOWN to be void, drop any name another statement defines flow-wide (declare, parameter, non-void producer), report each remaining name the flow reads (loopRefVars over every nested body). The resolver now returns known-ness, so 'possibly void' (the editor's policy) never produces MDL093", "insight": "A finding that says 'X declares nothing' has two consequences — no collision AND no definition; fixing the first and logging the second as follow-up left a CE gap. When a resolver's default is a policy (unknown counts as non-void), make the unknown state explicit before a second rule reads it: the CE0109 rule must use only knowledge, never the policy", "test": "`mdl/executor/validate_void_call_output_test.go`; `cmd/mxcli/check_void_calls_test.go` (TestCheck_ReadOfAStoredVoidCallOutput, PedApp stored void JS action, Boolean and unresolvable controls)"} diff --git a/.claude/skills/mendix/write-microflows/reference/pitfalls.md b/.claude/skills/mendix/write-microflows/reference/pitfalls.md index b4d13fe39a..080d00b4bc 100644 --- a/.claude/skills/mendix/write-microflows/reference/pitfalls.md +++ b/.claude/skills/mendix/write-microflows/reference/pitfalls.md @@ -238,7 +238,8 @@ either, whatever output name it carries — Studio Pro keeps one on such calls (a JavaScript action's is named after the action, e.g. `$RefreshEntity`), and two of them in one flow build clean. `describe` keeps printing the stored name so a round trip does not change the model. The name is not a variable: using -`$RefreshEntity` afterwards is CE0109 "Undefined variable". +`$RefreshEntity` afterwards is CE0109 "Undefined variable" (MDL093 — `check` +reports it when the script or, with `-p`, the project says the action is void). ### 10. Calling a Rule or Microflow Inside an Expression diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ac2b20156..0efb498742 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **`check` reports a read of a void action call's output name** (ako/mxcli#962) — `$V = call java action M.Void(…)` followed by anything reading `$V` passed `check` and failed the build with CE0109 "Undefined variable 'V'" (measured on mxbuild 11.13.0). It is now MDL093, for microflows and nanoflows, when the script or, with `-p`, the project says the action returns Void. A name something else defines (a declare, a parameter, a non-void producer) and an action nobody can resolve are not reported. - **`docker check` no longer modifies the project** (ako/mxcli#951) — `mx update-widgets` and `mx check` now run on a temporary copy, for MPR v1 and v2 alike, and what mx prints names the project's own paths. Before, a check rewrote an MPR v1 project's `.mpr` permanently (only v2 was restored from a snapshot), and `mx check` itself rewrote `theme-cache/` and created `deployment/sass/` even with `--no-update-widgets`. The output now says that widget definitions were normalised on a copy, and that a CE0463 the stored project still has is therefore not reported: `--no-update-widgets` checks the project as stored, `mxcli fix widgets` applies the normalisation (ako/mxcli#568, #646). Build output, caches and VCS folders are not copied; the copy goes to `$TMPDIR` and is removed afterwards. - **Parallel mxcli runs on one project no longer fail to save the catalog cache** (ako/mxcli#951) — eight parallel `lint` runs on a fresh copy printed `failed to create table catalog_meta: table catalog_meta already exists` or `database is locked`. The cache is now written to a temporary file next to it and renamed into place, so every run saves and a reader sees the old cache or the new one, never a half-written file; opening a current cache no longer writes to it. - **A lint rule that fails no longer costs the project score** (ako/mxcli#952) — a Starlark rule reading a struct field this mxcli does not expose (a rule written for a newer mxcli, such as one using `document_noun_title` under v0.24.0) is reported at info level as `rule needs a newer mxcli ()` instead of an error. All rule failures are kept out of `mxcli report`'s score, summary and categories and listed in their own "Rules That Could Not Run" section (`ruleFailures` in JSON); other failures stay `Starlark rule error` errors in `mxcli lint`. A configured rule severity no longer applies to the rule's own failure. A rule file that fails to load on an undefined name says the rule may need a newer mxcli. Measured on PedApp with v0.24.0 and rules from main: QUAL004 and CUSTOM002 crashed and scored as 2 errors. diff --git a/cmd/mxcli/check_void_calls_test.go b/cmd/mxcli/check_void_calls_test.go index 59d3a1e9c5..776204105c 100644 --- a/cmd/mxcli/check_void_calls_test.go +++ b/cmd/mxcli/check_void_calls_test.go @@ -83,3 +83,60 @@ end; t.Errorf("the unknown parameter (CE1613) is hidden behind the semantic error:\n%s", out) } } + +// ako/mxcli#962 item 1, end to end on PedApp: reading the output name of a +// call to a stored VOID JavaScript action is CE0109 in mxbuild 11.13.0 +// (measured on a PedApp copy), and `check -p` resolves the action through the +// project to say so (MDL093). Controls: the same read of a Boolean action's +// output, and of an action the project does not have. +func TestCheck_ReadOfAStoredVoidCallOutput(t *testing.T) { + src := filepath.Join("..", "..", "testdata", "pedapp") + if _, err := os.Stat(filepath.Join(src, "PedApp.mpr")); err != nil { + t.Skipf("PedApp fixture not found: %v", err) + } + dir := t.TempDir() + if err := copyTree(src, dir); err != nil { + t.Fatal(err) + } + _ = rootCmd.PersistentFlags().Set("project", filepath.Join(dir, "PedApp.mpr")) + defer func() { + _ = rootCmd.PersistentFlags().Set("project", "") + rootCmd.PersistentFlags().Lookup("project").Changed = false + }() + check := func(script string) (int, string) { + file := writeScript(t, t.TempDir(), "s.mdl", "mdl 1;\n"+script) + var code int + out := captureStd(t, func() { code = runCheckFiles(checkCmd, []string{file}) }) + return code, out + } + + code, out := check(`create nanoflow MyFirstModule.NF_ReadVoid () +begin + $V = call javascript action FeedbackModule.JS_RevokeUploadedFileFromMemory(fileBlobURL = 'a'); + $S = call javascript action FeedbackModule.JS_RevokeUploadedFileFromMemory(fileBlobURL = $V); +end; +`) + if code == 0 || !strings.Contains(out, "MDL093") || !strings.Contains(out, "CE0109") { + t.Errorf("reading a void call's output not reported (exit %d):\n%s", code, out) + } + + code, out = check(`create nanoflow MyFirstModule.NF_ReadBoolean () +begin + $IsStrict = call javascript action FeedbackModule.JS_isStrictMode(); + $S = call javascript action FeedbackModule.JS_RevokeUploadedFileFromMemory(fileBlobURL = toString($IsStrict)); +end; +`) + if code != 0 || strings.Contains(out, "MDL093") { + t.Errorf("control: reading a Boolean action's output reported (exit %d):\n%s", code, out) + } + + _, out = check(`create nanoflow MyFirstModule.NF_ReadUnknown () +begin + $U = call javascript action FeedbackModule.JS_NotThere(); + $S = call javascript action FeedbackModule.JS_RevokeUploadedFileFromMemory(fileBlobURL = $U); +end; +`) + if strings.Contains(out, "MDL093") { + t.Errorf("control: an unresolvable action's output reported as void:\n%s", out) + } +} diff --git a/mdl/executor/validate_microflow.go b/mdl/executor/validate_microflow.go index 3e670d59cc..b8c1d24932 100644 --- a/mdl/executor/validate_microflow.go +++ b/mdl/executor/validate_microflow.go @@ -162,6 +162,9 @@ func (v *microflowValidator) validate(body []ast.MicroflowStatement) { // by the build. See validate_microflow_ce_gaps.go for the measurements. v.checkReturnInLoop(body) v.checkDuplicateVariableNames(v.params, body) + // ako/mxcli#962: the other half of #953's void-call finding — the name is + // inert both ways, so reading it is CE0109. See validate_void_call_output.go. + v.checkVoidCallOutputUse(v.params, body) // mendixlabs/mxcli#1030: an error event is legal only on an error-handling // flow. See validate_microflow_raise_error.go. diff --git a/mdl/executor/validate_nanoflow.go b/mdl/executor/validate_nanoflow.go index 73f62f2e36..bace87a205 100644 --- a/mdl/executor/validate_nanoflow.go +++ b/mdl/executor/validate_nanoflow.go @@ -48,6 +48,7 @@ func validateNanoflowWith(stmt *ast.CreateNanoflowStmt, voids *voidCodeActions) // contains/find rewrite come from the parameters and the declares. v.seedPrimitiveKinds(stmt.Parameters, stmt.Body) v.checkDuplicateVariableNames(v.params, stmt.Body) + v.checkVoidCallOutputUse(v.params, stmt.Body) // The nanoflow restrictions exec's build refuses (validateNanoflow): an // action a nanoflow cannot hold, an error-handling clause its activity // rejects (CE6035), a Binary return. They ran only inside exec, so `check` diff --git a/mdl/executor/validate_void_call_output.go b/mdl/executor/validate_void_call_output.go new file mode 100644 index 0000000000..46cf66ace0 --- /dev/null +++ b/mdl/executor/validate_void_call_output.go @@ -0,0 +1,122 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// voidCallOutputRule reports a read of the output name of a void action call. +const voidCallOutputRule = "MDL093" + +// checkVoidCallOutputUse flags a variable that only a call to a VOID Java or +// JavaScript action names, and that the flow then reads — MDL093. +// +// Such a call keeps its output name in the model but declares nothing +// (ako/mxcli#953), so reading the name afterwards reads a variable that does not +// exist. Measured on mxbuild 11.13.0 (ako/mxcli#962), PedApp copy: +// +// void Java call `$V1 = …`, then `log … + $V1` CE0109 "Undefined variable 'V1'" +// void JS call `$V3 = …`, then `$V3` as a call argument CE0109 "Undefined variable 'V3'" +// non-void Java call `$V2 = …`, then `log … + $V2` 0 errors (control) +// void Java call `$V4 = …`, `declare $V4 …`, then `$V4` 0 errors +// +// So a name some other statement defines — a declare, a parameter, any +// non-void producer, anywhere in the flow — is not reported: variable names are +// flow-wide, and the definition is what the read resolves to. Only a call the +// resolver KNOWS is void counts; an unresolvable action is never reported +// here, whatever the duplicate-name policy does with it. +func (v *microflowValidator) checkVoidCallOutputUse(params []ast.MicroflowParam, body []ast.MicroflowStatement) { + if v.skipCEGapRules() || v.voids == nil { + return + } + voidNames := map[string]string{} // output name -> the action that "names" it + defined := map[string]bool{} + for _, p := range params { + defined[p.Name] = true + } + walkFlowStatements(body, func(s ast.MicroflowStatement) { + if v.voids.callIsKnownVoid(s) { + if name := outputVariableField(s); name != "" { + if _, seen := voidNames[name]; !seen { + voidNames[name] = stmtActionName(s) + } + } + return + } + if !v.buildsAsAProducer(s) { + return + } + for _, p := range statementProducedVars(s) { + defined[p.name] = true + } + }) + for name := range defined { + delete(voidNames, name) + } + if len(voidNames) == 0 { + return + } + + reported := map[string]bool{} + walkFlowStatements(body, func(s ast.MicroflowStatement) { + for _, ref := range loopRefVars(s) { + action, ok := voidNames[ref] + if !ok || reported[ref] { + continue + } + reported[ref] = true + v.addViolation(voidCallOutputRule, linter.SeverityError, + fmt.Sprintf("'$%s' is read, but the only statement naming it is a call to %s, which "+ + "returns nothing — a void action's output name declares no variable, so mxbuild "+ + "rejects this with CE0109 \"Undefined variable '%s'\"", ref, action, ref), + fmt.Sprintf("Drop the read of '$%s', or call an action that returns a value — the "+ + "output name on a void call is inert", ref)) + } + }) +} + +// stmtActionName names the action a Java/JavaScript call statement targets. +func stmtActionName(s ast.MicroflowStatement) string { + switch st := s.(type) { + case *ast.CallJavaActionStmt: + return "java action " + st.ActionName.String() + case *ast.CallJavaScriptActionStmt: + return "javascript action " + st.ActionName.String() + } + return statementProducerLabel(s) +} + +// walkFlowStatements calls fn for every statement of a flow body: nested +// branches, loop bodies and custom error-handler bodies included, since they +// all share the flow's one variable namespace. +func walkFlowStatements(body []ast.MicroflowStatement, fn func(ast.MicroflowStatement)) { + for _, s := range body { + fn(s) + switch st := s.(type) { + case *ast.LoopStmt: + walkFlowStatements(st.Body, fn) + case *ast.WhileStmt: + walkFlowStatements(st.Body, fn) + case *ast.IfStmt: + walkFlowStatements(st.ThenBody, fn) + walkFlowStatements(st.ElseBody, fn) + case *ast.EnumSplitStmt: + for _, c := range st.Cases { + walkFlowStatements(c.Body, fn) + } + walkFlowStatements(st.ElseBody, fn) + case *ast.InheritanceSplitStmt: + for _, c := range st.Cases { + walkFlowStatements(c.Body, fn) + } + walkFlowStatements(st.ElseBody, fn) + } + if eh := stmtErrorHandling(s); eh != nil { + walkFlowStatements(eh.Body, fn) + } + } +} diff --git a/mdl/executor/validate_void_call_output_test.go b/mdl/executor/validate_void_call_output_test.go new file mode 100644 index 0000000000..cc4b20fe1c --- /dev/null +++ b/mdl/executor/validate_void_call_output_test.go @@ -0,0 +1,103 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +func mdl093(vs []linter.Violation) []linter.Violation { + var out []linter.Violation + for _, v := range vs { + if v.RuleID == voidCallOutputRule { + out = append(out, v) + } + } + return out +} + +func logOf(name string) *ast.LogStmt { + return &ast.LogStmt{Level: ast.LogInfo, Message: &ast.BinaryExpr{ + Left: &ast.LiteralExpr{Kind: ast.LiteralString, Value: "x"}, Operator: "+", + Right: &ast.VariableExpr{Name: name}}} +} + +// ako/mxcli#962 item 1: a void call's output name declares nothing, so reading +// it is CE0109 "Undefined variable" (mxbuild 11.13.0, measured on a PedApp copy +// — see validate_void_call_output.go). Every "want 0" row is a control. +func TestMDL093_ReadOfAVoidCallOutput(t *testing.T) { + str := ast.DataType{Kind: ast.TypeString} + lit := &ast.LiteralExpr{Kind: ast.LiteralString, Value: "b"} + cond := &ast.VariableExpr{Name: "C"} + jsArg := func(out, action, arg string) *ast.CallJavaScriptActionStmt { + c := jsCall(out, action) + c.Arguments = []ast.CallArgument{{Name: "p", Value: &ast.VariableExpr{Name: arg}}} + return c + } + cases := []struct { + name string + flow ast.Statement + want int + }{ + {"microflow: void java call, then a log reads it", &ast.CreateMicroflowStmt{Name: qn("M", "Mf"), + Body: []ast.MicroflowStatement{javaCall("V", "JaVoid"), logOf("V")}}, 1}, + {"control: non-void java call, then a log reads it", &ast.CreateMicroflowStmt{Name: qn("M", "Mf"), + Body: []ast.MicroflowStatement{javaCall("V", "JaBool"), logOf("V")}}, 0}, + {"control: void call, then a declare of the name, then a read", &ast.CreateMicroflowStmt{Name: qn("M", "Mf"), + Body: []ast.MicroflowStatement{javaCall("V", "JaVoid"), + &ast.DeclareStmt{Variable: "V", Type: str, InitialValue: lit}, logOf("V")}}, 0}, + {"control: a parameter of the same name", &ast.CreateMicroflowStmt{Name: qn("M", "Mf"), + Parameters: []ast.MicroflowParam{{Name: "V", Type: str}}, + Body: []ast.MicroflowStatement{javaCall("V", "JaVoid"), logOf("V")}}, 0}, + {"control: an unresolvable action is never reported", &ast.CreateMicroflowStmt{Name: qn("M", "Mf"), + Body: []ast.MicroflowStatement{javaCall("V", "Elsewhere"), logOf("V")}}, 0}, + {"control: void call whose name is never read", &ast.CreateMicroflowStmt{Name: qn("M", "Mf"), + Body: []ast.MicroflowStatement{javaCall("V", "JaVoid"), logOf("Other")}}, 0}, + {"microflow: the read sits in a branch", &ast.CreateMicroflowStmt{Name: qn("M", "Mf"), + Parameters: []ast.MicroflowParam{{Name: "C", Type: ast.DataType{Kind: ast.TypeBoolean}}}, + Body: []ast.MicroflowStatement{javaCall("V", "JaVoid"), + &ast.IfStmt{Condition: cond, ThenBody: []ast.MicroflowStatement{logOf("V")}}}}, 1}, + {"nanoflow: void JS call, then its name as an argument", &ast.CreateNanoflowStmt{Name: qn("M", "Nf"), + Body: []ast.MicroflowStatement{jsCall("V", "JsVoid"), jsArg("S", "JsVoid", "V")}}, 1}, + {"nanoflow control: non-void JS call, then its name as an argument", &ast.CreateNanoflowStmt{Name: qn("M", "Nf"), + Body: []ast.MicroflowStatement{jsCall("V", "JsBool"), jsArg("S", "JsVoid", "V")}}, 0}, + {"one report per name, however often it is read", &ast.CreateMicroflowStmt{Name: qn("M", "Mf"), + Body: []ast.MicroflowStatement{javaCall("V", "JaVoid"), logOf("V"), logOf("V")}}, 1}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := mdl093(ValidateProgram(programWith(tc.flow), "")) + if len(got) != tc.want { + t.Fatalf("MDL093 count = %d, want %d: %v", len(got), tc.want, got) + } + if tc.want > 0 && !strings.Contains(got[0].Message, "CE0109") { + t.Errorf("message does not name the build error: %s", got[0].Message) + } + }) + } +} + +// The editor's "possibly void" policy (unknownIsVoid) spares an unresolvable +// call from MDL063, but must not turn a read of its output into MDL093: only a +// call KNOWN to be void declares nothing. +func TestMDL093_PossiblyVoidIsNotKnownVoid(t *testing.T) { + mf := &ast.CreateMicroflowStmt{Name: qn("M", "Mf"), + Body: []ast.MicroflowStatement{javaCall("V", "Elsewhere"), javaCall("V", "Elsewhere"), logOf("V")}} + voids := newVoidCodeActions(nil, nil) + voids.unknownIsVoid = true + vs := validateMicroflowWith(mf, voids) + if got := mdl093(vs); len(got) != 0 { + t.Errorf("MDL093 on a call that is only possibly void: %v", got) + } + if got := mdl063(vs); len(got) != 0 { + t.Errorf("MDL063 under unknownIsVoid on an unresolvable pair: %v", got) + } + // Control: the default policy still counts the pair. + if got := mdl063(validateMicroflowWith(mf, newVoidCodeActions(nil, nil))); len(got) != 1 { + t.Errorf("control: MDL063 = %v, want the unresolvable pair reported", got) + } +} diff --git a/mdl/executor/validate_void_code_calls.go b/mdl/executor/validate_void_code_calls.go index 8214d01c27..62e09f4c16 100644 --- a/mdl/executor/validate_void_code_calls.go +++ b/mdl/executor/validate_void_code_calls.go @@ -25,8 +25,15 @@ import ( // The name is inert: it neither collides with a later variable nor defines one. // // An action the resolver cannot find (no project, a runtime-provided System -// action) is NOT treated as void: the author wrote `$X =`, which normally asks -// for a value, and guessing void would silence a real CE0111. +// action) is NOT treated as void by `check`: the author wrote `$X =`, which +// normally asks for a value, and guessing void would silence a real CE0111. +// The editor makes the opposite trade (unknownIsVoid, ako/mxcli#962): a +// squiggle on a call it merely cannot resolve is a false refusal while typing, +// and `check` still runs before anything is written. +// +// 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. type voidCodeActions struct { // script holds the actions the script itself creates, keyed by // codeActionKey, valued true when the action returns Void. @@ -35,7 +42,16 @@ type voidCodeActions struct { open func() backend.FullBackend opened bool b backend.FullBackend - cache map[string]bool + cache map[string]voidness + // unknownIsVoid makes callIsVoid answer true for an action it cannot + // resolve. Set by the editor (NewFlowRules); `check` leaves it false. + unknownIsVoid bool +} + +// voidness is what the resolver knows about one action. +type voidness struct { + void bool // the action returns Void + known bool // the action was found, so void is an answer and not a default } func codeActionKey(javaScript bool, qn string) string { @@ -48,7 +64,7 @@ func codeActionKey(javaScript bool, qn string) string { // newVoidCodeActions collects the script's own action declarations; open, // which may be nil, supplies the project for the rest. func newVoidCodeActions(prog *ast.Program, open func() backend.FullBackend) *voidCodeActions { - r := &voidCodeActions{script: map[string]bool{}, open: open, cache: map[string]bool{}} + r := &voidCodeActions{script: map[string]bool{}, open: open, cache: map[string]voidness{}} if prog == nil { return r } @@ -75,53 +91,81 @@ func (r *voidCodeActions) project() backend.FullBackend { // isVoid reports whether the named action is known to return Void. func (r *voidCodeActions) isVoid(javaScript bool, qn string) bool { + return r.resolve(javaScript, qn).void +} + +// resolve looks the named action up: in the script first, then in the project. +func (r *voidCodeActions) resolve(javaScript bool, qn string) voidness { if r == nil || qn == "" { - return false + return voidness{} } key := codeActionKey(javaScript, qn) if v, ok := r.script[key]; ok { - return v + return voidness{void: v, known: true} } if v, ok := r.cache[key]; ok { return v } - void := false + var got voidness if b := r.project(); b != nil { if javaScript { if a, err := b.ReadJavaScriptActionByName(qn); err == nil && a != nil && a.ReturnType != nil { - void = a.ReturnType.TypeString() == "Void" + 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 { - void = a.ReturnType == nil || a.ReturnType.TypeString() == "Void" + got = voidness{void: a.ReturnType == nil || a.ReturnType.TypeString() == "Void", known: true} } } } - r.cache[key] = void - return void + r.cache[key] = got + return got } -// callIsVoid reports whether a statement is a call to a void Java or -// JavaScript action — one whose output name declares nothing. -func (r *voidCodeActions) callIsVoid(s ast.MicroflowStatement) bool { +// treatAsVoid applies the unknown-action policy to a resolution. +func (r *voidCodeActions) treatAsVoid(v voidness) bool { + if v.known { + return v.void + } + return r != nil && r.unknownIsVoid +} + +// callResolution resolves the action a statement calls; ok is false for a +// statement that is not a Java or JavaScript action call. +func (r *voidCodeActions) callResolution(s ast.MicroflowStatement) (v voidness, ok bool) { switch st := s.(type) { case *ast.CallJavaActionStmt: - return r.isVoid(false, st.ActionName.String()) + return r.resolve(false, st.ActionName.String()), true case *ast.CallJavaScriptActionStmt: - return r.isVoid(true, st.ActionName.String()) + return r.resolve(true, st.ActionName.String()), true } - return false + return voidness{}, false +} + +// callIsVoid reports whether a statement is a call to a void Java or +// JavaScript action — one whose output name declares nothing. An unresolvable +// action counts as void only under unknownIsVoid. +func (r *voidCodeActions) callIsVoid(s ast.MicroflowStatement) bool { + v, ok := r.callResolution(s) + return ok && r.treatAsVoid(v) +} + +// callIsKnownVoid is callIsVoid without the unknown-action policy: true only +// for a call to an action the script or the project says returns Void. +func (r *voidCodeActions) callIsKnownVoid(s ast.MicroflowStatement) bool { + v, ok := r.callResolution(s) + return ok && v.known && v.void } // actionIsVoidCall is callIsVoid for a stored action, used by describe. func (r *voidCodeActions) actionIsVoidCall(action any) bool { switch a := action.(type) { case *microflows.JavaActionCallAction: - return r.isVoid(false, a.JavaAction) + return r.treatAsVoid(r.resolve(false, a.JavaAction)) case *microflows.JavaScriptActionCallAction: - return r.isVoid(true, a.JavaScriptAction) + return r.treatAsVoid(r.resolve(true, a.JavaScriptAction)) } return false } From d7bbb31ef45548f1050e92241608ca173727d828 Mon Sep 17 00:00:00 2001 From: Ako Date: Sat, 3 Oct 2026 17:50:12 +0000 Subject: [PATCH 04/15] feat(catalog): add a commit ref kind for commit actions and committing create/change (#963) commit $Order on a loop variable wrote no refs row because refs had no commit kind. A commit action, or a create/change with commit Yes/YesWithoutEvents, now emits FLOW -> ENTITY 'commit', resolved through the same intra-flow variable map as change/delete. It stays out of the analysis graph and the caller kinds. The ref_kind skill test now reads every RefKind constant, so a new kind cannot ship undocumented. Catalog schema version 19. Co-Authored-By: Claude Opus 5.5 --- .../skills/fix-issue/findings/mdl-other.jsonl | 1 + .../skills/mendix/write-lint-rules/SKILL.md | 2 +- CHANGELOG.md | 1 + docs-site/src/internals/catalog-schema.md | 9 ++ mdl/catalog/builder_commit_refs_test.go | 117 ++++++++++++++++++ mdl/catalog/builder_graph.go | 5 + mdl/catalog/builder_references.go | 34 +++++ mdl/catalog/lint_rule_doc_vocabulary_test.go | 33 ++--- mdl/catalog/lint_rule_vocabulary_test.go | 2 +- mdl/catalog/tables.go | 6 +- mdl/executor/cmd_search.go | 3 +- 11 files changed, 195 insertions(+), 18 deletions(-) create mode 100644 mdl/catalog/builder_commit_refs_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-other.jsonl b/.claude/skills/fix-issue/findings/mdl-other.jsonl index ef6edcd7da..2814f3faac 100644 --- a/.claude/skills/fix-issue/findings/mdl-other.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-other.jsonl @@ -94,3 +94,4 @@ {"area": "mdl/linter", "date": "2026-10-03", "symptom": "mxcli report scores a project lower for lint rules that crash: under v0.24.0, project rules written by a newer mxcli (QUAL004, CUSTOM002 reading .document_noun_title) each produced an error-severity 'Starlark rule error: \"microflow\" struct has no .document_noun_title attribute' that counted 10 points against the project", "cause": "StarlarkRule.Check turned every evaluation error into an ordinary SeverityError violation, indistinguishable from a finding; BuildReport and Summarize counted it, and a configured rule severity was applied to it too", "fix": "ruleFailureViolation marks every failure Violation.RuleFailure; a missing struct attribute (matched on the evaluator message, since starlark flattens NoSuchAttrError via fmt.Errorf) becomes info 'rule needs a newer mxcli ()'; BuildReport splits RuleFailures out before counting and every report format lists them separately; Linter.Run skips the severity override for them; an 'undefined:' load failure gets a newer-mxcli hint", "insight": "The score measures the project, so anything about the tooling has to be partitioned out BEFORE counting, not filtered in the formatter. The control that makes the score assertion meaningful is a working rule's finding that does move the score", "issue": "ako/mxcli#952", "file": "mdl/linter/starlark.go (ruleFailureViolation); mdl/linter/report.go (BuildReport); mdl/linter/linter.go (Run); mdl/linter/report_format.go", "test": "mdl/linter/starlark_rule_failure_test.go"} {"area": "mdl/linter", "date": "2026-10-03", "symptom": "MPR012 (legacy static/dynamic image, CE0582) fires on pages with a native layout (Atlas_Core.NativePhone_Default), where mxbuild 11.13 builds them clean; `check --references` likewise refuses a classic `dropdown` on a native page as MDL-WIDGET40 (CE0582), also clean in mxbuild", "cause": "Both rules assume every page is rendered by the React client. Nothing recorded a layout's platform: ListLayouts left pages.Layout.Native false for every layout, and catalog layouts had only LayoutType, which cannot tell the platforms apart (native uses Default/Popup)", "file": "`mdl/backend/modelsdk/page.go` (`layoutIsNative`), `mdl/catalog/builder_pages.go` (layouts.Platform), `mdl/linter/context_catalog_tables.go` (`NativePages`), `mdl/linter/rules/legacy_image_widget.go`, `mdl/executor/validate_widget_attribute_type.go` (`layoutIsNative`)", "insight": "The platform is the content wrapper's TYPE (Forms$NativeLayoutContent), not a property. The native layouts live in Atlas_Core, a Marketplace module, so the page->layout join must not apply the notPlatformModule filter the iterators use. Sibling check MDL-WIDGET39 (CE2421, textbox on an enumeration) is NOT React-only: measured CE2421 on the native page too, so it keeps firing there", "refs": ["ako/mxcli#953"]} {"area": "mdl/linter", "date": "2026-10-03", "symptom": "MPR002 'Microflow X has no activities' on a microflow or nanoflow whose only content is `return ;` (e.g. a label formatter, `return $currentUser;`)", "cause": "ActivityCount excludes start and end events, so a flow that computes its result in the end event's return value counts 0 activities", "file": "`mdl/linter/rules/empty.go` (`returnsValue`)", "insight": "A non-Void ReturnType is the catalog's witness that the end event returns a value (mxbuild requires it on every end event), so no new column was needed; '' and 'Void' stay reported", "refs": ["ako/mxcli#953"]} +{"area": "mdl/catalog", "date": "2026-10-03", "symptom": "`commit $Order` (on a loop iterator, a parameter or a retrieved list) wrote no refs row; refs_to(entity) could not answer which flows commit an entity", "cause": "refs had no commit ref kind: microflowActionRef / microflowVarActionRef emitted create/change/delete only, and a create/change with commit carried no commit edge", "file": "`mdl/catalog/builder_references.go` (`microflowCommitRef`, `RefKindCommit`)", "insight": "Commit is a use of the entity type like change/delete, resolved through the same intra-flow varEntity map (so the loop-iterator fix of #1266 applies for free), and emitted as a second edge beside create/change rather than replacing them. It stays out of graphRefKinds (would double existing flow->entity edges) and callerRefKinds (a type use, not an invocation). The ref_kind vocabulary test now reads every RefKind constant from the declarations, so a new kind cannot ship undocumented.", "refs": ["ako/mxcli#963", "mendixlabs/mxcli#1266", "mendixlabs/mxcli#1267"]} diff --git a/.claude/skills/mendix/write-lint-rules/SKILL.md b/.claude/skills/mendix/write-lint-rules/SKILL.md index 9914e5cb6e..92defed0dc 100644 --- a/.claude/skills/mendix/write-lint-rules/SKILL.md +++ b/.claude/skills/mendix/write-lint-rules/SKILL.md @@ -552,7 +552,7 @@ Returned by `permissions()` (all types) or `permissions_for()` (entity-specific) | `target_type` | string | What it points AT, upper-case: `"ENTITY"`, `"ASSOCIATION"`, `"MICROFLOW"`, `"NANOFLOW"`, `"RULE"`, `"PAGE"`, `"LAYOUT"`, `"WORKFLOW"`, `"WIDGET"`, `"JAVA_ACTION"`, `"REST_OPERATION"`, `"REGULAR_EXPRESSION"`, `"ATTRIBUTE"`, `"ENUMERATION"`, `"ENUMERATION_VALUE"`. `LAYOUT`, `WIDGET`, `ATTRIBUTE`, `ENUMERATION` and `ENUMERATION_VALUE` are only ever targets; `SCHEDULED_EVENT` and `PROJECT_SETTINGS` only ever sources | | `target_id` | string | Target UUID | | `target_name` | string | `"Sales.Customer"`; three-part for an attribute or an enumeration value: `"Sales.Order.Total"`, `"Sales.OrderStatus.Open"` | -| `ref_kind` | string | How it references: `"call"`, `"create"`, `"retrieve"`, `"change"`, `"delete"`, `"show_page"`, `"datasource"`, `"action"`, `"layout"`, `"parameter"`, `"return"`, `"generalize"`, `"associate"`, `"home_page"`, `"login_page"`, `"menu_item"`, `"calculate"`, `"schedule"`, `"validate"`, `"settings"`, `"widget"`, `"sync"`, `"publish"`, `"event"`, `"member"` (binds/reads/writes an attribute or navigates an association), `"xpath"` (an XPath constraint names it), `"type"` (typed as an enumeration), `"value"` (an expression names an enumeration value), `"mapping"` (an import/export mapping maps the entity) — lower-case, unlike the types above. Attribute names used only through a variable in a free-text expression (`$Order/Total`) have no edge | +| `ref_kind` | string | How it references: `"call"`, `"create"`, `"retrieve"`, `"change"`, `"delete"`, `"commit"` (a commit action, or a create/change that commits — beside its `"create"`/`"change"` edge; a commit of a variable whose entity the flow cannot tell has no edge), `"show_page"`, `"datasource"`, `"action"`, `"layout"`, `"parameter"`, `"return"`, `"generalize"`, `"associate"`, `"home_page"`, `"login_page"`, `"menu_item"`, `"calculate"`, `"schedule"`, `"validate"`, `"settings"`, `"widget"`, `"sync"`, `"publish"`, `"event"`, `"member"` (binds/reads/writes an attribute or navigates an association), `"xpath"` (an XPath constraint names it), `"type"` (typed as an enumeration), `"value"` (an expression names an enumeration value), `"mapping"` (an import/export mapping maps the entity) — lower-case, unlike the types above. Attribute names used only through a variable in a free-text expression (`$Order/Total`) have no edge | | `module_name` | string | Source module | ### project_security diff --git a/CHANGELOG.md b/CHANGELOG.md index a2d2244ed2..71340a1522 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -133,6 +133,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added +- **A `commit` reference kind in the catalog** (ako/mxcli#963) — `refs` has a `commit` edge from a flow to the entity it commits: a commit action, or a create / change that commits (`Yes` or `YesWithoutEvents`), the latter beside its `create` / `change` edge. The entity of a committed variable is resolved like `change` / `delete`, loop iterators included, so `commit $Order` inside `loop $Order in $Orders` now has a row. `refs_to("M.Order")` in a Starlark rule answers which flows commit an order. `commit` is not in the analysis graph and not a caller kind. The catalog schema version is bumped, so a cached catalog rebuilds. - **`total_activity_count` on the Starlark microflow struct** (ako/mxcli#963) — the catalog's `TotalActivityCount` (loop bodies included, at any depth) for every flow `microflows()` yields: microflows, nanoflows and rules. `activity_count` keeps counting a loop as one activity. - **A project records which mxcli wrote its tooling, and an older binary says so** (ako/mxcli#952) — `mxcli init` and every `init --sync-skills` write `.ai-context/mxcli-tooling.json` (version, build time, date; rewritten only when the version changes). Any command that opens the project with `-p` and a binary **older** than the stamp warns once on stderr, naming both versions and how to update; `init --sync-skills` from an older binary **refuses** instead of rolling the skills, rules and CLAUDE.md back. Releases compare by number, nightlies by tag date, a release against a nightly by build date; dev builds are never reported. Binaries from v0.24.0 and earlier cannot read the stamp, so the regenerated `.claude/bootstrap-mxcli.sh` checks it before choosing a binary: an older `mxcli` on PATH is not linked in (it downloads `MXCLI_TAG` instead), an older `./mxcli` is replaced, and the download lands through a temporary file so a `./mxcli` symlink never has it written through into the PATH binary. - **`init --sync-skills` (alias `--sync`) refreshes the bundled lint rules and the mxcli section of CLAUDE.md / AGENTS.md** (ako/mxcli#952) — it used to refresh only the skills, so a project kept the lint rules and guidance of whichever mxcli first initialised it. Bundled rules are recognised by file name; your own rules beside them are never touched. CLAUDE.md and AGENTS.md are now written between `` / `` markers, and only that section is refreshed — by the sync and by a re-run of `mxcli init` — so project notes outside the markers survive. A file written before the markers is left alone by the sync, with a note; run `mxcli init` once to adopt them. diff --git a/docs-site/src/internals/catalog-schema.md b/docs-site/src/internals/catalog-schema.md index c55e849c25..1dd5655036 100644 --- a/docs-site/src/internals/catalog-schema.md +++ b/docs-site/src/internals/catalog-schema.md @@ -260,6 +260,7 @@ rather than trusting a list here: |---------|------| | `call` | flow calls a microflow / nanoflow / rule / Java action / REST operation | | `create` / `change` / `delete` / `retrieve` | flow acts on an entity object | +| `commit` | flow commits an entity object: a commit action, or a create / change that commits (`Yes` or `YesWithoutEvents`), beside its `create` / `change` edge | | `return` | flow returns an entity type | | `parameter` | page or flow parameter entity type | | `generalize` | entity extends entity | @@ -278,6 +279,14 @@ rather than trusting a list here: | `validate` | attribute validation rule uses a regular expression | | `widget` | page or snippet uses a pluggable / custom widget | +`change`, `delete` and `commit` act on a *variable*, so the entity is resolved +within the flow — from a parameter, a create or retrieve output, or a loop +iterator over one of those. A variable whose entity the flow cannot tell (a +microflow call's result, for one) has no edge. `commit` is not in the analysis +graph: the variable it commits comes from a parameter, create or retrieve that +already links the flow to the entity (or to the association it was retrieved +over), so it would mostly double existing edges. + `schedule`, `publish`, `event` and `settings` are **entry points**: something outside the call graph runs the microflow, so nothing in the model calls it. They are what stops `GRAPH_DEAD_ASSETS`, `LIST CALLERS OF` and lint rule QUAL004 diff --git a/mdl/catalog/builder_commit_refs_test.go b/mdl/catalog/builder_commit_refs_test.go new file mode 100644 index 0000000000..3a7cc8dcac --- /dev/null +++ b/mdl/catalog/builder_commit_refs_test.go @@ -0,0 +1,117 @@ +// SPDX-License-Identifier: Apache-2.0 + +package catalog + +import ( + "sort" + "testing" + + "github.com/mendixlabs/mxcli/mdl/backend/mock" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/domainmodel" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// `commit $Order` on a loop iterator wrote no refs row: refs had no commit kind +// at all (ako/mxcli#963). A flow that commits entity X now has a `commit` edge +// FLOW -> ENTITY, from a commit action and from a create or change that commits. +// A create/change with commit No is the control: it keeps its create/change edge +// and gains no commit edge. +func commitRefsCatalog(t *testing.T) *Catalog { + t.Helper() + const mod = model.ID("mod-t") + orders := listParam("Orders", "T.Order") + order := &domainmodel.Entity{Name: "Order"} + order.ID = "ent-order" + customer := &domainmodel.Entity{Name: "Customer"} + customer.ID = "ent-customer" + dm := &domainmodel.DomainModel{ContainerID: mod, Entities: []*domainmodel.Entity{order, customer}} + dm.ID = "dm-t" + flow := func(id, name string, objs ...microflows.MicroflowObject) *microflows.Microflow { + return µflows.Microflow{ + BaseElement: model.BaseElement{ID: model.ID(id)}, ContainerID: mod, Name: name, + Parameters: []*microflows.MicroflowParameter{orders}, + ObjectCollection: µflows.MicroflowObjectCollection{Objects: objs}, + } + } + + be := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + GetProjectSettingsFunc: func() (*model.ProjectSettings, error) { return &model.ProjectSettings{}, nil }, + ListModuleSettingsFunc: func() ([]*types.ModuleSettings, error) { return nil, nil }, + ListModulesFunc: func() ([]*model.Module, error) { + return []*model.Module{{BaseElement: model.BaseElement{ID: mod}, Name: "T"}}, nil + }, + ListDomainModelsFunc: func() ([]*domainmodel.DomainModel, error) { return []*domainmodel.DomainModel{dm}, nil }, + ListRulesFunc: func() ([]*microflows.Rule, error) { return nil, nil }, + GetNavigationFunc: func() (*types.NavigationDocument, error) { return &types.NavigationDocument{}, nil }, + ListMicroflowsFunc: func() ([]*microflows.Microflow, error) { + return []*microflows.Microflow{ + flow("mf-1", "CommitEach", iterLoop("l1", "Orders", "Order", + newAction("a1", µflows.CommitObjectsAction{CommitVariable: "Order", WithEvents: true}))), + flow("mf-2", "CommitList", + newAction("a2", µflows.CommitObjectsAction{CommitVariable: "$Orders"})), + flow("mf-3", "CreateCommit", + newAction("a3", µflows.CreateObjectAction{EntityQualifiedName: "T.Customer", OutputVariable: "C", Commit: microflows.CommitTypeYes})), + flow("mf-4", "ChangeCommit", iterLoop("l2", "Orders", "Order", + newAction("a4", µflows.ChangeObjectAction{ChangeVariable: "Order", Commit: microflows.CommitTypeYesWithoutEvents}))), + flow("mf-5", "NoCommit", + newAction("a5", µflows.CreateObjectAction{EntityQualifiedName: "T.Customer", OutputVariable: "C", Commit: microflows.CommitTypeNo}), + iterLoop("l3", "Orders", "Order", + newAction("a6", µflows.ChangeObjectAction{ChangeVariable: "Order", Commit: microflows.CommitTypeNo}))), + }, nil + }, + ListNanoflowsFunc: func() ([]*microflows.Nanoflow, error) { + return []*microflows.Nanoflow{{ + BaseElement: model.BaseElement{ID: "nf-1"}, ContainerID: mod, Name: "NF_CommitEach", + Parameters: []*microflows.MicroflowParameter{orders}, + ObjectCollection: µflows.MicroflowObjectCollection{Objects: []microflows.MicroflowObject{ + iterLoop("l4", "Orders", "Order", + newAction("a7", µflows.CommitObjectsAction{CommitVariable: "Order"})), + }}, + }}, nil + }, + } + cat, err := New() + if err != nil { + t.Fatalf("catalog.New: %v", err) + } + t.Cleanup(func() { cat.Close() }) + b := NewBuilder(cat, be) + b.SetFullMode(true) + if err := b.Build(nil); err != nil { + t.Fatalf("catalog build: %v", err) + } + return cat +} + +func TestCommitEmitsCommitRefs(t *testing.T) { + cat := commitRefsCatalog(t) + var got []string + for _, row := range queryRows(t, cat, `SELECT SourceName, TargetName, RefKind FROM refs + WHERE TargetType = 'ENTITY' AND RefKind IN ('create', 'change', 'commit')`) { + got = append(got, row[0].(string)+" "+row[2].(string)+" "+row[1].(string)) + } + sort.Strings(got) + want := []string{ + "T.ChangeCommit change T.Order", + "T.ChangeCommit commit T.Order", + "T.CommitEach commit T.Order", // commit of the loop iterator + "T.CommitList commit T.Order", + "T.CreateCommit commit T.Customer", + "T.CreateCommit create T.Customer", + "T.NF_CommitEach commit T.Order", + // Control: create/change with commit No — no commit edge. + "T.NoCommit change T.Order", + "T.NoCommit create T.Customer", + } + if len(got) != len(want) { + t.Fatalf("refs rows:\n%v\nwant:\n%v", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("refs rows:\n%v\nwant:\n%v", got, want) + } + } +} diff --git a/mdl/catalog/builder_graph.go b/mdl/catalog/builder_graph.go index 699a6cc601..12e2a9de01 100644 --- a/mdl/catalog/builder_graph.go +++ b/mdl/catalog/builder_graph.go @@ -18,6 +18,11 @@ import ( var graphRefKinds = []string{ "call", "retrieve", "create", "change", "delete", "associate", "generalize", "parameter", "return", + // Not "commit": its variable is resolved from a parameter, a create or a + // retrieve, each of which already links the flow to the entity (directly, + // or through the association an association retrieve names). Adding it + // would mostly double existing edges' weight and shift communities and + // centrality on every project that commits (ako/mxcli#963). // Entry points: something outside the call graph invokes these, so the // microflow they run is reachable even though nothing in the model calls it. Leaving one out does // not hide it from GRAPH_DEAD_ASSETS — that view asks only whether ANY refs diff --git a/mdl/catalog/builder_references.go b/mdl/catalog/builder_references.go index 710942beb5..0fb24e22a7 100644 --- a/mdl/catalog/builder_references.go +++ b/mdl/catalog/builder_references.go @@ -29,6 +29,7 @@ const ( RefKindMenuItem = "menu_item" // Navigation menu item page reference RefKindChange = "change" // Microflow changes an entity object RefKindDelete = "delete" // Microflow deletes an entity object + RefKindCommit = "commit" // Microflow commits an entity object (commit action, or create/change with commit) RefKindCalculate = "calculate" // Calculated attribute uses a microflow RefKindReturn = "return" // Microflow/nanoflow returns an entity type RefKindSchedule = "schedule" // Scheduled event runs a microflow @@ -267,6 +268,36 @@ func microflowVarActionRef(action microflows.MicroflowAction, varEntity map[stri return "", "", "", false } +// microflowCommitRef resolves the entity an action commits: a commit action's +// variable, or the object of a create or change that commits (Yes or +// YesWithoutEvents). It is a second edge beside the create/change one, so +// "which flows commit entity X" is one refs query — the question a commit in a +// loop or a commit without events starts from (mendixlabs/mxcli#1266, #1267). +// A commit of a variable whose entity is not known in the flow emits nothing, +// as for change and delete (ako/mxcli#963). +func microflowCommitRef(action microflows.MicroflowAction, varEntity map[string]string) (entity string, ok bool) { + commits := func(c microflows.CommitType) bool { + return c == microflows.CommitTypeYes || c == microflows.CommitTypeYesWithoutEvents + } + resolve := func(v string) (string, bool) { + qn, found := varEntity[strings.TrimPrefix(v, "$")] + return qn, found && qn != "" + } + switch a := action.(type) { + case *microflows.CommitObjectsAction: + return resolve(a.CommitVariable) + case *microflows.CreateObjectAction: + if commits(a.Commit) && a.EntityQualifiedName != "" { + return a.EntityQualifiedName, true + } + case *microflows.ChangeObjectAction: + if commits(a.Commit) { + return resolve(a.ChangeVariable) + } + } + return "", false +} + // assocEnds are an association's endpoint entities: From owns the reference // (BSON ParentPointer), To is referenced (ChildPointer). type assocEnds struct{ From, To string } @@ -431,6 +462,9 @@ func (b *Builder) buildReferences() error { if tt, tn, rk, ok := microflowVarActionRef(act.Action, varEntity); ok { emit(tt, tn, rk) } + if qn, ok := microflowCommitRef(act.Action, varEntity); ok { + emit(RefObjectEntity, qn, RefKindCommit) + } } } diff --git a/mdl/catalog/lint_rule_doc_vocabulary_test.go b/mdl/catalog/lint_rule_doc_vocabulary_test.go index f07c6948f0..5b18916137 100644 --- a/mdl/catalog/lint_rule_doc_vocabulary_test.go +++ b/mdl/catalog/lint_rule_doc_vocabulary_test.go @@ -225,26 +225,31 @@ func TestSkillDocumentsRealAttributeDataTypes(t *testing.T) { docRowValues(t, doc, "data_type"), real, "AttributeType.GetTypeName") } -// TestSkillDocumentsRealRefKinds covers ref_kind, which documents no examples -// today. Whatever it documents must be a kind buildReferences emits. +// TestSkillDocumentsRealRefKinds pins the ref_kind row both ways: whatever it +// documents must be a kind buildReferences emits, and every RefKind constant +// must be documented — read from the declarations, so a new kind (commit, +// ako/mxcli#963) cannot ship without a line a rule author can find. func TestSkillDocumentsRealRefKinds(t *testing.T) { doc := lintRuleSkillDoc(t) + declared := constantsWithPrefix(t, "builder_references.go", "RefKind") + // CONTROL: a scan that collected nothing would pass whatever the row said. + if len(declared) < 20 { + t.Fatalf("found only %d RefKind* constants — the scan is broken", len(declared)) + } real := map[string]bool{} - for _, k := range []string{ - RefKindCall, RefKindCreate, RefKindRetrieve, RefKindShowPage, - RefKindGeneralize, RefKindAssociate, RefKindLayout, RefKindDatasource, - RefKindParameter, RefKindAction, RefKindHomePage, RefKindLoginPage, - RefKindMenuItem, RefKindChange, RefKindDelete, RefKindCalculate, - RefKindReturn, RefKindSchedule, RefKindValidate, RefKindSettings, - RefKindWidget, RefKindSync, RefKindPublish, RefKindEvent, - RefKindMember, RefKindXPath, RefKindType, RefKindValue, RefKindMapping, - } { - real[k] = true + for _, v := range declared { + real[v] = true } - assertDocumentedValuesExist(t, "ref_kind", - docRowValues(t, doc, "ref_kind"), real, "buildReferences") + documented := docRowValues(t, doc, "ref_kind") + assertDocumentedValuesExist(t, "ref_kind", documented, real, "buildReferences") + + for name, v := range declared { + if !slices.Contains(documented, v) { + t.Errorf("%s = %q is emitted but the skill's ref_kind row does not document it", name, v) + } + } } // TestEveryObjectTypeConstantIsInAPublishedList is the other half of the pin. diff --git a/mdl/catalog/lint_rule_vocabulary_test.go b/mdl/catalog/lint_rule_vocabulary_test.go index 61b6445f39..f95d6df118 100644 --- a/mdl/catalog/lint_rule_vocabulary_test.go +++ b/mdl/catalog/lint_rule_vocabulary_test.go @@ -106,7 +106,7 @@ func TestQUAL004EntryKindsAreRealRefKinds(t *testing.T) { RefKindParameter, RefKindAction, RefKindHomePage, RefKindLoginPage, RefKindMenuItem, RefKindChange, RefKindDelete, RefKindCalculate, RefKindReturn, RefKindSchedule, RefKindValidate, RefKindSettings, - RefKindWidget, RefKindSync, RefKindPublish, RefKindEvent, + RefKindWidget, RefKindSync, RefKindPublish, RefKindEvent, RefKindCommit, } { known[k] = true } diff --git a/mdl/catalog/tables.go b/mdl/catalog/tables.go index e89ac53f71..2826edd2e2 100644 --- a/mdl/catalog/tables.go +++ b/mdl/catalog/tables.go @@ -7,6 +7,10 @@ package catalog // // History: // +// 19 (commit refs): refs gains RefKind "commit" (FLOW -> ENTITY) for a +// commit action and a create/change that commits (ako/mxcli#963). No +// column changes, but a cached catalog would keep answering "no flow +// commits X" for every entity until something else rebuilt it. // 18 — layouts_data.Platform ("Web" / "Native"), the content wrapper's // type. LayoutType cannot tell the platforms apart ("Popup" is native), // and MPR012 needs it to stay off native pages, where CE0582 does not @@ -101,7 +105,7 @@ package catalog // SnapshotSource / SourceId / SourceBranch / SourceRevision columns // from every row (issue #576). // 1 — initial flat schema with denormalized snapshot columns on every row. -const CatalogSchemaVersion = "18" +const CatalogSchemaVersion = "19" // MetaSchemaVersion is the catalog_meta key that records the schema version // the cache was built against. diff --git a/mdl/executor/cmd_search.go b/mdl/executor/cmd_search.go index c75c5a64f2..74614a20db 100644 --- a/mdl/executor/cmd_search.go +++ b/mdl/executor/cmd_search.go @@ -35,7 +35,8 @@ import ( // list. // // Deliberately excluded: 'datasource', 'parameter', 'return', 'retrieve', -// 'create', 'change', 'delete', 'associate', 'generalize', 'layout' and 'sync'. +// 'create', 'change', 'delete', 'commit', 'associate', 'generalize', 'layout' +// and 'sync'. // Those are uses of a TYPE or a LAYOUT, not invocations, and folding them in // would make `show callers of ` a synonym for `show references to`. var callerRefKinds = []string{ From da494c2033c88c0bb2ae8523f4d30524f2d4de3d Mon Sep 17 00:00:00 2001 From: Ako Date: Sat, 3 Oct 2026 17:53:16 +0000 Subject: [PATCH 05/15] fix(describe): duplicate output variable warning is flow-wide The same output name in each if/else branch, or inside a loop and again after it, is CE0111 in mxbuild 11.13.0. describe only warned when one assignment reached the other, so it treated branches and loop bodies as scopes. Count names over the whole flow instead; void calls stay excluded. The test that pinned branch scoping now asserts the warning. Part of #962 (item 2). Co-Authored-By: Claude Opus 5.5 --- .../fix-issue/findings/mdl-executor.jsonl | 1 + CHANGELOG.md | 1 + .../cmd_microflows_duplicate_output_test.go | 41 +++++- mdl/executor/cmd_microflows_show.go | 130 ++++-------------- 4 files changed, 66 insertions(+), 107 deletions(-) diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index 249d772378..076f555c07 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -851,3 +851,4 @@ {"area": "mdl/executor", "date": "2026-10-03", "symptom": "ako/mxcli#950 item 2: describe of a flow ending `return [%CurrentUser%];` prints `return $[%CurrentUser%];`, which does not parse; `return if … then … else …` likewise became `return $if …` (two TestApp WorkflowCommons microflows)", "cause": "formatActivity's EndEvent branch added `$` to any return value without one of + ' \" ( ) — a character blacklist standing in for 'is a bare variable name'", "file": "`mdl/executor/cmd_microflows_format_action.go` (isBareReturnVariable)", "insight": "Restore a stripped sigil only for the positive shape it was stripped from (a bare name, optionally /path); a blacklist of characters lets every new expression form through. The other `$`-adding sites in describe prefix variable-NAME fields, not expressions, and are safe", "refs": ["ako/mxcli#950"]} {"area": "mdl/executor", "date": "2026-10-03", "symptom": "ako/mxcli#950 item 3: describe of a navigation list prints item actions as `show_page 'Mod.Page'` (does not parse — TestApp Rules.Entity_Menu, 3 syntax errors); with that fixed, exec refuses the description with `item inside navigationlist requires a name` because Studio Pro leaves items unnamed", "cause": "extractNavigationListItemAction had a private copy of the page-action rendering in the legacy form instead of the shared renderClientActionMDL; buildNavigationListItemV3 required a name Studio Pro never stores; and once exec accepted it, the writer wrote `Name: \"\"` and no ConditionalVisibilitySettings where Studio Pro stores no Name key and a null slot (6 of 6 items in TestApp), so GetPut still rewrote the snippet", "file": "`mdl/executor/cmd_pages_describe_parse.go` (extractNavigationListItemAction), `mdl/executor/cmd_pages_builder_v3_widgets.go` (buildNavigationListItemV3), `mdl/executor/cmd_pages_describe_output.go` (item header), `mdl/backend/modelsdk/widget_write.go` (navListItemToGen, Forms$NavigationListItem NullFields)", "insight": "Fixing the reported parse error only exposed the next law: the issue said the empty item name 'parses fine', which was true and irrelevant — exec refused it, and after that the writer rewrote it. An unnamed item with no Name key passes mx check at 11.14.0, contrary to the old ledger note that the key is mandatory (that applies to a NAMED item's key, not its absence). Run the whole describe → check → exec → describe chain on the Studio Pro-authored document before declaring a round-trip bug fixed", "refs": ["ako/mxcli#950"]} {"area": "mdl/executor", "date": "2026-10-03", "symptom": "ako/mxcli#962 item 1: `$V = call java action M.VoidAction(...)` then `log ... + $V` (or `$V` as a JS call argument) passes `check` and `exec`, then mxbuild 11.13.0 fails CE0109 \"Undefined variable 'V'\"", "cause": "#953 taught MDL063 that a void call's output name declares nothing, but no rule read the other half: the name cannot be READ either. The resolver only answered void/not-void, so 'unknown' and 'non-void' were the same answer", "file": "`mdl/executor/validate_void_call_output.go` (checkVoidCallOutputUse, MDL093); `mdl/executor/validate_void_code_calls.go` (resolve -> voidness{void, known})", "fix": "MDL093: collect output names of calls KNOWN to be void, drop any name another statement defines flow-wide (declare, parameter, non-void producer), report each remaining name the flow reads (loopRefVars over every nested body). The resolver now returns known-ness, so 'possibly void' (the editor's policy) never produces MDL093", "insight": "A finding that says 'X declares nothing' has two consequences — no collision AND no definition; fixing the first and logging the second as follow-up left a CE gap. When a resolver's default is a policy (unknown counts as non-void), make the unknown state explicit before a second rule reads it: the CE0109 rule must use only knowledge, never the policy", "test": "`mdl/executor/validate_void_call_output_test.go`; `cmd/mxcli/check_void_calls_test.go` (TestCheck_ReadOfAStoredVoidCallOutput, PedApp stored void JS action, Boolean and unresolvable controls)"} +{"area": "mdl/executor", "date": "2026-10-03", "symptom": "ako/mxcli#962 item 2: describe of a microflow with the same output name in each if/else branch (or inside a loop and again after it) printed no duplicate-variable warning; mxbuild 11.13.0 reports CE0111 for both", "cause": "duplicateOutputVariableWarnings only warned when one assignment could REACH another (a reachability walk written for #710's performance), so exclusive branches and a loop body vs. the flow after it were treated as separate scopes; a test pinned the branch scoping", "file": "`mdl/executor/cmd_microflows_show.go` (duplicateOutputVariableWarnings)", "fix": "Count non-void output names over every object collection (loop bodies included); warn for any name created twice. Linear, so #710's cost concern disappears with the reachability walk", "insight": "The reachability model encoded a belief (exclusive paths may reuse a name) that no one had measured; MDL063 had already been aligned to flow-wide names in #958, so two renderings of the same rule disagreed. When one rule is corrected against mxbuild, grep for the other places that encode the same rule — here describe's header warning", "test": "`mdl/executor/cmd_microflows_duplicate_output_test.go` (TestFormatMicroflowActivitiesWarnsForExclusiveBranchOutputs, TestFormatMicroflowActivitiesNamesAreFlowWide)"} diff --git a/CHANGELOG.md b/CHANGELOG.md index 0efb498742..eb690d6c6c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **`describe` warns about a duplicate output variable across if/else branches and loop bodies** (ako/mxcli#962) — a flow's variable names are unique flow-wide: the same output name in each branch of an if/else, or inside a loop and again after it, is CE0111 in mxbuild 11.13.0 (measured in a microflow). The `-- WARNING: duplicate output variable` header only fired when one assignment could reach the other, so it called both models valid. Calls to void actions are still never counted. - **`check` reports a read of a void action call's output name** (ako/mxcli#962) — `$V = call java action M.Void(…)` followed by anything reading `$V` passed `check` and failed the build with CE0109 "Undefined variable 'V'" (measured on mxbuild 11.13.0). It is now MDL093, for microflows and nanoflows, when the script or, with `-p`, the project says the action returns Void. A name something else defines (a declare, a parameter, a non-void producer) and an action nobody can resolve are not reported. - **`docker check` no longer modifies the project** (ako/mxcli#951) — `mx update-widgets` and `mx check` now run on a temporary copy, for MPR v1 and v2 alike, and what mx prints names the project's own paths. Before, a check rewrote an MPR v1 project's `.mpr` permanently (only v2 was restored from a snapshot), and `mx check` itself rewrote `theme-cache/` and created `deployment/sass/` even with `--no-update-widgets`. The output now says that widget definitions were normalised on a copy, and that a CE0463 the stored project still has is therefore not reported: `--no-update-widgets` checks the project as stored, `mxcli fix widgets` applies the normalisation (ako/mxcli#568, #646). Build output, caches and VCS folders are not copied; the copy goes to `$TMPDIR` and is removed afterwards. - **Parallel mxcli runs on one project no longer fail to save the catalog cache** (ako/mxcli#951) — eight parallel `lint` runs on a fresh copy printed `failed to create table catalog_meta: table catalog_meta already exists` or `database is locked`. The cache is now written to a temporary file next to it and renamed into place, so every run saves and a reader sees the old cache or the new one, never a half-written file; opening a current cache no longer writes to it. diff --git a/mdl/executor/cmd_microflows_duplicate_output_test.go b/mdl/executor/cmd_microflows_duplicate_output_test.go index 9ee63e835f..97b3e80a8a 100644 --- a/mdl/executor/cmd_microflows_duplicate_output_test.go +++ b/mdl/executor/cmd_microflows_duplicate_output_test.go @@ -109,7 +109,11 @@ func TestFormatMicroflowActivitiesWarnsAboutDuplicateModelOutputs(t *testing.T) } } -func TestFormatMicroflowActivitiesDoesNotWarnForExclusiveBranchOutputs(t *testing.T) { +// A flow's names are flow-wide: the same output name in each branch of an +// if/else is CE0111 in mxbuild 11.13.0 (ako/mxcli#962, measured in a +// microflow with a non-void Java call and with a retrieve). This test pinned the +// opposite — branch scoping — until the measurement contradicted it. +func TestFormatMicroflowActivitiesWarnsForExclusiveBranchOutputs(t *testing.T) { oc := µflows.MicroflowObjectCollection{ Objects: []microflows.MicroflowObject{ µflows.StartEvent{ @@ -172,8 +176,8 @@ func TestFormatMicroflowActivitiesDoesNotWarnForExclusiveBranchOutputs(t *testin lines := formatMicroflowActivities(&ExecContext{}, µflows.Microflow{ObjectCollection: oc}, nil, nil) got := strings.Join(lines, "\n") - if strings.Contains(got, "-- WARNING: duplicate output variable $Result") { - t.Fatalf("exclusive branch outputs must not be warned as linear duplicates:\n%s", got) + if !strings.Contains(got, "-- WARNING: duplicate output variable $Result") { + t.Fatalf("the same output name in both branches is CE0111 and must be warned:\n%s", got) } } @@ -258,3 +262,34 @@ func TestFormatMicroflowActivitiesHighComplexityCompletes(t *testing.T) { // Just needs to complete; path enumeration would be 2^120. _ = duplicateOutputVariableWarnings(oc, func(any) bool { return false }) } + +// A loop body opens no scope either: a non-void Java call inside a loop and +// another of the same name after it is CE0111 (mxbuild 11.13.0, ako/mxcli#962). +// The old reachability walk missed it — the second call does not reach the +// loop. Control: distinct names warn nothing. +func TestFormatMicroflowActivitiesNamesAreFlowWide(t *testing.T) { + start := µflows.StartEvent{} + start.ID = "start" + end := µflows.EndEvent{} + end.ID = "end" + loop := µflows.LoopedActivity{ObjectCollection: µflows.MicroflowObjectCollection{ + Objects: []microflows.MicroflowObject{act("in", "X", 10)}, + }} + loop.ID = "loop" + oc := µflows.MicroflowObjectCollection{ + Objects: []microflows.MicroflowObject{start, loop, act("after", "X", 400), end}, + Flows: []*microflows.SequenceFlow{ + {OriginID: "start", DestinationID: "loop"}, + {OriginID: "loop", DestinationID: "after"}, + {OriginID: "after", DestinationID: "end"}, + }, + } + if !warnsDuplicate(t, oc, "X") { + t.Error("an output inside a loop and the same name after it not warned") + } + + loop.ObjectCollection.Objects = []microflows.MicroflowObject{act("in", "Y", 10)} + if warnsDuplicate(t, oc, "X") || warnsDuplicate(t, oc, "Y") { + t.Error("control: distinct names warned") + } +} diff --git a/mdl/executor/cmd_microflows_show.go b/mdl/executor/cmd_microflows_show.go index 5ba682a54c..06438ef668 100644 --- a/mdl/executor/cmd_microflows_show.go +++ b/mdl/executor/cmd_microflows_show.go @@ -868,134 +868,56 @@ func formatMicroflowActivities( return append(microflowBodyWarnings(ctx, mf, labels, declaredCrossed), lines...) } -// duplicateOutputVariableWarnings flags output-variable names that are assigned by -// two activities which can BOTH run on a single execution path — i.e. one activity -// reaches the other. Assignments in mutually-exclusive branches (then/else, enum -// cases) are legal and not flagged. +// duplicateOutputVariableWarnings flags output-variable names that two +// activities of one flow both create. // -// This uses reachability, which is O(V*E). The previous implementation enumerated -// every execution path (cloning the visited set at each branch), which is O(2^b) -// in the number of branch points and made `describe microflow` time out on -// high-complexity flows — 20 sequential if/end-if diamonds already took ~10s -// (issue #710). Microflow control flow is a DAG (loops are nested inside -// LoopedActivity nodes, not back-edges), so reachability is exact and cheap. +// A flow's variable names are unique FLOW-WIDE: neither an if/else branch nor a +// loop body opens a scope. Measured on mxbuild 11.13.0 (ako/mxcli#962, PedApp +// copy), a non-void Java call — and a retrieve — of the same name in each +// branch of an if/else is CE0111 "Duplicate variable name" in a microflow, as +// #953 measured for a nanoflow. This used to warn only when one assignment +// could reach the other, which called the exclusive-branch case legal. +// +// That reachability walk is also gone, and with it the cost #710 was about: a +// count per name is linear in the number of activities. // // isVoidCall, when it says so, marks an action whose output name declares no // variable — a call to a void Java/JavaScript action. func duplicateOutputVariableWarnings(oc *microflows.MicroflowObjectCollection, isVoidCall func(action any) bool) []string { - warningPositions := make(map[string]model.Point) - record := func(name string, pos model.Point) { - if _, ok := warningPositions[name]; !ok { - warningPositions[name] = pos - } - } - - type assignment struct { - id model.ID - pos model.Point - } - - var walk func(collection *microflows.MicroflowObjectCollection, inherited map[string]model.Point) - walk = func(collection *microflows.MicroflowObjectCollection, inherited map[string]model.Point) { + count := map[string]int{} + first := map[string]model.Point{} + var walk func(collection *microflows.MicroflowObjectCollection) + walk = func(collection *microflows.MicroflowObjectCollection) { if collection == nil { return } - flowsByOrigin := make(map[model.ID][]*microflows.SequenceFlow) - for _, flow := range collection.Flows { - flowsByOrigin[flow.OriginID] = append(flowsByOrigin[flow.OriginID], flow) - } - - // reachableFrom(id) = node IDs reachable from id via normal flows (excluding - // id). Memoized; the in-progress guard tolerates any stray cycle. - cache := make(map[model.ID]map[model.ID]bool) - inProgress := make(map[model.ID]bool) - var reachableFrom func(id model.ID) map[model.ID]bool - reachableFrom = func(id model.ID) map[model.ID]bool { - if r, ok := cache[id]; ok { - return r - } - if inProgress[id] { - return nil - } - inProgress[id] = true - r := make(map[model.ID]bool) - for _, flow := range findNormalFlows(flowsByOrigin[id]) { - if flow.DestinationID == "" { - continue - } - r[flow.DestinationID] = true - for k := range reachableFrom(flow.DestinationID) { - r[k] = true - } - } - delete(inProgress, id) - cache[id] = r - return r - } - - assignments := make(map[string][]assignment) - var loops []*microflows.LoopedActivity for _, obj := range collection.Objects { switch o := obj.(type) { case *microflows.ActionActivity: if name := actionOutputVariableName(o.Action); name != "" && !isVoidCall(o.Action) { - assignments[name] = append(assignments[name], assignment{id: o.GetID(), pos: o.GetPosition()}) - } - case *microflows.LoopedActivity: - loops = append(loops, o) - } - } - - for name, list := range assignments { - // An outer assignment on the entry path collides with any assignment here. - if pos, ok := inherited[name]; ok { - record(name, pos) - continue - } - // Within this collection: a duplicate iff one assignment reaches another. - for i := range list { - reach := reachableFrom(list[i].id) - for j := range list { - if i != j && reach[list[j].id] { - record(name, list[i].pos) - } - } - } - } - - // Recurse into loop bodies. Names visible on entry to a loop body are the - // inherited names plus names assigned in this collection by an activity that - // reaches the loop node. - for _, loop := range loops { - childInherited := make(map[string]model.Point, len(inherited)) - for n, p := range inherited { - childInherited[n] = p - } - for name, list := range assignments { - if _, ok := childInherited[name]; ok { - continue - } - for _, a := range list { - if a.id != loop.GetID() && reachableFrom(a.id)[loop.GetID()] { - childInherited[name] = a.pos - break + if count[name] == 0 { + first[name] = o.GetPosition() } + count[name]++ } + case *microflows.LoopedActivity: + walk(o.ObjectCollection) } - walk(loop.ObjectCollection, childInherited) } } - walk(oc, nil) + walk(oc) var names []string - for name := range warningPositions { - names = append(names, name) + for name, n := range count { + if n > 1 { + names = append(names, name) + } } sort.Strings(names) warnings := make([]string, 0, len(names)) for _, name := range names { - pos := warningPositions[name] + pos := first[name] warnings = append(warnings, fmt.Sprintf("-- WARNING: duplicate output variable $%s at position (%d, %d) - model is invalid; open in Studio Pro to fix", name, pos.X, pos.Y)) } return warnings From 0c6baf7f7783f4cf750450ad2fb65aa7742bc1a2 Mon Sep 17 00:00:00 2001 From: Ako Date: Sat, 3 Oct 2026 17:58:50 +0000 Subject: [PATCH 06/15] fix(lsp): resolve void action calls through the workspace project The language server ran the flow rules without a project, so two calls to a stored void action with the same output name were flagged MDL063. It now uses executor.FlowRules: actions resolve through the script and the workspace project, an unresolvable action is treated as possibly void (for MDL063 only, never MDL093), and project answers are cached for 30s between keystrokes because one read costs ~300ms on PedApp. Part of #962 (item 3). Co-Authored-By: Claude Opus 5.5 --- .../skills/fix-issue/findings/cmd-mxcli.jsonl | 1 + CHANGELOG.md | 1 + cmd/mxcli/lsp.go | 11 +- cmd/mxcli/lsp_diagnostics.go | 12 +- cmd/mxcli/lsp_void_calls_test.go | 78 ++++++++++++ mdl/executor/validate_void_code_calls.go | 119 ++++++++++++++++++ mdl/executor/validate_void_code_calls_test.go | 37 ++++++ 7 files changed, 253 insertions(+), 6 deletions(-) create mode 100644 cmd/mxcli/lsp_void_calls_test.go diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl b/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl index 6566f47f77..83a4661ce7 100644 --- a/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl +++ b/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl @@ -156,3 +156,4 @@ {"area": "cmd/mxcli", "date": "2026-10-03", "symptom": "A project whose CLAUDE.md, skills and lint rules were written by a newer mxcli is served by an older binary (v0.24.0 on PATH) with no warning: 'mdl 1;' is a parse error and shipped lint rules crash, all reading as project defects", "cause": "Nothing recorded which mxcli wrote the tooling; .claude/bootstrap-mxcli.sh linked whatever mxcli was on PATH; init --sync-skills refreshed only .ai-context/skills, never .claude/lint-rules or CLAUDE.md/AGENTS.md", "fix": "init and every sync write .ai-context/mxcli-tooling.json; root PersistentPreRun warns once on stderr when the binary is provably older (release by number, nightly by tag date, mixed by build date, dev never); sync refuses from an older binary; the bootstrap script carries a POSIX-sh copy of the ordering and neither links an older PATH binary nor keeps an older ./mxcli, downloading via a temp file + mv; sync also refreshes bundled lint rules by name and the CLAUDE.md/AGENTS.md section between mxcli:begin/end markers", "insight": "A binary cannot warn about a stamp it predates, so the guard for already-shipped binaries must live in the generated script, which the newer mxcli regenerates. The sh and Go comparisons share one test table so they cannot drift. curl -o ./mxcli on a symlinked ./mxcli would overwrite the PATH binary — always download to a temp name and rename", "issue": "ako/mxcli#952", "file": "cmd/mxcli/tooling_stamp.go; cmd/mxcli/init_tooling_sync.go; cmd/mxcli/init_hook.go (bootstrapScriptTemplate); cmd/mxcli/main.go; cmd/mxcli/init.go", "test": "cmd/mxcli/tooling_stamp_test.go; cmd/mxcli/init_hook_version_test.go; cmd/mxcli/init_tooling_sync_test.go"} {"area": "cmd/mxcli", "date": "2026-10-03", "symptom": "CONV006 emits one finding per entity x role x CREATE/DELETE (111 on a mid-sized app), the same advice repeated per role, and the per-finding Security score is driven by role count rather than by entities", "cause": "The Starlark rule appended a violation inside the permissions_for() loop", "file": "`.claude/lint-rules/conv006_no_create_delete_rights.star` (synced to `cmd/mxcli/lint-rules/`)", "insight": "Group per entity and per right with de-duplicated sorted roles (a role can hold several access rules on one entity). Test both rule copies (.claude and the embedded one) like SEC008's test does", "refs": ["ako/mxcli#953"]} {"area": "cmd/mxcli", "date": "2026-10-03", "symptom": "`mxcli report` could not score a project's own modules: `lint` has --modules, `report` had only --exclude", "cause": "Feature gap; and the LintContext module filter alone would not make the score exact, because project-level findings (CONV008 role mappings, project security) carry no module and are reported regardless", "file": "`cmd/mxcli/cmd_report.go`, `mdl/linter/report.go` (`ScopeToModules`, Report.Modules)", "insight": "Filter the scored violations to those located in a selected module, and print the selection in every format so a module score is not mistaken for the project's", "refs": ["ako/mxcli#953"]} +{"area": "cmd/mxcli", "date": "2026-10-03", "symptom": "ako/mxcli#962 item 3: in VS Code, two calls to a stored void JavaScript action with the same output name (Studio Pro's `$ReturnValueName`/`$RefreshEntity` shape) are squiggled MDL063, while `mxcli check -p` passes them", "cause": "runSemanticValidation called executor.ValidateMicroflow/ValidateNanoflow, which pass a nil void-action resolver: without the project every call output counts as a declaration", "file": "`cmd/mxcli/lsp_diagnostics.go` (runSemanticValidation); `mdl/executor/validate_void_code_calls.go` (FlowRules, CodeActionCache)", "fix": "executor.NewFlowRules(prog, s.findMprPath(), s.codeActions): resolves actions through the script and the workspace project (opened lazily), treats an unresolvable action as possibly void (unknownIsVoid) for MDL063 but never for MDL093, and shares project answers across keystrokes for 30s since one action read costs ~300ms on PedApp", "insight": "An entry point that wraps a richer internal API with nil arguments (ValidateMicroflow = validateMicroflowWith(stmt, nil)) silently gives every caller the weakest behaviour; a fix landed in the richer API (#958) does not reach them. Grep the exported wrapper's callers when the internal one gains a parameter. Measure the cost before putting project I/O on a keystroke path", "test": "`cmd/mxcli/lsp_void_calls_test.go` (PedApp: void pair clean with and without project, Boolean pair control, MDL093 only with project); `mdl/executor/validate_void_code_calls_test.go` (TestCodeActionCache_SharesProjectAnswersAcrossRuns)"} diff --git a/CHANGELOG.md b/CHANGELOG.md index eb690d6c6c..30bb92cb5c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **The editor no longer squiggles calls to a project's void actions** (ako/mxcli#962) — the language server validated flows without the project, so two calls to a stored void Java or JavaScript action carrying the same output name (the shape Studio Pro writes) were flagged MDL063. It now resolves the actions through the workspace project, remembering the answers for 30 seconds between keystrokes; an action it cannot resolve is treated as possibly void, so it is neither a duplicate nor an MDL093. `check` keeps the strict reading. - **`describe` warns about a duplicate output variable across if/else branches and loop bodies** (ako/mxcli#962) — a flow's variable names are unique flow-wide: the same output name in each branch of an if/else, or inside a loop and again after it, is CE0111 in mxbuild 11.13.0 (measured in a microflow). The `-- WARNING: duplicate output variable` header only fired when one assignment could reach the other, so it called both models valid. Calls to void actions are still never counted. - **`check` reports a read of a void action call's output name** (ako/mxcli#962) — `$V = call java action M.Void(…)` followed by anything reading `$V` passed `check` and failed the build with CE0109 "Undefined variable 'V'" (measured on mxbuild 11.13.0). It is now MDL093, for microflows and nanoflows, when the script or, with `-p`, the project says the action returns Void. A name something else defines (a declare, a parameter, a non-void producer) and an action nobody can resolve are not reported. - **`docker check` no longer modifies the project** (ako/mxcli#951) — `mx update-widgets` and `mx check` now run on a temporary copy, for MPR v1 and v2 alike, and what mx prints names the project's own paths. Before, a check rewrote an MPR v1 project's `.mpr` permanently (only v2 was restored from a snapshot), and `mx check` itself rewrote `theme-cache/` and created `deployment/sass/` even with `--no-update-widgets`. The output now says that widget definitions were normalised on a copy, and that a CE0463 the stored project still has is therefore not reported: `--no-update-widgets` checks the project as stored, `mxcli fix widgets` applies the normalisation (ako/mxcli#568, #646). Build output, caches and VCS folders are not copied; the copy goes to `$TMPDIR` and is removed afterwards. diff --git a/cmd/mxcli/lsp.go b/cmd/mxcli/lsp.go index dbfeedfc68..fe16c38550 100644 --- a/cmd/mxcli/lsp.go +++ b/cmd/mxcli/lsp.go @@ -71,6 +71,10 @@ type mdlServer struct { // keeping design-property file I/O out of the per-keystroke path). themeRegistryOnce sync.Once themeRegistry *executor.ThemeRegistry + + // codeActions remembers the project's Java/JavaScript action return types + // between keystrokes, for the flow rules' void-call handling (#962). + codeActions *executor.CodeActionCache } // ensureThemeRegistry loads the project's design-property registry once per @@ -83,9 +87,10 @@ func (s *mdlServer) ensureThemeRegistry() { func newMDLServer(client protocol.Client) *mdlServer { return &mdlServer{ - client: client, - docs: make(map[uri.URI]string), - cache: newLSPCache(), + client: client, + docs: make(map[uri.URI]string), + cache: newLSPCache(), + codeActions: executor.NewCodeActionCache(), } } diff --git a/cmd/mxcli/lsp_diagnostics.go b/cmd/mxcli/lsp_diagnostics.go index 8ad4c37238..9521beccd2 100644 --- a/cmd/mxcli/lsp_diagnostics.go +++ b/cmd/mxcli/lsp_diagnostics.go @@ -100,7 +100,8 @@ func parseMDLDiagnostics(text string) []protocol.Diagnostic { } // documentDiagnostics is what the editor reports for a document as typed: parse -// errors, and when it parses, the checks `mxcli check` runs without a project. +// errors, and when it parses, the checks `mxcli check` runs inline (a flow's +// Java/JavaScript action calls resolved through the workspace project, if any). func (s *mdlServer) documentDiagnostics(docURI uri.URI, text string) []protocol.Diagnostic { text, diags := checkableDocument(docURI, text) diags = append(diags, parseMDLDiagnostics(text)...) @@ -318,6 +319,11 @@ func (s *mdlServer) runSemanticValidation(text string) []protocol.Diagnostic { s.ensureWidgetRegistry() s.ensureThemeRegistry() + // A call to a stored void Java/JavaScript action declares no variable; the + // flow rules can only know that through the project (ako/mxcli#962). + flows := executor.NewFlowRules(prog, s.findMprPath(), s.codeActions) + defer flows.Close() + var diags []protocol.Diagnostic for i, stmt := range prog.Statements { var violations []linter.Violation @@ -331,14 +337,14 @@ func (s *mdlServer) runSemanticValidation(text string) []protocol.Diagnostic { violations = append(violations, executor.ValidateAlterEntity(alterStmt)...) } if mfStmt, ok := stmt.(*ast.CreateMicroflowStmt); ok { - violations = append(violations, executor.ValidateMicroflow(mfStmt)...) + violations = append(violations, flows.Microflow(mfStmt)...) violations = append(violations, executor.ValidateFlowParameterAnnotations( "microflow '"+mfStmt.Name.String()+"'", mfStmt.Parameters)...) } // The editor reports an unusable parameter annotation for the same // reason `check` does — a typo of @position parses and does nothing. if nfStmt, ok := stmt.(*ast.CreateNanoflowStmt); ok { - violations = append(violations, executor.ValidateNanoflow(nfStmt)...) + violations = append(violations, flows.Nanoflow(nfStmt)...) violations = append(violations, executor.ValidateFlowParameterAnnotations( "nanoflow '"+nfStmt.Name.String()+"'", nfStmt.Parameters)...) } diff --git a/cmd/mxcli/lsp_void_calls_test.go b/cmd/mxcli/lsp_void_calls_test.go new file mode 100644 index 0000000000..f0ad74cc1b --- /dev/null +++ b/cmd/mxcli/lsp_void_calls_test.go @@ -0,0 +1,78 @@ +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/executor" + "go.lsp.dev/uri" +) + +func lspRules(t *testing.T, s *mdlServer, text string) string { + t.Helper() + var rules []string + for _, d := range s.documentDiagnostics(uri.File("/w/s.mdl"), text) { + rules = append(rules, fmt.Sprintf("[%v] %s", d.Code, d.Message)) + } + return strings.Join(rules, "\n") +} + +// ako/mxcli#962 item 3: the editor validated flows without the project, so two +// calls to a stored VOID JavaScript action (the Studio Pro shape; mxbuild +// 11.13.0 builds it clean, #953) were squiggled MDL063. With the workspace's +// project it resolves the action; without one, an action it cannot resolve is +// treated as possibly void. Controls: a Boolean action's pair is still +// reported, and a read of a known-void call's output is MDL093. +func TestLSP_VoidCallsResolveThroughTheProject(t *testing.T) { + src := filepath.Join("..", "..", "testdata", "pedapp") + if _, err := os.Stat(filepath.Join(src, "PedApp.mpr")); err != nil { + t.Skipf("PedApp fixture not found: %v", err) + } + dir := t.TempDir() + if err := copyTree(src, dir); err != nil { + t.Fatal(err) + } + withProject := &mdlServer{mprPath: filepath.Join(dir, "PedApp.mpr"), codeActions: executor.NewCodeActionCache()} + noProject := &mdlServer{} + + voidPair := `create nanoflow MyFirstModule.NF_RevokeTwice () +begin + $ReturnValueName = call javascript action FeedbackModule.JS_RevokeUploadedFileFromMemory(fileBlobURL = 'a'); + $ReturnValueName = call javascript action FeedbackModule.JS_RevokeUploadedFileFromMemory(fileBlobURL = 'b'); +end; +` + if got := lspRules(t, withProject, voidPair); strings.Contains(got, "MDL063") { + t.Errorf("with the project: two calls to a stored void action squiggled:\n%s", got) + } + if got := lspRules(t, noProject, voidPair); strings.Contains(got, "MDL063") { + t.Errorf("without a project: an unresolvable pair squiggled, not treated as possibly void:\n%s", got) + } + + boolPair := `create nanoflow MyFirstModule.NF_StrictTwice () +begin + $IsStrict = call javascript action FeedbackModule.JS_isStrictMode(); + $IsStrict = call javascript action FeedbackModule.JS_isStrictMode(); +end; +` + if got := lspRules(t, withProject, boolPair); !strings.Contains(got, "MDL063") { + t.Errorf("control: two calls to a stored Boolean action not reported:\n%s", got) + } + + readVoid := `create nanoflow MyFirstModule.NF_ReadVoid () +begin + $V = call javascript action FeedbackModule.JS_RevokeUploadedFileFromMemory(fileBlobURL = 'a'); + $S = call javascript action FeedbackModule.JS_RevokeUploadedFileFromMemory(fileBlobURL = $V); +end; +` + if got := lspRules(t, withProject, readVoid); !strings.Contains(got, "MDL093") { + t.Errorf("with the project: a read of a void call's output not reported:\n%s", got) + } + if got := lspRules(t, noProject, readVoid); strings.Contains(got, "MDL093") { + t.Errorf("without a project: a possibly-void call's output read reported as MDL093:\n%s", got) + } +} diff --git a/mdl/executor/validate_void_code_calls.go b/mdl/executor/validate_void_code_calls.go index 62e09f4c16..e56f7f823b 100644 --- a/mdl/executor/validate_void_code_calls.go +++ b/mdl/executor/validate_void_code_calls.go @@ -3,8 +3,12 @@ package executor import ( + "sync" + "time" + "github.com/mendixlabs/mxcli/mdl/ast" "github.com/mendixlabs/mxcli/mdl/backend" + "github.com/mendixlabs/mxcli/mdl/linter" "github.com/mendixlabs/mxcli/sdk/microflows" ) @@ -46,6 +50,8 @@ type voidCodeActions struct { // unknownIsVoid makes callIsVoid answer true for an action it cannot // resolve. Set by the editor (NewFlowRules); `check` leaves it false. unknownIsVoid bool + // shared, when set, carries project resolutions across runs (the editor). + shared *CodeActionCache } // voidness is what the resolver knows about one action. @@ -106,6 +112,10 @@ func (r *voidCodeActions) resolve(javaScript bool, qn string) voidness { if v, ok := r.cache[key]; ok { return v } + if v, ok := r.shared.get(key); ok { + r.cache[key] = v + return v + } var got voidness if b := r.project(); b != nil { if javaScript { @@ -121,6 +131,7 @@ func (r *voidCodeActions) resolve(javaScript bool, qn string) voidness { } } r.cache[key] = got + r.shared.put(key, got) return got } @@ -169,3 +180,111 @@ func (r *voidCodeActions) actionIsVoidCall(action any) bool { } return false } + +// FlowRules runs the microflow and nanoflow rule sets (ValidateMicroflow, +// ValidateNanoflow) for the editor, with the Java/JavaScript actions the flows +// call resolved through the script and, when there is one, the project. +// +// The language server validated without a project, so every call to a stored +// void action counted as declaring its output name, and two such calls — the +// ordinary Studio Pro shape — were squiggled as MDL063 (ako/mxcli#962). Here an +// action the script and the project cannot answer for is treated as POSSIBLY +// void (unknownIsVoid): the editor should not refuse what it cannot see, and +// `check`, which runs before exec writes anything, keeps the strict reading. +// The project is opened only when a call needs it; Close releases it. +type FlowRules struct { + voids *voidCodeActions + b backend.FullBackend +} + +// NewFlowRules prepares the flow rules for one program. projectPath may be +// empty: then nothing is read from disk and only the script answers. cache, +// which may be nil, keeps the project's answers between runs. +func NewFlowRules(prog *ast.Program, projectPath string, cache *CodeActionCache) *FlowRules { + f := &FlowRules{} + f.voids = newVoidCodeActions(prog, func() backend.FullBackend { + f.b = openProjectForValidation(projectPath) + return f.b + }) + f.voids.unknownIsVoid = true + if projectPath != "" && cache != nil { + cache.forProject(projectPath) + f.voids.shared = cache + } + return f +} + +// CodeActionCache keeps what a project said about its Java/JavaScript actions +// across FlowRules runs. The editor validates on every keystroke, and reading +// one action from the project costs about 300ms on PedApp — paid again for +// every change of a document that calls one. An entry lives codeActionCacheTTL, +// so an action whose return type changes in Studio Pro is re-read soon after. +type CodeActionCache struct { + mu sync.Mutex + project string + entries map[string]voidness + stamp map[string]time.Time + now func() time.Time +} + +const codeActionCacheTTL = 30 * time.Second + +// NewCodeActionCache returns an empty cache. +func NewCodeActionCache() *CodeActionCache { + return &CodeActionCache{now: time.Now} +} + +// forProject empties the cache when it last served another project. +func (c *CodeActionCache) forProject(path string) { + c.mu.Lock() + defer c.mu.Unlock() + if c.project != path || c.entries == nil { + c.project = path + c.entries = map[string]voidness{} + c.stamp = map[string]time.Time{} + } +} + +func (c *CodeActionCache) get(key string) (voidness, bool) { + if c == nil { + return voidness{}, false + } + c.mu.Lock() + defer c.mu.Unlock() + v, ok := c.entries[key] + if !ok || c.now().Sub(c.stamp[key]) > codeActionCacheTTL { + return voidness{}, false + } + return v, true +} + +func (c *CodeActionCache) put(key string, v voidness) { + if c == nil { + return + } + c.mu.Lock() + defer c.mu.Unlock() + if c.entries == nil { + return + } + c.entries[key] = v + c.stamp[key] = c.now() +} + +// Microflow is ValidateMicroflow with this program's action resolution. +func (f *FlowRules) Microflow(stmt *ast.CreateMicroflowStmt) []linter.Violation { + return validateMicroflowWith(stmt, f.voids) +} + +// Nanoflow is ValidateNanoflow with this program's action resolution. +func (f *FlowRules) Nanoflow(stmt *ast.CreateNanoflowStmt) []linter.Violation { + return validateNanoflowWith(stmt, f.voids) +} + +// Close releases the project, if a call made the rules open it. +func (f *FlowRules) Close() { + if f.b != nil { + _ = f.b.Disconnect() + f.b = nil + } +} diff --git a/mdl/executor/validate_void_code_calls_test.go b/mdl/executor/validate_void_code_calls_test.go index 37a51268e9..35dc708931 100644 --- a/mdl/executor/validate_void_code_calls_test.go +++ b/mdl/executor/validate_void_code_calls_test.go @@ -10,6 +10,7 @@ package executor import ( "strings" "testing" + "time" "github.com/mendixlabs/mxcli/mdl/ast" "github.com/mendixlabs/mxcli/mdl/backend" @@ -246,3 +247,39 @@ func TestDescribeDuplicateWarning_SkipsVoidCalls(t *testing.T) { t.Fatalf("control: non-void duplicate not warned: %v", w) } } + +// The editor validates on every keystroke and reading an action from the +// project costs ~300ms on PedApp, so FlowRules shares resolutions through a +// CodeActionCache: a second run does not open the project, an entry older than +// the TTL is read again, and a different project starts empty (ako/mxcli#962). +func TestCodeActionCache_SharesProjectAnswersAcrossRuns(t *testing.T) { + opens := 0 + b := &mock.MockBackend{ + ReadJavaScriptActionByNameFunc: func(name string) (*types.JavaScriptAction, error) { + return &types.JavaScriptAction{ReturnType: &types.VoidType{}}, nil + }, + } + open := func() backend.FullBackend { opens++; return b } + now := time.Unix(0, 0) + cache := NewCodeActionCache() + cache.now = func() time.Time { return now } + run := func(project string) bool { + cache.forProject(project) + r := newVoidCodeActions(nil, open) + r.shared = cache + return r.isVoid(true, "M.JsVoid") + } + if void := run("a.mpr"); !void || opens != 1 { + t.Fatalf("first run: void=%v opens=%d, want true/1", void, opens) + } + if !run("a.mpr") || opens != 1 { + t.Errorf("second run re-opened the project (opens=%d)", opens) + } + now = now.Add(codeActionCacheTTL + time.Second) + if run("a.mpr"); opens != 2 { + t.Errorf("an expired entry was served (opens=%d, want 2)", opens) + } + if run("b.mpr"); opens != 3 { + t.Errorf("another project was served a's answers (opens=%d, want 3)", opens) + } +} From 25073afea3d99831f84c497ecbd51adf2a32c803 Mon Sep 17 00:00:00 2001 From: Ako Date: Sat, 3 Oct 2026 18:00:28 +0000 Subject: [PATCH 07/15] feat(catalog): record widget parent, depth, appearance and primary action widgets_data gains ParentWidgetId (nearest indexed ancestor, so skipped wrappers, layout grid rows/columns, tab pages and pluggable property / object-list items are transparent), Depth (0 at the page or snippet root; a list view template is a level), Class, Style, DynamicClasses, ActionType (raw $Type of Action, else OnClickAction, else ClickAction) and HasConfirmation (ConfirmationInfo on a microflow/nanoflow/workflow call). Catalog schema 19. Tested on hand-built shapes and on Studio Pro-authored TestApp pages. mendixlabs/mxcli#1268 Co-Authored-By: Claude Opus 5.5 --- docs-site/src/internals/catalog-schema.md | 38 +++- docs-site/src/tools/catalog-tables.md | 32 +++ mdl/catalog/builder_pages.go | 117 ++++++++-- mdl/catalog/builder_pages_tree_test.go | 215 ++++++++++++++++++ .../builder_pages_tree_testapp_test.go | 85 +++++++ mdl/catalog/tables.go | 23 +- 6 files changed, 492 insertions(+), 18 deletions(-) create mode 100644 mdl/catalog/builder_pages_tree_test.go create mode 100644 mdl/catalog/builder_pages_tree_testapp_test.go diff --git a/docs-site/src/internals/catalog-schema.md b/docs-site/src/internals/catalog-schema.md index c55e849c25..63d220615a 100644 --- a/docs-site/src/internals/catalog-schema.md +++ b/docs-site/src/internals/catalog-schema.md @@ -219,15 +219,45 @@ CREATE TABLE activities_data ( ### WIDGETS +One row per widget instance; written by `refresh catalog full` only. + ```sql CREATE TABLE WIDGETS ( - DocumentName TEXT, -- Parent page/snippet - WidgetName TEXT, -- Widget instance name - WidgetType TEXT, -- e.g., "Forms$TextBox", "CustomWidgets$ComboBox" - ModuleName TEXT + Id TEXT PRIMARY KEY, + Name TEXT, + WidgetType TEXT, -- "Forms$TextBox", or a pluggable widget's id + ContainerId TEXT, -- the page or snippet + ContainerQualifiedName TEXT, + ContainerType TEXT, -- "PAGE" or "SNIPPET" + ModuleName TEXT, + Folder TEXT, + EntityRef TEXT, + AttributeRef TEXT, + MicroflowRef TEXT, + NanoflowRef TEXT, + PageRef TEXT, + Description TEXT, + ParentWidgetId TEXT, -- nearest catalogued ancestor; '' at the root + Depth INTEGER, -- catalogued ancestors; 0 at the root + Class TEXT, -- Appearance + Style TEXT, + DynamicClasses TEXT, + ActionType TEXT, -- $Type of Action / OnClickAction / ClickAction + HasConfirmation INTEGER, -- 1 when that action has a ConfirmationInfo + ProjectId TEXT, + SnapshotId TEXT ); ``` +The tree columns (schema 19) skip what the walk does not index: the synthetic +`conditionalVisibilityWidget…` container, layout grid rows and columns, tab +pages, and a pluggable widget's properties and object-list items, so a widget in +a data grid 2 column has the grid as its parent. A list view template is +indexed (it carries its own entity) and so is a level of its own. The walk does +not follow a snippet call; the snippet's widgets are rows of the snippet, depth +0 at its root. `HasConfirmation` can only be 1 for a microflow, nanoflow or +workflow call — `Forms$DeleteClientAction` has no confirmation property. + ### REFS The reference graph: one row per edge. Populated by `refresh catalog full`. diff --git a/docs-site/src/tools/catalog-tables.md b/docs-site/src/tools/catalog-tables.md index 6eb59ed312..516c89671d 100644 --- a/docs-site/src/tools/catalog-tables.md +++ b/docs-site/src/tools/catalog-tables.md @@ -198,6 +198,38 @@ ORDER BY Name; Page **templates** are not pages and are not in this table — see `CATALOG.PAGE_TEMPLATES`. +### CATALOG.WIDGETS + +One row per widget **instance** on a page or snippet. Full build only +(`refresh catalog full`); a fast build leaves the table empty. + +| Column | Description | +|--------|-------------| +| `Id` | The widget's element ID | +| `Name` | Widget name | +| `WidgetType` | Storage type (`Forms$DataView`, `Forms$ActionButton`, …); a pluggable widget's id (`com.mendix.widget.web.datagrid.Datagrid`) | +| `ContainerId`, `ContainerQualifiedName`, `ContainerType` | The page or snippet holding the widget; `ContainerType` is `PAGE` or `SNIPPET` | +| `ModuleName`, `Folder` | The container's module and folder | +| `EntityRef`, `AttributeRef`, `MicroflowRef`, `NanoflowRef`, `PageRef` | What the widget's own content references: datasource entity, bound attribute, action or datasource flow, the page its action opens | +| `ParentWidgetId` | `Id` of the nearest widget **in this table** that encloses it; empty at the page or snippet root. What the catalog skips is transparent: the synthetic `conditionalVisibilityWidget…` container, layout grid rows and columns, tab pages, a pluggable widget's properties and object-list items. A widget in a layout grid column or a data grid 2 column has the grid as its parent | +| `Depth` | Number of ancestors in this table: 0 at the root. A list view **template** is a row of its own, so its widgets are two below the list view. A snippet call is not entered: a snippet's widgets have their own rows, depth 0 at the snippet root | +| `Class`, `Style`, `DynamicClasses` | The widget's Appearance | +| `ActionType` | Stored type of the primary action — `Action` (buttons), else `OnClickAction` (containers), else `ClickAction` (list views, images): `Forms$DeleteClientAction`, `Forms$MicroflowAction`, `Forms$CallNanoflowClientAction`, `Forms$FormAction` (show page), `Forms$NoAction`, …; empty for a widget without one. A pluggable widget's actions are not read | +| `HasConfirmation` | 1 when the primary action asks for confirmation. Only microflow, nanoflow and workflow calls have that setting; a delete action never does | + +```sql +-- Widgets nested more than five deep, deepest first +SELECT ContainerQualifiedName, Name, WidgetType, Depth +FROM CATALOG.WIDGETS +WHERE Depth > 5 +ORDER BY Depth DESC; + +-- Buttons that delete without going through a flow +SELECT ContainerQualifiedName, Name +FROM CATALOG.WIDGETS +WHERE ActionType = 'Forms$DeleteClientAction'; +``` + ### CATALOG.PAGE_TEMPLATES The starting points Studio Pro's "new page" dialog offers (`Forms$PageTemplate`). diff --git a/mdl/catalog/builder_pages.go b/mdl/catalog/builder_pages.go index bc23e38f3f..e478174a3d 100644 --- a/mdl/catalog/builder_pages.go +++ b/mdl/catalog/builder_pages.go @@ -36,8 +36,9 @@ func (b *Builder) buildPages() error { widgetStmt, err = b.tx.Prepare(` INSERT INTO widgets_data (Id, Name, WidgetType, ContainerId, ContainerQualifiedName, ContainerType, ModuleName, Folder, EntityRef, AttributeRef, MicroflowRef, NanoflowRef, PageRef, Description, + ParentWidgetId, Depth, Class, Style, DynamicClasses, ActionType, HasConfirmation, ProjectId, SnapshotId) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) `) if err != nil { return err @@ -117,6 +118,8 @@ func (b *Builder) buildPages() error { w.NanoflowRef, w.PageRef, "", + w.ParentID, w.Depth, w.Class, w.Style, w.DynamicClasses, + w.ActionType, w.HasConfirmation, projectID, snapshotID, ); err != nil { return fmt.Errorf("insert widget %s for page %s: %w", w.Name, qualifiedName, err) @@ -159,8 +162,9 @@ func (b *Builder) buildSnippets() error { widgetStmt, err = b.tx.Prepare(` INSERT INTO widgets_data (Id, Name, WidgetType, ContainerId, ContainerQualifiedName, ContainerType, ModuleName, Folder, EntityRef, AttributeRef, MicroflowRef, NanoflowRef, PageRef, Description, + ParentWidgetId, Depth, Class, Style, DynamicClasses, ActionType, HasConfirmation, ProjectId, SnapshotId) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) `) if err != nil { return err @@ -208,6 +212,8 @@ func (b *Builder) buildSnippets() error { string(sn.ID), qualifiedName, "SNIPPET", moduleName, folder, w.EntityRef, w.AttributeRef, w.MicroflowRef, w.NanoflowRef, w.PageRef, "", + w.ParentID, w.Depth, w.Class, w.Style, w.DynamicClasses, + w.ActionType, w.HasConfirmation, projectID, snapshotID, ); err != nil { return fmt.Errorf("insert widget %s for snippet %s: %w", w.Name, qualifiedName, err) @@ -295,6 +301,73 @@ type rawWidgetInfo struct { MicroflowRef string // action/datasource microflow (Forms$MicroflowSettings.Microflow, …) NanoflowRef string // action/datasource nanoflow PageRef string // action page (Forms$PageSettings.Form) — see scanWidgetOwnRefs + + // Tree position (mendixlabs/mxcli#1268). ParentID is the nearest INDEXED + // ancestor, so a wrapper the walk skips (the synthetic + // conditionalVisibilityWidget DivContainer) and the non-widget holders in + // between (a layout grid's rows and columns, a tab page, a pluggable + // widget's property or object-list item) are transparent: a widget in a + // layout grid column or a data grid 2 column is parented to the grid. + // Depth is the number of indexed ancestors — 0 at the page or snippet root. + // A list view template IS indexed (issue #940), so it is a level of its + // own: list view d, template d+1, the template's widgets d+2. The walk + // does not enter a snippet call, so depth restarts at 0 inside the snippet. + ParentID string + Depth int + + // Appearance (Forms$Appearance — the only home of these since Mendix 8, + // pluggable widgets included). + Class string + Style string + DynamicClasses string + + // ActionType is the stored $Type of the widget's primary action: Action + // (action button, link button), else OnClickAction (containers, layout + // grids), else ClickAction (list views, images). Raw, e.g. + // "Forms$DeleteClientAction", "Forms$MicroflowAction", "Forms$NoAction". + // HasConfirmation reports a ConfirmationInfo on that action; only + // microflow, nanoflow and workflow calls can carry one — a delete action + // has no confirmation setting in the model at all. + ActionType string + HasConfirmation bool +} + +// widgetActionKeys are, in priority order, the keys under which a widget stores +// its primary client action. +var widgetActionKeys = []string{"Action", "OnClickAction", "ClickAction"} + +// widgetPrimaryAction returns the $Type of a widget's primary action and whether +// that action asks for confirmation. A microflow call keeps its confirmation in +// MicroflowSettings.ConfirmationInfo; nanoflow and workflow calls hold +// ConfirmationInfo directly. Unset, the property is stored as null. +func widgetPrimaryAction(w map[string]any) (actionType string, hasConfirmation bool) { + for _, key := range widgetActionKeys { + action, ok := w[key].(map[string]any) + if !ok { + continue + } + actionType = extractString(action["$Type"]) + if actionType == "" { + continue + } + if _, ok := action["ConfirmationInfo"].(map[string]any); ok { + hasConfirmation = true + } else if settings, ok := action["MicroflowSettings"].(map[string]any); ok { + _, hasConfirmation = settings["ConfirmationInfo"].(map[string]any) + } + return actionType, hasConfirmation + } + return "", false +} + +// widgetAppearance reads a widget's Class, Style and DynamicClasses. +func widgetAppearance(w map[string]any) (class, style, dynamicClasses string) { + appearance, ok := w["Appearance"].(map[string]any) + if !ok { + return "", "", "" + } + return extractString(appearance["Class"]), extractString(appearance["Style"]), + extractString(appearance["DynamicClasses"]) } // widgetChildKeys are the keys under which a widget nests *other* widgets. The @@ -453,8 +526,17 @@ func isTransparentDivWrapper(w map[string]any, widgetType string) bool { return len(getBsonArrayElements(appearance["DesignProperties"])) == 0 } -// extractWidgetsRecursive recursively extracts widgets from a widget map. +// extractWidgetsRecursive recursively extracts widgets from a widget map that +// sits at the root of its page or snippet. func extractWidgetsRecursive(w map[string]any) []rawWidgetInfo { + return walkWidget(w, "", 0) +} + +// walkWidget extracts w and every widget nested in it. parentID and depth are +// those w gets if it is indexed — the nearest indexed ancestor and the number +// of indexed ancestors — and are passed through unchanged to its children when +// it is not. +func walkWidget(w map[string]any, parentID string, depth int) []rawWidgetInfo { var result []rawWidgetInfo // Extract this widget's info @@ -481,6 +563,9 @@ func extractWidgetsRecursive(w map[string]any) []rawWidgetInfo { // Extract datasource entity + action microflow/nanoflow references from this // widget's own content (not its child widgets). widget.EntityRef, widget.MicroflowRef, widget.NanoflowRef, widget.PageRef = scanWidgetOwnRefs(w) + widget.Class, widget.Style, widget.DynamicClasses = widgetAppearance(w) + widget.ActionType, widget.HasConfirmation = widgetPrimaryAction(w) + widget.ParentID, widget.Depth = parentID, depth // Index user-authored containers, but skip the synthetic // "conditionalVisibilityWidget*" wrapper that mxcli / Studio Pro insert as a @@ -488,15 +573,17 @@ func extractWidgetsRecursive(w map[string]any) []rawWidgetInfo { // Previously ALL DivContainers were dropped, so real `container` widgets — // which carry Class/Style/DynamicClasses/DesignProperties and OnClick — were // invisible to `show widgets` and untargetable by `update widgets`. + childParent, childDepth := parentID, depth if !isTransparentDivWrapper(w, widget.WidgetType) { result = append(result, widget) + childParent, childDepth = widget.ID, depth+1 } // Recurse into child widgets childWidgets := getBsonArrayElements(w["Widgets"]) for _, child := range childWidgets { if childMap, ok := child.(map[string]any); ok { - result = append(result, extractWidgetsRecursive(childMap)...) + result = append(result, walkWidget(childMap, childParent, childDepth)...) } } @@ -510,7 +597,7 @@ func extractWidgetsRecursive(w map[string]any) []rawWidgetInfo { colWidgets := getBsonArrayElements(colMap["Widgets"]) for _, cw := range colWidgets { if cwMap, ok := cw.(map[string]any); ok { - result = append(result, extractWidgetsRecursive(cwMap)...) + result = append(result, walkWidget(cwMap, childParent, childDepth)...) } } } @@ -530,7 +617,7 @@ func extractWidgetsRecursive(w map[string]any) []rawWidgetInfo { // in active use. Issue #940. for _, tpl := range getBsonArrayElements(w["Templates"]) { if tplMap, ok := tpl.(map[string]any); ok { - result = append(result, extractWidgetsRecursive(tplMap)...) + result = append(result, walkWidget(tplMap, childParent, childDepth)...) } } @@ -538,7 +625,7 @@ func extractWidgetsRecursive(w map[string]any) []rawWidgetInfo { footerWidgets := getBsonArrayElements(w["FooterWidgets"]) for _, fw := range footerWidgets { if fwMap, ok := fw.(map[string]any); ok { - result = append(result, extractWidgetsRecursive(fwMap)...) + result = append(result, walkWidget(fwMap, childParent, childDepth)...) } } @@ -549,7 +636,7 @@ func extractWidgetsRecursive(w map[string]any) []rawWidgetInfo { tpWidgets := getBsonArrayElements(tpMap["Widgets"]) for _, tw := range tpWidgets { if twMap, ok := tw.(map[string]any); ok { - result = append(result, extractWidgetsRecursive(twMap)...) + result = append(result, walkWidget(twMap, childParent, childDepth)...) } } } @@ -557,7 +644,7 @@ func extractWidgetsRecursive(w map[string]any) []rawWidgetInfo { // Handle CustomWidget nested widgets in properties — both kinds of container. if obj, ok := w["Object"].(map[string]any); ok { - result = append(result, widgetsInPropertyBag(obj)...) + result = append(result, widgetsInPropertyBag(obj, childParent, childDepth)...) } // Handle NavigationList items @@ -567,7 +654,7 @@ func extractWidgetsRecursive(w map[string]any) []rawWidgetInfo { itemWidgets := getBsonArrayElements(itemMap["Widgets"]) for _, iw := range itemWidgets { if iwMap, ok := iw.(map[string]any); ok { - result = append(result, extractWidgetsRecursive(iwMap)...) + result = append(result, walkWidget(iwMap, childParent, childDepth)...) } } } @@ -596,7 +683,11 @@ func extractWidgetsRecursive(w map[string]any) []rawWidgetInfo { // An object-list item is itself a property bag, so the walk recurses: a column // holding a nested widget that has its own object list is covered without a // second case. -func widgetsInPropertyBag(bag map[string]any) []rawWidgetInfo { +// +// parentID and depth belong to the pluggable widget's children: a property and +// an object-list item are not widgets, so a widget in a column is parented to +// the grid. +func widgetsInPropertyBag(bag map[string]any, parentID string, depth int) []rawWidgetInfo { var result []rawWidgetInfo for _, prop := range getBsonArrayElements(bag["Properties"]) { propMap, ok := prop.(map[string]any) @@ -609,12 +700,12 @@ func widgetsInPropertyBag(bag map[string]any) []rawWidgetInfo { } for _, pw := range getBsonArrayElements(value["Widgets"]) { if pwMap, ok := pw.(map[string]any); ok { - result = append(result, extractWidgetsRecursive(pwMap)...) + result = append(result, walkWidget(pwMap, parentID, depth)...) } } for _, obj := range getBsonArrayElements(value["Objects"]) { if objMap, ok := obj.(map[string]any); ok { - result = append(result, widgetsInPropertyBag(objMap)...) + result = append(result, widgetsInPropertyBag(objMap, parentID, depth)...) } } } diff --git a/mdl/catalog/builder_pages_tree_test.go b/mdl/catalog/builder_pages_tree_test.go new file mode 100644 index 0000000000..25d080f9dd --- /dev/null +++ b/mdl/catalog/builder_pages_tree_test.go @@ -0,0 +1,215 @@ +// SPDX-License-Identifier: Apache-2.0 + +package catalog + +import "testing" + +// Tests for the widget tree, appearance and action columns +// (mendixlabs/mxcli#1268). The walker used to flatten a page into a list with +// no parent, so a lint rule could not ask how deeply a widget is nested, and it +// read neither the widget's Appearance nor its action. + +func wid(id, name, typ string, extra map[string]any) map[string]any { + m := map[string]any{"$ID": id, "Name": name, "$Type": typ} + for k, v := range extra { + m[k] = v + } + return m +} + +func bsonList(items ...any) []any { return append([]any{int32(3)}, items...) } + +// assertTree checks each named widget's parent (by name, "" for the root) and depth. +func assertTree(t *testing.T, rows []rawWidgetInfo, want map[string]struct { + parent string + depth int +}) { + t.Helper() + nameByID := map[string]string{} + for _, r := range rows { + nameByID[r.ID] = r.Name + } + for name, w := range want { + got := widgetByName(rows, name) + if got == nil { + t.Errorf("widget %q not indexed", name) + continue + } + if p := nameByID[got.ParentID]; p != w.parent || (w.parent == "" && got.ParentID != "") { + t.Errorf("%s: parent = %q (id %q), want %q", name, p, got.ParentID, w.parent) + } + if got.Depth != w.depth { + t.Errorf("%s: depth = %d, want %d", name, got.Depth, w.depth) + } + } +} + +type tp = struct { + parent string + depth int +} + +// A skipped wrapper is transparent: its child is parented to the wrapper's own +// parent at the wrapper's depth, not left dangling on an ID absent from the table. +func TestWidgetTree_ParentThroughSkippedWrapper(t *testing.T) { + page := wid("dv", "dataView1", "Forms$DataView", map[string]any{ + "Widgets": bsonList( + wid("wrap", "conditionalVisibilityWidget1", "Forms$DivContainer", map[string]any{ + "Widgets": bsonList(wid("tb", "textBox1", "Forms$TextBox", nil)), + }), + ), + }) + rows := extractWidgetsRecursive(page) + if widgetByName(rows, "conditionalVisibilityWidget1") != nil { + t.Fatal("synthetic wrapper must stay unindexed") + } + assertTree(t, rows, map[string]tp{ + "dataView1": {"", 0}, + "textBox1": {"dataView1", 1}, + }) +} + +// Layout grid rows and columns, and tab pages, are not widgets: a widget in a +// column is the grid's child, one in a tab page the tab container's. +func TestWidgetTree_LayoutGridAndTabContainer(t *testing.T) { + page := wid("lg", "layoutGrid1", "Forms$LayoutGrid", map[string]any{ + "Rows": bsonList(map[string]any{ + "$ID": "row", "$Type": "Forms$LayoutGridRow", + "Columns": bsonList(map[string]any{ + "$ID": "col", "$Type": "Forms$LayoutGridColumn", + "Widgets": bsonList( + wid("tc", "tabContainer1", "Forms$TabContainer", map[string]any{ + "TabPages": bsonList(map[string]any{ + "$ID": "tp", "$Type": "Forms$TabPage", + "Widgets": bsonList(wid("lbl", "label1", "Forms$Label", nil)), + }), + }), + ), + }), + }), + }) + assertTree(t, extractWidgetsRecursive(page), map[string]tp{ + "layoutGrid1": {"", 0}, + "tabContainer1": {"layoutGrid1", 1}, + "label1": {"tabContainer1", 2}, + }) +} + +// A list view template IS indexed (#940), so it is a level of its own. +func TestWidgetTree_ListViewTemplate(t *testing.T) { + lv := wid("lv", "listView1", "Forms$ListView", map[string]any{ + "Widgets": bsonList(wid("t1", "text1", "Forms$DynamicText", nil)), + "Templates": bsonList(wid("tpl", "", "Forms$ListViewTemplate", map[string]any{ + "Widgets": bsonList(wid("b1", "btnA", "Forms$ActionButton", nil)), + })), + }) + rows := extractWidgetsRecursive(lv) + assertTree(t, rows, map[string]tp{ + "listView1": {"", 0}, + "text1": {"listView1", 1}, + }) + // A template has no name, so it and its child are checked by ID. + for _, r := range rows { + if r.ID == "tpl" && (r.ParentID != "lv" || r.Depth != 1) { + t.Errorf("template: parent %q depth %d, want lv 1", r.ParentID, r.Depth) + } + } + if btn := widgetByName(rows, "btnA"); btn == nil || btn.ParentID != "tpl" || btn.Depth != 2 { + t.Errorf("btnA = %+v, want parent tpl at depth 2", btn) + } +} + +// A widget in a DataGrid2 column (an object-list item) is the grid's child, and +// so is one in a child slot (Value.Widgets). +func TestWidgetTree_PluggablePropertyWidgets(t *testing.T) { + grid := gridWithWidgetInAColumn() + obj := grid["Object"].(map[string]any) + obj["Properties"] = append(obj["Properties"].([]any), map[string]any{ + "TypePointer": "t-empty", + "Value": map[string]any{ + "Widgets": bsonList(wid("slot", "emptyText1", "Forms$Text", nil)), + }, + }) + page := wid("c", "container1", "Forms$DivContainer", map[string]any{"Widgets": bsonList(grid)}) + assertTree(t, extractWidgetsRecursive(page), map[string]tp{ + "container1": {"", 0}, + "grid1": {"container1", 1}, + "pb1": {"grid1", 2}, + "emptyText1": {"grid1", 2}, + }) +} + +// Page and snippet roots are depth 0 with no parent, including the second and +// later root widgets. +func TestWidgetTree_SnippetRoots(t *testing.T) { + rows := extractSnippetWidgets(map[string]any{ + "Widgets": bsonList( + wid("a", "a1", "Forms$Label", nil), + wid("b", "b1", "Forms$DivContainer", map[string]any{ + "Widgets": bsonList(wid("c", "c1", "Forms$Label", nil)), + }), + ), + }) + assertTree(t, rows, map[string]tp{ + "a1": {"", 0}, + "b1": {"", 0}, + "c1": {"b1", 1}, + }) +} + +func TestWidgetAppearance(t *testing.T) { + rows := extractWidgetsRecursive(wid("c", "container1", "Forms$DivContainer", map[string]any{ + "Appearance": map[string]any{ + "$Type": "Forms$Appearance", + "Class": "card mx-2", + "Style": "color: red;", + "DynamicClasses": "if $currentObject/Done then 'done' else ''", + }, + })) + got := rows[0] + if got.Class != "card mx-2" || got.Style != "color: red;" || + got.DynamicClasses != "if $currentObject/Done then 'done' else ''" { + t.Errorf("appearance = %q / %q / %q", got.Class, got.Style, got.DynamicClasses) + } +} + +// The primary action is Action, else OnClickAction, else ClickAction; a +// confirmation lives in MicroflowSettings for a microflow call and directly on +// a nanoflow or workflow call. A delete action has no confirmation property. +func TestWidgetPrimaryAction(t *testing.T) { + confirm := map[string]any{"$Type": "Forms$ConfirmationInfo"} + cases := []struct { + name string + widget map[string]any + wantType string + wantConfirm bool + }{ + {"delete button", map[string]any{"Action": map[string]any{"$Type": "Forms$DeleteClientAction"}}, + "Forms$DeleteClientAction", false}, + {"microflow with confirmation", map[string]any{"Action": map[string]any{ + "$Type": "Forms$MicroflowAction", + "MicroflowSettings": map[string]any{"$Type": "Forms$MicroflowSettings", "ConfirmationInfo": confirm}, + }}, "Forms$MicroflowAction", true}, + {"microflow without confirmation", map[string]any{"Action": map[string]any{ + "$Type": "Forms$MicroflowAction", + "MicroflowSettings": map[string]any{"$Type": "Forms$MicroflowSettings", "ConfirmationInfo": nil}, + }}, "Forms$MicroflowAction", false}, + {"nanoflow with confirmation", map[string]any{"Action": map[string]any{ + "$Type": "Forms$CallNanoflowClientAction", "ConfirmationInfo": confirm, + }}, "Forms$CallNanoflowClientAction", true}, + {"container on-click", map[string]any{"OnClickAction": map[string]any{"$Type": "Forms$FormAction"}}, + "Forms$FormAction", false}, + {"list view click", map[string]any{"ClickAction": map[string]any{"$Type": "Forms$NoAction"}}, + "Forms$NoAction", false}, + {"no action", map[string]any{}, "", false}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + rows := extractWidgetsRecursive(wid("w", "w1", "Forms$ActionButton", c.widget)) + if rows[0].ActionType != c.wantType || rows[0].HasConfirmation != c.wantConfirm { + t.Errorf("got (%q, %v), want (%q, %v)", rows[0].ActionType, rows[0].HasConfirmation, + c.wantType, c.wantConfirm) + } + }) + } +} diff --git a/mdl/catalog/builder_pages_tree_testapp_test.go b/mdl/catalog/builder_pages_tree_testapp_test.go new file mode 100644 index 0000000000..74574a81f6 --- /dev/null +++ b/mdl/catalog/builder_pages_tree_testapp_test.go @@ -0,0 +1,85 @@ +// SPDX-License-Identifier: Apache-2.0 + +package catalog + +import ( + "fmt" + "testing" +) + +// The widget tree, appearance and action columns (mendixlabs/mxcli#1268) on +// Studio Pro-authored pages: ako/TestApp read through the real model reader. +// Each expected value was read off the project. + +// Every parent is a row of the same page or snippet, every child sits one level +// below its parent, and only a root has depth 0 — over all of TestApp's +// widgets, so a holder the walk forgot to make transparent would surface as a +// dangling parent somewhere. +func TestTestAppWidgetTreeIsConsistent(t *testing.T) { + cat := testAppActivities(t) + for q, what := range map[string]string{ + `SELECT count(*) FROM widgets_data c WHERE c.ParentWidgetId != '' AND NOT EXISTS + (SELECT 1 FROM widgets_data p WHERE p.Id = c.ParentWidgetId AND p.ContainerId = c.ContainerId)`: "dangling parents", + `SELECT count(*) FROM widgets_data c JOIN widgets_data p ON p.Id = c.ParentWidgetId + WHERE c.Depth != p.Depth + 1`: "children not one below their parent", + `SELECT count(*) FROM widgets_data WHERE (ParentWidgetId = '') != (Depth = 0)`: "roots not at depth 0", + } { + if n := fmt.Sprint(one(t, cat, q)[0]); n != "0" { + t.Errorf("%s: %s", what, n) + } + } + // CONTROL: the invariants above hold vacuously on an empty or flat table. + if n := fmt.Sprint(one(t, cat, `SELECT count(*) FROM widgets_data WHERE Depth > 0`)[0]); n == "0" { + t.Fatal("no nested widgets in TestApp — the tree was not built") + } +} + +// A button nine levels down, through a layout grid column, two data views, a +// tab container and a data grid 2 column: the column, the row and the tab page +// are not widgets, so the chain names only catalogued widgets. +func TestTestAppWidgetAncestorChain(t *testing.T) { + cat := testAppActivities(t) + res, err := cat.Query(`WITH RECURSIVE chain(id, name, depth, pid) AS ( + SELECT Id, Name, Depth, ParentWidgetId FROM widgets_data + WHERE ContainerQualifiedName = 'WorkflowCommons.ManageTaskAssignments' AND Name = 'actionButton5' + UNION ALL + SELECT w.Id, w.Name, w.Depth, w.ParentWidgetId FROM widgets_data w JOIN chain c ON w.Id = c.pid) + SELECT name, depth FROM chain ORDER BY depth`) + if err != nil { + t.Fatal(err) + } + want := []string{"layoutGrid1", "dataView1", "container1", "dataView2", "container13", + "tabContainer1", "dataGrid23", "container5", "actionButton5"} + var got []string + for i, row := range res.Rows { + got = append(got, fmt.Sprint(row[0])) + if d := fmt.Sprint(row[1]); d != fmt.Sprint(i) { + t.Errorf("%v at depth %s, want %d", row[0], d, i) + } + } + if fmt.Sprint(got) != fmt.Sprint(want) { + t.Errorf("ancestor chain = %v\nwant %v", got, want) + } +} + +func TestTestAppWidgetAppearanceAndAction(t *testing.T) { + cat := testAppActivities(t) + row := one(t, cat, `SELECT Class, Style FROM widgets_data + WHERE ContainerQualifiedName = 'Administration.Account_Edit' AND Name = 'label4'`) + if row[0] != "alert alert-warning" || row[1] != "width:100%;" { + t.Errorf("Account_Edit.label4 class/style = %q / %q", row[0], row[1]) + } + // Mendix's own Account_Overview deletes directly — the case the + // delete-button rule flags — and a delete action has no confirmation. + row = one(t, cat, `SELECT ActionType, HasConfirmation FROM widgets_data + WHERE ContainerQualifiedName = 'Administration.Account_Overview' AND Name = 'actionButton4'`) + if row[0] != "Forms$DeleteClientAction" || fmt.Sprint(row[1]) != "0" { + t.Errorf("Account_Overview.actionButton4 = %v / %v", row[0], row[1]) + } + // A microflow call whose confirmation is set in Studio Pro. + row = one(t, cat, `SELECT ActionType, HasConfirmation FROM widgets_data + WHERE ContainerQualifiedName = 'WorkflowCommons.ManageTaskAssignments' AND Name = 'actionButton4'`) + if row[0] != "Forms$MicroflowAction" || fmt.Sprint(row[1]) != "1" { + t.Errorf("ManageTaskAssignments.actionButton4 = %v / %v", row[0], row[1]) + } +} diff --git a/mdl/catalog/tables.go b/mdl/catalog/tables.go index e89ac53f71..3df7847f22 100644 --- a/mdl/catalog/tables.go +++ b/mdl/catalog/tables.go @@ -7,6 +7,10 @@ package catalog // // History: // +// 19 (widget tree, class/style and actions): widgets_data gains +// ParentWidgetId, Depth, Class, Style, DynamicClasses, ActionType and +// HasConfirmation (mendixlabs/mxcli#1268). Without the bump a cached +// catalog fails every widgets() with "no such column". // 18 — layouts_data.Platform ("Web" / "Native"), the content wrapper's // type. LayoutType cannot tell the platforms apart ("Popup" is native), // and MPR012 needs it to stay off native pages, where CE0582 does not @@ -101,7 +105,7 @@ package catalog // SnapshotSource / SourceId / SourceBranch / SourceRevision columns // from every row (issue #576). // 1 — initial flat schema with denormalized snapshot columns on every row. -const CatalogSchemaVersion = "18" +const CatalogSchemaVersion = "19" // MetaSchemaVersion is the catalog_meta key that records the schema version // the cache was built against. @@ -709,6 +713,22 @@ func (c *Catalog) createTables() error { -- (issue #773). PageRef TEXT, Description TEXT, + -- Tree position (mendixlabs/mxcli#1268): the nearest INDEXED + -- ancestor (skipped wrappers and grid rows/columns are + -- transparent; empty at the root) and the number of indexed + -- ancestors (0 at the page or snippet root; a list view template + -- is a level; a snippet call is not entered). + ParentWidgetId TEXT, + Depth INTEGER DEFAULT 0, + -- Forms$Appearance + Class TEXT, + Style TEXT, + DynamicClasses TEXT, + -- Stored $Type of the primary action (Action, else OnClickAction, + -- else ClickAction) and whether it carries a ConfirmationInfo — + -- only microflow, nanoflow and workflow calls can. + ActionType TEXT, + HasConfirmation INTEGER DEFAULT 0, ProjectId TEXT, SnapshotId TEXT )`, @@ -1420,6 +1440,7 @@ func (c *Catalog) createTables() error { `CREATE INDEX IF NOT EXISTS idx_activities_type ON activities_data(ActivityType)`, `CREATE INDEX IF NOT EXISTS idx_widgets_container ON widgets_data(ContainerId)`, `CREATE INDEX IF NOT EXISTS idx_widgets_type ON widgets_data(WidgetType)`, + `CREATE INDEX IF NOT EXISTS idx_widgets_parent ON widgets_data(ParentWidgetId)`, `CREATE INDEX IF NOT EXISTS idx_widget_defs_kind ON widget_definitions_data(WidgetKind)`, `CREATE INDEX IF NOT EXISTS idx_widget_defs_mdlname ON widget_definitions_data(MdlName)`, `CREATE INDEX IF NOT EXISTS idx_widget_def_props_widget ON widget_definition_properties_data(WidgetId)`, From aafc723497a258c2bfda653dd30ff14d2c287035 Mon Sep 17 00:00:00 2001 From: Ako Date: Sat, 3 Oct 2026 18:00:28 +0000 Subject: [PATCH 08/15] feat(lint): expose widget tree, appearance and action to Starlark widgets() widgets() structs gain parent_widget_id, depth, class_name, style, dynamic_classes, action_type, has_confirmation and page_ref. mendixlabs/mxcli#1268 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + mdl/linter/context.go | 30 ++++++++++-- mdl/linter/rules/legacy_image_widget_test.go | 7 ++- mdl/linter/starlark.go | 10 ++++ mdl/linter/widgets_projection_test.go | 48 ++++++++++++++++++++ 5 files changed, 91 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ac2b20156..a3270bf098 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -132,6 +132,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added +- **The catalog's `widgets` table records the widget tree, appearance and primary action** (mendixlabs/mxcli#1268) — `ParentWidgetId` (the nearest catalogued ancestor; skipped wrappers, layout grid rows and columns, tab pages and data grid 2 columns are transparent), `Depth` (0 at the page or snippet root), `Class`, `Style`, `DynamicClasses`, `ActionType` (the stored type of the button, on-click or click action, e.g. `Forms$DeleteClientAction`) and `HasConfirmation`. Starlark `widgets()` exposes them as `parent_widget_id`, `depth`, `class_name`, `style`, `dynamic_classes`, `action_type`, `has_confirmation`, plus `page_ref`, so a lint rule can flag deep nesting, inline styles, classes outside an allow-list and delete buttons. A delete action has no confirmation setting in Mendix; the enforceable rule is "no button uses the delete action directly". The catalog schema version is bumped, so a cached catalog rebuilds. - **A project records which mxcli wrote its tooling, and an older binary says so** (ako/mxcli#952) — `mxcli init` and every `init --sync-skills` write `.ai-context/mxcli-tooling.json` (version, build time, date; rewritten only when the version changes). Any command that opens the project with `-p` and a binary **older** than the stamp warns once on stderr, naming both versions and how to update; `init --sync-skills` from an older binary **refuses** instead of rolling the skills, rules and CLAUDE.md back. Releases compare by number, nightlies by tag date, a release against a nightly by build date; dev builds are never reported. Binaries from v0.24.0 and earlier cannot read the stamp, so the regenerated `.claude/bootstrap-mxcli.sh` checks it before choosing a binary: an older `mxcli` on PATH is not linked in (it downloads `MXCLI_TAG` instead), an older `./mxcli` is replaced, and the download lands through a temporary file so a `./mxcli` symlink never has it written through into the PATH binary. - **`init --sync-skills` (alias `--sync`) refreshes the bundled lint rules and the mxcli section of CLAUDE.md / AGENTS.md** (ako/mxcli#952) — it used to refresh only the skills, so a project kept the lint rules and guidance of whichever mxcli first initialised it. Bundled rules are recognised by file name; your own rules beside them are never touched. CLAUDE.md and AGENTS.md are now written between `` / `` markers, and only that section is refreshed — by the sync and by a re-run of `mxcli init` — so project notes outside the markers survive. A file written before the markers is left alone by the sync, with a note; run `mxcli init` once to adopt them. - **`mxcli init` and `mxcli new` create `mdlsource/`** (ako/mxcli#952) — the directory the generated CLAUDE.md says scripts live in, with a README. diff --git a/mdl/linter/context.go b/mdl/linter/context.go index f72d76276f..c3f8adc2ac 100644 --- a/mdl/linter/context.go +++ b/mdl/linter/context.go @@ -848,6 +848,17 @@ type Widget struct { AttributeRef string MicroflowRef string // Qualified name of an action/datasource microflow, if any NanoflowRef string // Qualified name of an action/datasource nanoflow, if any + PageRef string // Qualified name of the page the widget's action opens, if any + + // Tree position and appearance (mendixlabs/mxcli#1268); see + // catalog.rawWidgetInfo for the exact semantics. + ParentWidgetID string // nearest catalogued ancestor; "" at the root + Depth int // 0 at the page or snippet root + Class string + Style string + DynamicClasses string + ActionType string // stored $Type of the primary action, e.g. "Forms$DeleteClientAction" + HasConfirmation bool // only microflow / nanoflow / workflow calls can have one } // Widgets returns an iterator over all widgets (excluding system modules). @@ -856,7 +867,9 @@ func (ctx *LintContext) Widgets() iter.Seq[Widget] { rows, err := ctx.db.Query(fmt.Sprintf(` SELECT w.Id, w.Name, w.WidgetType, w.ContainerId, w.ContainerQualifiedName, w.ContainerType, w.ModuleName, w.EntityRef, w.AttributeRef, - w.MicroflowRef, w.NanoflowRef + w.MicroflowRef, w.NanoflowRef, w.PageRef, + w.ParentWidgetId, w.Depth, w.Class, w.Style, w.DynamicClasses, + w.ActionType, w.HasConfirmation FROM widgets w LEFT JOIN modules m ON w.ModuleName = m.Name WHERE %s AND %s @@ -870,9 +883,12 @@ func (ctx *LintContext) Widgets() iter.Seq[Widget] { for rows.Next() { var w Widget - var containerID, containerQName, containerType, entityRef, attrRef, mfRef, nfRef sql.NullString + var containerID, containerQName, containerType, entityRef, attrRef, mfRef, nfRef, pageRef sql.NullString + var parentID, class, style, dynClasses, actionType sql.NullString + var depth, hasConfirmation sql.NullInt64 err := rows.Scan(&w.ID, &w.Name, &w.WidgetType, &containerID, &containerQName, - &containerType, &w.ModuleName, &entityRef, &attrRef, &mfRef, &nfRef) + &containerType, &w.ModuleName, &entityRef, &attrRef, &mfRef, &nfRef, &pageRef, + &parentID, &depth, &class, &style, &dynClasses, &actionType, &hasConfirmation) if err != nil { ctx.recordQueryError("Widgets (row scan)", err) continue @@ -884,6 +900,14 @@ func (ctx *LintContext) Widgets() iter.Seq[Widget] { w.AttributeRef = attrRef.String w.MicroflowRef = mfRef.String w.NanoflowRef = nfRef.String + w.PageRef = pageRef.String + w.ParentWidgetID = parentID.String + w.Depth = int(depth.Int64) + w.Class = class.String + w.Style = style.String + w.DynamicClasses = dynClasses.String + w.ActionType = actionType.String + w.HasConfirmation = hasConfirmation.Int64 != 0 if ctx.IsExcluded(w.ModuleName) { continue diff --git a/mdl/linter/rules/legacy_image_widget_test.go b/mdl/linter/rules/legacy_image_widget_test.go index 3ceb8214ec..18a3cf89fc 100644 --- a/mdl/linter/rules/legacy_image_widget_test.go +++ b/mdl/linter/rules/legacy_image_widget_test.go @@ -110,8 +110,11 @@ func TestLegacyImageWidgetRule_SkipsNativePages(t *testing.T) { ('p1', 'Logboek_Images', 'MyFirstModule.Logboek_Images', 'MyFirstModule', '', '', '', 'Atlas_Core.Atlas_Default', '', 1), ('p2', 'Login_Native', 'MyFirstModule.Login_Native', 'MyFirstModule', '', '', '', 'Atlas_Core.NativePhone_Default', '', 1)`, `CREATE TABLE widgets (Id TEXT, Name TEXT, WidgetType TEXT, ContainerId TEXT, ContainerQualifiedName TEXT, - ContainerType TEXT, ModuleName TEXT, EntityRef TEXT, AttributeRef TEXT, MicroflowRef TEXT, NanoflowRef TEXT)`, - `INSERT INTO widgets VALUES + ContainerType TEXT, ModuleName TEXT, EntityRef TEXT, AttributeRef TEXT, MicroflowRef TEXT, NanoflowRef TEXT, + PageRef TEXT, ParentWidgetId TEXT, Depth INTEGER, Class TEXT, Style TEXT, DynamicClasses TEXT, + ActionType TEXT, HasConfirmation INTEGER)`, + `INSERT INTO widgets (Id, Name, WidgetType, ContainerId, ContainerQualifiedName, ContainerType, + ModuleName, EntityRef, AttributeRef, MicroflowRef, NanoflowRef) VALUES ('w1', 'imgWith', 'Forms$StaticImageViewer', 'p1', 'MyFirstModule.Logboek_Images', 'PAGE', 'MyFirstModule', '', '', '', ''), ('w2', 'imgNative', 'Forms$StaticImageViewer', 'p2', 'MyFirstModule.Login_Native', 'PAGE', 'MyFirstModule', '', '', '', '')`, } { diff --git a/mdl/linter/starlark.go b/mdl/linter/starlark.go index 769aa7242e..fcf5485410 100644 --- a/mdl/linter/starlark.go +++ b/mdl/linter/starlark.go @@ -1050,6 +1050,16 @@ func widgetToStarlark(w Widget) starlark.Value { // they were dropped from the Starlark projection (findings #35). "microflow_ref": starlark.String(w.MicroflowRef), "nanoflow_ref": starlark.String(w.NanoflowRef), + "page_ref": starlark.String(w.PageRef), + // Tree position, appearance and primary action (mendixlabs/mxcli#1268). + // `class` is a Starlark keyword, hence class_name. + "parent_widget_id": starlark.String(w.ParentWidgetID), + "depth": starlark.MakeInt(w.Depth), + "class_name": starlark.String(w.Class), + "style": starlark.String(w.Style), + "dynamic_classes": starlark.String(w.DynamicClasses), + "action_type": starlark.String(w.ActionType), + "has_confirmation": starlark.Bool(w.HasConfirmation), }) } diff --git a/mdl/linter/widgets_projection_test.go b/mdl/linter/widgets_projection_test.go index c56c67eb9e..fdec911eb1 100644 --- a/mdl/linter/widgets_projection_test.go +++ b/mdl/linter/widgets_projection_test.go @@ -59,3 +59,51 @@ func TestWidgets_ProjectsMicroflowNanoflowRef(t *testing.T) { t.Errorf("NanoflowRef = %q, want empty", found.NanoflowRef) } } + +// TestWidgets_TreeAppearanceActionReachStarlark guards mendixlabs/mxcli#1268: +// widgets_data records each widget's parent, depth, appearance and primary +// action, and widgets() must hand every one of them to a Starlark rule under +// its documented snake_case name. +func TestWidgets_TreeAppearanceActionReachStarlark(t *testing.T) { + cat, err := catalog.NewFromFile(filepath.Join(t.TempDir(), "cat.db")) + if err != nil { + t.Fatalf("NewFromFile: %v", err) + } + defer cat.Close() + db := cat.CatalogDB() + if _, err := db.Exec( + `INSERT INTO modules_data (Id, Name, ProjectId, SnapshotId) VALUES (?,?,?,?)`, + "mod-1", "Sales", "default", "s1", + ); err != nil { + t.Fatalf("insert module: %v", err) + } + if _, err := db.Exec( + `INSERT INTO widgets_data + (Id, Name, WidgetType, ContainerId, ContainerQualifiedName, ContainerType, + ModuleName, PageRef, ParentWidgetId, Depth, Class, Style, DynamicClasses, + ActionType, HasConfirmation, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)`, + "w-2", "btnDelete", "Forms$ActionButton", "c-1", "Sales.Order_Overview", "PAGE", + "Sales", "Sales.Order_Edit", "w-1", 3, "btn-danger", "color: red;", "'x'", + "Forms$MicroflowAction", 1, "default", "s1", + ); err != nil { + t.Fatalf("insert widget: %v", err) + } + + vs, _ := runSrc(t, linter.NewLintContext(cat, nil), ` +def check(): + out = [] + for w in widgets(): + out.append(violation(message = "|".join([w.parent_widget_id, str(w.depth), + w.class_name, w.style, w.dynamic_classes, w.action_type, + str(w.has_confirmation), w.page_ref]))) + return out +`) + if len(vs) != 1 { + t.Fatalf("got %d violations, want 1", len(vs)) + } + want := "w-1|3|btn-danger|color: red;|'x'|Forms$MicroflowAction|True|Sales.Order_Edit" + if vs[0].Message != want { + t.Errorf("projection = %q\nwant %q", vs[0].Message, want) + } +} From 335e8ad5e59c4e410fa1b70481084c34bdf318f4 Mon Sep 17 00:00:00 2001 From: Ako Date: Sat, 3 Oct 2026 18:00:28 +0000 Subject: [PATCH 09/15] docs(skill): document the widget struct's new fields and pin action_type The widget table moves to write-lint-rules/catalog-tables.md (SKILL.md was over the 700-line bound) with an example rule for inline styles, a class allow-list and direct delete buttons. The vocabulary test scopes action_type per section (activity vs widget), holds documented widget action types to codec-registered storage names, and pins that only flow calls carry a ConfirmationInfo. mendixlabs/mxcli#1268 Co-Authored-By: Claude Opus 5.5 --- .../skills/mendix/write-lint-rules/SKILL.md | 22 ++---- .../mendix/write-lint-rules/catalog-tables.md | 49 +++++++++++- mdl/catalog/lint_rule_doc_vocabulary_test.go | 74 ++++++++++++++++++- 3 files changed, 127 insertions(+), 18 deletions(-) diff --git a/.claude/skills/mendix/write-lint-rules/SKILL.md b/.claude/skills/mendix/write-lint-rules/SKILL.md index 3b34fc88ac..2fe400361c 100644 --- a/.claude/skills/mendix/write-lint-rules/SKILL.md +++ b/.claude/skills/mendix/write-lint-rules/SKILL.md @@ -160,7 +160,9 @@ def check(): > `CreateChangeAction` and `CommitAction` that appear in `.mpr` documents never > reach a rule. A rule that allow-lists the storage names flags every microflow > that opens a page — the inversion measured at 49% false positives in -> mendixlabs/mxcli#1027. +> mendixlabs/mxcli#1027. The one exception is **`widget.action_type`**, which +> is the raw stored type of a page action (`"Forms$DeleteClientAction"`) — +> page actions have no SDK-name mapping in the catalog. > > To check a value against your own project rather than trusting any list: > @@ -246,20 +248,10 @@ def check(): | `default_value` | string | `"https://example.com"` | | `exposed_to_client` | bool | `true` if constant is exposed to client | -### widget -| Property | Type | Example | -|----------|------|---------| -| `id` | string | Widget UUID | -| `name` | string | Widget name | -| `widget_type` | string | The widget's storage type, e.g. `"Forms$DataView"`, `"Forms$DivContainer"`, `"Forms$ActionButton"`; a pluggable widget's id, e.g. `"com.mendix.widget.web.datagrid.Datagrid"` | -| `container_id` | string | Container UUID | -| `container_qualified_name` | string | `"Sales.Customer_Overview"` | -| `container_type` | string | `"PAGE"` or `"SNIPPET"` | -| `module_name` | string | `"Sales"` | -| `entity_ref` | string | Referenced entity qualified name | -| `attribute_ref` | string | Referenced attribute path | -| `microflow_ref` | string | Action/datasource microflow qualified name (e.g. a microflow-datasource ListView), else `""` | -| `nanoflow_ref` | string | Action/datasource nanoflow qualified name, else `""` | +**widget** — the struct returned by `widgets()` — identity, references, tree +position (`parent_widget_id`, `depth`), appearance (`class_name`, `style`) and +primary action (`action_type`, `has_confirmation`) — is documented in +[catalog-tables.md](catalog-tables.md#widget), with an example rule. ### snippet | Property | Type | Example | diff --git a/.claude/skills/mendix/write-lint-rules/catalog-tables.md b/.claude/skills/mendix/write-lint-rules/catalog-tables.md index 8b50f78549..a9ddbecced 100644 --- a/.claude/skills/mendix/write-lint-rules/catalog-tables.md +++ b/.claude/skills/mendix/write-lint-rules/catalog-tables.md @@ -1,8 +1,8 @@ # Catalog-table builtins: object properties The structs returned by `modules()`, `associations()`, `entity_event_handlers()`, -`navigation_menu_items()`, `jar_dependencies()`, `strings()`, `layouts()` and -`published_rest_operations()`. The functions themselves are listed in +`navigation_menu_items()`, `jar_dependencies()`, `strings()`, `layouts()`, +`published_rest_operations()` and `widgets()`. The functions themselves are listed in [SKILL.md](SKILL.md) under "Available Query Functions". Every builtin leaves out System and Marketplace modules, except `navigation_menu_items()`, whose rows belong to the project rather than to a module. @@ -112,3 +112,48 @@ Returned by `published_rest_operations()`. | `microflow` | string | Qualified name of the microflow that implements the operation | | `deprecated` | bool | Marked deprecated | | `module_name` | string | `"Sales"` | + +### widget +Returned by `widgets()` (full catalog — auto-detected). + +| Property | Type | Example | +|----------|------|---------| +| `id` | string | Widget UUID | +| `name` | string | Widget name | +| `widget_type` | string | The widget's storage type, e.g. `"Forms$DataView"`, `"Forms$DivContainer"`, `"Forms$ActionButton"`; a pluggable widget's id, e.g. `"com.mendix.widget.web.datagrid.Datagrid"` | +| `container_id` | string | Container UUID | +| `container_qualified_name` | string | `"Sales.Customer_Overview"` | +| `container_type` | string | `"PAGE"` or `"SNIPPET"` | +| `module_name` | string | `"Sales"` | +| `entity_ref` | string | Referenced entity qualified name | +| `attribute_ref` | string | Referenced attribute path | +| `microflow_ref` | string | Action/datasource microflow qualified name (e.g. a microflow-datasource ListView), else `""` | +| `nanoflow_ref` | string | Action/datasource nanoflow qualified name, else `""` | +| `page_ref` | string | The page the widget's action opens (show page, create object then open page), else `""` | +| `parent_widget_id` | string | `id` of the nearest catalogued ancestor widget; `""` at the page or snippet root. Wrappers the catalog skips (the synthetic `conditionalVisibilityWidget…` container) and non-widget holders (layout grid rows and columns, tab pages, a pluggable widget's properties and object-list items) are transparent: a widget in a layout grid column or a data grid 2 column has the grid as its parent | +| `depth` | int | Number of catalogued ancestors: `0` at the page or snippet root. A list view **template** is a catalogued row of its own, so a widget inside one is two below the list view. Depth does **not** cross a snippet call: a snippet's widgets start at `0` in the snippet, whichever page calls it | +| `class_name` | string | The widget's `Class` (Appearance), e.g. `"card mx-2"`, else `""`. Named `class_name` because `class` is a Starlark keyword | +| `style` | string | The inline `Style` (Appearance), e.g. `"width:100%;"`, else `""` | +| `dynamic_classes` | string | The `Dynamic classes` expression (Appearance), else `""` | +| `action_type` | string | Stored type of the widget's primary action — the button's action, else a container's on-click action, else a list view's or image's click action: `"Forms$DeleteClientAction"`, `"Forms$MicroflowAction"`, `"Forms$CallNanoflowClientAction"`, `"Forms$FormAction"` (show page), `"Forms$SaveChangesClientAction"`, `"Forms$CancelChangesClientAction"`, `"Forms$ClosePageClientAction"`, `"Forms$CreateObjectClientAction"`, `"Forms$OpenLinkClientAction"`, `"Forms$NoAction"`; `""` for a widget with no action property. Pluggable widgets' actions (inside their property bag) are not read | +| `has_confirmation` | bool | The primary action asks for confirmation. Only a microflow, nanoflow or workflow call can; a delete action (`"Forms$DeleteClientAction"`) has no confirmation setting at all, so "a delete must confirm" is enforced as "no button uses the delete action directly — call a microflow or nanoflow with a confirmation" | + +```python +# Inline styles, classes outside an allow-list, and direct delete buttons. +ALLOWED = ["btn-primary", "card", "mx-2"] +def check(): + out = [] + for w in widgets(): + loc = location(module=w.module_name, document_type=w.container_type.lower(), + document_name=w.container_qualified_name.split(".")[-1], + document_id=w.container_id) + if w.style != "": + out.append(violation(message="inline style on " + w.name, location=loc)) + for c in w.class_name.split(" "): + if c != "" and c not in ALLOWED: + out.append(violation(message="class '" + c + "' not allowed", location=loc)) + if w.action_type == "Forms$DeleteClientAction": + out.append(violation(message=w.name + " deletes directly; call a microflow with a confirmation", + location=loc)) + return out +``` diff --git a/mdl/catalog/lint_rule_doc_vocabulary_test.go b/mdl/catalog/lint_rule_doc_vocabulary_test.go index f07c6948f0..963b643670 100644 --- a/mdl/catalog/lint_rule_doc_vocabulary_test.go +++ b/mdl/catalog/lint_rule_doc_vocabulary_test.go @@ -8,11 +8,14 @@ import ( "go/token" "os" "path/filepath" + "reflect" "regexp" "slices" "strings" "testing" + "github.com/mendixlabs/mxcli/modelsdk/codec" + "github.com/mendixlabs/mxcli/modelsdk/gen/pages" "github.com/mendixlabs/mxcli/sdk/domainmodel" "github.com/mendixlabs/mxcli/sdk/microflows" ) @@ -82,6 +85,23 @@ func docRowValues(t *testing.T, doc, field string) []string { return out } +// docSectionRowValues is docRowValues restricted to one "###
" of the +// doc, for a field name two structs share: widget.action_type and +// activity.action_type hold different vocabularies, and the first row in the +// file would otherwise answer for both. +func docSectionRowValues(t *testing.T, doc, section, field string) []string { + t.Helper() + start := strings.Index(doc, "\n### "+section+"\n") + if start < 0 { + t.Fatalf("no \"### %s\" section in write-lint-rules — renamed or removed, which silently disables this check", section) + } + body := doc[start+1:] + if end := strings.Index(body[4:], "\n### "); end >= 0 { + body = body[:end+4] + } + return docRowValues(t, body, field) +} + func assertDocumentedValuesExist(t *testing.T, field string, documented []string, real map[string]bool, produced string) { t.Helper() for _, v := range documented { @@ -141,7 +161,7 @@ func microflowActionLabels(t *testing.T) map[string]bool { func TestSkillDocumentsRealActionTypes(t *testing.T) { doc := lintRuleSkillDoc(t) assertDocumentedValuesExist(t, "action_type", - docRowValues(t, doc, "action_type"), + docSectionRowValues(t, doc, "activity", "action_type"), microflowActionLabels(t), "getMicroflowActionType") } @@ -407,3 +427,55 @@ func TestSkillDocumentsRealActivityPropertyVocabulary(t *testing.T) { } } } + +// widget.action_type is the RAW stored $Type of a widget's primary action +// (mendixlabs/mxcli#1268). The issue's own example compared it to +// "DeleteAction", which no widget stores — the storage name is +// "Forms$DeleteClientAction" — so a rule written from memory matched nothing +// and passed. Every documented value must be a client-action storage name the +// codec knows. +func TestSkillDocumentsRealWidgetActionTypes(t *testing.T) { + documented := docSectionRowValues(t, lintRuleSkillDoc(t), "widget", "action_type") + real := map[string]bool{"": true} // a widget with no action property + for _, v := range documented { + if _, ok := codec.DefaultRegistry.Lookup(v); ok && strings.HasPrefix(v, "Forms$") && strings.HasSuffix(v, "Action") { + real[v] = true + } + } + // CONTROL: the lookup must accept the stored name and refuse the + // qualified-looking one, or the set above proves nothing. + if _, ok := codec.DefaultRegistry.Lookup("Forms$DeleteClientAction"); !ok { + t.Fatal("codec registry does not know Forms$DeleteClientAction — the lookup is broken") + } + if _, ok := codec.DefaultRegistry.Lookup("DeleteAction"); ok { + t.Fatal("codec registry accepts the bare \"DeleteAction\" — the lookup is not a vocabulary") + } + assertDocumentedValuesExist(t, "widget.action_type", documented, real, "widgetPrimaryAction") + if !slices.Contains(documented, "Forms$DeleteClientAction") { + t.Error("widget.action_type does not document Forms$DeleteClientAction, the value the delete-button rule needs") + } +} + +// The skill tells rule authors that a delete action has no confirmation, so +// has_confirmation is always false for one, and that only microflow, nanoflow +// and workflow calls carry one. That is a fact about the metamodel, pinned here +// so that a Mendix version adding one to the delete action fails this test +// instead of leaving the documented rule quietly wrong. +func TestWidgetConfirmationOnlyOnFlowCalls(t *testing.T) { + has := func(v any) bool { + _, ok := reflect.TypeOf(v).MethodByName("ConfirmationInfo") + return ok + } + if has(&pages.DeleteClientAction{}) { + t.Error("Forms$DeleteClientAction now has a ConfirmationInfo — widgetPrimaryAction and the skill's delete rule need revisiting") + } + for name, v := range map[string]any{ + "MicroflowSettings": &pages.MicroflowSettings{}, + "CallNanoflowClientAction": &pages.CallNanoflowClientAction{}, + "CallWorkflowClientAction": &pages.CallWorkflowClientAction{}, + } { + if !has(v) { + t.Errorf("%s has no ConfirmationInfo — widgetPrimaryAction reads it there", name) + } + } +} From 00649d9095833c136ec022f4c3b910f48a19ccd6 Mon Sep 17 00:00:00 2001 From: Ako Date: Sat, 3 Oct 2026 18:02:09 +0000 Subject: [PATCH 10/15] fix(catalog): the widget-tree change takes schema version 20 #963's commit refs and #1268's widget columns both bumped 18 -> 19 on parallel branches. A cache built at 19 by either alone would never rebuild for the other, the 15/16 collision again. Co-Authored-By: Claude Opus 5.5 --- mdl/catalog/tables.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/mdl/catalog/tables.go b/mdl/catalog/tables.go index 77c80cb72b..b90c0fb79c 100644 --- a/mdl/catalog/tables.go +++ b/mdl/catalog/tables.go @@ -7,10 +7,12 @@ package catalog // // History: // -// 19 (widget tree, class/style and actions): widgets_data gains +// 20 (widget tree, class/style and actions): widgets_data gains // ParentWidgetId, Depth, Class, Style, DynamicClasses, ActionType and // HasConfirmation (mendixlabs/mxcli#1268). Without the bump a cached -// catalog fails every widgets() with "no such column". +// catalog fails every widgets() with "no such column". Its own number: +// the commit-refs change below took 19 in parallel, and a cache built at +// 19 by either alone would never rebuild for the other (as 15/16 did). // 19 (commit refs): refs gains RefKind "commit" (FLOW -> ENTITY) for a // commit action and a create/change that commits (ako/mxcli#963). No // column changes, but a cached catalog would keep answering "no flow @@ -109,7 +111,7 @@ package catalog // SnapshotSource / SourceId / SourceBranch / SourceRevision columns // from every row (issue #576). // 1 — initial flat schema with denormalized snapshot columns on every row. -const CatalogSchemaVersion = "19" +const CatalogSchemaVersion = "20" // MetaSchemaVersion is the catalog_meta key that records the schema version // the cache was built against. From cb9ab804399a3ee6cd1cac8f413e20b924d08a80 Mon Sep 17 00:00:00 2001 From: Ako Date: Sat, 3 Oct 2026 18:06:52 +0000 Subject: [PATCH 11/15] fix(tui,eval): run mx check on a temporary copy of the project The TUI checker (after every change) and the eval runner's mx_check ran a plain `mx check `, which writes theme-cache/web/ and deployment/sass/ into the project. Measured with mx 11.14 on a v1 and a v2 copy of the testapp: the model is left alone, those two folders are added. New docker.MxCheckOnCopy runs `mx check` on copyProjectToTemp's copy (the #956 helper, renamed now that it is not check-only) with output paths rewritten to the project's, and applies PrepareMxCommand, which these two callers lacked. mxCheckCmd takes extra args for the TUI's -j/-w/-d. Part of ako/mxcli#961. Co-Authored-By: Claude Opus 5.5 --- cmd/mxcli/docker/check.go | 39 +++++- cmd/mxcli/docker/check_copy.go | 26 ++-- cmd/mxcli/docker/check_integration_test.go | 45 +++++++ cmd/mxcli/docker/check_readonly_test.go | 10 +- cmd/mxcli/evalrunner/checks.go | 11 +- cmd/mxcli/evalrunner/checks_readonly_test.go | 111 ++++++++++++++++ cmd/mxcli/tui/checker.go | 10 +- cmd/mxcli/tui/checker_readonly_test.go | 125 +++++++++++++++++++ docs-site/src/tools/docker-check.md | 2 +- 9 files changed, 347 insertions(+), 32 deletions(-) create mode 100644 cmd/mxcli/evalrunner/checks_readonly_test.go create mode 100644 cmd/mxcli/tui/checker_readonly_test.go diff --git a/cmd/mxcli/docker/check.go b/cmd/mxcli/docker/check.go index 0167520123..968de23c46 100644 --- a/cmd/mxcli/docker/check.go +++ b/cmd/mxcli/docker/check.go @@ -61,8 +61,8 @@ func copyFile(src, dst string) error { // that behave like the real tools without needing mx. var resolveMxForCheck = ResolveMxForVersion -var mxCheckCmd = func(mxPath, mprPath string, w, stderr io.Writer) error { - cmd := exec.Command(mxPath, "check", mprPath) +var mxCheckCmd = func(mxPath, mprPath string, args []string, w, stderr io.Writer) error { + cmd := exec.Command(mxPath, append([]string{"check", mprPath}, args...)...) cmd.Stdout = w cmd.Stderr = stderr PrepareMxCommand(cmd) @@ -104,7 +104,7 @@ func Check(opts CheckOptions) error { if abs, err := filepath.Abs(projectPath); err == nil { projectPath = abs } - workMpr, cleanup, err := copyProjectForCheck(projectPath) + workMpr, cleanup, err := copyProjectToTemp(projectPath) if err != nil { return fmt.Errorf("copy the project to a temporary directory for checking: %w\n"+ " mx writes into the project it checks, so docker check never runs it on the original;\n"+ @@ -130,7 +130,7 @@ func Check(opts CheckOptions) error { // Run mx check fmt.Fprintf(w, "Checking project %s...\n", opts.ProjectPath) - checkErr := mxCheckCmd(mxPath, workMpr, out, errOut) + checkErr := mxCheckCmd(mxPath, workMpr, nil, out, errOut) out.Flush() errOut.Flush() @@ -150,6 +150,37 @@ func Check(opts CheckOptions) error { return nil } +// MxCheckOnCopy runs `mx check args...` on a temporary copy of the +// project, so the check cannot write into it, and returns mx's error (a non-zero +// exit when the project has errors). Output naming the copy is rewritten to name +// the project. It does not run update-widgets: it checks the project as stored. +// +// Every caller that runs a plain `mx check` should go through this: mx check +// writes theme-cache/ and deployment/sass/ into the project it is given — the TUI +// checker and the eval runner each did, on every run (ako/mxcli#961). +func MxCheckOnCopy(mxPath, mprPath string, args []string, stdout, stderr io.Writer) error { + if stdout == nil { + stdout = io.Discard + } + if stderr == nil { + stderr = io.Discard + } + projectPath, err := filepath.Abs(mprPath) + if err != nil { + return err + } + workMpr, cleanup, err := copyProjectToTemp(projectPath) + if err != nil { + return fmt.Errorf("copy the project to a temporary directory for mx check: %w", err) + } + defer cleanup() + out := newPathRewriter(stdout, filepath.Dir(workMpr), filepath.Dir(projectPath)) + errOut := newPathRewriter(stderr, filepath.Dir(workMpr), filepath.Dir(projectPath)) + defer out.Flush() + defer errOut.Flush() + return mxCheckCmd(mxPath, workMpr, args, out, errOut) +} + // mxBinaryName returns the platform-specific mx binary name. func mxBinaryName() string { if runtime.GOOS == "windows" { diff --git a/cmd/mxcli/docker/check_copy.go b/cmd/mxcli/docker/check_copy.go index 4c9106749a..b76d70c809 100644 --- a/cmd/mxcli/docker/check_copy.go +++ b/cmd/mxcli/docker/check_copy.go @@ -13,20 +13,21 @@ import ( "sync" ) -// copyProjectForCheck copies the project that holds mprPath into a fresh -// temporary directory, so `docker check` can run `mx update-widgets` and -// `mx check` there instead of on the user's project (ako/mxcli#951). +// copyProjectToTemp copies the project that holds mprPath into a fresh +// temporary directory, so mx can be run there instead of on the user's project +// (ako/mxcli#951, #961). // -// Both tools write into the project they are given: update-widgets rewrites the -// model (and converts MPRv2 to MPRv1), and mx check compiles the theme into -// theme-cache/ and writes deployment/sass/. A snapshot/restore of the v2 storage -// covered only the first half of that, and only on v2 — an MPRv1 project's .mpr -// was rewritten permanently by a "check". +// Every mx tool writes into the project it is given: update-widgets rewrites +// the model (and converts MPRv2 to MPRv1), mx check compiles the theme into +// theme-cache/ and writes deployment/sass/, and MxBuild regenerates javasource/ +// proxies, the .launch/.classpath/.project files and all of deployment/. A +// snapshot/restore of the v2 storage covered only the model, and only on v2 — an +// MPRv1 project's .mpr was rewritten permanently by a "check" or a "build". // // Only what mx reads is copied: build output, caches and VCS metadata are skipped // (copyProjectTree), so the cost is the model plus the widget, theme and source // folders. cleanup removes the copy; it is never nil and safe to defer. -func copyProjectForCheck(mprPath string) (workMpr string, cleanup func(), err error) { +func copyProjectToTemp(mprPath string) (workMpr string, cleanup func(), err error) { cleanup = func() {} abs, err := filepath.Abs(mprPath) if err != nil { @@ -37,7 +38,7 @@ func copyProjectForCheck(mprPath string) (workMpr string, cleanup func(), err er } else if info.IsDir() { return "", cleanup, fmt.Errorf("%s is a directory, not a project file", abs) } - tmp, err := os.MkdirTemp("", "mxcli-check-*") + tmp, err := os.MkdirTemp("", "mxcli-copy-*") if err != nil { return "", cleanup, err } @@ -52,8 +53,9 @@ func copyProjectForCheck(mprPath string) (workMpr string, cleanup func(), err er return filepath.Join(dst, filepath.Base(abs)), cleanup, nil } -// checkCopySkipRoot are top-level project folders mx check neither needs nor -// should see: build output and caches it regenerates. +// checkCopySkipRoot are top-level project folders mx neither needs nor should +// see: build output and caches it regenerates. .docker/ holds `docker build`'s +// own output directory, which MxBuild writes to by absolute path. var checkCopySkipRoot = map[string]bool{ "deployment": true, // MxBuild / mx check output "releases": true, // exported .mda packages diff --git a/cmd/mxcli/docker/check_integration_test.go b/cmd/mxcli/docker/check_integration_test.go index fcfe2bf82e..3a668b61fe 100644 --- a/cmd/mxcli/docker/check_integration_test.go +++ b/cmd/mxcli/docker/check_integration_test.go @@ -135,3 +135,48 @@ func TestCheck_LeavesProjectUntouched(t *testing.T) { } } } + +// TestMxCheckOnCopy_LeavesProjectUntouched: the plain `mx check` the TUI +// checker and the eval runner run (ako/mxcli#961) leaves an MPRv1 and an MPRv2 +// project byte-identical with the same mtimes, and still reports what mx found. +// Measured before the fix with mx 11.14: theme-cache/web/ and deployment/sass/ +// were added to the project in both formats. +func TestMxCheckOnCopy_LeavesProjectUntouched(t *testing.T) { + mxPath, err := ResolveMx("") + if err != nil { + t.Skipf("mx not resolvable: %v", err) + } + for _, format := range []string{"v2", "v1"} { + t.Run(format, func(t *testing.T) { + dir, err := os.MkdirTemp("", "mxc") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { os.RemoveAll(dir) }) + cmd := exec.Command(mxPath, "create-project") + cmd.Dir = dir + PrepareMxCommand(cmd) + if out, err := cmd.CombinedOutput(); err != nil { + t.Skipf("mx create-project failed: %v\n%s", err, out) + } + mprPath := filepath.Join(dir, "App.mpr") + if format == "v1" { + if err := updateWidgetsCmd(mxPath, mprPath, io.Discard, io.Discard); err != nil { + t.Skipf("could not produce a v1 fixture: %v", err) + } + } + jsonPath := filepath.Join(t.TempDir(), "check.json") + before := treeState(t, dir) + var out bytes.Buffer + if err := MxCheckOnCopy(mxPath, mprPath, []string{"-j", jsonPath}, &out, &out); err != nil { + t.Fatalf("MxCheckOnCopy: %v\n%s", err, out.String()) + } + if d := diffStates(before, treeState(t, dir)); len(d) > 0 { + t.Errorf("mx check modified the project:\n %v", d) + } + if _, err := os.Stat(jsonPath); err != nil { + t.Errorf("mx check wrote no JSON result: %v", err) + } + }) + } +} diff --git a/cmd/mxcli/docker/check_readonly_test.go b/cmd/mxcli/docker/check_readonly_test.go index edf4a717df..14d92f9fed 100644 --- a/cmd/mxcli/docker/check_readonly_test.go +++ b/cmd/mxcli/docker/check_readonly_test.go @@ -114,7 +114,7 @@ func stubTools(t *testing.T) (seen *[]string) { } return os.RemoveAll(filepath.Join(dir, "mprcontents")) } - mxCheckCmd = func(_, mprPath string, w, _ io.Writer) error { + mxCheckCmd = func(_, mprPath string, _ []string, w, _ io.Writer) error { paths = append(paths, "check "+mprPath) dir := filepath.Dir(mprPath) // Compiles the theme and writes the sass entry point. @@ -251,16 +251,16 @@ func TestCopyProjectForCheck_TempDirInsideProject(t *testing.T) { t.Fatal(err) } t.Setenv("TMPDIR", tmp) - work, cleanup, err := copyProjectForCheck(mpr) + work, cleanup, err := copyProjectToTemp(mpr) if err != nil { - t.Fatalf("copyProjectForCheck: %v", err) + t.Fatalf("copyProjectToTemp: %v", err) } defer cleanup() if b, err := os.ReadFile(work); err != nil || string(b) != "model" { t.Fatalf("copy of the model = %q, %v", b, err) } filepath.WalkDir(filepath.Dir(work), func(p string, d fs.DirEntry, err error) error { - if err == nil && strings.HasPrefix(d.Name(), "mxcli-check-") { + if err == nil && strings.HasPrefix(d.Name(), "mxcli-copy-") { t.Errorf("the copy contains a copy of itself: %s", p) return filepath.SkipDir } @@ -269,7 +269,7 @@ func TestCopyProjectForCheck_TempDirInsideProject(t *testing.T) { } func TestCopyProjectForCheck_MissingProject(t *testing.T) { - if _, _, err := copyProjectForCheck(filepath.Join(t.TempDir(), "nope.mpr")); err == nil { + if _, _, err := copyProjectToTemp(filepath.Join(t.TempDir(), "nope.mpr")); err == nil { t.Error("want an error for a project file that does not exist") } } diff --git a/cmd/mxcli/evalrunner/checks.go b/cmd/mxcli/evalrunner/checks.go index 3aa2dd92a0..9e309f619d 100644 --- a/cmd/mxcli/evalrunner/checks.go +++ b/cmd/mxcli/evalrunner/checks.go @@ -8,6 +8,8 @@ import ( "os/exec" "path/filepath" "strings" + + "github.com/mendixlabs/mxcli/cmd/mxcli/docker" ) // CheckOptions configures how checks are executed. @@ -246,13 +248,10 @@ func checkMxCheck(check Check, opts CheckOptions) CheckResult { return CheckResult{Check: check, Passed: false, Detail: "mx binary not found"} } - cmd := exec.Command(mxPath, "check", opts.ProjectPath) + // On a temporary copy: mx check writes theme-cache/ and deployment/sass/ + // into the project it checks (ako/mxcli#961). var stdout, stderr bytes.Buffer - cmd.Stdout = &stdout - cmd.Stderr = &stderr - - err := cmd.Run() - if err != nil { + if err := docker.MxCheckOnCopy(mxPath, opts.ProjectPath, nil, &stdout, &stderr); err != nil { // Parse error output for error count output := stdout.String() + stderr.String() return CheckResult{Check: check, Passed: false, Detail: fmt.Sprintf("mx check failed: %s", firstLine(output))} diff --git a/cmd/mxcli/evalrunner/checks_readonly_test.go b/cmd/mxcli/evalrunner/checks_readonly_test.go new file mode 100644 index 0000000000..3edd876fc0 --- /dev/null +++ b/cmd/mxcli/evalrunner/checks_readonly_test.go @@ -0,0 +1,111 @@ +// SPDX-License-Identifier: Apache-2.0 + +package evalrunner + +import ( + "crypto/sha256" + "encoding/hex" + "fmt" + "io/fs" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + "time" +) + +// stubMx does to the project it checks what the real `mx check` does — writes +// theme-cache/ and deployment/sass/ — and reports the path it was given. +const stubMx = `#!/bin/sh +[ "$1" = check ] || exit 2 +dir=$(dirname "$2") +mkdir -p "$dir/theme-cache/web" "$dir/deployment/sass" +echo recompiled > "$dir/theme-cache/web/theme.compiled.css" +echo x > "$dir/deployment/sass/main.scss" +echo "Loading $2" +[ -z "$MX_STUB_FAIL" ] || exit 1 +` + +func treeHashes(t *testing.T, root string) map[string]string { + t.Helper() + m := map[string]string{} + filepath.WalkDir(root, func(p string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + rel, _ := filepath.Rel(root, p) + if d.IsDir() { + m[rel+"/"] = "dir" + return nil + } + b, _ := os.ReadFile(p) + info, _ := d.Info() + sum := sha256.Sum256(b) + m[rel] = hex.EncodeToString(sum[:]) + " " + info.ModTime().UTC().Format(time.RFC3339Nano) + return nil + }) + return m +} + +// TestCheckMxCheck_DoesNotModifyProject: the eval runner's mx-check check must +// not write theme-cache/ or deployment/ into the project it grades +// (ako/mxcli#961), and still reports pass/fail and the project's own path. +func TestCheckMxCheck_DoesNotModifyProject(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("stub mx is a shell script") + } + mx := filepath.Join(t.TempDir(), "mx") + if err := os.WriteFile(mx, []byte(stubMx), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("TMPDIR", t.TempDir()) + for _, tc := range []struct { + format string + fail bool + }{{"v1", false}, {"v2", false}, {"v1", true}, {"v2", true}} { + t.Run(fmt.Sprintf("%s/fail=%v", tc.format, tc.fail), func(t *testing.T) { + if tc.fail { + t.Setenv("MX_STUB_FAIL", "1") + } + proj := t.TempDir() + files := []string{"App.mpr", "theme/web/main.scss"} + if tc.format == "v2" { + files = append(files, "mprcontents/ab/cd/abcd.mxunit") + } + for _, f := range files { + p := filepath.Join(proj, f) + os.MkdirAll(filepath.Dir(p), 0o755) + if err := os.WriteFile(p, []byte(f), 0o644); err != nil { + t.Fatal(err) + } + } + mpr := filepath.Join(proj, "App.mpr") + before := treeHashes(t, proj) + + res := checkMxCheck(Check{}, CheckOptions{ProjectPath: mpr, MxPath: mx}) + if res.Passed == tc.fail { + t.Errorf("Passed = %v, want %v (%s)", res.Passed, !tc.fail, res.Detail) + } + if tc.fail && !strings.Contains(res.Detail, "Loading "+mpr) { + t.Errorf("detail does not name the project: %q", res.Detail) + } + + after := treeHashes(t, proj) + var diff []string + for k, v := range after { + if before[k] != v { + diff = append(diff, k) + } + } + for k := range before { + if _, ok := after[k]; !ok { + diff = append(diff, "removed "+k) + } + } + if len(diff) > 0 { + t.Errorf("the eval runner's mx check modified the project: %s", strings.Join(diff, ", ")) + } + }) + } +} diff --git a/cmd/mxcli/tui/checker.go b/cmd/mxcli/tui/checker.go index 4c4024fc24..59d892e9fe 100644 --- a/cmd/mxcli/tui/checker.go +++ b/cmd/mxcli/tui/checker.go @@ -3,8 +3,8 @@ package tui import ( "encoding/json" "fmt" + "io" "os" - "os/exec" "strings" tea "github.com/charmbracelet/bubbletea" @@ -149,9 +149,11 @@ func runMxCheck(projectPath string) tea.Cmd { jsonFile.Close() defer os.Remove(jsonPath) - Trace("checker: running %s check %s -j %s -w -d", mxPath, projectPath, jsonPath) - cmd := exec.Command(mxPath, "check", projectPath, "-j", jsonPath, "-w", "-d") - _, runErr := cmd.CombinedOutput() + // On a temporary copy: mx check writes theme-cache/ and + // deployment/sass/ into the project it checks, and this runs after + // every change (ako/mxcli#961). + Trace("checker: running %s check (on a copy of) %s -j %s -w -d", mxPath, projectPath, jsonPath) + runErr := docker.MxCheckOnCopy(mxPath, projectPath, []string{"-j", jsonPath, "-w", "-d"}, io.Discard, io.Discard) checkErrors, parseErr := parseCheckJSON(jsonPath) if parseErr != nil { diff --git a/cmd/mxcli/tui/checker_readonly_test.go b/cmd/mxcli/tui/checker_readonly_test.go new file mode 100644 index 0000000000..179d593c1c --- /dev/null +++ b/cmd/mxcli/tui/checker_readonly_test.go @@ -0,0 +1,125 @@ +// SPDX-License-Identifier: Apache-2.0 + +package tui + +import ( + "crypto/sha256" + "encoding/hex" + "io/fs" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + "time" + + tea "github.com/charmbracelet/bubbletea" +) + +// stubMxScript is an `mx` that does to the project it checks what the real one +// does — it writes theme-cache/ and deployment/sass/ — and writes a JSON result +// with one error to the -j file. +const stubMxScript = `#!/bin/sh +[ "$1" = check ] || exit 2 +dir=$(dirname "$2") +mkdir -p "$dir/theme-cache/web" "$dir/deployment/sass" +echo recompiled > "$dir/theme-cache/web/theme.compiled.css" +echo x > "$dir/deployment/sass/main.scss" +shift 2 +while [ $# -gt 0 ]; do + if [ "$1" = -j ]; then + echo '{"errors":[{"code":"CE0001","message":"stub error","module-name":"M","document-name":"Page '"'"'P'"'"'"}]}' > "$2" + shift + fi + shift +done +exit 1 +` + +// treeHashes records each file's hash and mtime under root. +func treeHashes(t *testing.T, root string) map[string]string { + t.Helper() + m := map[string]string{} + filepath.WalkDir(root, func(p string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + rel, _ := filepath.Rel(root, p) + if d.IsDir() { + m[rel+"/"] = "dir" + return nil + } + b, _ := os.ReadFile(p) + info, _ := d.Info() + sum := sha256.Sum256(b) + m[rel] = hex.EncodeToString(sum[:]) + " " + info.ModTime().UTC().Format(time.RFC3339Nano) + return nil + }) + return m +} + +// TestRunMxCheck_DoesNotModifyProject: the TUI checker runs mx check after every +// change; it must not write theme-cache/ or deployment/ into the project +// (ako/mxcli#961). Measured with the real mx 11.14 on a v1 and a v2 project: +// a plain `mx check` leaves the model alone but adds theme-cache/web/ and +// deployment/sass/ in both formats, so a stub that does the same is enough here. +func TestRunMxCheck_DoesNotModifyProject(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("stub mx is a shell script") + } + for _, format := range []string{"v1", "v2"} { + t.Run(format, func(t *testing.T) { + bin := t.TempDir() + if err := os.WriteFile(filepath.Join(bin, "mx"), []byte(stubMxScript), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", bin+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("TMPDIR", t.TempDir()) + + proj := t.TempDir() + files := []string{"App.mpr", "theme/web/main.scss", "widgets/W.mpk"} + if format == "v2" { + files = append(files, "mprcontents/ab/cd/abcd.mxunit") + } + for _, f := range files { + p := filepath.Join(proj, f) + os.MkdirAll(filepath.Dir(p), 0o755) + if err := os.WriteFile(p, []byte(f), 0o644); err != nil { + t.Fatal(err) + } + } + mpr := filepath.Join(proj, "App.mpr") + before := treeHashes(t, proj) + + var result MxCheckResultMsg + batch := runMxCheck(mpr)().(tea.BatchMsg) + for _, c := range batch { + if m, ok := c().(MxCheckResultMsg); ok { + result = m + } + } + if result.Err != nil { + t.Fatalf("check: %v", result.Err) + } + if len(result.Errors) != 1 || result.Errors[0].Code != "CE0001" { + t.Errorf("errors = %+v, want the stub's CE0001", result.Errors) + } + + after := treeHashes(t, proj) + var diff []string + for k, v := range after { + if before[k] != v { + diff = append(diff, k) + } + } + for k := range before { + if _, ok := after[k]; !ok { + diff = append(diff, "removed "+k) + } + } + if len(diff) > 0 { + t.Errorf("the TUI checker modified the project: %s", strings.Join(diff, ", ")) + } + }) + } +} diff --git a/docs-site/src/tools/docker-check.md b/docs-site/src/tools/docker-check.md index e69cbdfcd3..8cd97c3891 100644 --- a/docs-site/src/tools/docker-check.md +++ b/docs-site/src/tools/docker-check.md @@ -71,7 +71,7 @@ mxcli docker build -p app.mpr --skip-check ## Integration with the TUI -When using mxcli in interactive REPL mode, the TUI can auto-check the project on file changes, giving immediate feedback on whether MDL modifications introduced errors. +When using mxcli in interactive REPL mode, the TUI can auto-check the project on file changes, giving immediate feedback on whether MDL modifications introduced errors. Like `docker check`, it runs `mx check` on a temporary copy, so the project's `theme-cache/` and `deployment/` are not touched; it does not run update-widgets, so it checks the project as stored. `mxcli eval`'s `mx_check` check does the same. ## Related Pages From 3b03f012d2a676ce7a18d59092af8631f954efc3 Mon Sep 17 00:00:00 2001 From: Ako Date: Sat, 3 Oct 2026 18:06:53 +0000 Subject: [PATCH 12/15] fix(docker): build from a temporary copy; write only the output directory `docker build` (and docker run / reload, which call it) ran update-widgets on the project under a snapshot that restored only MPRv2 storage, then mx check and MxBuild on the project itself. Measured on the 11.14 testapp: an MPRv1 .mpr was rewritten, every MPRv2 .mxunit was rewritten and put back with new mtimes, and theme-cache/, deployment/, 160 javasource/ proxies, the .launch file, .classpath and .project were written into it. buildOnCopy now runs all three tools on one copyProjectToTemp copy and writes only the PAD output directory (absolute, default .docker/build). MxBuild still sees the widget-normalised model, from the copy. The PAD differs from an in-place build in the same 9 files in which two in-place builds of identical copies differ (cache-bust stamps, operation ids, native metro paths), and the rebuild time is unchanged (58s vs 59s). runUpdateWidgets (the v2 snapshot) has no caller left and is removed with its tests; build_readonly_test.go covers v1 and v2 with stub tools, and TestBuild_LeavesProjectUntouched with real mx and MxBuild. Part of ako/mxcli#961. Co-Authored-By: Claude Opus 5.5 --- cmd/mxcli/cmd_fix.go | 6 +- cmd/mxcli/docker.go | 12 +- cmd/mxcli/docker/build.go | 183 ++++++++++++++------ cmd/mxcli/docker/build_integration_test.go | 67 ++++++++ cmd/mxcli/docker/build_readonly_test.go | 189 +++++++++++++++++++++ cmd/mxcli/docker/update_widgets.go | 66 ------- cmd/mxcli/docker/update_widgets_test.go | 151 +--------------- docs-site/src/tools/docker-build.md | 16 ++ 8 files changed, 417 insertions(+), 273 deletions(-) create mode 100644 cmd/mxcli/docker/build_readonly_test.go diff --git a/cmd/mxcli/cmd_fix.go b/cmd/mxcli/cmd_fix.go index 79d1efbcfd..15571b790f 100644 --- a/cmd/mxcli/cmd_fix.go +++ b/cmd/mxcli/cmd_fix.go @@ -71,9 +71,9 @@ CE0463 "The definition of this widget has changed" is what a project reports when its stored widget instances are older than the widget packages installed beside them — the normal state after any headless module or widget install. -'mxcli docker check' already runs this step, but under a snapshot that is -restored afterwards, so the check passes and the stored model stays stale. This -persists the resync instead, which is what Studio Pro's "Update all widgets" +'mxcli docker check' and 'mxcli docker build' already run this step, but on a +temporary copy of the project, so the check passes and the stored model stays +stale. This persists the resync instead, which is what Studio Pro's "Update all widgets" does. It is also more complete than 'mxcli widget sync', which reconciles widget schemas itself and clears only part of the same errors.`, Example: ` mxcli fix widgets -p app.mpr`, diff --git a/cmd/mxcli/docker.go b/cmd/mxcli/docker.go index ea2ac40cc9..bba7c5990c 100644 --- a/cmd/mxcli/docker.go +++ b/cmd/mxcli/docker.go @@ -113,9 +113,17 @@ This command: 1. Detects the Mendix project version (requires >= 11.6.1) 2. Locates MxBuild and a JDK matching the project's JavaVersion (auto-downloads MxBuild from CDN if not found) -3. Runs MxBuild with --target=portable-app-package +3. Copies the project to a temporary directory and runs 'mx update-widgets', + 'mx check' and MxBuild (--target=portable-app-package) on that copy 4. Applies version-aware patches to fix known PAD issues +The project itself is not modified; only the output directory is written. +Each of the mx tools writes into the project it is given (update-widgets +rewrites the model, mx check and MxBuild write theme-cache/, deployment/ +and javasource/ proxies), so they never run on the original. Widget +definitions are normalised on the copy only; 'mxcli fix widgets' applies +that to the project. + MxBuild is cached at ~/.mxcli/mxbuild/{version}/ and reused across builds. You can also pre-download with: mxcli setup mxbuild -p app.mpr @@ -507,7 +515,7 @@ func init() { dockerBuildCmd.Flags().StringP("output", "o", "", "Output directory for PAD package") dockerBuildCmd.Flags().Bool("dry-run", false, "Detect tools and show patch plan without building") dockerBuildCmd.Flags().Bool("skip-check", false, "Skip 'mx check' pre-build validation") - dockerBuildCmd.Flags().Bool("no-update-widgets", false, "Skip 'mx update-widgets' before check") + dockerBuildCmd.Flags().Bool("no-update-widgets", false, "Skip 'mx update-widgets' (run on the temporary copy) before check and build") // Check command flags dockerCheckCmd.Flags().String("mxbuild-path", "", "Path to MxBuild/Mendix installation (used to find mx)") diff --git a/cmd/mxcli/docker/build.go b/cmd/mxcli/docker/build.go index fcb27c1a80..30d56d9224 100644 --- a/cmd/mxcli/docker/build.go +++ b/cmd/mxcli/docker/build.go @@ -40,6 +40,100 @@ type BuildOptions struct { Stdout io.Writer } +// buildSteps is what buildOnCopy needs once the tools are resolved. An empty +// MxPath skips update-widgets and the check (--skip-check, or no mx found). +type buildSteps struct { + ProjectPath string // absolute + MxPath string + MxBuildPath string + JavaHome string + OutputDir string // absolute + SkipUpdateWidgets bool + DryRun bool +} + +// mxbuildCmd runs MxBuild. A package variable so tests can substitute a stub +// that writes into the project it is given the way MxBuild does. +var mxbuildCmd = func(mxbuildPath, javaHome, outputDir, mprPath string, w, stderr io.Writer) error { + cmd := exec.Command(mxbuildPath, + "--target=portable-app-package", + fmt.Sprintf("--java-home=%s", javaHome), + fmt.Sprintf("--java-exe-path=%s", JavaExePath(javaHome)), + fmt.Sprintf("-o=%s", outputDir), + mprPath, + ) + cmd.Stdout = w + cmd.Stderr = stderr + PrepareMxCommand(cmd) + return cmd.Run() +} + +// buildOnCopy runs the steps of a build that read the model — mx update-widgets, +// mx check and MxBuild — on one temporary copy of the project, and writes only +// the PAD package to s.OutputDir (ako/mxcli#961). +// +// The copy is what lets the build use the widget-normalised model without +// changing the project: update-widgets used to run on the project itself, under +// a snapshot that restored only an MPRv2 project's storage, so an MPRv1 .mpr was +// rewritten permanently by every build, and mx check and MxBuild wrote +// theme-cache/, deployment/, javasource/ proxies and the Eclipse files into it. +// A dry run without a check has nothing to run and copies nothing. +func buildOnCopy(s buildSteps, w, stderr io.Writer) error { + if s.DryRun && s.MxPath == "" { + return nil + } + workMpr, cleanup, err := copyProjectToTemp(s.ProjectPath) + if err != nil { + return fmt.Errorf("copy the project to a temporary directory for building: %w\n"+ + " mx and MxBuild write into the project they are given, so docker build never runs them on the original;\n"+ + " set TMPDIR to a disk with room for the project", err) + } + defer cleanup() + out := newPathRewriter(w, filepath.Dir(workMpr), filepath.Dir(s.ProjectPath)) + errOut := newPathRewriter(stderr, filepath.Dir(workMpr), filepath.Dir(s.ProjectPath)) + defer out.Flush() + defer errOut.Flush() + fmt.Fprintln(w, "Building from a temporary copy of the project (mx and MxBuild write into the project") + fmt.Fprintln(w, " they are given; the project on disk is not changed, only the output directory is written).") + + if s.MxPath != "" { + if !s.SkipUpdateWidgets { + // Normalise pluggable widget definitions so neither the check nor + // MxBuild reports CE0463 for definitions that only need a resync. + fmt.Fprintln(w, "Normalising widget definitions on the temporary copy (the project keeps its own;") + fmt.Fprintln(w, " `mxcli fix widgets` applies the normalisation to it)...") + if err := updateWidgetsCmd(s.MxPath, workMpr, out, errOut); err != nil { + out.Flush() + fmt.Fprintf(w, "Warning: update-widgets failed (continuing): %v\n", err) + } + } + fmt.Fprintln(w, "Checking project for errors...") + err := mxCheckCmd(s.MxPath, workMpr, nil, out, errOut) + out.Flush() + errOut.Flush() + if err != nil { + return fmt.Errorf("project has errors (fix them or use --skip-check to bypass): %w", err) + } + fmt.Fprintln(w, " Project check passed.") + } + if s.DryRun { + return nil + } + + if err := os.MkdirAll(s.OutputDir, 0755); err != nil { + return fmt.Errorf("creating output directory: %w", err) + } + fmt.Fprintf(w, "Running MxBuild (target=portable-app-package)...\n") + fmt.Fprintf(w, " Output: %s\n", s.OutputDir) + err = mxbuildCmd(s.MxBuildPath, s.JavaHome, s.OutputDir, workMpr, out, errOut) + out.Flush() + errOut.Flush() + if err != nil { + return fmt.Errorf("mxbuild failed: %w", err) + } + return nil +} + // Build runs MxBuild to create a Portable App Distribution package and applies patches. func Build(opts BuildOptions) error { w := opts.Stdout @@ -93,33 +187,41 @@ func Build(opts BuildOptions) error { fmt.Fprintf(w, " JAVA_HOME: %s\n", javaHome) fmt.Fprintf(w, " Java: %s\n", javaVersionString(javaHome)) - // Step 4: Pre-build check + // Steps 4 and 5: update-widgets, mx check and MxBuild, on one temporary copy + // of the project (ako/mxcli#961). Each of them writes into the project it is + // given — update-widgets rewrites the model (an MPRv1 .mpr permanently, an + // MPRv2 one into MPRv1), mx check writes theme-cache/ and deployment/sass/, + // MxBuild regenerates javasource/ proxies, the .launch/.classpath/.project + // files and deployment/ — and the build needs none of that in the project: + // its product is the PAD package in the output directory. + projectPath, err := filepath.Abs(opts.ProjectPath) + if err != nil { + return err + } + outputDir := opts.OutputDir + if outputDir == "" { + outputDir = filepath.Join(filepath.Dir(projectPath), ".docker", "build") + } + if abs, err := filepath.Abs(outputDir); err == nil { + outputDir = abs + } + mxPath := "" if !opts.SkipCheck { - fmt.Fprintln(w, "Checking project for errors...") - mxPath, err := ResolveMxForVersion(opts.MxBuildPath, pv.ProductVersion) - if err != nil { - fmt.Fprintf(w, " Skipping check: %v\n", err) - } else { - // Run update-widgets before check to prevent false CE0463 errors. - // runUpdateWidgets preserves the project's on-disk storage format: the bare - // invocation this replaced converted MPRv2 projects to MPRv1 and deleted - // mprcontents/ (mendixlabs/mxcli#808). restore is deferred to Build's exit - // rather than run here, so both `mx check` and MxBuild below see the - // widget-normalized model; only the on-disk format is put back. - if !opts.SkipUpdateWidgets { - restore := runUpdateWidgets(mxPath, opts.ProjectPath, w, os.Stderr) - defer restore() - } - - cmd := exec.Command(mxPath, "check", opts.ProjectPath) - cmd.Stdout = w - cmd.Stderr = os.Stderr - PrepareMxCommand(cmd) - if err := cmd.Run(); err != nil { - return fmt.Errorf("project has errors (fix them or use --skip-check to bypass): %w", err) - } - fmt.Fprintln(w, " Project check passed.") - } + if mxPath, err = ResolveMxForVersion(opts.MxBuildPath, pv.ProductVersion); err != nil { + fmt.Fprintf(w, "Skipping check: %v\n", err) + mxPath = "" + } + } + if err := buildOnCopy(buildSteps{ + ProjectPath: projectPath, + MxPath: mxPath, + MxBuildPath: mxbuildPath, + JavaHome: javaHome, + OutputDir: outputDir, + SkipUpdateWidgets: opts.SkipUpdateWidgets, + DryRun: opts.DryRun, + }, w, os.Stderr); err != nil { + return err } // Dry-run: stop here and show what would happen @@ -149,35 +251,6 @@ func Build(opts BuildOptions) error { return nil } - // Step 5: Run MxBuild - outputDir := opts.OutputDir - if outputDir == "" { - outputDir = filepath.Join(filepath.Dir(opts.ProjectPath), ".docker", "build") - } - if err := os.MkdirAll(outputDir, 0755); err != nil { - return fmt.Errorf("creating output directory: %w", err) - } - - fmt.Fprintf(w, "Running MxBuild (target=portable-app-package)...\n") - fmt.Fprintf(w, " Output: %s\n", outputDir) - - javaExePath := JavaExePath(javaHome) - - cmd := exec.Command(mxbuildPath, - "--target=portable-app-package", - fmt.Sprintf("--java-home=%s", javaHome), - fmt.Sprintf("--java-exe-path=%s", javaExePath), - fmt.Sprintf("-o=%s", outputDir), - opts.ProjectPath, - ) - cmd.Stdout = w - cmd.Stderr = os.Stderr - PrepareMxCommand(cmd) - - if err := cmd.Run(); err != nil { - return fmt.Errorf("mxbuild failed: %w", err) - } - // Step 5b: Extract PAD ZIP if MxBuild produced one if err := extractPADZip(outputDir, w); err != nil { return fmt.Errorf("extracting PAD zip: %w", err) diff --git a/cmd/mxcli/docker/build_integration_test.go b/cmd/mxcli/docker/build_integration_test.go index 0963de303b..60b5b27158 100644 --- a/cmd/mxcli/docker/build_integration_test.go +++ b/cmd/mxcli/docker/build_integration_test.go @@ -6,9 +6,11 @@ package docker import ( "bytes" + "io" "os" "os/exec" "path/filepath" + "strings" "testing" "github.com/mendixlabs/mxcli/mdl/types" @@ -105,3 +107,68 @@ func mprProductVersion(t *testing.T, mprPath string) *mxversion.ProjectVersion { defer reader.Disconnect() return reader.ProjectVersion() } + +// TestBuild_LeavesProjectUntouched is the end-to-end guard for ako/mxcli#961: +// with real mx and MxBuild, a full `docker build` of an MPRv1 and an MPRv2 +// project leaves every file outside the output directory (.docker/) +// byte-identical with the same mtime, and still produces the PAD package. +// Before the fix, update-widgets rewrote the v1 .mpr (and every v2 .mxunit was +// rewritten and restored with new mtimes), and mx check / MxBuild wrote +// theme-cache/, deployment/, javasource/ proxies, the .launch file, .classpath +// and .project into the project. +func TestBuild_LeavesProjectUntouched(t *testing.T) { + mxPath, err := ResolveMx("") + if err != nil { + t.Skipf("mx not resolvable: %v", err) + } + for _, format := range []string{"v2", "v1"} { + t.Run(format, func(t *testing.T) { + // Not t.TempDir(): see TestCheck_LeavesProjectUntouched. + dir, err := os.MkdirTemp("", "bld") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { os.RemoveAll(dir) }) + cmd := exec.Command(mxPath, "create-project") + cmd.Dir = dir + PrepareMxCommand(cmd) + if out, err := cmd.CombinedOutput(); err != nil { + t.Skipf("mx create-project failed: %v\n%s", err, out) + } + mprPath := filepath.Join(dir, "App.mpr") + if pv := mprProductVersion(t, mprPath); !pv.IsAtLeastFull(11, 6, 1) { + t.Skipf("Build requires Mendix >= 11.6.1; scaffolded project is %s", pv.ProductVersion) + } + major, _ := ProjectJavaMajor(mprPath) + if _, err := resolveJDK(major); err != nil { + t.Skipf("no JDK for Java %d: %v", javaMajorOrDefault(major), err) + } + if format == "v1" { + if err := updateWidgetsCmd(mxPath, mprPath, io.Discard, io.Discard); err != nil { + t.Skipf("could not produce a v1 fixture: %v", err) + } + if v := mprStorageVersion(t, mprPath); v != types.MPRVersionV1 { + t.Skipf("fixture is %v, not v1", v) + } + } + + before := treeState(t, dir) + var stdout bytes.Buffer + if err := Build(BuildOptions{ProjectPath: mprPath, Stdout: &stdout}); err != nil { + t.Fatalf("Build: %v\n%s", err, stdout.String()) + } + var changed []string + for _, d := range diffStates(before, treeState(t, dir)) { + if !strings.Contains(d, ".docker") { + changed = append(changed, d) + } + } + if len(changed) > 0 { + t.Errorf("docker build modified the project:\n %s", strings.Join(changed, "\n ")) + } + if _, err := os.Stat(filepath.Join(dir, ".docker", "build", "Dockerfile")); err != nil { + t.Errorf("no PAD in the output directory: %v\n%s", err, stdout.String()) + } + }) + } +} diff --git a/cmd/mxcli/docker/build_readonly_test.go b/cmd/mxcli/docker/build_readonly_test.go new file mode 100644 index 0000000000..a22e0f505b --- /dev/null +++ b/cmd/mxcli/docker/build_readonly_test.go @@ -0,0 +1,189 @@ +// SPDX-License-Identifier: Apache-2.0 + +// ako/mxcli#961 item 1: `docker build` must not modify the user's project; it +// writes only its output directory. +// +// Measured on the 11.14 testapp before the fix: an MPRv1 build rewrote the .mpr +// (update-widgets ran on the project, and only an MPRv2 project's storage was +// restored afterwards), every MPRv2 .mxunit was rewritten and put back with a +// new mtime, and mx check / MxBuild wrote theme-cache/, deployment/, 160 +// javasource/ proxies, TestApp.launch, .classpath and .project into it. All three +// tools now run on one temporary copy. +package docker + +import ( + "bytes" + "io" + "os" + "path/filepath" + "strings" + "testing" +) + +// stubMxBuild replaces MxBuild with a stub that writes into the project it is +// given what the real one does, records the model it saw, and writes a PAD +// marker to the output directory. +func stubMxBuild(t *testing.T) (sawModel *string, sawPath *string) { + t.Helper() + var model, path string + orig := mxbuildCmd + t.Cleanup(func() { mxbuildCmd = orig }) + mxbuildCmd = func(_, _, outputDir, mprPath string, w, _ io.Writer) error { + path = mprPath + b, err := os.ReadFile(mprPath) + if err != nil { + return err + } + model = string(b) + dir := filepath.Dir(mprPath) + for f, content := range map[string]string{ + "deployment/model/model.mdp": "built", + "javasource/app/proxies/Entity.java": "generated proxy", + strings.TrimSuffix(filepath.Base(mprPath), ".mpr") + ".launch": "rewritten", + ".classpath": "eclipse", + "theme-cache/web/theme.compiled.css": "recompiled by mxbuild", + } { + p := filepath.Join(dir, f) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + return err + } + if err := os.WriteFile(p, []byte(content), 0o644); err != nil { + return err + } + } + if err := os.MkdirAll(outputDir, 0o755); err != nil { + return err + } + io.WriteString(w, "Building "+mprPath+"\n") + return os.WriteFile(filepath.Join(outputDir, "Dockerfile"), []byte("FROM x"), 0o644) + } + return &model, &path +} + +func TestBuildOnCopy_DoesNotModifyProject(t *testing.T) { + for _, tc := range []struct { + name string + fixture func(*testing.T) string + skipCheck bool + skipUpdate bool + dryRun bool + }{ + {"v1", v1Fixture, false, false, false}, + {"v2", v2Fixture, false, false, false}, + {"v1 --no-update-widgets", v1Fixture, false, true, false}, + {"v2 --no-update-widgets", v2Fixture, false, true, false}, + {"v1 --skip-check", v1Fixture, true, false, false}, + {"v2 --skip-check", v2Fixture, true, false, false}, + {"v1 --dry-run", v1Fixture, false, false, true}, + {"v2 --dry-run", v2Fixture, false, false, true}, + } { + t.Run(tc.name, func(t *testing.T) { + mprPath := tc.fixture(t) + projectDir := filepath.Dir(mprPath) + addProjectDirs(t, projectDir) + if err := os.WriteFile(strings.TrimSuffix(mprPath, ".mpr")+".launch", []byte("original launch"), 0o644); err != nil { + t.Fatal(err) + } + seen := stubTools(t) + sawModel, sawPath := stubMxBuild(t) + origModel, err := os.ReadFile(mprPath) + if err != nil { + t.Fatal(err) + } + + tmpRoot := t.TempDir() + t.Setenv("TMPDIR", tmpRoot) + outputDir := filepath.Join(projectDir, ".docker", "build") + + before := treeState(t, projectDir) + mxPath := "mx" + if tc.skipCheck { + mxPath = "" + } + var out bytes.Buffer + if err := buildOnCopy(buildSteps{ + ProjectPath: mprPath, + MxPath: mxPath, + MxBuildPath: "mxbuild", + JavaHome: "/jdk", + OutputDir: outputDir, + SkipUpdateWidgets: tc.skipUpdate, + DryRun: tc.dryRun, + }, &out, io.Discard); err != nil { + t.Fatalf("buildOnCopy: %v\n%s", err, out.String()) + } + + // Nothing but the output directory changed. + after := treeState(t, projectDir) + var changed []string + for _, d := range diffStates(before, after) { + if !strings.Contains(d, ".docker") { + changed = append(changed, d) + } + } + if len(changed) > 0 { + t.Errorf("docker build modified the project:\n %s", strings.Join(changed, "\n ")) + } + + // No tool was pointed at the project itself. + all := append([]string{}, *seen...) + if *sawPath != "" { + all = append(all, "mxbuild "+*sawPath) + } + for _, s := range all { + if strings.HasPrefix(strings.Fields(s)[1], projectDir+string(filepath.Separator)) { + t.Errorf("a tool was pointed at the project itself: %s", s) + } + } + + if tc.dryRun { + if *sawPath != "" { + t.Error("MxBuild ran on a dry run") + } + } else { + if _, err := os.Stat(filepath.Join(outputDir, "Dockerfile")); err != nil { + t.Errorf("the PAD was not written to the output directory: %v", err) + } + // MxBuild builds the widget-normalised model, which is what + // update-widgets is there for — on the copy, not the project. + wantNormalised := !tc.skipCheck && !tc.skipUpdate + if got := *sawModel == "rewritten by update-widgets"; got != wantNormalised { + t.Errorf("MxBuild saw the normalised model = %v, want %v", got, wantNormalised) + } + if !wantNormalised && *sawModel != string(origModel) { + t.Error("MxBuild did not see the project's model as stored") + } + if !strings.Contains(out.String(), "Building "+mprPath) { + t.Errorf("MxBuild output not reported against the project's path:\n%s", out.String()) + } + } + + if strings.Contains(out.String(), tmpRoot) { + t.Errorf("output leaks the temporary copy's path:\n%s", out.String()) + } + if !strings.Contains(out.String(), "temporary copy") { + t.Errorf("output does not say the build runs on a copy:\n%s", out.String()) + } + if entries, _ := os.ReadDir(tmpRoot); len(entries) != 0 { + t.Errorf("temporary copy left behind in %s: %v", tmpRoot, entries) + } + }) + } +} + +// TestBuildOnCopy_CheckFailureStopsBeforeMxBuild: a failing check still aborts +// the build, as it did when the check ran on the project. +func TestBuildOnCopy_CheckFailureStopsBeforeMxBuild(t *testing.T) { + mprPath := v1Fixture(t) + stubTools(t) + mxCheckCmd = func(string, string, []string, io.Writer, io.Writer) error { return io.ErrUnexpectedEOF } + _, sawPath := stubMxBuild(t) + t.Setenv("TMPDIR", t.TempDir()) + err := buildOnCopy(buildSteps{ProjectPath: mprPath, MxPath: "mx", OutputDir: filepath.Join(t.TempDir(), "out")}, io.Discard, io.Discard) + if err == nil || !strings.Contains(err.Error(), "project has errors") { + t.Fatalf("err = %v, want the check failure", err) + } + if *sawPath != "" { + t.Error("MxBuild ran after a failed check") + } +} diff --git a/cmd/mxcli/docker/update_widgets.go b/cmd/mxcli/docker/update_widgets.go index 91c6bfc555..e7eafe1aaf 100644 --- a/cmd/mxcli/docker/update_widgets.go +++ b/cmd/mxcli/docker/update_widgets.go @@ -3,13 +3,10 @@ package docker import ( - "fmt" "io" "os" "os/exec" "path/filepath" - - "github.com/mendixlabs/mxcli/mdl/types" ) // updateWidgetsPathArg returns an absolute form of the .mpr path for the @@ -38,69 +35,6 @@ var updateWidgetsCmd = func(mxPath, pathArg string, w, stderr io.Writer) error { return cmd.Run() } -// runUpdateWidgets runs `mx update-widgets` on the project — normalizing pluggable -// widget definitions so the caller's check/build does not report false CE0463 -// ("widget definition changed") errors — while preserving an MPRv2 project's -// on-disk storage format. -// -// The protection is needed because `mx update-widgets` rewrites an MPRv2 project -// into the self-contained MPRv1 format: it inlines every unit into the .mpr (adding -// a Unit.Contents column) and deletes mprcontents/. A command that checks or builds -// must not mutate the source project's storage format — doing so silently desyncs -// the working tree from a Git repository that tracks the mprcontents/ files, breaks -// a running `mxcli run --local` watch loop, and has been observed to leave Studio -// Pro unable to open the project. So on a v2 project the .mpr + mprcontents/ are -// snapshotted first and put back afterwards. MPRv1 projects are already single-file -// and need no protection. -// -// The caller must `defer restore()` rather than calling it immediately: the check / -// MxBuild step has to run against the widget-normalized model, or the CE0463 false -// positives this step exists to suppress come straight back. Only the on-disk -// format is restored, once the caller is done with the model. -// -// restore is never nil, is safe to defer, and never panics. -// -// This lives on the operation, not on a call site, because it was previously -// implemented in `Check` only — `Build` carried its own bare invocation and kept -// converting projects (mendixlabs/mxcli#763, then #808). `Check` no longer uses -// it: it runs update-widgets on a temporary copy (copyProjectForCheck), because a -// check must not modify the project at all — this restores only the v2 storage, -// and an MPRv1 project was rewritten permanently (ako/mxcli#951). `Build` is -// expected to write the project's deployment/, and still uses it. -func runUpdateWidgets(mxPath, projectPath string, w, stderr io.Writer) (restore func()) { - restore = func() {} - if projectPath == "" { - return restore - } - - if reader, err := openReadOnly(projectPath); err == nil { - isV2 := reader.Version() == types.MPRVersionV2 - contentsDir := reader.ContentsDir() - reader.Disconnect() - if isV2 { - _, snapRestore, snapErr := snapshotStorageFormat(projectPath, contentsDir) - if snapErr != nil { - // Can't protect the format — skip update-widgets rather than risk an - // unrecoverable v2 -> v1 conversion. A CE0463 false positive is the - // lesser evil compared to a silent, unrestorable format change. - fmt.Fprintf(w, "Warning: could not snapshot MPRv2 storage (skipping update-widgets to avoid a v2->v1 conversion): %v\n", snapErr) - return restore - } - restore = snapRestore - } - } - - fmt.Fprintf(w, "Updating widget definitions in %s...\n", projectPath) - if err := updateWidgetsCmd(mxPath, updateWidgetsPathArg(projectPath), w, stderr); err != nil { - // Non-fatal: warn and let the caller continue. The snapshot is still restored - // by the returned func — a failed run may have converted the project first. - fmt.Fprintf(w, "Warning: update-widgets failed (continuing): %v\n", err) - return restore - } - fmt.Fprintln(w, "Widget definitions updated.") - return restore -} - // snapshotStorageFormat backs up the MPRv2 storage files (.mpr index + mprcontents/) // to a temp directory and returns a restore function that puts them back, undoing // any v2 -> v1 conversion performed by an intervening `mx update-widgets`. The diff --git a/cmd/mxcli/docker/update_widgets_test.go b/cmd/mxcli/docker/update_widgets_test.go index 42ac99966f..ef47c53fef 100644 --- a/cmd/mxcli/docker/update_widgets_test.go +++ b/cmd/mxcli/docker/update_widgets_test.go @@ -7,15 +7,13 @@ // invocation, so `mxcli docker build`, `docker run` and `docker reload` still // converted the project silently while reporting success. // -// The fix makes the protection a property of the operation rather than of one -// call site: runUpdateWidgets snapshots the v2 storage, runs the step, and returns -// the restore func for the caller to defer. These tests pin that behaviour, so a -// third call site cannot reintroduce the bug by forgetting the snapshot. +// That snapshot/restore (runUpdateWidgets) is gone: since ako/mxcli#951 and #961 +// both `docker check` and `docker build` run update-widgets on a temporary copy, +// which protects an MPRv1 project as well (check_readonly_test.go, +// build_readonly_test.go). The fixtures below are shared with those tests. package docker import ( - "bytes" - "io" "os" "path/filepath" "testing" @@ -63,14 +61,6 @@ func v1Fixture(t *testing.T) string { return p } -// stubUpdateWidgets swaps the mx invocation for the duration of a test. -func stubUpdateWidgets(t *testing.T, fn func(mxPath, pathArg string, w, stderr io.Writer) error) { - t.Helper() - prev := updateWidgetsCmd - updateWidgetsCmd = fn - t.Cleanup(func() { updateWidgetsCmd = prev }) -} - // convertToV1 mimics what `mx update-widgets` does to an MPRv2 project: rewrite // the .mpr as a self-contained v1 file and delete the mprcontents/ tree. func convertToV1(t *testing.T, mprPath string) { @@ -82,136 +72,3 @@ func convertToV1(t *testing.T, mprPath string) { t.Fatalf("simulating conversion (mprcontents): %v", err) } } - -// TestRunUpdateWidgets_RestoresV2AfterConversion is the core regression: whatever -// update-widgets does to the on-disk format, the restore must undo it. -func TestRunUpdateWidgets_RestoresV2AfterConversion(t *testing.T) { - mprPath := v2Fixture(t) - contentsDir := filepath.Join(filepath.Dir(mprPath), "mprcontents") - - orig, err := os.ReadFile(mprPath) - if err != nil { - t.Fatal(err) - } - - ran := false - stubUpdateWidgets(t, func(_, _ string, _, _ io.Writer) error { - ran = true - convertToV1(t, mprPath) - return nil - }) - - var out bytes.Buffer - restore := runUpdateWidgets("mx", mprPath, &out, io.Discard) - if !ran { - t.Fatal("update-widgets was not run on a project whose format could be snapshotted") - } - - // Before restore the caller's check/build step still sees the normalized model — - // that is the whole reason restore is deferred rather than run immediately. - if _, err := os.Stat(contentsDir); !os.IsNotExist(err) { - t.Fatalf("fixture setup wrong: mprcontents/ should be gone at this point (err=%v)", err) - } - - restore() - - if got, err := os.ReadFile(mprPath); err != nil { - t.Fatalf("read restored .mpr: %v", err) - } else if !bytes.Equal(got, orig) { - t.Errorf(".mpr not restored: got %d bytes, want the original %d (#808)", len(got), len(orig)) - } - if v := storageVersion(t, mprPath); v != types.MPRVersionV2 { - t.Errorf("project left as %v; MPRv2 storage must be preserved (#808)", v) - } - if entries, err := os.ReadDir(contentsDir); err != nil { - t.Errorf("mprcontents/ not restored: %v", err) - } else if len(entries) == 0 { - t.Error("mprcontents/ restored but empty") - } -} - -// TestRunUpdateWidgets_V1NeedsNoSnapshot: a v1 project is already single-file, so -// there is nothing to protect — the step must still run, and restore must be a -// harmless no-op rather than clobbering the project. -func TestRunUpdateWidgets_V1NeedsNoSnapshot(t *testing.T) { - mprPath := v1Fixture(t) - orig, err := os.ReadFile(mprPath) - if err != nil { - t.Fatal(err) - } - - ran := false - stubUpdateWidgets(t, func(_, _ string, _, _ io.Writer) error { - ran = true - return nil - }) - - var out bytes.Buffer - restore := runUpdateWidgets("mx", mprPath, &out, io.Discard) - if !ran { - t.Error("update-widgets was skipped on an MPRv1 project, which needs no protection") - } - if restore == nil { - t.Fatal("restore is nil — callers defer it unconditionally") - } - restore() - - if got, err := os.ReadFile(mprPath); err != nil { - t.Fatalf("read .mpr: %v", err) - } else if !bytes.Equal(got, orig) { - t.Error("restore modified an MPRv1 project it never snapshotted") - } -} - -// TestRunUpdateWidgets_SkipsStepWhenSnapshotFails is the fail-safe: if the storage -// cannot be backed up, running update-widgets risks an unrecoverable conversion. -// A CE0463 false positive is the lesser evil, so the step must not run at all. -func TestRunUpdateWidgets_SkipsStepWhenSnapshotFails(t *testing.T) { - mprPath := v2Fixture(t) - // Remove mprcontents/ so the snapshot's directory copy fails. The project still - // reads as v2 (detection keys off the absent Unit.Contents column), so this is - // the "v2, but cannot be protected" case rather than a v1 project. - if err := os.RemoveAll(filepath.Join(filepath.Dir(mprPath), "mprcontents")); err != nil { - t.Fatal(err) - } - if v := storageVersion(t, mprPath); v != types.MPRVersionV2 { - t.Fatalf("fixture no longer reads as MPRv2 (%v) — test premise broken", v) - } - - ran := false - stubUpdateWidgets(t, func(_, _ string, _, _ io.Writer) error { - ran = true - return nil - }) - - var out bytes.Buffer - restore := runUpdateWidgets("mx", mprPath, &out, io.Discard) - restore() - - if ran { - t.Error("update-widgets ran without a usable snapshot — risks an unrecoverable v2->v1 conversion (#808)") - } - if !bytes.Contains(out.Bytes(), []byte("skipping update-widgets")) { - t.Errorf("the skip must be reported to the user, got:\n%s", out.String()) - } -} - -// TestRunUpdateWidgets_RestoresWhenStepFails: a failed update-widgets is non-fatal, -// but it may still have converted the project before failing, so the snapshot must -// be restored (and its temp dir cleaned up) on that path too. -func TestRunUpdateWidgets_RestoresWhenStepFails(t *testing.T) { - mprPath := v2Fixture(t) - - stubUpdateWidgets(t, func(_, _ string, _, _ io.Writer) error { - convertToV1(t, mprPath) - return io.ErrUnexpectedEOF - }) - - var out bytes.Buffer - restore := runUpdateWidgets("mx", mprPath, &out, io.Discard) - restore() - - if v := storageVersion(t, mprPath); v != types.MPRVersionV2 { - t.Errorf("project left as %v after a failed update-widgets; the snapshot must still be restored (#808)", v) - } -} diff --git a/docs-site/src/tools/docker-build.md b/docs-site/src/tools/docker-build.md index 96082cc8fe..d7d2244785 100644 --- a/docs-site/src/tools/docker-build.md +++ b/docs-site/src/tools/docker-build.md @@ -15,6 +15,22 @@ mxcli docker build -p app.mpr 3. **PAD patching** -- Applies Platform-Agnostic Deployment patches (Phase 1) to the build output 4. **Produces artifact** -- Generates a deployable Mendix deployment package (MDA file) +## The Project Is Not Modified + +`mx update-widgets`, `mx check` and MxBuild all write into the project they are +given: update-widgets rewrites the model (an MPRv1 `.mpr` in place, an MPRv2 +project into MPRv1), mx check compiles the theme into `theme-cache/` and +`deployment/sass/`, and MxBuild regenerates the `javasource/` proxies, the +`.launch`, `.classpath` and `.project` files and all of `deployment/`. + +So `docker build` copies the project to a temporary directory (in `$TMPDIR`) +and runs all three there. Widget definitions are normalised on that copy, so the +build uses the normalised model while the project keeps its own; run +`mxcli fix widgets` to apply the normalisation to the project. The only thing +written is the output directory (`.docker/build/` by default, or `-o`). Build +output, caches and VCS folders (`deployment/`, `.docker/`, `.git/`, +`node_modules/`, ...) are not copied. + ## PAD Patching PAD (Platform-Agnostic Deployment) patching modifies the build output to be compatible with container-based deployment platforms. This is essential for deploying Mendix applications to Kubernetes, Cloud Foundry, or other container orchestrators. From 62256cbe729642959d24f83ac6dfb328c267cfff Mon Sep 17 00:00:00 2001 From: Ako Date: Sat, 3 Oct 2026 18:06:57 +0000 Subject: [PATCH 13/15] docs: changelog, finding and package-operations pattern for #961 Co-Authored-By: Claude Opus 5.5 --- .claude/skills/fix-issue/findings/cmd-mxcli.jsonl | 1 + CHANGELOG.md | 1 + docs-wiki/bug-patterns/package-operations-damage.md | 13 +++++++++++++ 3 files changed, 15 insertions(+) diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl b/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl index 6566f47f77..7ca8b0be84 100644 --- a/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl +++ b/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl @@ -156,3 +156,4 @@ {"area": "cmd/mxcli", "date": "2026-10-03", "symptom": "A project whose CLAUDE.md, skills and lint rules were written by a newer mxcli is served by an older binary (v0.24.0 on PATH) with no warning: 'mdl 1;' is a parse error and shipped lint rules crash, all reading as project defects", "cause": "Nothing recorded which mxcli wrote the tooling; .claude/bootstrap-mxcli.sh linked whatever mxcli was on PATH; init --sync-skills refreshed only .ai-context/skills, never .claude/lint-rules or CLAUDE.md/AGENTS.md", "fix": "init and every sync write .ai-context/mxcli-tooling.json; root PersistentPreRun warns once on stderr when the binary is provably older (release by number, nightly by tag date, mixed by build date, dev never); sync refuses from an older binary; the bootstrap script carries a POSIX-sh copy of the ordering and neither links an older PATH binary nor keeps an older ./mxcli, downloading via a temp file + mv; sync also refreshes bundled lint rules by name and the CLAUDE.md/AGENTS.md section between mxcli:begin/end markers", "insight": "A binary cannot warn about a stamp it predates, so the guard for already-shipped binaries must live in the generated script, which the newer mxcli regenerates. The sh and Go comparisons share one test table so they cannot drift. curl -o ./mxcli on a symlinked ./mxcli would overwrite the PATH binary — always download to a temp name and rename", "issue": "ako/mxcli#952", "file": "cmd/mxcli/tooling_stamp.go; cmd/mxcli/init_tooling_sync.go; cmd/mxcli/init_hook.go (bootstrapScriptTemplate); cmd/mxcli/main.go; cmd/mxcli/init.go", "test": "cmd/mxcli/tooling_stamp_test.go; cmd/mxcli/init_hook_version_test.go; cmd/mxcli/init_tooling_sync_test.go"} {"area": "cmd/mxcli", "date": "2026-10-03", "symptom": "CONV006 emits one finding per entity x role x CREATE/DELETE (111 on a mid-sized app), the same advice repeated per role, and the per-finding Security score is driven by role count rather than by entities", "cause": "The Starlark rule appended a violation inside the permissions_for() loop", "file": "`.claude/lint-rules/conv006_no_create_delete_rights.star` (synced to `cmd/mxcli/lint-rules/`)", "insight": "Group per entity and per right with de-duplicated sorted roles (a role can hold several access rules on one entity). Test both rule copies (.claude and the embedded one) like SEC008's test does", "refs": ["ako/mxcli#953"]} {"area": "cmd/mxcli", "date": "2026-10-03", "symptom": "`mxcli report` could not score a project's own modules: `lint` has --modules, `report` had only --exclude", "cause": "Feature gap; and the LintContext module filter alone would not make the score exact, because project-level findings (CONV008 role mappings, project security) carry no module and are reported regardless", "file": "`cmd/mxcli/cmd_report.go`, `mdl/linter/report.go` (`ScopeToModules`, Report.Modules)", "insight": "Filter the scored violations to those located in a selected module, and print the selection in every format so a module score is not mistaken for the project's", "refs": ["ako/mxcli#953"]} +{"area": "cmd/mxcli/docker", "date": "2026-10-03", "symptom": "`mxcli docker build` (and docker run / reload, which call it) rewrote an MPRv1 project's .mpr, rewrote every MPRv2 .mxunit (restored with new mtimes), and wrote theme-cache/, deployment/, javasource/ proxies, the .launch file, .classpath and .project into the project; the TUI checker and `mxcli eval`'s mx_check wrote theme-cache/web/ and deployment/sass/ on every run", "cause": "update-widgets ran on the project under a snapshot that restored only v2 storage, and mx check / MxBuild ran on the project itself; the TUI and eval runner ran a bare `mx check `", "file": "`cmd/mxcli/docker/build.go` (`buildOnCopy`), `cmd/mxcli/docker/check.go` (`MxCheckOnCopy`), `cmd/mxcli/tui/checker.go` (`runMxCheck`), `cmd/mxcli/evalrunner/checks.go` (`checkMxCheck`)", "insight": "Run update-widgets, mx check and MxBuild on one temporary copy (copyProjectToTemp) and write only the output directory. Measured on the 11.14 testapp: the PAD from the copy differs from the in-place build in the same 9 files as two in-place builds of identical copies differ (cache-bust stamps, operation ids, the native metro paths), so the output is equivalent; rebuild time unchanged (58s vs 59s), because a PAD build gains nothing from the project's deployment/. 11.14's blank app needs JDK 25, so the real-mx build test skips without it; run it with 11.13's mx.", "refs": ["ako/mxcli#961", "ako/mxcli#951", "mendixlabs/mxcli#808"]} diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ac2b20156..1dc389f264 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **`docker build`, the TUI checker and `mxcli eval` no longer modify the project** (ako/mxcli#961) — `docker build` (and `docker run` / `docker reload`, which use it) now runs `mx update-widgets`, `mx check` and MxBuild on one temporary copy and writes only its output directory (`.docker/build/` or `-o`). Before, it rewrote an MPR v1 project's `.mpr`, rewrote every MPR v2 `.mxunit` (restored with new mtimes), and wrote `theme-cache/`, `deployment/`, `javasource/` proxies, the `.launch` file, `.classpath` and `.project` into the project. The build still uses the widget-normalised model, from the copy, and the package it produces is the same; `mxcli fix widgets` applies the normalisation to the project. **The project's `deployment/` is no longer refreshed by `docker build`** — `run --local` builds its own. The TUI auto-check and eval's `mx_check` ran a plain `mx check` on the project, which wrote `theme-cache/web/` and `deployment/sass/` every time; they now check a copy too. - **`docker check` no longer modifies the project** (ako/mxcli#951) — `mx update-widgets` and `mx check` now run on a temporary copy, for MPR v1 and v2 alike, and what mx prints names the project's own paths. Before, a check rewrote an MPR v1 project's `.mpr` permanently (only v2 was restored from a snapshot), and `mx check` itself rewrote `theme-cache/` and created `deployment/sass/` even with `--no-update-widgets`. The output now says that widget definitions were normalised on a copy, and that a CE0463 the stored project still has is therefore not reported: `--no-update-widgets` checks the project as stored, `mxcli fix widgets` applies the normalisation (ako/mxcli#568, #646). Build output, caches and VCS folders are not copied; the copy goes to `$TMPDIR` and is removed afterwards. - **Parallel mxcli runs on one project no longer fail to save the catalog cache** (ako/mxcli#951) — eight parallel `lint` runs on a fresh copy printed `failed to create table catalog_meta: table catalog_meta already exists` or `database is locked`. The cache is now written to a temporary file next to it and renamed into place, so every run saves and a reader sees the old cache or the new one, never a half-written file; opening a current cache no longer writes to it. - **A lint rule that fails no longer costs the project score** (ako/mxcli#952) — a Starlark rule reading a struct field this mxcli does not expose (a rule written for a newer mxcli, such as one using `document_noun_title` under v0.24.0) is reported at info level as `rule needs a newer mxcli ()` instead of an error. All rule failures are kept out of `mxcli report`'s score, summary and categories and listed in their own "Rules That Could Not Run" section (`ruleFailures` in JSON); other failures stay `Starlark rule error` errors in `mxcli lint`. A configured rule severity no longer applies to the rule's own failure. A rule file that fails to load on an undefined name says the rule may need a newer mxcli. Measured on PedApp with v0.24.0 and rules from main: QUAL004 and CUSTOM002 crashed and scored as 2 errors. diff --git a/docs-wiki/bug-patterns/package-operations-damage.md b/docs-wiki/bug-patterns/package-operations-damage.md index 1c73469c4f..98c37dfda9 100644 --- a/docs-wiki/bug-patterns/package-operations-damage.md +++ b/docs-wiki/bug-patterns/package-operations-damage.md @@ -29,6 +29,19 @@ works. Nothing about the output says so. mxcli's repair commands exist because o this: let the tool convert, read the units back, restore v2, and write only the changed ones through mxcli's own writer. +**Every mx tool writes into the project it is given — a check and a build too.** +The protection was added one call site at a time and each time covered only the +files someone expected to change: a v2 snapshot around update-widgets in `docker +check` (#763), then in `docker build` (#808), while a v1 `.mpr` was rewritten +permanently and a plain `mx check` wrote `theme-cache/` and `deployment/sass/` +from `docker check`, `docker build`, the TUI checker and the eval runner alike +(#951, #961). What found them was a whole-tree hash+mtime diff before and after, +not a check of the expected files. The fix that holds is a temporary copy for +any mx run that is not *meant* to change the project (`docker.MxCheckOnCopy`, +`copyProjectToTemp`); the build reads the widget-normalised model from the copy +and produces the same package. A new `exec.Command(mx…, project)` needs that +decision made explicitly: expected write, or copy. + **Version numbers are not identity.** Matching an installed module against a marketplace release by version *number* is ambiguous — a blank project ships two different modules that both published a 4.1.0. The module's `AppStoreGuid` is the From 2c5a08c714cb88b37bcced90911f28c6d435d79a Mon Sep 17 00:00:00 2001 From: Ako Date: Sat, 3 Oct 2026 18:05:14 +0000 Subject: [PATCH 14/15] fix(lint,check): MPR010 skips native pages and snippets The layout-grid advice is about Bootstrap label/input columns, which a native page does not have. Measured on mxbuild 11.13.0 (PedApp copy): a bare form DataView on a NativePhone_Default page, or in a native snippet, builds clean, and wrapping it in a layoutgrid as advised is CE6858. lint skips pages on a native layout (LintContext.NativePages, so the rule now declares CatalogFull) and snippets of Type Native; check -p asks the project whether a reported page's layout is native. Part of #962 (item 4). Co-Authored-By: Claude Opus 5.5 --- .../skills/fix-issue/findings/mdl-other.jsonl | 1 + .../mendix/create-page/reference/examples.md | 2 + CHANGELOG.md | 1 + cmd/mxcli/check_native_layout_grid_test.go | 48 +++++++++++ mdl/executor/validate_page_layout.go | 41 +++++++++- mdl/executor/validate_page_layout_test.go | 39 ++++++++- mdl/executor/validate_program.go | 30 ++++--- mdl/linter/rules/catalog_mode_guard_test.go | 2 +- mdl/linter/rules/dataview_layout_grid.go | 19 ++++- mdl/linter/rules/dataview_layout_grid_test.go | 81 ++++++++++++++++++- 10 files changed, 246 insertions(+), 18 deletions(-) create mode 100644 cmd/mxcli/check_native_layout_grid_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-other.jsonl b/.claude/skills/fix-issue/findings/mdl-other.jsonl index ef6edcd7da..57c8875209 100644 --- a/.claude/skills/fix-issue/findings/mdl-other.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-other.jsonl @@ -94,3 +94,4 @@ {"area": "mdl/linter", "date": "2026-10-03", "symptom": "mxcli report scores a project lower for lint rules that crash: under v0.24.0, project rules written by a newer mxcli (QUAL004, CUSTOM002 reading .document_noun_title) each produced an error-severity 'Starlark rule error: \"microflow\" struct has no .document_noun_title attribute' that counted 10 points against the project", "cause": "StarlarkRule.Check turned every evaluation error into an ordinary SeverityError violation, indistinguishable from a finding; BuildReport and Summarize counted it, and a configured rule severity was applied to it too", "fix": "ruleFailureViolation marks every failure Violation.RuleFailure; a missing struct attribute (matched on the evaluator message, since starlark flattens NoSuchAttrError via fmt.Errorf) becomes info 'rule needs a newer mxcli ()'; BuildReport splits RuleFailures out before counting and every report format lists them separately; Linter.Run skips the severity override for them; an 'undefined:' load failure gets a newer-mxcli hint", "insight": "The score measures the project, so anything about the tooling has to be partitioned out BEFORE counting, not filtered in the formatter. The control that makes the score assertion meaningful is a working rule's finding that does move the score", "issue": "ako/mxcli#952", "file": "mdl/linter/starlark.go (ruleFailureViolation); mdl/linter/report.go (BuildReport); mdl/linter/linter.go (Run); mdl/linter/report_format.go", "test": "mdl/linter/starlark_rule_failure_test.go"} {"area": "mdl/linter", "date": "2026-10-03", "symptom": "MPR012 (legacy static/dynamic image, CE0582) fires on pages with a native layout (Atlas_Core.NativePhone_Default), where mxbuild 11.13 builds them clean; `check --references` likewise refuses a classic `dropdown` on a native page as MDL-WIDGET40 (CE0582), also clean in mxbuild", "cause": "Both rules assume every page is rendered by the React client. Nothing recorded a layout's platform: ListLayouts left pages.Layout.Native false for every layout, and catalog layouts had only LayoutType, which cannot tell the platforms apart (native uses Default/Popup)", "file": "`mdl/backend/modelsdk/page.go` (`layoutIsNative`), `mdl/catalog/builder_pages.go` (layouts.Platform), `mdl/linter/context_catalog_tables.go` (`NativePages`), `mdl/linter/rules/legacy_image_widget.go`, `mdl/executor/validate_widget_attribute_type.go` (`layoutIsNative`)", "insight": "The platform is the content wrapper's TYPE (Forms$NativeLayoutContent), not a property. The native layouts live in Atlas_Core, a Marketplace module, so the page->layout join must not apply the notPlatformModule filter the iterators use. Sibling check MDL-WIDGET39 (CE2421, textbox on an enumeration) is NOT React-only: measured CE2421 on the native page too, so it keeps firing there", "refs": ["ako/mxcli#953"]} {"area": "mdl/linter", "date": "2026-10-03", "symptom": "MPR002 'Microflow X has no activities' on a microflow or nanoflow whose only content is `return ;` (e.g. a label formatter, `return $currentUser;`)", "cause": "ActivityCount excludes start and end events, so a flow that computes its result in the end event's return value counts 0 activities", "file": "`mdl/linter/rules/empty.go` (`returnsValue`)", "insight": "A non-Void ReturnType is the catalog's witness that the end event returns a value (mxbuild requires it on every end event), so no new column was needed; '' and 'Void' stay reported", "refs": ["ako/mxcli#953"]} +{"area": "mdl/linter", "date": "2026-10-03", "symptom": "ako/mxcli#962 item 4: MPR010 'DataView contains input fields but is not inside a layout grid' fires on a page with layout Atlas_Core.NativePhone_Default (lint and check); the bare form builds clean there and following the advice is CE6858 'Please update Atlas UI to version 2.4 or higher to use Layout Grid on Native pages' (mxbuild 11.13.0, PedApp)", "cause": "Same web-only assumption MPR012 had (#953): the rule's premise is Bootstrap label/input columns, which only the web client renders, but it walked every page and snippet regardless of platform", "file": "`mdl/linter/rules/dataview_layout_grid.go` (skip ctx.NativePages(), snippets with raw Type 'Native', RequiredCatalogMode CatalogFull); `mdl/executor/validate_page_layout.go` (validatePageLayoutGridWith + projectNativeLayouts)", "fix": "Lint: skip native pages via LintContext.NativePages() (declares CatalogFull since LayoutRef is full-only — NativePages added to the catalog-mode guard list) and native snippets via the snippet's own Type. Check: ask the project whether the page's layout is native, only when the page has something to report", "insight": "A platform-specific rule needs its platform in the predicate, and there are two places a page's platform lives: the layout (pages) and the document's own Type (snippets). Also measure the ADVICE, not only the warning: here following it produced a new CE, which settles 'does it apply' faster than reasoning about rendering. A rule that starts reading NativePages() silently returns nothing on a fast catalog unless it declares CatalogFull — the guard test only catches it if the method is in its list", "test": "`mdl/linter/rules/dataview_layout_grid_test.go` (TestDataViewLayoutGridRule_SkipsNativePagesAndSnippets); `mdl/executor/validate_page_layout_test.go` (TestValidatePageLayoutGrid_SkipsNativeLayouts); `cmd/mxcli/check_native_layout_grid_test.go`"} diff --git a/.claude/skills/mendix/create-page/reference/examples.md b/.claude/skills/mendix/create-page/reference/examples.md index 11f5b3256b..6165f5dd3c 100644 --- a/.claude/skills/mendix/create-page/reference/examples.md +++ b/.claude/skills/mendix/create-page/reference/examples.md @@ -19,6 +19,8 @@ create or modify page CRM.CustomerEdit -- width are expressed in Bootstrap grid columns and only render correctly -- inside a layoutgrid → row → column. A DataView with input fields placed -- directly on the page (no grid) is flagged by lint rule MPR010 / mxcli check. + -- Web pages only: on a native layout leave the form bare — a layoutgrid there + -- needs a newer Atlas UI (CE6858) and native forms are not grid-based. layoutgrid mainGrid { row { column (desktopwidth: autofill) { diff --git a/CHANGELOG.md b/CHANGELOG.md index 30bb92cb5c..c055698b41 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **MPR010 no longer tells a native page to use a layout grid** (ako/mxcli#962) — the "wrap the form in a layoutgrid" advice is about Bootstrap columns, which a native page does not have: a form DataView directly on an `Atlas_Core.NativePhone_Default` page builds clean in mxbuild 11.13.0, and following the advice there is CE6858 ("update Atlas UI … to use Layout Grid on Native pages"). `lint` skips pages on a native layout and snippets of type Native (MPR010 now needs the full catalog, which a default `lint` builds anyway); `check -p` skips pages whose layout the project says is native. - **The editor no longer squiggles calls to a project's void actions** (ako/mxcli#962) — the language server validated flows without the project, so two calls to a stored void Java or JavaScript action carrying the same output name (the shape Studio Pro writes) were flagged MDL063. It now resolves the actions through the workspace project, remembering the answers for 30 seconds between keystrokes; an action it cannot resolve is treated as possibly void, so it is neither a duplicate nor an MDL093. `check` keeps the strict reading. - **`describe` warns about a duplicate output variable across if/else branches and loop bodies** (ako/mxcli#962) — a flow's variable names are unique flow-wide: the same output name in each branch of an if/else, or inside a loop and again after it, is CE0111 in mxbuild 11.13.0 (measured in a microflow). The `-- WARNING: duplicate output variable` header only fired when one assignment could reach the other, so it called both models valid. Calls to void actions are still never counted. - **`check` reports a read of a void action call's output name** (ako/mxcli#962) — `$V = call java action M.Void(…)` followed by anything reading `$V` passed `check` and failed the build with CE0109 "Undefined variable 'V'" (measured on mxbuild 11.13.0). It is now MDL093, for microflows and nanoflows, when the script or, with `-p`, the project says the action returns Void. A name something else defines (a declare, a parameter, a non-void producer) and an action nobody can resolve are not reported. diff --git a/cmd/mxcli/check_native_layout_grid_test.go b/cmd/mxcli/check_native_layout_grid_test.go new file mode 100644 index 0000000000..fa0b150b67 --- /dev/null +++ b/cmd/mxcli/check_native_layout_grid_test.go @@ -0,0 +1,48 @@ +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// ako/mxcli#962 item 4, end to end on PedApp: `check -p` reads from the +// project that Atlas_Core.NativePhone_Default is a native layout and does not +// give a page on it the layout-grid advice (MPR010) — on mxbuild 11.13.0 the +// bare form builds clean there and a layoutgrid is CE6858. The web page in the +// same script is the control. +func TestCheck_MPR010SkipsNativePages(t *testing.T) { + src := filepath.Join("..", "..", "testdata", "pedapp") + if _, err := os.Stat(filepath.Join(src, "PedApp.mpr")); err != nil { + t.Skipf("PedApp fixture not found: %v", err) + } + dir := t.TempDir() + if err := copyTree(src, dir); err != nil { + t.Fatal(err) + } + _ = checkCmd.InheritedFlags() // merges the persistent -p into checkCmd.Flags() + _ = rootCmd.PersistentFlags().Set("project", filepath.Join(dir, "PedApp.mpr")) + defer func() { + _ = rootCmd.PersistentFlags().Set("project", "") + rootCmd.PersistentFlags().Lookup("project").Changed = false + }() + file := writeScript(t, t.TempDir(), "s.mdl", `mdl 1; +create persistent entity MyFirstModule.Thing (Name: String(100)); +create page MyFirstModule.P_NativeForm (Title: 'N', Layout: Atlas_Core.NativePhone_Default, Params: ($Thing: MyFirstModule.Thing)) { + dataview dvNative (DataSource: $Thing) { textbox tb1 (Attribute: Name) } +}; +create page MyFirstModule.P_WebForm (Title: 'W', Layout: Atlas_Core.Atlas_Default, Params: ($Thing: MyFirstModule.Thing)) { + dataview dvWeb (DataSource: $Thing) { textbox tb2 (Attribute: Name) } +}; +`) + out := captureStd(t, func() { runCheckFiles(checkCmd, []string{file}) }) + if strings.Contains(out, "dvNative") { + t.Errorf("MPR010 advised a layoutgrid on a native page:\n%s", out) + } + if !strings.Contains(out, "dvWeb") || !strings.Contains(out, "MPR010") { + t.Errorf("control: the web page's bare form not reported:\n%s", out) + } +} diff --git a/mdl/executor/validate_page_layout.go b/mdl/executor/validate_page_layout.go index c61aa9669d..63f0163953 100644 --- a/mdl/executor/validate_page_layout.go +++ b/mdl/executor/validate_page_layout.go @@ -12,6 +12,7 @@ import ( "strings" "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend" "github.com/mendixlabs/mxcli/mdl/linter" ) @@ -20,16 +21,50 @@ import ( // are laid out in grid columns (independent of the data source). Any layoutgrid // ancestor satisfies the rule (grid → column → container → dataview is fine); a // display-only / container DataView (no inputs) is not flagged. -func ValidatePageLayoutGrid(prog *ast.Program) []linter.Violation { +// +// nativeLayout, which may be nil, tells a native layout; a page on one is +// skipped. The advice is web-only (ako/mxcli#962). Label and input widths in +// Bootstrap grid columns are the web client's; a native page is rendered by +// React Native. Measured on mxbuild 11.13.0 (PedApp copy): a form DataView +// placed directly on an Atlas_Core.NativePhone_Default page builds clean, and +// FOLLOWING the advice there — wrapping it in a layoutgrid — is CE6858 "Please +// update Atlas UI to version 2.4 or higher to use Layout Grid on Native pages". +func ValidatePageLayoutGrid(prog *ast.Program, nativeLayout func(layout string) bool) []linter.Violation { var out []linter.Violation for _, stmt := range prog.Statements { - if label, widgets, ok := documentWidgets(stmt); ok { - out = append(out, checkLayoutGridTree(widgets, false, label)...) + label, widgets, ok := documentWidgets(stmt) + if !ok { + continue + } + found := checkLayoutGridTree(widgets, false, label) + // Asked only when there is something to report, so a script whose + // pages are fine never opens the project for this. + if page, isPage := stmt.(*ast.CreatePageStmtV3); isPage && len(found) > 0 && + nativeLayout != nil && page.Layout != "" && nativeLayout(page.Layout) { + continue } + out = append(out, found...) } return out } +// projectNativeLayouts answers "is this layout native?" from the project that +// project() opens — nil when there is none. A layout the project does not hold +// (one the script creates) reads as web, which keeps a check as loud as it was. +func projectNativeLayouts(project func() backend.FullBackend) func(string) bool { + var ctx *ExecContext + return func(layout string) bool { + if ctx == nil { + b := project() + if b == nil { + return false + } + ctx = &ExecContext{Backend: b} + } + return layoutIsNative(ctx, layout) + } +} + func checkLayoutGridTree(widgets []*ast.WidgetV3, underGrid bool, locationPrefix string) []linter.Violation { var out []linter.Violation for _, w := range widgets { diff --git a/mdl/executor/validate_page_layout_test.go b/mdl/executor/validate_page_layout_test.go index 9413ccc71c..d8dac3d480 100644 --- a/mdl/executor/validate_page_layout_test.go +++ b/mdl/executor/validate_page_layout_test.go @@ -17,7 +17,7 @@ func layoutGridMessages(t *testing.T, src string) []string { t.Fatalf("parse errors: %v", errs) } var msgs []string - for _, v := range ValidatePageLayoutGrid(prog) { + for _, v := range ValidatePageLayoutGrid(prog, nil) { msgs = append(msgs, v.Message) } return msgs @@ -91,3 +91,40 @@ func TestValidatePageLayoutGrid_DisplayOnlyClean(t *testing.T) { t.Fatalf("display-only DataView should not warn, got %v", msgs) } } + +// ako/mxcli#962 item 4: the advice is web-only — a form DataView directly on +// a NativePhone_Default page builds clean on mxbuild 11.13.0, and a layoutgrid +// there is CE6858. With a way to tell native layouts, check skips that page; +// the web page beside it is the control, and a page whose form is fine never +// asks (so a clean script does not open the project for this). +func TestValidatePageLayoutGrid_SkipsNativeLayouts(t *testing.T) { + src := `create page M.P_Native (title: 'N', layout: Atlas_Core.NativePhone_Default, params: ($T: M.T)) { + dataview dvNative (datasource: $T) { textbox tb1 (attribute: Name) } +}; +create page M.P_Web (title: 'W', layout: Atlas_Core.Atlas_Default, params: ($T: M.T)) { + dataview dvWeb (datasource: $T) { textbox tb2 (attribute: Name) } +}; +create page M.P_Fine (title: 'F', layout: Atlas_Core.NativePhone_Default) { + dynamictext t (content: 'x') +};` + prog, errs := visitor.Build(src) + if len(errs) > 0 { + t.Fatalf("parse errors: %v", errs) + } + var asked []string + native := func(layout string) bool { + asked = append(asked, layout) + return layout == "Atlas_Core.NativePhone_Default" + } + vs := ValidatePageLayoutGrid(prog, native) + if len(vs) != 1 || !strings.Contains(vs[0].Message, "dvWeb") { + t.Fatalf("want only the web page's dvWeb reported, got %v", vs) + } + if len(asked) != 2 { + t.Errorf("asked about %v; a page with nothing to report must not ask", asked) + } + // Control: without a project nothing is known, and both pages warn. + if vs := ValidatePageLayoutGrid(prog, nil); len(vs) != 2 { + t.Errorf("without a layout resolver: %d warnings, want 2", len(vs)) + } +} diff --git a/mdl/executor/validate_program.go b/mdl/executor/validate_program.go index 98462e98e4..32c9695de0 100644 --- a/mdl/executor/validate_program.go +++ b/mdl/executor/validate_program.go @@ -27,19 +27,25 @@ func ValidateProgram(prog *ast.Program, projectPath string) []linter.Violation { // Statement-level checks that need no project connection. var violations []linter.Violation securityEnabled := programEnablesSecurity(prog) - // Which Java/JavaScript action calls target a void action — such a call - // declares no variable (MDL063, #953). The script answers for the actions - // it creates; the project, opened only if a call needs it, for the rest. - var voidsProject backend.FullBackend - voids := newVoidCodeActions(prog, func() backend.FullBackend { - voidsProject = openProjectForValidation(projectPath) - return voidsProject - }) + // The project, opened at most once and only if a check needs it. + var lazyProject backend.FullBackend + lazyOpened := false + project := func() backend.FullBackend { + if !lazyOpened { + lazyOpened = true + lazyProject = openProjectForValidation(projectPath) + } + return lazyProject + } defer func() { - if voidsProject != nil { - _ = voidsProject.Disconnect() + if lazyProject != nil { + _ = lazyProject.Disconnect() } }() + // Which Java/JavaScript action calls target a void action — such a call + // declares no variable (MDL063, #953). The script answers for the actions + // it creates; the project for the rest. + voids := newVoidCodeActions(prog, project) for _, stmt := range prog.Statements { // Check enumeration values for reserved words if enumStmt, ok := stmt.(*ast.CreateEnumerationStmt); ok { @@ -182,7 +188,9 @@ func ValidateProgram(prog *ast.Program, projectPath string) []linter.Violation { // wrapped in a layout grid — its label/input widths only render correctly // inside a layoutgrid. Same rule as the MPR010 lint rule, surfaced at // authoring time on the AST. - violations = append(violations, ValidatePageLayoutGrid(prog)...) + // A page on a native layout is exempt (ako/mxcli#962); only the project + // can say which layouts are native. + violations = append(violations, ValidatePageLayoutGrid(prog, projectNativeLayouts(project))...) // Warn (MDL-OFFLINE01) when a page binds an attribute across more than one // association in a project that has an offline navigation profile. Mendix diff --git a/mdl/linter/rules/catalog_mode_guard_test.go b/mdl/linter/rules/catalog_mode_guard_test.go index e7d3d09fe9..4314ca45d7 100644 --- a/mdl/linter/rules/catalog_mode_guard_test.go +++ b/mdl/linter/rules/catalog_mode_guard_test.go @@ -20,7 +20,7 @@ import ( // mdl/linter/starlark_catalog_mode_guard_test.go; keep the two in step. var fullOnlyContext = []string{ "Widgets", "XPathExpressions", "ActivitiesFor", "Permissions", "PermissionsFor", - "FindReferences", "WidgetCount", + "FindReferences", "WidgetCount", "NativePages", } func TestRulesReadingFullOnlyDataDeclareFullCatalog(t *testing.T) { diff --git a/mdl/linter/rules/dataview_layout_grid.go b/mdl/linter/rules/dataview_layout_grid.go index 95e1048938..dce21b3b69 100644 --- a/mdl/linter/rules/dataview_layout_grid.go +++ b/mdl/linter/rules/dataview_layout_grid.go @@ -24,6 +24,14 @@ import ( // not flagged. "Inside a layout grid" means any layoutgrid ancestor — a DataView // under grid → column → container → dataview is fine; only a form DataView with no // layoutgrid ancestor at all is flagged. +// +// The advice is web-only (ako/mxcli#962): the grid columns are Bootstrap's, and a +// native page or snippet is rendered by React Native. Measured on mxbuild 11.13.0 +// (PedApp copy): a form DataView placed directly on an +// Atlas_Core.NativePhone_Default page builds clean, and wrapping it in a +// layoutgrid as the rule advised is CE6858 "Please update Atlas UI to version +// 2.4 or higher to use Layout Grid on Native pages". So a page whose layout is +// native, and a snippet whose Type is Native, are skipped. type DataViewLayoutGridRule struct{} // NewDataViewLayoutGridRule creates a new dataview-layout-grid rule. @@ -36,6 +44,10 @@ func (r *DataViewLayoutGridRule) Name() string { return "Dat func (r *DataViewLayoutGridRule) Category() string { return "design" } func (r *DataViewLayoutGridRule) DefaultSeverity() linter.Severity { return linter.SeverityWarning } +// RequiredCatalogMode: NativePages() joins pages.LayoutRef, which only a full +// catalog build records; on a fast one every page would read as web. +func (r *DataViewLayoutGridRule) RequiredCatalogMode() linter.CatalogMode { return linter.CatalogFull } + func (r *DataViewLayoutGridRule) Description() string { return "A DataView containing input widgets (a form) should be wrapped in a layout grid so label and input widths render correctly" } @@ -54,6 +66,10 @@ func (r *DataViewLayoutGridRule) Check(ctx *linter.LintContext) []linter.Violati if err != nil || raw == nil { return } + // A native snippet says so itself (Forms$Snippet Type "Native"). + if docType == "snippet" && extractStr(raw["Type"]) == "Native" { + return + } for _, root := range rootWidgetNodes(raw) { walkForUngridedDataView(root, false, func(dv map[string]any) { violations = append(violations, linter.Violation{ @@ -73,8 +89,9 @@ func (r *DataViewLayoutGridRule) Check(ctx *linter.LintContext) []linter.Violati } } + native := ctx.NativePages() for p := range ctx.Pages() { - if ctx.IsExcluded(p.ModuleName) { + if ctx.IsExcluded(p.ModuleName) || native[p.QualifiedName] { continue } check(p.ID, p.QualifiedName, p.ModuleName, "page") diff --git a/mdl/linter/rules/dataview_layout_grid_test.go b/mdl/linter/rules/dataview_layout_grid_test.go index 458f94bdcb..a65df2c9b6 100644 --- a/mdl/linter/rules/dataview_layout_grid_test.go +++ b/mdl/linter/rules/dataview_layout_grid_test.go @@ -2,7 +2,19 @@ package rules -import "testing" +import ( + "sort" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/catalog" + "github.com/mendixlabs/mxcli/mdl/linter" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" + "github.com/mendixlabs/mxcli/sdk/pages" + "github.com/mendixlabs/mxcli/sdk/security" +) func textbox(name string) map[string]any { return map[string]any{"$Type": "Forms$TextBox", "Name": name} @@ -113,3 +125,70 @@ func TestWalkForUngridedDataView_DisplayOnlyIgnored(t *testing.T) { t.Fatalf("display-only DataView should not be flagged, got %v", got) } } + +// rawUnitReader serves raw units by ID; everything else is empty. +type rawUnitReader struct{ units map[model.ID]map[string]any } + +func (r rawUnitReader) GetMicroflow(model.ID) (*microflows.Microflow, error) { return nil, nil } +func (r rawUnitReader) ListMicroflows() ([]*microflows.Microflow, error) { return nil, nil } +func (r rawUnitReader) GetProjectSecurity() (*security.ProjectSecurity, error) { + return nil, nil +} +func (r rawUnitReader) GetNavigation() (*types.NavigationDocument, error) { return nil, nil } +func (r rawUnitReader) ListPages() ([]*pages.Page, error) { return nil, nil } +func (r rawUnitReader) ListModules() ([]*model.Module, error) { return nil, nil } +func (r rawUnitReader) ListFolders() ([]*types.FolderInfo, error) { return nil, nil } +func (r rawUnitReader) GetRawUnit(id model.ID) (map[string]any, error) { return r.units[id], nil } +func (r rawUnitReader) ListScheduledEvents() ([]*model.ScheduledEvent, error) { + return nil, nil +} + +// ako/mxcli#962 item 4: the layout-grid advice is web-only. Measured on mxbuild +// 11.13.0 (PedApp copy): a form DataView directly on an +// Atlas_Core.NativePhone_Default page, or in a snippet of Type Native, builds +// clean, and wrapping it in a layoutgrid there is CE6858. The web page and the +// web snippet are the controls. +func TestDataViewLayoutGridRule_SkipsNativePagesAndSnippets(t *testing.T) { + cat, err := catalog.New() + if err != nil { + t.Fatal(err) + } + defer cat.Close() + for _, stmt := range []string{ + `INSERT INTO modules_data (Id, Name, QualifiedName, ModuleName, Source) VALUES + ('m1', 'MyFirstModule', 'MyFirstModule', 'MyFirstModule', ''), + ('m2', 'Atlas_Core', 'Atlas_Core', 'Atlas_Core', 'Atlas_Core.mpk')`, + `INSERT INTO layouts_data (Id, Name, QualifiedName, ModuleName, LayoutType, Platform) VALUES + ('l1', 'Atlas_Default', 'Atlas_Core.Atlas_Default', 'Atlas_Core', 'Responsive', 'Web'), + ('l2', 'NativePhone_Default', 'Atlas_Core.NativePhone_Default', 'Atlas_Core', 'Default', 'Native')`, + `INSERT INTO pages_data (Id, Name, QualifiedName, ModuleName, LayoutRef) VALUES + ('p1', 'P_WebForm', 'MyFirstModule.P_WebForm', 'MyFirstModule', 'Atlas_Core.Atlas_Default'), + ('p2', 'P_NativeForm', 'MyFirstModule.P_NativeForm', 'MyFirstModule', 'Atlas_Core.NativePhone_Default')`, + `INSERT INTO snippets_data (Id, Name, QualifiedName, ModuleName) VALUES + ('s1', 'SN_Web', 'MyFirstModule.SN_Web', 'MyFirstModule'), + ('s2', 'SN_Native', 'MyFirstModule.SN_Native', 'MyFirstModule')`, + } { + if _, err := cat.CatalogDB().Exec(stmt); err != nil { + t.Fatalf("%s: %v", stmt, err) + } + } + page := func(dv string) map[string]any { + return map[string]any{"FormCall": map[string]any{"Arguments": []any{ + map[string]any{"Widgets": []any{dataView(dv, textbox("tb"))}}}}} + } + snippet := func(typ, dv string) map[string]any { + return map[string]any{"Type": typ, "Widgets": []any{dataView(dv, textbox("tb"))}} + } + reader := rawUnitReader{units: map[model.ID]map[string]any{ + "p1": page("dvWeb"), "p2": page("dvNative"), + "s1": snippet("Web", "dvSnipWeb"), "s2": snippet("Native", "dvSnipNative"), + }} + var got []string + for _, v := range NewDataViewLayoutGridRule().Check(linter.NewLintContext(cat, reader)) { + got = append(got, v.Location.DocumentName) + } + sort.Strings(got) + if strings.Join(got, ",") != "P_WebForm,SN_Web" { + t.Fatalf("reported %v, want only the web page and the web snippet", got) + } +} From bff250cab62de1d0d8e825107eedc78e79e25d1b Mon Sep 17 00:00:00 2001 From: Ako Date: Sat, 3 Oct 2026 18:05:14 +0000 Subject: [PATCH 15/15] test(check): merge the persistent -p flag in the MDL093 end-to-end test Without checkCmd.InheritedFlags() the test only saw the project when another test had merged the flags first. Part of #962. Co-Authored-By: Claude Opus 5.5 --- cmd/mxcli/check_void_calls_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cmd/mxcli/check_void_calls_test.go b/cmd/mxcli/check_void_calls_test.go index 776204105c..46b26350db 100644 --- a/cmd/mxcli/check_void_calls_test.go +++ b/cmd/mxcli/check_void_calls_test.go @@ -98,6 +98,7 @@ func TestCheck_ReadOfAStoredVoidCallOutput(t *testing.T) { if err := copyTree(src, dir); err != nil { t.Fatal(err) } + _ = checkCmd.InheritedFlags() // merges the persistent -p into checkCmd.Flags() _ = rootCmd.PersistentFlags().Set("project", filepath.Join(dir, "PedApp.mpr")) defer func() { _ = rootCmd.PersistentFlags().Set("project", "")