From 5fb12412943a59e7fafe7846eb413fbb6562f5af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Go=C5=82embiewski?= Date: Tue, 22 Sep 2026 22:29:20 +0200 Subject: [PATCH 1/3] fix(lint): compare entity_type to the value the linter returns ARCH002 and ARCH003 guard with `entity.entity_type != "PERSISTENT"`. The catalog stores PERSISTENT, but LintContext.Entities normalizes the kind to Persistent (the CASE in its Entities query) before a Starlark rule reads it, so the guard held for every entity and neither rule's body ever ran. Nothing said so: no error, no output, and both still appeared under `--list-rules`, which reads as a project with nothing to report. Measured on a generated app with five persistent entities, none of them carrying a unique attribute: ARCH003 reported 0 findings before the change and 5 after. The test loads the two shipped rule files themselves against a one-entity fixture, so a rule that goes back to the stored spelling fails here rather than going quiet again. Reverting either fix in turn takes that rule's subtest to "got 0 violations, want 1". --- .../lint-rules/data_change_microflows.star | 2 +- .claude/lint-rules/entity_business_key.star | 2 +- mdl/linter/starlark_shipped_rules_test.go | 80 +++++++++++++++++++ 3 files changed, 82 insertions(+), 2 deletions(-) create mode 100644 mdl/linter/starlark_shipped_rules_test.go diff --git a/.claude/lint-rules/data_change_microflows.star b/.claude/lint-rules/data_change_microflows.star index 51c4d77b9d..7dc9e42f72 100644 --- a/.claude/lint-rules/data_change_microflows.star +++ b/.claude/lint-rules/data_change_microflows.star @@ -38,7 +38,7 @@ def check(): # Check each persistent entity for entity in entities(): # Skip non-persistent and view entities (they don't need data-change microflows) - if entity.entity_type != "PERSISTENT": + if entity.entity_type != "Persistent": continue # Get all references to this entity diff --git a/.claude/lint-rules/entity_business_key.star b/.claude/lint-rules/entity_business_key.star index 3fc0ae456a..9b8538ba26 100644 --- a/.claude/lint-rules/entity_business_key.star +++ b/.claude/lint-rules/entity_business_key.star @@ -56,7 +56,7 @@ def check(): for entity in entities(): # Skip non-persistent entities (they don't need business keys) - if entity.entity_type != "PERSISTENT": + if entity.entity_type != "Persistent": continue # Skip system/administration entities that typically use internal IDs diff --git a/mdl/linter/starlark_shipped_rules_test.go b/mdl/linter/starlark_shipped_rules_test.go new file mode 100644 index 0000000000..c59eaa794c --- /dev/null +++ b/mdl/linter/starlark_shipped_rules_test.go @@ -0,0 +1,80 @@ +// SPDX-License-Identifier: Apache-2.0 + +package linter_test + +import ( + "database/sql" + "path/filepath" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/catalog" + "github.com/mendixlabs/mxcli/mdl/linter" + _ "modernc.org/sqlite" +) + +// The catalog stores an entity's kind as PERSISTENT / NON_PERSISTENT / VIEW and +// LintContext.Entities normalizes it to Persistent / NonPersistent / View before a +// Starlark rule ever sees it. A rule that compares entity_type to the stored +// spelling therefore skips every entity, reports nothing, and passes every project +// in silence -- there is no error and no output to notice. +// +// These two rules ship with mxcli and are copied into every project by `mxcli init`, +// so the check runs against the files themselves rather than against a copy of their +// text: a rule reintroducing the stored spelling has to fail here. +func TestShippedEntityRulesSeeAPersistentEntity(t *testing.T) { + db := shippedRuleFixtureDB(t) + + for _, tc := range []struct { + rule string + want string + }{ + {"entity_business_key.star", "Order"}, + {"data_change_microflows.star", "Order"}, + } { + t.Run(tc.rule, func(t *testing.T) { + path := filepath.Join("..", "..", ".claude", "lint-rules", tc.rule) + r, err := linter.LoadStarlarkRule(path) + if err != nil { + t.Fatalf("LoadStarlarkRule(%s): %v", tc.rule, err) + } + got := r.Check(linter.NewLintContextFromDB(db)) + if len(got) != 1 { + t.Fatalf("got %d violations, want 1 -- the fixture is one persistent entity with "+ + "no unique attribute and no microflow touching it: %v", len(got), got) + } + if msg := got[0].Message; !strings.Contains(msg, tc.want) { + t.Errorf("violation says %q, want it to name %q", msg, tc.want) + } + }) + } +} + +// One module, one persistent entity, no attributes marked unique and nothing +// referring to it: the shape both rules exist to report. +func shippedRuleFixtureDB(t *testing.T) catalog.CatalogDB { + t.Helper() + db, err := sql.Open("sqlite", ":memory:") + if err != nil { + t.Fatal(err) + } + stmts := []string{ + `CREATE TABLE modules (Id TEXT, Name TEXT, Source TEXT)`, + `INSERT INTO modules VALUES ('m1', 'Sales', '')`, + `CREATE TABLE entities ( + Id TEXT, Name TEXT, QualifiedName TEXT, ModuleName TEXT, Folder TEXT, + EntityType TEXT, Description TEXT, Generalization TEXT, + AttributeCount INTEGER, AccessRuleCount INTEGER, ValidationRuleCount INTEGER, + HasEventHandlers INTEGER, IsExternal INTEGER, + HasCreatedDate INTEGER, HasChangedDate INTEGER, + HasOwner INTEGER, HasChangedBy INTEGER)`, + `INSERT INTO entities VALUES + ('e1','Order','Sales.Order','Sales','','PERSISTENT','','',1,1,0,0,0, 0,0,0,0)`, + } + for _, s := range stmts { + if _, err := db.Exec(s); err != nil { + t.Fatalf("fixture %q: %v", s, err) + } + } + return catalog.WrapSqlDB(db) +} From 84449c76c3f6ffe4108a5675510b329abd69b25f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Go=C5=82embiewski?= Date: Tue, 22 Sep 2026 22:29:20 +0200 Subject: [PATCH 2/3] docs(skill): say which entity_type values a rule can compare against The field table and the worked example in write-lint-rules both used "persistent", a third spelling that matches nothing. A rule written from either reads as working and reports on no project at all, which is how the two rules fixed in the previous commit were written. --- .claude/skills/mendix/write-lint-rules/SKILL.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.claude/skills/mendix/write-lint-rules/SKILL.md b/.claude/skills/mendix/write-lint-rules/SKILL.md index 0b32d4b405..3e6bf7d842 100644 --- a/.claude/skills/mendix/write-lint-rules/SKILL.md +++ b/.claude/skills/mendix/write-lint-rules/SKILL.md @@ -156,7 +156,7 @@ def check(): | `qualified_name` | string | `"Sales.Customer"` | | `module_name` | string | `"Sales"` | | `folder` | string | `"DomainModel"` — folder path within module | -| `entity_type` | string | `"persistent"`, `"NonPersistent"`, `"view"` | +| `entity_type` | string | exactly `"Persistent"`, `"NonPersistent"` or `"View"` — any other spelling matches nothing and the rule silently reports nothing | | `description` | string | Documentation text | | `generalization` | string | Parent entity qualified name | | `attribute_count` | int | Number of attributes | @@ -475,7 +475,7 @@ SEVERITY = "warning" def check(): violations = [] for e in entities(): - if e.entity_type == "persistent" and not e.is_external and e.access_rule_count == 0: + if e.entity_type == "Persistent" and not e.is_external and e.access_rule_count == 0: violations.append(violation( message="persistent entity '{}' has no access rules".format(e.qualified_name), location=location(module=e.module_name, document_type="entity", document_name=e.name), From 4305002e7535f1996f080a7151239e71c21e2d35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Go=C5=82embiewski?= Date: Tue, 22 Sep 2026 22:29:20 +0200 Subject: [PATCH 3/3] docs: record the dead entity_type comparison in the changelog and findings --- .claude/skills/fix-issue/findings/cmd-mxcli.jsonl | 1 + CHANGELOG.md | 2 ++ 2 files changed, 3 insertions(+) diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl b/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl index 5859918bf2..1922d3184c 100644 --- a/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl +++ b/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl @@ -118,3 +118,4 @@ {"area": "cmd/mxcli", "date": "2026-09-18", "symptom": "mendixlabs/mxcli#1025: `mxcli syntax` advertises `mxcli syntax workflow user-task targeting` in its own help and answers `Unknown topic: workflow user-task targeting`. Same for `workflow user-task` and `workflow parallel-split`, all of which `mxcli syntax workflow` lists as sub-topics; `--json` was the only route that reached them.", "cause": "The CLI built its path with `strings.Join(args, \".\")` and never split an argument, so a topic handed over as ONE string — a quoted copy-paste, a tool wrapper, `sh -c` — became the path `workflow user-task targeting`, which matches nothing. The REPL's `help` had resolved multi-word topics since it was written (`resolveHelpPath`, greedy hyphen-joining): one question, two answers, and the CLI held the weaker copy. The #955 segment-match fallback could not save it either — it passed the DOTTED path to `BySegmentMatch`, and no segment contains a '.', so that fallback was silently dead for every multi-word query.", "file": "cmd/mxcli/syntax/topic.go (new: Lookup, topicWords, resolvePath), cmd/mxcli/help.go, mdl/executor/cmd_misc.go (resolveHelpPath deleted), mdl/grammar/domains/MDLSettings.g4 (helpStatement, helpTopicWord), mdl/visitor/visitor_query.go (ExitHelpStatement); tests cmd/mxcli/cmd_syntax_test.go, cmd/mxcli/syntax/topic_test.go, mdl/executor/cmd_misc_test.go, mdl/visitor/visitor_help_topic_test.go; example mdl-examples/bug-tests/syntax-1025-topic-drilldown.mdl", "insight": "**The spaces in the reported error message were the whole diagnosis, and reading them as a paraphrase cost an hour.** The command prints the path it built, and the CLI joins on '.', so `Unknown topic: workflow user-task targeting` cannot come from the command as documented — it can only come from the topic arriving as a single argument. Every line of the report follows from that and nothing else does: `syntax workflow` works (one word), `--json` works (the flag is not part of the topic), the three multi-word forms fail. Take a quoted error message literally, character for character, before assuming the reporter retyped it. **The reported version is downloadable and settles it in one run**: `mxcli setup mxcli`'s own URL shape (`releases/download//mxcli-linux-amd64`, NOT the goreleaser `_Linux_x86_64.tar.gz` that 404s) fetched v0.20.0, where the unquoted command works and the quoted one reproduces the message verbatim — so 'fixed since' and 'never broken' were both wrong. **The guard that matters is not the three cases from the report** but `TestEveryRegisteredPathIsReachableBySpelling`: every registered path, tried dotted, as separate arguments, and as one string. The registry prints dotted paths and then tells the reader to drill down with words, so a spelling that does not resolve is the command contradicting its own output; a per-case test would have passed the day someone added a topic with a new shape. Control: stub the whitespace split in `topicWords` and it fails with the reported path, spaces and all. **The grammar half has a trap the CLI half does not, and only the EXISTING suite caught it.** `helpStatement: IDENTIFIER (identifierOrKeyword)*` is the grammar's catch-all — a statement that is just an identifier and some words — so whatever it can swallow, it swallows from the statement that should have had it. Widening it to `(DOT? helpTopicWord)*` to take `help workflow.user-task` made `Sec.ApiUser` a complete statement of its own, and `create module role Sec.ApiUser` then parsed, WITH NO PARSE ERROR, as CREATE MODULE (named \"role\") followed by a help topic — two statements, wrong types, six unrelated security tests red. `(helpTopicWord (DOT? helpTopicWord)*)?` — a topic word before any dot — leaves `.ApiUser` unconsumable and restores the old disambiguation. Bisect a grammar regression by SHAPE, not by reading the ATN: adding the unused rule alone was clean, the hyphen alone was clean, the leading optional DOT was the whole of it, and three regenerations said so in about a minute. **When widening a permissive rule, the test to add is not for the new spelling but for what the rule must still NOT swallow** (TestHelpRuleDoesNotSwallowATrailingQualifiedName).", "refs": ["mendixlabs/mxcli#1025", "#955"]} {"area": "cmd/mxcli", "date": "2026-09-18", "symptom": "`mxcli report` scores a project against rules the team disabled in `lint-config.yaml`. `mxcli lint` honours the config, the report's SCORE does not move, so the score cannot be calibrated at all. Reported at 66/100 against a 99/100 blank-app baseline, where 61 of 86 findings were two deliberately-accepted rules", "cause": "`cmd_report.go` never called `linter.FindConfigFile`/`LoadConfig` — it went straight from `linter.New` to `BuildReport`. Separately it carried its own INLINE copy of the built-in rule list, one rule behind `builtinLintRules()` (missing MDL-FLOW01), so the two commands scored one project against two rule sets. One root cause: report re-implemented lint's setup instead of sharing it", "file": "`cmd/mxcli/cmd_report.go`, `cmd/mxcli/cmd_lint.go`, new `cmd/mxcli/lint_setup.go` (`projectLintRules`, `applyLintConfig`), `mdl/linter/linter.go` (`RuleEnabled`)", "insight": "Same class as #904 in the opposite direction: there a silently reduced rule set made the score falsely HIGH, here an unread config makes it falsely LOW — and both are invisible because a score carries no provenance. **A value test cannot guard the inline copy**: both commands build rules inside a cobra RunE, so nothing a unit test can call notices a second list being re-added. The guard is therefore structural — grep `cmd_report.go` for `lint.AddRule(rules.New` — with a POSITIVE CONTROL first (assert `builtinLintRules` still constructs rules) so it cannot pass vacuously, the same shape as `scripts/check-tunnel-deps.sh`. Take the LintContext out of `applyLintConfig`'s signature: `NewLintContext(nil, nil)` panics, and a nil-guard added only to make a test compile is how a helper acquires behaviour nothing needs", "refs": ["#525", "#904"]} {"area": "cmd/mxcli/marketplace", "date": "2026-09-20", "symptom": "`mxcli marketplace install ... -p app.mpr` (project named by a RELATIVE path) fails with `install the package's bundled files: package entry \"manifest.json\" would write outside the project` \u2014 after the module has already been transplanted into the model. `SHOW MODULES` lists the module, `mx check` is clean, but no bundled file (themesource/, widgets/) landed and the command exited 1. Absolute `-p` paths work.", "cause": "`InstallPackageFiles` builds `dst := filepath.Join(projectDir, clean)` and then checks `strings.HasPrefix(filepath.Clean(dst), filepath.Clean(projectDir)+os.PathSeparator)`. With projectDir == \".\" (from `filepath.Dir(\"app.mpr\")`), Join drops the dot, so dst is `manifest.json` and the prefix is `./` \u2014 every legitimate entry fails the zip-slip guard. The guard was checking the joined path (the shape CodeQL recognises) but never anchored the project directory first.", "file": "`cmd/mxcli/marketplace/update.go` (`InstallPackageFiles`: `filepath.Abs(projectDir)` before the loop), test `cmd/mxcli/marketplace/install_relative_dir_test.go`", "insight": "A containment guard has two inputs and both must be canonical \u2014 the entry AND the root. The traversal tests only ever passed an absolute t.TempDir(), so the root was canonical by accident and the relative case had no coverage; the first real CLI invocation with `-p app.mpr` hit it. Worse, the transplant runs BEFORE the file step, so the failure lands on a half-installed module: the model has it, the disk does not, and the exit code says failure. Order the steps so the cheap, reversible file copy can be validated before the model write, or at least say in the error that the model was already changed. Prove-by-revert done: the new test fails on the unpatched function with the exact reported message.", "refs": []} +{"area": "cmd/mxcli", "date": "2026-09-22", "symptom": "ARCH002 and ARCH003 never report anything. `mxcli lint` runs them, prints no error and lists them under --list-rules, yet a project whose entities have no business key and no data-change microflow comes back clean.", "cause": "Both rules guard with `if entity.entity_type != \"PERSISTENT\": continue`. The catalog column stores PERSISTENT, but LintContext.Entities normalizes it (CASE WHEN 'PERSISTENT' THEN 'Persistent') before Starlark sees it, so the guard is true for every entity and the body never runs. The write-lint-rules skill documented a third spelling again, \"persistent\", in both its field table and its worked example.", "file": ".claude/lint-rules/entity_business_key.star, .claude/lint-rules/data_change_microflows.star, mdl/linter/context.go", "insight": "A Starlark rule that compares a field to a value that never occurs fails open: no error, no output, and --list-rules still shows it, so it looks like a project with nothing to report. The literals a rule may compare against are worth asserting in a test that loads the shipped rule file itself; measured on a generated app, fixing one spelling took ARCH003 from 0 findings to 5.", "refs": ""} diff --git a/CHANGELOG.md b/CHANGELOG.md index 32dc27fa18..3aca3f4a10 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **Two bundled lint rules reported nothing, on every project, since they were written** — `ARCH002` (entities without a data-change microflow) and `ARCH003` (entities without a business key) both skip an entity with `if entity.entity_type != "PERSISTENT"`. The catalog stores `PERSISTENT`, but `LintContext.Entities` normalizes the kind to `Persistent` before a Starlark rule reads it, so the guard held for every entity and the body never ran. There was nothing to notice: no error, no output, and `--list-rules` still listed both. Measured on a generated app of five persistent entities, `ARCH003` went from 0 findings to 5 once the spelling matched. The `write-lint-rules` skill had documented a third spelling, `"persistent"`, in its field table and in its worked example; both now say what the API returns, and a test loads the two shipped rule files and fails if either stops seeing a persistent entity. + - **A page's image-collection reference passed `mxcli check --references` and failed the build** (mendixlabs/mxcli#1149) — `staticimage imgAll (Image: 'Atlas_UI_Resources.Atlas_Icons.checkbox_checked')` in a Selection helper's custom state checked clean, exec'd cleanly and then came back as `[error] [CE1613] "The selected image … no longer exists."`, once per state. The report asks for syntax, but the syntax landed with #1057 — describe emits the three `staticimage` lines and re-running the description reports `Unchanged page`, measured on a blank 11.14.0 project. What was missing is that nothing resolved the name #1057 had made writable. Two holes, and fixing either alone leaves the reported script unchecked. The image reference was collected by **widget type** (`if w.Type == "image"`), so the pluggable widget was resolved and the two widgets #1057 gave the same property — `staticimage`'s `Image` and `dynamicimage`'s `DefaultImage` — were not; it is a table now, so adding a widget that names an image means adding a row. And a page's widgets live in **two** AST fields: `Widgets` is the bare body, while `placeholder { … }` content is held apart in `Placeholders` (#532). All three page validators walked the first alone, so **every** reference inside a placeholder block — microflow, nanoflow, page, snippet, entity, image — was validated by nothing. Measured, the same button in the two positions: inside `placeholder Main` → `✓ All references valid`; in the bare body → `microflow not found`. That is the shape mxcli's own skills, examples and DESCRIBE output write, so it was the common case rather than an edge one, and it is the third copy of one walk — `validateIconRefs` (#1008) and `forEachWidget` had each grown the placeholder arm separately — so the roots are now collected once.